Bug 157659 - containingBlockFor*Position functions should take the renderer instead of the parent.
Summary: containingBlockFor*Position functions should take the renderer instead of the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-12 20:10 PDT by zalan
Modified: 2016-05-16 11:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.68 KB, patch)
2016-05-12 20:47 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (682.15 KB, application/zip)
2016-05-12 21:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.45 MB, application/zip)
2016-05-12 22:04 PDT, Build Bot
no flags Details
Patch (12.72 KB, patch)
2016-05-15 04:13 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (697.55 KB, application/zip)
2016-05-15 05:11 PDT, Build Bot
no flags Details
Patch (12.72 KB, patch)
2016-05-15 06:54 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2016-05-12 20:10:44 PDT
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.
Comment 1 zalan 2016-05-12 20:47:27 PDT
Created attachment 278808 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-05-12 20:59:28 PDT
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.
Comment 3 zalan 2016-05-12 21:21:12 PDT
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.
Comment 4 zalan 2016-05-12 21:39:04 PDT
Also, no more ugly const_cast<RenderBlock*>s!
Comment 5 Simon Fraser (smfr) 2016-05-12 21:40:11 PDT
(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 6 Build Bot 2016-05-12 21:46:15 PDT
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
Comment 7 Build Bot 2016-05-12 21:46:19 PDT
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 8 Build Bot 2016-05-12 22:04:34 PDT
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
Comment 9 Build Bot 2016-05-12 22:04:38 PDT
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
Comment 10 zalan 2016-05-12 22:23:40 PDT
(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)
Comment 11 zalan 2016-05-12 22:25:16 PDT
(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.
Comment 12 zalan 2016-05-15 04:13:03 PDT
Created attachment 278963 [details]
Patch
Comment 13 Build Bot 2016-05-15 05:11:16 PDT
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
Comment 14 Build Bot 2016-05-15 05:11:20 PDT
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
Comment 15 zalan 2016-05-15 06:54:54 PDT
Created attachment 278966 [details]
Patch
Comment 16 Simon Fraser (smfr) 2016-05-15 08:39:23 PDT
4am patches!
Comment 17 zalan 2016-05-15 10:40:02 PDT
(In reply to comment #16)
> 4am patches!
Important patches are important!
Comment 18 WebKit Commit Bot 2016-05-16 11:43:05 PDT
Comment on attachment 278966 [details]
Patch

Clearing flags on attachment: 278966

Committed r200953: <http://trac.webkit.org/changeset/200953>
Comment 19 WebKit Commit Bot 2016-05-16 11:43:13 PDT
All reviewed patches have been landed.  Closing bug.