Bug 117365 - [CSS Regions] RenderRegions should have a RenderLayer+Backing when they contain a Composited RenderLayer
Summary: [CSS Regions] RenderRegions should have a RenderLayer+Backing when they conta...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on:
Blocks: 84900
  Show dependency tree / graph
 
Reported: 2013-06-07 16:09 PDT by Alexandru Chiculita
Modified: 2013-08-14 18:32 PDT (History)
10 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 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.
Comment 1 Alexandru Chiculita 2013-06-12 16:41:12 PDT 
Created attachment 204545 [details]
Patch V1
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Alexandru Chiculita 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.
Comment 5 Dave Hyatt 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.
Comment 6 Alexandru Chiculita 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.
Comment 7 Dave Hyatt 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.
Comment 8 Alexandru Chiculita 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.
Comment 9 Alexandru Chiculita 2013-07-11 05:48:18 PDT 
Created attachment 206451 [details]
Patch V3
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Alexandru Chiculita 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.
Comment 13 Alexandru Chiculita 2013-07-24 01:15:40 PDT 
Created attachment 207382 [details]
Patch V4
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Alexandru Chiculita 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.
Comment 18 Alexandru Chiculita 2013-08-01 08:53:46 PDT 
Created attachment 207929 [details]
Patch V6

This time including binary files :)
Comment 19 Build Bot 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
Comment 20 Simon Fraser (smfr) 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?
Comment 21 Dave Hyatt 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.
Comment 22 Alexandru Chiculita 2013-08-14 13:45:05 PDT 
Created attachment 208756 [details]
Patch V7

Added needsLayout checks.
Comment 23 Dave Hyatt 2013-08-14 13:47:28 PDT 
Comment on attachment 208756 [details]
Patch V7

r=me
Comment 24 WebKit Commit Bot 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>
Comment 25 WebKit Commit Bot 2013-08-14 14:35:04 PDT 
All reviewed patches have been landed.  Closing bug.