containingBlockForFixedPosition/containingBlockForAbsolutePosition/containingBlockForObjectInFlow functions expect the renderer's parent to be passed in. It is rather misleading and highly error prone. we should just call them with the renderer itself.
Created attachment 278808 [details] Patch
Comment on attachment 278808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278808&action=review I am not convinced by this. > Source/WebCore/dom/Element.cpp:979 > - if (auto containingBlock = containingBlockForAbsolutePosition(&renderer)) { > + if (auto containingBlock = containingBlockForAbsolutePosition(renderer)) { Isn't this a behavior change? It used to start looking from renderer->parent(), and now starts looking from renderer. > Source/WebCore/rendering/LogicalSelectionOffsetCaches.h:94 > + m_containingBlockForFixedPosition.setBlock(containingBlockForFixedPosition(rootBlock), nullptr); > + m_containingBlockForAbsolutePosition.setBlock(containingBlockForAbsolutePosition(rootBlock), nullptr); > + m_containingBlockForInflowPosition.setBlock(containingBlockForObjectInFlow(rootBlock), nullptr); Shame that we call parent() 3 times now. > Source/WebCore/rendering/RenderElement.cpp:2248 > + auto* containingBlock = renderer.parent(); Naming the variable 'containingBlock' is misleading. It's 'candidateForContainingBlock'. > Source/WebCore/rendering/RenderElement.h:493 > -RenderBlock* containingBlockForFixedPosition(const RenderElement*); > -RenderBlock* containingBlockForAbsolutePosition(const RenderElement*); > -RenderBlock* containingBlockForObjectInFlow(const RenderElement*); > +RenderBlock* containingBlockForFixedPosition(const RenderObject&); > +RenderBlock* containingBlockForAbsolutePosition(const RenderObject&); > +RenderBlock* containingBlockForObjectInFlow(const RenderObject&); This makes me think that the functions do the right thing, but just have bad names. They are also free functions, not members. > Source/WebCore/rendering/RenderInline.cpp:174 > - auto* container = containingBlockForAbsolutePosition(this); > + auto* container = containingBlockForAbsolutePosition(*this); Behavior change here. > Source/WebCore/rendering/RenderLineBreak.cpp:244 > - auto* containingBlock = containingBlockForObjectInFlow(this); > + auto* containingBlock = containingBlockForObjectInFlow(*this); Here too.
The behavior changes are intended. The name "containingBlockForAbsolutePosition" suggests that when you pass in a renderer, it returns its containing block. The fact that you need to pass in the parent is not even indicated by the name of the 'in' parameter. There are already callsites that missed this (->behavior change related changes). They call the functions with the renderer instead of its parent. -and if you call it with a containing block renderer, you probably don't expect it to return itself. I am not convinced that renaming the functions is the right approach here as what we need is a function that takes a renderer and returns its containing block and not a function that take a renderer's parent and returns the the parent's child's (renderer) containing block.
Also, no more ugly const_cast<RenderBlock*>s!
(In reply to comment #3) > The behavior changes are intended. In this case they should be carefully described in the changelog, and ideally tested by layout tests. > The name "containingBlockForAbsolutePosition" suggests that when you pass in > a renderer, it returns its containing block. The fact that you need to pass > in the parent is not even indicated by the name of the 'in' parameter. There > are already callsites that missed this (->behavior change related changes). > They call the functions with the renderer instead of its parent. -and if you > call it with a containing block renderer, you probably don't expect it to > return itself. > I am not convinced that renaming the functions is the right approach here as > what we need is a function that takes a renderer and returns its containing > block and not a function that take a renderer's parent and returns the the > parent's child's (renderer) containing block. Ideally these functions would be internal to RenderObject/Element and the member functions would have the expected behavior. Most people just need to call containingBlock() anyway. Can we do it that way?
Comment on attachment 278808 [details] Patch Attachment 278808 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1312903 New failing tests: fast/text/find-hidden-text.html
Created attachment 278814 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 278808 [details] Patch Attachment 278808 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1312932 New failing tests: fast/block/positioning/crash-when-positioned-inline-has-positioned-child.html
Created attachment 278816 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #5) > (In reply to comment #3) > > The behavior changes are intended. > > In this case they should be carefully described in the changelog, and > ideally tested by layout tests. > > > The name "containingBlockForAbsolutePosition" suggests that when you pass in > > a renderer, it returns its containing block. The fact that you need to pass > > in the parent is not even indicated by the name of the 'in' parameter. There > > are already callsites that missed this (->behavior change related changes). > > They call the functions with the renderer instead of its parent. -and if you > > call it with a containing block renderer, you probably don't expect it to > > return itself. > > I am not convinced that renaming the functions is the right approach here as > > what we need is a function that takes a renderer and returns its containing > > block and not a function that take a renderer's parent and returns the the > > parent's child's (renderer) containing block. > > Ideally these functions would be internal to RenderObject/Element and the > member functions would have the expected behavior. Most people just need to > call containingBlock() anyway. Can we do it that way? Sure. (They were internal to RenderElement before r198701 but we moved them out of the class and made them free functions. -security considerations)
(In reply to comment #10) > (In reply to comment #5) > > (In reply to comment #3) > > > The behavior changes are intended. > > > > In this case they should be carefully described in the changelog, and > > ideally tested by layout tests. > > > > > The name "containingBlockForAbsolutePosition" suggests that when you pass in > > > a renderer, it returns its containing block. The fact that you need to pass > > > in the parent is not even indicated by the name of the 'in' parameter. There > > > are already callsites that missed this (->behavior change related changes). > > > They call the functions with the renderer instead of its parent. -and if you > > > call it with a containing block renderer, you probably don't expect it to > > > return itself. > > > I am not convinced that renaming the functions is the right approach here as > > > what we need is a function that takes a renderer and returns its containing > > > block and not a function that take a renderer's parent and returns the the > > > parent's child's (renderer) containing block. > > > > Ideally these functions would be internal to RenderObject/Element and the > > member functions would have the expected behavior. Most people just need to > > call containingBlock() anyway. Can we do it that way? > Sure. > (They were internal to RenderElement before r198701 but we moved them out of > the class and made them free functions. -security considerations) Well, they were public members.
Created attachment 278963 [details] Patch
Comment on attachment 278963 [details] Patch Attachment 278963 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1324953 New failing tests: http/tests/performance/performance-resource-timing-cached-entries.html fast/text/find-hidden-text.html
Created attachment 278964 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 278966 [details] Patch
4am patches!
(In reply to comment #16) > 4am patches! Important patches are important!
Comment on attachment 278966 [details] Patch Clearing flags on attachment: 278966 Committed r200953: <http://trac.webkit.org/changeset/200953>
All reviewed patches have been landed. Closing bug.