Bug 150908 – Continuations with anonymous wrappers inside misplaces child renderers.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[alan]

Created attachment 264821 [details]
test reduction

see attached test case.

[alan]

Created attachment 264822 [details]
testing remove permutation

[alan]

Created attachment 264840 [details]
Patch

[Darin Adler]

Comment on attachment 264840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264840&action=review

I can’t review, Hyatt probably needs to, but I have spelling and other similar comments.

> Source/WebCore/rendering/RenderInline.cpp:433
> +            RenderElement* anonymousParent = rendererToMove->parent();

I would consider auto* here instead of naming the type, since we’d like to use as specific a type as possible.

> Source/WebCore/rendering/RenderInline.cpp:444
> +                // Reparent the whole anon wrapper tree.

I don’t think “anon” is a helpful abbreviation here. Lets spell out the word.

> Source/WebCore/rendering/RenderInline.cpp:455
> +            // FIXME: When the anon wrapper has mutliple children, we end up traversing up to the topmost wrapper

Typo: "mutliple" instead of "multiple".

> Source/WebCore/rendering/RenderInline.cpp:497
> +            RenderObject* renderer = currentChild->nextSibling();
>              while (renderer) {

I suggest we write this as a for loop, even if the “advance to next” part is empty because it’s done inside the end of the loop rather than at the end:

    for (auto* renderer = currentChild->nextSibling(); renderer; ) {

> Source/WebCore/rendering/RenderInline.cpp:498
>                  RenderObject* tmp = renderer;

The name “tmp” here is really lame.

> Source/WebCore/rendering/RenderInline.cpp:522
> +    RenderObject* renderer = currentChild->nextSibling();
>      while (renderer) {
>          RenderObject* tmp = renderer;

Same comment, for loop is better, tmp is lame.

> Source/WebCore/rendering/RenderInline.cpp:598
> +    RenderBoxModelObject* beforeChildAnchestor = nullptr;

Typo here: "anchestor" instead of "ancestor".

> Source/WebCore/rendering/RenderInline.cpp:599
> +    // In case of anon wrappers, the parent of the beforeChild is mostly irrelevant. What we need is

More "anon".

[Said Abou-Hallawa]

Comment on attachment 264840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264840&action=review

>> Source/WebCore/rendering/RenderInline.cpp:497
>>              while (renderer) {
> 
> I suggest we write this as a for loop, even if the “advance to next” part is empty because it’s done inside the end of the loop rather than at the end:
> 
>     for (auto* renderer = currentChild->nextSibling(); renderer; ) {

I also like to delete the nodes from the end if possible. Since current is not null and it is the parent of currentChild, this loop may be written like this:

for (auto* renderer = current->lastChild(); renderer != currentChild; renderer = renderer->previousSibling()) {
    currentInline.removeChildInternal(*renderer, NotifyChildren);
    cloneInline->addChildIgnoringContinuation(renderer);
    renderer->setNeedsLayoutAndPrefWidthsRecalc();
}

[alan]

(In reply to comment #4)
> Comment on attachment 264840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264840&action=review
> 
> >> Source/WebCore/rendering/RenderInline.cpp:497
> >>              while (renderer) {
> > 
> > I suggest we write this as a for loop, even if the “advance to next” part is empty because it’s done inside the end of the loop rather than at the end:
> > 
> >     for (auto* renderer = currentChild->nextSibling(); renderer; ) {
> 
> I also like to delete the nodes from the end if possible. Since current is
> not null and it is the parent of currentChild, this loop may be written like
> this:
> 
> for (auto* renderer = current->lastChild(); renderer != currentChild;
> renderer = renderer->previousSibling()) {
>     currentInline.removeChildInternal(*renderer, NotifyChildren);
>     cloneInline->addChildIgnoringContinuation(renderer);
>     renderer->setNeedsLayoutAndPrefWidthsRecalc();
> }
I don't see any reason why removing from the end is better. -and since we are reparenting the renderers, it would revers the sibling order.

[alan]

Created attachment 265023 [details]
Patch

[alan]

(In reply to comment #3)
> Comment on attachment 264840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264840&action=review
> 
> I can’t review, Hyatt probably needs to, but I have spelling and other
> similar comments.
> 
> > Source/WebCore/rendering/RenderInline.cpp:433
> > +            RenderElement* anonymousParent = rendererToMove->parent();
> 
> I would consider auto* here instead of naming the type, since we’d like to
> use as specific a type as possible.
> 
> > Source/WebCore/rendering/RenderInline.cpp:444
> > +                // Reparent the whole anon wrapper tree.
> 
> I don’t think “anon” is a helpful abbreviation here. Lets spell out the word.
> 
> > Source/WebCore/rendering/RenderInline.cpp:455
> > +            // FIXME: When the anon wrapper has mutliple children, we end up traversing up to the topmost wrapper
> 
> Typo: "mutliple" instead of "multiple".
> 
> > Source/WebCore/rendering/RenderInline.cpp:497
> > +            RenderObject* renderer = currentChild->nextSibling();
> >              while (renderer) {
> 
> I suggest we write this as a for loop, even if the “advance to next” part is
> empty because it’s done inside the end of the loop rather than at the end:
> 
>     for (auto* renderer = currentChild->nextSibling(); renderer; ) {
> 
> > Source/WebCore/rendering/RenderInline.cpp:498
> >                  RenderObject* tmp = renderer;
> 
> The name “tmp” here is really lame.
> 
> > Source/WebCore/rendering/RenderInline.cpp:522
> > +    RenderObject* renderer = currentChild->nextSibling();
> >      while (renderer) {
> >          RenderObject* tmp = renderer;
> 
> Same comment, for loop is better, tmp is lame.
> 
> > Source/WebCore/rendering/RenderInline.cpp:598
> > +    RenderBoxModelObject* beforeChildAnchestor = nullptr;
> 
> Typo here: "anchestor" instead of "ancestor".
> 
> > Source/WebCore/rendering/RenderInline.cpp:599
> > +    // In case of anon wrappers, the parent of the beforeChild is mostly irrelevant. What we need is
> 
> More "anon".
Done.

[Darin Adler]

Comment on attachment 265023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265023&action=review

> Source/WebCore/rendering/RenderInline.cpp:429
> +    RenderObject* rendererToMove = beforeChild;
> +    RenderObject* nextSibling = nullptr;
> +    while (rendererToMove) {

For loop would work well here.

    for (RenderObject* rendererToMove = beforeChild; rendererToMove; rendererToMove = nextSibling) {

But note that this works only because the nextSibling local variable is outside the loop, and I don’t understand why it is. It does not seem to be used after the loop. Instead we could declare it inside the loop and then leave off the third clause of the for. That would be just like the other loops.

> Source/WebCore/rendering/RenderInline.cpp:623
> -    bool bcpInline = beforeChildParent->isInline();
> +    bool bcpInline = beforeChildAncestor->isInline();

The name of this local variable, "bcpInline" was short for “before child parent is inline”. But it’s now “before child ancestor”, this relatively poor name is even worse. How about just writing it all the way out:

    bool beforeChildAncestorIsInline = beforeChildAncestor->isInline();

Or maybe not even using local variable at all?

[alan]

Created attachment 265224 [details]
Patch

[alan]

(In reply to comment #8)
> Comment on attachment 265023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265023&action=review
> 
> > Source/WebCore/rendering/RenderInline.cpp:429
> > +    RenderObject* rendererToMove = beforeChild;
> > +    RenderObject* nextSibling = nullptr;
> > +    while (rendererToMove) {
> 
> For loop would work well here.
> 
>     for (RenderObject* rendererToMove = beforeChild; rendererToMove;
> rendererToMove = nextSibling) {
> 
> But note that this works only because the nextSibling local variable is
> outside the loop, and I don’t understand why it is. It does not seem to be
> used after the loop. Instead we could declare it inside the loop and then
> leave off the third clause of the for. That would be just like the other
> loops.
> 
> > Source/WebCore/rendering/RenderInline.cpp:623
> > -    bool bcpInline = beforeChildParent->isInline();
> > +    bool bcpInline = beforeChildAncestor->isInline();
> 
> The name of this local variable, "bcpInline" was short for “before child
> parent is inline”. But it’s now “before child ancestor”, this relatively
> poor name is even worse. How about just writing it all the way out:
> 
>     bool beforeChildAncestorIsInline = beforeChildAncestor->isInline();
> 
> Or maybe not even using local variable at all?
Done. Thanks for the reviews!

[WebKit Commit Bot]

Comment on attachment 265224 [details]
Patch

Rejecting attachment 265224 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 265224, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/411951

[alan]

Committed r192275: <http://trac.webkit.org/changeset/192275>

[alan]

rdar://problem/23251240