Bug 150908

Summary: Continuations with anonymous wrappers inside misplaces child renderers.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, hyatt, kondapallykalyan, simon.fraser
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test reduction
none
testing remove permutation
none
Patch
none
Patch
none
Patch commit-queue: commit-queue-

Description zalan 2015-11-04 15:33:00 PST
Created attachment 264821 [details]
test reduction

see attached test case.
Comment 1 zalan 2015-11-04 15:34:15 PST
Created attachment 264822 [details]
testing remove permutation
Comment 2 zalan 2015-11-04 20:19:05 PST
Created attachment 264840 [details]
Patch
Comment 3 Darin Adler 2015-11-06 09:29:06 PST
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".
Comment 4 Said Abou-Hallawa 2015-11-06 11:42:47 PST
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();
}
Comment 5 zalan 2015-11-07 21:06:26 PST
(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.
Comment 6 zalan 2015-11-08 14:36:16 PST
Created attachment 265023 [details]
Patch
Comment 7 zalan 2015-11-08 14:37:30 PST
(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.
Comment 8 Darin Adler 2015-11-09 08:49:59 PST
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?
Comment 9 zalan 2015-11-10 13:35:24 PST
Created attachment 265224 [details]
Patch
Comment 10 zalan 2015-11-10 13:35:56 PST
(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!
Comment 11 WebKit Commit Bot 2015-11-10 15:16:18 PST
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
Comment 12 zalan 2015-11-10 15:18:51 PST
Committed r192275: <http://trac.webkit.org/changeset/192275>
Comment 13 zalan 2015-11-11 08:57:50 PST
rdar://problem/23251240