Bug 117365

Summary:

[CSS Regions] RenderRegions should have a RenderLayer+Backing when they contain a Composited RenderLayer

Product:

WebKit

Reporter:

Alexandru Chiculita <achicu>

Component:

Layout and Rendering

Assignee:

Alexandru Chiculita <achicu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, esprehn+autocc, glenn, hyatt, kondapallykalyan, rniwa, sam, simon.fraser, WebkitBugTracker

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

84900

Attachments:

Description	Flags
Patch V1	hyatt: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Patch V2	none
Patch V3	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Patch V4	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none
Patch V5	none
Patch V6	buildbot: commit-queue-
Patch V7	none

Alexandru Chiculita

Reported 2013-06-07 16:09:28 PDT

In order to enable compositing in RenderRegions we need to make the regions become RenderLayers when composited layers are displayed in these regions.

Attachments
Patch V1 (54.30 KB, patch) 2013-06-12 16:41 PDT, Alexandru Chiculita	hyatt: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (714.46 KB, application/zip) 2013-06-13 08:17 PDT, Build Bot	no flags	Details
Patch V2 (55.53 KB, patch) 2013-06-28 01:51 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch V3 (55.93 KB, patch) 2013-07-11 05:48 PDT, Alexandru Chiculita	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (485.22 KB, application/zip) 2013-07-11 17:56 PDT, Build Bot	no flags	Details
Patch V4 (56.23 KB, patch) 2013-07-24 01:15 PDT, Alexandru Chiculita	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (493.19 KB, application/zip) 2013-07-24 06:09 PDT, Build Bot	no flags	Details
Patch V5 (70.90 KB, patch) 2013-07-31 11:49 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Patch V6 (74.00 KB, patch) 2013-08-01 08:53 PDT, Alexandru Chiculita	buildbot: commit-queue-	Details Formatted Diff Diff
Patch V7 (74.44 KB, patch) 2013-08-14 13:45 PDT, Alexandru Chiculita	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Alexandru Chiculita

Comment 1 2013-06-12 16:41:12 PDT

Created attachment 204545 [details] Patch V1

Build Bot

Comment 2 2013-06-13 08:17:14 PDT

Comment on attachment 204545 [details] Patch V1 Attachment 204545 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/848116 New failing tests: editing/pasteboard/4641033.html

Build Bot

Comment 3 2013-06-13 08:17:17 PDT

Created attachment 204588 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

Alexandru Chiculita

Comment 4 2013-06-14 14:16:14 PDT

