Bug 125756

Created attachment 219272 [details] Patch

Comment on attachment 219272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219272&action=review > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:6 > + window.internals.settings.setAcceleratedCompositingForFixedPositionEnabled(false); What is the purpose of this line? You could add the reason as a comment in the code. > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:17 > + <p>That that a fixed positioned element whose parent gets a 3d transform dynamically is correctly re-positioned relative to the parent.</p> You probably meant to say that "<p>Tests that a ...". > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed2-expected.html:14 > + <p>That that a fixed positioned element whose parent dynamically looses its transformation is correctly re-positioned relative to the view.</p> You probably meant to say that "<p>Tests that a ...".

(In reply to comment #2) > (From update of attachment 219272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219272&action=review > > > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:6 > > + window.internals.settings.setAcceleratedCompositingForFixedPositionEnabled(false); > > What is the purpose of this line? > You could add the reason as a comment in the code. At this moment, accelerated compositing is not yet enabled for fixed positioned elements that are part of the named flow. > > > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed-expected.html:17 > > + <p>That that a fixed positioned element whose parent gets a 3d transform dynamically is correctly re-positioned relative to the parent.</p> > > You probably meant to say that "<p>Tests that a ...". > > > LayoutTests/fast/regions/fixed-in-named-flow-cb-changed2-expected.html:14 > > + <p>That that a fixed positioned element whose parent dynamically looses its transformation is correctly re-positioned relative to the view.</p> > > You probably meant to say that "<p>Tests that a ...". Right, thanks for catching this. I will change when i will land the patch (after review).

Created attachment 219425 [details] Patch 2

Comment on attachment 219425 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=219425&action=review > Source/WebCore/rendering/FlowThreadController.cpp:290 > +// The list of layers is returned already sorted by z-Index. I think the word “already” only makes sense here if you know how the code used to work and so should be removed. The “I” in “z-index” should not be capitalized. > Source/WebCore/rendering/FlowThreadController.cpp:301 > + // Iterate over the lists of fixed positioned layers collected at flow thread layer level I don’t understand how this comment adds to the readability of the code. A for loop already says “iterate”. The if statements already says “fixed position” and the variable name also says “fixed positioned layers”, albeit with “positioned” abbreviated to “pos”. The other variable name says “flow thread layer”. The comment just repeats exactly what the code says. Please omit such comments. > Source/WebCore/rendering/FlowThreadController.cpp:323 > + // Sort the fixed layers list Same thing about this comment. Fine to write this as a placeholder before writing the code, but once you write the code the comment seems superfluous.

Created attachment 219429 [details] Patch for landing

(In reply to comment #5) > (From update of attachment 219425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219425&action=review > > > Source/WebCore/rendering/FlowThreadController.cpp:290 > > +// The list of layers is returned already sorted by z-Index. > > I think the word “already” only makes sense here if you know how the code used to work and so should be removed. The “I” in “z-index” should not be capitalized. > > > Source/WebCore/rendering/FlowThreadController.cpp:301 > > + // Iterate over the lists of fixed positioned layers collected at flow thread layer level > > I don’t understand how this comment adds to the readability of the code. A for loop already says “iterate”. The if statements already says “fixed position” and the variable name also says “fixed positioned layers”, albeit with “positioned” abbreviated to “pos”. The other variable name says “flow thread layer”. The comment just repeats exactly what the code says. Please omit such comments. > > > Source/WebCore/rendering/FlowThreadController.cpp:323 > > + // Sort the fixed layers list > > Same thing about this comment. Fine to write this as a placeholder before writing the code, but once you write the code the comment seems superfluous. Thank you for the review. I removed the unnecessary comments and changed the first comment.

Comment on attachment 219429 [details] Patch for landing Clearing flags on attachment: 219429 Committed r160717: <http://trac.webkit.org/changeset/160717>

All reviewed patches have been landed. Closing bug.

This seems to have broken 3 tests: http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r160717%20(1758)/results.html

Re-opened since this is blocked by bug 125863

Rolled out in <http://trac.webkit.org/changeset/160720>. I think that it's only the two new tests that were broken, fast/frames/sandboxed-iframe-navigation-allowed.html passed on the next run, so it's probably a flake.

Looks like there were assertion failures too: http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160718%20(1142)/results.html Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010bb7cb2a WTFCrash + 42 (Assertions.cpp:341) 1 com.apple.WebCore 0x000000010da55f42 WebCore::RenderLayer::negZOrderList() const + 82 (RenderLayer.h:550) 2 com.apple.WebCore 0x000000010da54c5f WebCore::FlowThreadController::collectFixedPositionedLayers(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow>&) const + 223 (FlowThreadController.cpp:302) 3 com.apple.WebCore 0x000000010e8ac488 WebCore::RenderLayer::paintFixedLayersInNamedFlows(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 136 (RenderLayer.cpp:4048) 4 com.apple.WebCore 0x000000010e8ab79b WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 2347 (RenderLayer.cpp:4185) 5 com.apple.WebCore 0x000000010e8aae68 WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 264 (RenderLayer.cpp:3896) 6 com.apple.WebCore 0x000000010e8a9d3c WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 1612 (RenderLayer.cpp:3877)

(In reply to comment #13) > Looks like there were assertion failures too: > > http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160718%20(1142)/results.html > > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.JavaScriptCore 0x000000010bb7cb2a WTFCrash + 42 (Assertions.cpp:341) > 1 com.apple.WebCore 0x000000010da55f42 WebCore::RenderLayer::negZOrderList() const + 82 (RenderLayer.h:550) > 2 com.apple.WebCore 0x000000010da54c5f WebCore::FlowThreadController::collectFixedPositionedLayers(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow>&) const + 223 (FlowThreadController.cpp:302) > 3 com.apple.WebCore 0x000000010e8ac488 WebCore::RenderLayer::paintFixedLayersInNamedFlows(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 136 (RenderLayer.cpp:4048) > 4 com.apple.WebCore 0x000000010e8ab79b WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 2347 (RenderLayer.cpp:4185) > 5 com.apple.WebCore 0x000000010e8aae68 WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 264 (RenderLayer.cpp:3896) > 6 com.apple.WebCore 0x000000010e8a9d3c WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int) + 1612 (RenderLayer.cpp:3877) Thanks, investigating. Looks like it hits the assertion only on WebKit1.

Created attachment 220740 [details] Patch 3

Darin, would you like to review this one too please? I managed to fix the assertions in the debug build from the previous fix you reviewed as well as fixing the paths in the tests. Thank you

Created attachment 221165 [details] Patch

Comment on attachment 221165 [details] Patch Clearing flags on attachment: 221165 Committed r162050: <http://trac.webkit.org/changeset/162050>

All reviewed patches have been landed. Closing bug.