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

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

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

[Alexandru Chiculita]

Created attachment 204545 [details]
Patch V1

[Build Bot]

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]

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]

(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 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]

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 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]

(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]

Created attachment 206451 [details]
Patch V3

[Build Bot]

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]

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]

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]

Created attachment 207382 [details]
Patch V4

[Build Bot]

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]

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 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]

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]

Created attachment 207929 [details]
Patch V6

This time including binary files :)

[Build Bot]

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 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 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]

Created attachment 208756 [details]
Patch V7

Added needsLayout checks.

[Dave Hyatt]

Comment on attachment 208756 [details]
Patch V7

r=me

[WebKit Commit Bot]

Comment on attachment 208756 [details]
Patch V7

Clearing flags on attachment: 208756

Committed r154072: <http://trac.webkit.org/changeset/154072>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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