(In reply to comment #2) > (From update of attachment 204545 [details]) > Attachment 204545 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/848116 > > New failing tests: > editing/pasteboard/4641033.html This is unrelated and I've seen it on other bugs/patches.

Dave Hyatt

Comment 5 2013-06-27 12:17:29 PDT

Comment on attachment 204545 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=204545&action=review r- > Source/WebCore/rendering/RenderBox.cpp:169 > +RenderBlock* RenderBox::containingBlockForFloatingObject() I think this needs to be renamed. containingBlock is a precise CSS term, and that is not what you're returning here. This is the outermost block that has the float in its floating object list. outermostBlockContainingFloatingObject is probably ok.

Alexandru Chiculita

Comment 6 2013-06-28 01:51:33 PDT

Created attachment 205679 [details] Patch V2 Updated to outermostBlockContainingFloatingObject. Also, the RenderNamedFlowThread is hitting the max object size limit enforced by the RenderArena, so I converted the new hashmaps to OwnPtrs instead.

Dave Hyatt

Comment 7 2013-07-10 11:13:25 PDT

Comment on attachment 205679 [details] Patch V2 Looks fine, but shouldn't you have the ifdef around the rest of the code and not just the mapping update? At the very least it seems like you should not have the extra member variable cost of OwnPtr<LayerToRegionMap> m_layerToRegionMap; when accelerated compositing is turned off.

Alexandru Chiculita

Comment 8 2013-07-11 04:31:10 PDT

(In reply to comment #7) > (From update of attachment 205679 [details]) > Looks fine, but shouldn't you have the ifdef around the rest of the code and not just the mapping update? At the very least it seems like you should not have the extra member variable cost of OwnPtr<LayerToRegionMap> m_layerToRegionMap; when accelerated compositing is turned off. You're right, good catch.

Alexandru Chiculita

Comment 9 2013-07-11 05:48:18 PDT

Created attachment 206451 [details] Patch V3

Build Bot

Comment 10 2013-07-11 17:56:19 PDT

Comment on attachment 206451 [details] Patch V3 Attachment 206451 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1024897 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html

Build Bot

Comment 11 2013-07-11 17:56:22 PDT

Created attachment 206499 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

Alexandru Chiculita

Comment 12 2013-07-15 02:07:32 PDT

Looks l(In reply to comment #10) > (From update of attachment 206451 [details]) > Attachment 206451 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/1024897 > > New failing tests: > fullscreen/full-screen-iframe-with-max-width-height.html Looks like this test is unrelated to my change. I couldn't make it fail locally.

Alexandru Chiculita

Comment 13 2013-07-24 01:15:40 PDT

Created attachment 207382 [details] Patch V4

Build Bot

Comment 14 2013-07-24 06:09:50 PDT

Comment on attachment 207382 [details] Patch V4 Attachment 207382 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1094473 New failing tests: http/tests/security/cross-origin-plugin-private-browsing-toggled.html

Build Bot

Comment 15 2013-07-24 06:09:52 PDT

Created attachment 207389 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

Build Bot

Comment 16 2013-07-29 11:11:15 PDT

Comment on attachment 207382 [details] Patch V4 Attachment 207382 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1299023

Alexandru Chiculita

Comment 17 2013-07-31 11:49:26 PDT

Created attachment 207867 [details] Patch V5 Added code to invalidate the layer map when there's a layer added without a layout trigger.

Alexandru Chiculita

Comment 18 2013-08-01 08:53:46 PDT

Created attachment 207929 [details] Patch V6 This time including binary files :)

Build Bot

Comment 19 2013-08-13 02:55:37 PDT

Comment on attachment 207929 [details] Patch V6 Attachment 207929 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1452371

Simon Fraser (smfr)

Comment 20 2013-08-14 11:41:27 PDT

Comment on attachment 207929 [details] Patch V6 View in context: https://bugs.webkit.org/attachment.cgi?id=207929&action=review > LayoutTests/fast/regions/layers/dynamic-layer-added-with-no-layout.html:28 > + // Force a layout > + document.body.offsetTop; Why is this necessary?

Dave Hyatt

Comment 21 2013-08-14 11:43:04 PDT

Comment on attachment 207929 [details] Patch V6 Looks good. Maybe you could add asserts that you don't need layout inside the update floating objects painting container stuff? That will protect against people calling it at a bad time. I'd also bail if you need layout in addition to asserting.

Alexandru Chiculita

Comment 22 2013-08-14 13:45:05 PDT

Created attachment 208756 [details] Patch V7 Added needsLayout checks.

Dave Hyatt

Comment 23 2013-08-14 13:47:28 PDT

Comment on attachment 208756 [details] Patch V7 r=me

WebKit Commit Bot

Comment 24 2013-08-14 14:34:59 PDT

Comment on attachment 208756 [details] Patch V7 Clearing flags on attachment: 208756 Committed r154072: <http://trac.webkit.org/changeset/154072>

WebKit Commit Bot

Comment 25 2013-08-14 14:35:04 PDT

All reviewed patches have been landed. Closing bug.

Simon Fraser (smfr)

Comment 26 2013-08-14 18:32:58 PDT

Looks like some other tests need new baselines: http://build.webkit.org/results/Apple%20Lion%20Release%20WK2%20(Tests)/r154084%20(12081)/fast/repaint/japanese-rl-selection-repaint-in-regions-pretty-diff.html

Note You need to log in before you can comment on or make changes to this bug.