Bug 93170 – Inline continuations create :after generated content on style recalcs

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Elliott Sprehn]

Created attachment 156496 [details]
Reduction

Given <span><div></div></span> with :after content  on span a style recalc will cause the insertion of the content into the continuations in the div.

[Elliott Sprehn]

Created attachment 156497 [details]
Counter badness

[Elliott Sprehn]

Created attachment 156499 [details]
Merge continuation multiple :after

This show badness where the :after content from the continuation chain is merged back into the original element since we assume the continuation isn't needed anymore, but since the content is duplication you end up with :after anonymous blocks in places where they shouldn't be (in the middle of the RenderObjectChildList).

I can't figure out a way to make this crash yet, but having generated :after anonymous blocks in the middle of the children, and having more than one of them, seems like a recipe for disaster.

[Abhishek Arya]

can you paste the rendertree before and after the style recalc. this is really interesting and i think i might have an idea about what is going wrong. I remember sometime back i discovered a bug in this area. anyway, lets discuss this in person.

void RenderInline::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
{
    RenderBoxModelObject::styleDidChange(diff, oldStyle);

    // Ensure that all of the split inlines pick up the new style. We
    // only do this if we're an inline, since we don't want to propagate
    // a block's style to the other inlines.
    // e.g., <font>foo <h4>goo</h4> moo</font>.  The <font> inlines before
    // and after the block share the same style, but the block doesn't
    // need to pass its style on to anyone else.
    RenderStyle* newStyle = style();
    RenderInline* continuation = inlineElementContinuation();
    for (RenderInline* currCont = continuation; currCont; currCont = currCont->inlineElementContinuation()) {
        RenderBoxModelObject* nextCont = currCont->continuation();
        currCont->setContinuation(0);
        currCont->setStyle(newStyle);
        currCont->setContinuation(nextCont);
    }

[Abhishek Arya]

(In reply to comment #0)
> Created an attachment (id=156496) [details]
> Reduction
> 
> Given <span><div></div></span> with :after content  on span a style recalc will cause the insertion of the content into the continuations in the div.

the initial rendertree should be like

RenderBlock(anonymous)
   RenderInline (span)
RenderBlock(anonymous)
   RenderBlock (div)
RenderBlock(anonymous)
  RenderInline(span)
     RenderWhatever (for :after content)

After the style recalc where does the badness happen ?

[Abhishek Arya]

*** Bug 93707 has been marked as a duplicate of this bug. ***

[Takashi Sakamoto]

Created attachment 166444 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #4)
> (In reply to comment #0)
> > Created an attachment (id=156496) [details] [details]
> > Reduction
> > 
> > Given <span><div></div></span> with :after content  on span a style recalc will cause the insertion of the content into the continuations in the div.
> 
> the initial rendertree should be like
> 
> RenderBlock(anonymous)
>    RenderInline (span)
> RenderBlock(anonymous)
>    RenderBlock (div)
> RenderBlock(anonymous)
>   RenderInline(span)
>      RenderWhatever (for :after content)
> 
> After the style recalc where does the badness happen ?

I ran DumpRenderTree for the following html :
<style>
    span {
        display: inline;
    }
    span:before {
        content: "A";
    }
    span:after {
        content: "B";
    }
</style>
<span>
  <div></div>
  <div></div>
</span>

Since ":before" and ":after" exist, the result looks like:

  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584
      RenderBlock (anonymous) at (0,0) size 784x20
        RenderInline {SPAN} at (0,0) size 11x19
          RenderInline (generated) at (0,0) size 11x19
            RenderText at (0,0) size 11x19
              text run at (0,0) width 11: "A"
          RenderText {#text} at (0,0) size 0x0
      RenderBlock (anonymous) at (0,20) size 784x0
        RenderBlock {DIV} at (0,0) size 784x0
      RenderBlock (anonymous) at (0,20) size 784x0
        RenderInline {SPAN} at (0,0) size 0x0
      RenderBlock (anonymous) at (0,20) size 784x0
        RenderBlock {DIV} at (0,0) size 784x0
      RenderBlock (anonymous) at (0,20) size 784x20
        RenderInline {SPAN} at (0,0) size 11x19
          RenderInline (generated) at (0,0) size 11x19
            RenderText at (0,0) size 11x19
              text run at (0,0) width 11: "B"

So there are 3 RenderInline{SPAN}, because two DIVs were inserted into inline {SPAN}. 
(Each DIV splits the last RenderInline{SPAN} into two RenderInline{SPAN}s by using RenderBlock (anonymous). If <style> has neither :BEFORE nor :AFTER, the "split" doesn't occur.)

When RenderInline::styleDidChange is invoked, because of setContinuation(0), there is no way to know whether the given continuation is last one or not. So...

 RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584
      RenderBlock (anonymous) at (0,0) size 784x20
        RenderInline {SPAN} at (0,0) size 11x19
          RenderInline (generated) at (0,0) size 11x19
            RenderText at (0,0) size 11x19
              text run at (0,0) width 11: "A"
          RenderText {#text} at (0,0) size 0x0
      RenderBlock (anonymous) at (0,20) size 784x0
        RenderBlock {DIV} at (0,0) size 784x0
      RenderBlock (anonymous) at (0,20) size 784x0
        RenderInline {SPAN} at (0,0) size 0x0
+          RenderInline (generated) at (0,0) size 11x19
+            RenderText at (0,0) size 11x19
+              text run at (0,0) width 11: "B"
      RenderBlock (anonymous) at (0,20) size 784x0
        RenderBlock {DIV} at (0,0) size 784x0
      RenderBlock (anonymous) at (0,20) size 784x20
        RenderInline {SPAN} at (0,0) size 11x19
          RenderInline (generated) at (0,0) size 11x19
            RenderText at (0,0) size 11x19
              text run at (0,0) width 11: "B"

Best regards,
Takashi Sakamoto

[Abhishek Arya]

Comment on attachment 166444 [details]
Patch

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

> Source/WebCore/rendering/RenderInline.cpp:176
> +        if (hasPseudoAfter && !currCont->inlineElementContinuation())

These two lines are unneeded i think since you update :before, :after content later.

197 if (!isAnonymous() && document()->styleSheetCollection()->usesBeforeAfterRules()) {
198        children()->updateBeforeAfterContent(this, BEFORE);
199        children()->updateBeforeAfterContent(this, AFTER);
200    }

> Source/WebCore/rendering/RenderInline.cpp:182
> +    if (hasPseudoAfter)

I don't like the setting/unsetting :after style hack. Also, i don't think we want to create :before content here. So, it should be better to have like a global bool in RenderObjectChildList and then use it to bail out in RenderObjectChildList::updateBeforeAfterContent. Then you can set/unset it here with a TemporaryChange around the for loop. Basically the gist is we shouldn't be updating our :before, :after while we are inside the for loop.

[Takashi Sakamoto]

Created attachment 166603 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 166444 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/rendering/RenderInline.cpp:176
>> +        if (hasPseudoAfter && !currCont->inlineElementContinuation())
> 
> These two lines are unneeded i think since you update :before, :after content later.
> 
> 197 if (!isAnonymous() && document()->styleSheetCollection()->usesBeforeAfterRules()) {
> 198        children()->updateBeforeAfterContent(this, BEFORE);
> 199        children()->updateBeforeAfterContent(this, AFTER);
> 200    }

I see.
This is my mistake.

>> Source/WebCore/rendering/RenderInline.cpp:182
>> +    if (hasPseudoAfter)
> 
> I don't like the setting/unsetting :after style hack. Also, i don't think we want to create :before content here. So, it should be better to have like a global bool in RenderObjectChildList and then use it to bail out in RenderObjectChildList::updateBeforeAfterContent. Then you can set/unset it here with a TemporaryChange around the for loop. Basically the gist is we shouldn't be updating our :before, :after while we are inside the for loop.

I see.
I added a new static method, i.e. enableUpdateBeforeAfterContent to class RenderObjectChildList to control creating :before / :after content.

[Abhishek Arya]

Comment on attachment 166603 [details]
Patch

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

One more round.

> Source/WebCore/rendering/RenderInline.cpp:169
> +    bool updateEnabled = RenderObjectChildList::enableUpdateBeforeAfterContent(false);

you can use TemporaryChange to scope this easily. see comment below.

> Source/WebCore/rendering/RenderObjectChildList.cpp:443
> +    if (!m_enableUpdateBeforeAfterContent)

Why so late return ? Why not after the line " if (owner->style()->styleType() == BEFORE || owner->style()->styleType() == AFTER)"

> Source/WebCore/rendering/RenderObjectChildList.h:59
> +    static bool enableUpdateBeforeAfterContent(bool enable)

You don't need this. Just use TemporaryChange.h, see http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Tools/TestWebKitAPI/Tests/WTF/TemporaryChange.cpp&exact_package=chromium&q=%22TemporaryChange%3Cbool%22&type=cs&l=36. Make m_enableUpdateBeforeAfterContent a global static that could be manipulated by  RenderInline::styleDidChange.

[Takashi Sakamoto]

Created attachment 166612 [details]
Patch

[Takashi Sakamoto]

Created attachment 166633 [details]
Patch

[Takashi Sakamoto]

I'm sorry. I said wrong things.
I looked at my old patch for bug 93707 again and found that we need to enable updating :after content during curr->setStyle.
To show why we need, I added one more layout test to this patch. The test updates className to apply / not to apply :after to inline elements. If we disable updating :after content in curr->setStyle, no generated renderer for :after content is created.

Best regards,
Takashi Sakamoto

(In reply to comment #11)
> (From update of attachment 166603 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166603&action=review
> 
> One more round.
> 
> > Source/WebCore/rendering/RenderInline.cpp:169
> > +    bool updateEnabled = RenderObjectChildList::enableUpdateBeforeAfterContent(false);
> 
> you can use TemporaryChange to scope this easily. see comment below.
> 
> > Source/WebCore/rendering/RenderObjectChildList.cpp:443
> > +    if (!m_enableUpdateBeforeAfterContent)
> 
> Why so late return ? Why not after the line " if (owner->style()->styleType() == BEFORE || owner->style()->styleType() == AFTER)"
> 
> > Source/WebCore/rendering/RenderObjectChildList.h:59
> > +    static bool enableUpdateBeforeAfterContent(bool enable)
> 
> You don't need this. Just use TemporaryChange.h, see http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Tools/TestWebKitAPI/Tests/WTF/TemporaryChange.cpp&exact_package=chromium&q=%22TemporaryChange%3Cbool%22&type=cs&l=36. Make m_enableUpdateBeforeAfterContent a global static that could be manipulated by  RenderInline::styleDidChange.

[Takashi Sakamoto]

I forgot to say about :before content.

The reason why we don't need to do anything for :before content in RenderInline::styleDidChange is that it is easy to know whether the given render object is the top renderer of continuation list.
Just compare renderer with renderer->node()->renderer(). This is not affected by setContinuation(0).

Best regards,
Takashi Sakamoto

(In reply to comment #14)
> I'm sorry. I said wrong things.
> I looked at my old patch for bug 93707 again and found that we need to enable updating :after content during curr->setStyle.
> To show why we need, I added one more layout test to this patch. The test updates className to apply / not to apply :after to inline elements. If we disable updating :after content in curr->setStyle, no generated renderer for :after content is created.
> 
> Best regards,
> Takashi Sakamoto
> 
> (In reply to comment #11)
> > (From update of attachment 166603 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=166603&action=review
> > 
> > One more round.
> > 
> > > Source/WebCore/rendering/RenderInline.cpp:169
> > > +    bool updateEnabled = RenderObjectChildList::enableUpdateBeforeAfterContent(false);
> > 
> > you can use TemporaryChange to scope this easily. see comment below.
> > 
> > > Source/WebCore/rendering/RenderObjectChildList.cpp:443
> > > +    if (!m_enableUpdateBeforeAfterContent)
> > 
> > Why so late return ? Why not after the line " if (owner->style()->styleType() == BEFORE || owner->style()->styleType() == AFTER)"
> > 
> > > Source/WebCore/rendering/RenderObjectChildList.h:59
> > > +    static bool enableUpdateBeforeAfterContent(bool enable)
> > 
> > You don't need this. Just use TemporaryChange.h, see http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Tools/TestWebKitAPI/Tests/WTF/TemporaryChange.cpp&exact_package=chromium&q=%22TemporaryChange%3Cbool%22&type=cs&l=36. Make m_enableUpdateBeforeAfterContent a global static that could be manipulated by  RenderInline::styleDidChange.

[Takashi Sakamoto]

I wrote the document to explain this patch:

https://docs.google.com/document/d/1YsXo7dAs7EOdckXy27Z8b9KlydT1p5SZH0fARudkFoI/edit


(In reply to comment #15)
> I forgot to say about :before content.
> 
> The reason why we don't need to do anything for :before content in RenderInline::styleDidChange is that it is easy to know whether the given render object is the top renderer of continuation list.
> Just compare renderer with renderer->node()->renderer(). This is not affected by setContinuation(0).
> 
> Best regards,
> Takashi Sakamoto
> 
> (In reply to comment #14)
> > I'm sorry. I said wrong things.
> > I looked at my old patch for bug 93707 again and found that we need to enable updating :after content during curr->setStyle.
> > To show why we need, I added one more layout test to this patch. The test updates className to apply / not to apply :after to inline elements. If we disable updating :after content in curr->setStyle, no generated renderer for :after content is created.
> > 
> > Best regards,
> > Takashi Sakamoto
> > 
> > (In reply to comment #11)
> > > (From update of attachment 166603 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=166603&action=review
> > > 
> > > One more round.
> > > 
> > > > Source/WebCore/rendering/RenderInline.cpp:169
> > > > +    bool updateEnabled = RenderObjectChildList::enableUpdateBeforeAfterContent(false);
> > > 
> > > you can use TemporaryChange to scope this easily. see comment below.
> > > 
> > > > Source/WebCore/rendering/RenderObjectChildList.cpp:443
> > > > +    if (!m_enableUpdateBeforeAfterContent)
> > > 
> > > Why so late return ? Why not after the line " if (owner->style()->styleType() == BEFORE || owner->style()->styleType() == AFTER)"
> > > 
> > > > Source/WebCore/rendering/RenderObjectChildList.h:59
> > > > +    static bool enableUpdateBeforeAfterContent(bool enable)
> > > 
> > > You don't need this. Just use TemporaryChange.h, see http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Tools/TestWebKitAPI/Tests/WTF/TemporaryChange.cpp&exact_package=chromium&q=%22TemporaryChange%3Cbool%22&type=cs&l=36. Make m_enableUpdateBeforeAfterContent a global static that could be manipulated by  RenderInline::styleDidChange.

[Abhishek Arya]

Comment on attachment 166633 [details]
Patch

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

> Source/WebCore/rendering/RenderInline.cpp:177
> +            RenderObjectChildList::m_enableUpdateAfterContent = !nextTarget;

You don't need lines 176 and 177. As i said in last comment, you are explicitly updating :before, :after content later in function, so it will place it correctly at end.

nextTarget = currCont->inlineElementContinuation();
RenderObjectChildList::m_enableUpdateAfterContent = !nextTarget;

> Source/WebCore/rendering/RenderObjectChildList.cpp:379
> +    if (type == AFTER && !m_enableUpdateAfterContent)

You don't need to just check AFTER, make it generic and rename m_enableUpdateAfterContent to s_enableUpdateBeforeAfterContent. For :before content, you will bail early here instead of wasting time and going till " if (newContentWanted && type == BEFORE && owner->isElementContinuation())"

> Source/WebCore/rendering/RenderObjectChildList.h:64
> +    static bool m_enableUpdateAfterContent;

rename to s_enableUpdateBeforeAfterContent. we s_ for statics.

[Abhishek Arya]

(In reply to comment #15)
> I forgot to say about :before content.
> 
> The reason why we don't need to do anything for :before content in RenderInline::styleDidChange is that it is easy to know whether the given render object is the top renderer of continuation list.
> Just compare renderer with renderer->node()->renderer(). This is not affected by setContinuation(0).

Yeah i know it will bail out on "if (newContentWanted && type == BEFORE && owner->isElementContinuation())", but we will waste on every continuation calculating before content, when we could bail out early.
switch (type) {
    case BEFORE:
        child = beforePseudoElementRenderer(owner);
        break;

> 
> Best regards,
> Takashi Sakamoto
> 
> (In reply to comment #14)
> > I'm sorry. I said wrong things.
> > I looked at my old patch for bug 93707 again and found that we need to enable updating :after content during curr->setStyle.
> > To show why we need, I added one more layout test to this patch. The test updates className to apply / not to apply :after to inline elements. If we disable updating :after content in curr->setStyle, no generated renderer for :after content is created.
> > 
> > Best regards,
> > Takashi Sakamoto
> > 
> > (In reply to comment #11)
> > > (From update of attachment 166603 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=166603&action=review
> > > 
> > > One more round.
> > > 
> > > > Source/WebCore/rendering/RenderInline.cpp:169
> > > > +    bool updateEnabled = RenderObjectChildList::enableUpdateBeforeAfterContent(false);
> > > 
> > > you can use TemporaryChange to scope this easily. see comment below.
> > > 
> > > > Source/WebCore/rendering/RenderObjectChildList.cpp:443
> > > > +    if (!m_enableUpdateBeforeAfterContent)
> > > 
> > > Why so late return ? Why not after the line " if (owner->style()->styleType() == BEFORE || owner->style()->styleType() == AFTER)"
> > > 
> > > > Source/WebCore/rendering/RenderObjectChildList.h:59
> > > > +    static bool enableUpdateBeforeAfterContent(bool enable)
> > > 
> > > You don't need this. Just use TemporaryChange.h, see http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Tools/TestWebKitAPI/Tests/WTF/TemporaryChange.cpp&exact_package=chromium&q=%22TemporaryChange%3Cbool%22&type=cs&l=36. Make m_enableUpdateBeforeAfterContent a global static that could be manipulated by  RenderInline::styleDidChange.

[Takashi Sakamoto]

Created attachment 166809 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 166633 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/rendering/RenderInline.cpp:177
>> +            RenderObjectChildList::m_enableUpdateAfterContent = !nextTarget;
> 
> You don't need lines 176 and 177. As i said in last comment, you are explicitly updating :before, :after content later in function, so it will place it correctly at end.
> 
> nextTarget = currCont->inlineElementContinuation();
> RenderObjectChildList::m_enableUpdateAfterContent = !nextTarget;

Firstly I think so. However I found that I need lines 176 and 177. Talking about the first continuation (= node()->renderer()), I agree that we don't need.
However talking about the rest of the continuation list, this RenderInline::styleDidChange is invoked via line 180's currCont->setStyle(newStyle). 
In such case,  later updating :before and :after content cannot see whether the RenderInline instance is the last node of continuation list. 

I think, currently there is no way to directly invoke RenderInline::styleDidChange for the second, the third continuation ...

>> Source/WebCore/rendering/RenderObjectChildList.cpp:379
>> +    if (type == AFTER && !m_enableUpdateAfterContent)
> 
> You don't need to just check AFTER, make it generic and rename m_enableUpdateAfterContent to s_enableUpdateBeforeAfterContent. For :before content, you will bail early here instead of wasting time and going till " if (newContentWanted && type == BEFORE && owner->isElementContinuation())"

I see. 
I added the flag to just enable/disable updating :after content, but lines 175-182 in RenderInline.cpp, we don't need to update :BEFORE. Because the continuations updated in line 175-182 cannot have :before content style.
Done.

>> Source/WebCore/rendering/RenderObjectChildList.h:64
>> +    static bool m_enableUpdateAfterContent;
> 
> rename to s_enableUpdateBeforeAfterContent. we s_ for statics.

Thanks.
Done.

[Abhishek Arya]

(In reply to comment #20)
> (From update of attachment 166633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166633&action=review
> 
> Thank you for reviewing.
> 
> >> Source/WebCore/rendering/RenderInline.cpp:177
> >> +            RenderObjectChildList::m_enableUpdateAfterContent = !nextTarget;
> > 
> > You don't need lines 176 and 177. As i said in last comment, you are explicitly updating :before, :after content later in function, so it will place it correctly at end.
> > 
> > nextTarget = currCont->inlineElementContinuation();
> > RenderObjectChildList::m_enableUpdateAfterContent = !nextTarget;
> 
> Firstly I think so. However I found that I need lines 176 and 177. Talking about the first continuation (= node()->renderer()), I agree that we don't need.
> However talking about the rest of the continuation list, this RenderInline::styleDidChange is invoked via line 180's currCont->setStyle(newStyle). 
> In such case,  later updating :before and :after content cannot see whether the RenderInline instance is the last node of continuation list. 
> 
> I think, currently there is no way to directly invoke RenderInline::styleDidChange for the second, the third continuation ...

Yes, you are right, we need to enable it for the last continuation in the chain to update :after content. Yeah upload another patch for the variable name change nit and adding comment.

> 
> >> Source/WebCore/rendering/RenderObjectChildList.cpp:379
> >> +    if (type == AFTER && !m_enableUpdateAfterContent)
> > 
> > You don't need to just check AFTER, make it generic and rename m_enableUpdateAfterContent to s_enableUpdateBeforeAfterContent. For :before content, you will bail early here instead of wasting time and going till " if (newContentWanted && type == BEFORE && owner->isElementContinuation())"
> 
> I see. 
> I added the flag to just enable/disable updating :after content, but lines 175-182 in RenderInline.cpp, we don't need to update :BEFORE. Because the continuations updated in line 175-182 cannot have :before content style.
> Done.

Exactly we know in the loop, we won't be updating :before content, so no need to waste time.

> 
> >> Source/WebCore/rendering/RenderObjectChildList.h:64
> >> +    static bool m_enableUpdateAfterContent;
> > 
> > rename to s_enableUpdateBeforeAfterContent. we s_ for statics.
> 
> Thanks.
> Done.

[Takashi Sakamoto]

Created attachment 166815 [details]
Patch

[Elliott Sprehn]

Comment on attachment 166809 [details]
Patch

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

> Source/WebCore/rendering/RenderInline.cpp:177
> +            RenderObjectChildList::s_enableUpdateBeforeAfterContent = !nextTarget;

This naming isn't clear. It's not a target of anything and that naming is confusing in the DOM/Rendering area. Could we use next and current instead?

> Source/WebCore/rendering/RenderInline.cpp:181
> +            currCont->setContinuation(nextCont);

Is it ever possible that nextCont is not null, but nextTarget is (meaning nextCont is not isInline())? Looking at the code it doesn't seem like you could have a continuation chain that ends in a non-inline one here which means you could simplify this loop a bunch.

Try adding ASSERT(!current->continuation() || current->inlineElementContinuation()); to this loop and running the tests.

> LayoutTests/fast/css-generated-content/after-with-inline-continuation.html:45
> +<!-- If test passes, you should see A A A B B B. -->

Same, this should be inline like the original test.

> LayoutTests/fast/css-generated-content/dynamic-apply-after-for-inline.html:23
> +        ul.className = '';

Replace all this with ul.classList.toggle('closed')

> LayoutTests/fast/css-generated-content/dynamic-apply-after-for-inline.html:45
> +<!-- If test passes, only 1 'before' and 1 'after' are shown. -->

Can you put this text inline instead of in a comment?

<p>....</p> It's much easier to tell what's supposed to be going on.

[Abhishek Arya]

(In reply to comment #23)
> (From update of attachment 166809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166809&action=review
> 
> > Source/WebCore/rendering/RenderInline.cpp:177
> > +            RenderObjectChildList::s_enableUpdateBeforeAfterContent = !nextTarget;
> 
> This naming isn't clear. It's not a target of anything and that naming is confusing in the DOM/Rendering area. Could we use next and current instead?

He fixed that in last patch. i had told him informally over chat.

> 
> > Source/WebCore/rendering/RenderInline.cpp:181
> > +            currCont->setContinuation(nextCont);
> 
> Is it ever possible that nextCont is not null, but nextTarget is (meaning nextCont is not isInline())? Looking at the code it doesn't seem like you could have a continuation chain that ends in a non-inline one here which means you could simplify this loop a bunch.
> 
> Try adding ASSERT(!current->continuation() || current->inlineElementContinuation()); to this loop and running the tests.

We cannot have non-inline as the last continuation. These continuations are created for the blocks inside inline case [Blocks inside Inline Flows in https://www.webkit.org/blog/115/webcore-rendering-ii-blocks-and-inlines/]. Also, i did thought about simplifying the loop but it does not look straightforward, we need to jump to every next inline element continuation [so it will skip over the block inside inline and also update styles of the inline continuation], whereas the nextCont is always the next continuation which can be the block inside inline or the inline continuation. What was your idea on the simplification ?

> 
> > LayoutTests/fast/css-generated-content/after-with-inline-continuation.html:45
> > +<!-- If test passes, you should see A A A B B B. -->
> 

Thanks for the layout test suggestions.

> Same, this should be inline like the original test.
> 
> > LayoutTests/fast/css-generated-content/dynamic-apply-after-for-inline.html:23
> > +        ul.className = '';
> 
> Replace all this with ul.classList.toggle('closed')
> 
> > LayoutTests/fast/css-generated-content/dynamic-apply-after-for-inline.html:45
> > +<!-- If test passes, only 1 'before' and 1 'after' are shown. -->
> 
> Can you put this text inline instead of in a comment?
> 
> <p>....</p> It's much easier to tell what's supposed to be going on.

[Abhishek Arya]

Comment on attachment 166815 [details]
Patch

Don't think we should hold on the patch based on the layout test suggestions. The other question i did seem to answer. So, this one going to cq+

[WebKit Review Bot]

Comment on attachment 166815 [details]
Patch

Clearing flags on attachment: 166815

Committed r130555: <http://trac.webkit.org/changeset/130555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.