Bug 157659 – containingBlockFor*Position functions should take the renderer instead of the parent.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

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.

[alan]

Created attachment 278808 [details]
Patch

[Simon Fraser (smfr)]

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.

[alan]

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.

[alan]

Also, no more ugly const_cast<RenderBlock*>s!

[Simon Fraser (smfr)]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[alan]

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

[alan]

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

[alan]

Created attachment 278963 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[alan]

Created attachment 278966 [details]
Patch

[Simon Fraser (smfr)]

4am patches!

[alan]

(In reply to comment #16)
> 4am patches!
Important patches are important!

[WebKit Commit Bot]

Comment on attachment 278966 [details]
Patch

Clearing flags on attachment: 278966

Committed r200953: <http://trac.webkit.org/changeset/200953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.