Created attachment 160788[details]
Concept
We should move the generated content nodes into the DOM. This has the nice side effect of vastly simplifying the inside of the render tree in favor of centralized code for insert/remove from the DOM. From inside the render tree we should then be able to ignore if a node is from :before or an actual <span>.

Note: Trivially this could be implemented by making a subclass of Node that override previousSibling/nextSibling to go through parentNode removing the need for book keeping code to maintain the prev/next ptrs. Unfortunately those aren't virtual methods :(

Created attachment 166332[details]
WIP
WIP patch that attempts to move :before and :after into the DOM. This reuses the existing renderers for generated content so we can do this in multiple steps. The current issues are infinite loops in RenderCounter, broken CSS inheritance in CSS tables, and a bug with counters and styleRecalc causing crashes. On the plus side this patch fixes tons of layout bugs with generated content which is awesome.

Comment on attachment 166585[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=166585&action=review>> Source/WebCore/dom/Element.cpp:1214
>> + beforeContent->setNeedsStyleRecalc();
>
> Is this safe during recalcStyle? Will this crawl up and tell our parents, and possibyl our previous siblings they need style-recalc again?
I assumed it was safe because just a few lines below we do this on our children:
if ((forceCheckOfNextElementSibling || forceCheckOfAnyElementSibling))
element->setNeedsStyleRecalc();
I didn't see a difference either way in the tests I ran, but it seems like a recalc of our style should mean a recalc of the pseudo since they're derived from them.
>> Source/WebCore/dom/Element.cpp:1860
>> +}
>
> Why PassRefPtr? These functxions are not creating a new object, or expecting the caller to take ownership, are they?
I'll switch to bare ptrs on the return types.
>> Source/WebCore/dom/PseudoElement.cpp:79
>> + child->setStyle(renderStyle());
>
> So this assumes that we don't ever modify this style... since otherwise we'd modify our parent's style, or? Don't we normally just unconditionally inherit styles? I guess these are really siblings of the element in question instead of children...
This code is just copy and paste from RenderObjectChildList's updateBeforeAfterStyle so it might be wrong. If the PseudoElement had font-size: 50%; and then I inherit for the text here, do I get 25% or do I keep the 50%?
>> Source/WebCore/dom/PseudoElement.cpp:112
>> + image->setImageResource(RenderImageResourceStyleImage::create(const_cast<StyleImage*>(styleImage)));
>
> Why are these casts safe?
They're safe because the switch() checks the type, this switch is actually copy and pasted from RenderObjectChildList. They're pretty gross though... I'll put together a separate patch that fixes this while I continue here.
>> Source/WebCore/rendering/RenderObject.cpp:135
>> + image->setStyle(style);
>
> Are we now triggering a style change and causing badness?
Ack. This is a bad merge. I'll revert, I have a bunch of merge badness in this file it seems.

(In reply to comment #14)
> (From update of attachment 166585[details])
> Why aren't we just using the Shadow DOM plumbing? A lot of this code seems to duplicate it.
Which "a lot" are you talking about?

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 166585[details] [details])
> > Why aren't we just using the Shadow DOM plumbing? A lot of this code seems to duplicate it.
>
> Which "a lot" are you talking about?
Code in Element, Node, NodeRenderingContext, adding stuff to RareNodeData. Can we see if we can maybe reframe :before/:after in terms of shadow DOM? I would be happy to help.

Created attachment 166797[details]
Patch
The crash was some kind of heap corruption that comes from generated content in display: run-in; so Ive disabled support for that for now. This wont work quite right with Shadow DOM yet either, but as Dimitri mentioned we might take a different approach then, but Id like to get this in before that since Shadow DOM is all fluxy right now anyway. Either way this patch now passes all the tests except for RunIn ones (and a few where the patch fixed bugs)

Created attachment 167144[details]
Patch
This now passes all the tests except for ones that this fixes (yay). I don't have the build files updated for non-Chromium ports yet, but if someone wants to review this while I get that and the test output sorted...

Comment on attachment 167144[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=167144&action=review>> Source/WebCore/dom/ElementRareData.h:46
>> + void clearGeneratedContent();
>
> This doesn't seem to get called anywhere. Where do you delete generated content nodes when a selector no longer applies? Presumably we have some tests that cover this.
Woops, that method name is leftover from a previous iteration of the code. In Element::recalcStyle if we already have a :before or :after we call recalcStyle on it which would detach() the PseudoElement if there's no more content resulting in a PseudoElement without a renderer(). You'll see there's a check there:
afterContent->recalcStyle(change);
if (!afterContent->renderer())
setAfterPseudoElement(0); // free the after content.
We also remove it in Element::detach.

(In reply to comment #22)
> Created an attachment (id=167431) [details]
> Patch
I've identified some places where our tests are lacking or bugs this fixes that we need more tests (ex. animations work on :before and :after now) so I'll work on that, but code is good for review!

(In reply to comment #29)
> Created an attachment (id=167604) [details]
> Patch
This passes all tests except a few more than have to do with generated content on inputs:
[4/13] fast/forms/date-multiple-fields/date-multiple-fields-appearance-pseudo-elements.html failed unexpectedly (image diff)
[5/13] fast/forms/month-multiple-fields/month-multiple-fields-appearance-pseudo-elements.html failed unexpectedly (image diff)
[9/13] fast/forms/week-multiple-fields/week-multiple-fields-appearance-pseudo-elements.html failed unexpectedly (image diff)
These tests are bad because they're checking that you can add :before and :after content to the special <input> elements which is wrong. We shouldn't even support generated content on those things yet (and we don't support it for most input types).

Comment on attachment 167604[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=167604&action=review
Overall this looks good. I need to give it a few more passes before I fully understand it and can comment intelligently.
> Source/WebCore/dom/PseudoElement.cpp:76
> + if (RenderObject* renderer = this->renderer()) {
Early return probably is cleaner than indenting this whole block.
> Source/WebCore/dom/PseudoElement.cpp:106
> + if (child->isText()) {
> + child->setStyle(renderStyle());
> + } else if (child->isImage()) {
I think single-line ifs don't have { } in webkit.
> LayoutTests/ChangeLog:10
> + Note: The output of table-row-group-to-inline.html is wrong (but always was) until Bug 86261 is fixed.
Could you explain the changes we see in the image results?

(In reply to comment #32)
> (From update of attachment 167604[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167604&action=review
> ...
> > LayoutTests/ChangeLog:10
> > + Note: The output of table-row-group-to-inline.html is wrong (but always was) until Bug 86261 is fixed.
>
> Could you explain the changes we see in the image results?
I'll update the ChangeLog. There's always been a bug where we leave anonymous RenderTable wrappers when removing nodes, that's why you see the output of the test on two lines in the old -expected.png, but now the way generated content gets inserted is the same as DOM nodes so instead of:
RenderText
RenderTable
RenderText
which used to put this on two lines we end up with:
RenderTable
RenderText
RenderText
Which puts it all on one line. It *should* be all on one line, but it shouldn't be pushed down a line. To fix this we need to clean up anonymous table wrappers.

(In reply to comment #34)
> (From update of attachment 167604[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167604&action=review
>
>
> > Source/WebCore/dom/Element.cpp:1858
> > +PseudoId Element::pseudoId() const
> > +{
> > + return hasRareData() ? elementRareData()->m_pseudoId : NOPSEUDO;
> > +}
>
> I wonder how hot any of these are and if they'll need to be inlined.
Not sure. I originally had a lot of work in this patch for that but most cases don't need that perf difference. hasRareData() is a bitfield check and this method isn't virtual so I'd hope it's reasonably fast.
>
> > Source/WebCore/dom/Node.cpp:476
> > + if (isElementNode() && !previousSibling()) {
>
> I think previousSibling is cheaper than isElementNode() and should probably be first.
isElementNode() is a bitfield check in the DOM so it's fast.
> > Source/WebCore/dom/PseudoElement.cpp:89
> > ...
> > + child->destroy();
>
> unconditional-create/conditional-destroy patterns like this are inefficent. I believe this is how existing code works, so this is fine... but hopefully the destroy() branch is very uncommon. :)
This should be very uncommon.
>
> > Source/WebCore/dom/PseudoElement.cpp:110
> > + // Images must inherit the pseudo so the width/height styles on the element don't affect it.
> > + RefPtr<RenderStyle> style = RenderStyle::create();
> > + style->inheritFrom(renderStyle());
> > + child->setStyle(style.release());
>
> It's unclear to me why images are special. Is the non-inherit path an optimization? Does text need to not-inherit? If so, why? Do those reasons not apply to images?
I added a comment. Images in generated content should always be intrinsically sized. If we didn't inherit
then the width/height on the generated content node itself would change the sizing of the image inside it.
> > Source/WebCore/dom/PseudoElement.cpp:132
> > + switch (content->type()) {
> > + case CONTENT_NONE:
> > + ASSERT_NOT_REACHED();
> > + return 0;
>
> Is this switch copied from somewhere? Can we share code?
Well, it's copied from the old RenderObjectChildList but modified a bit. In a follow up patch I want to move renderer creation to virtual methods on the ContentData classes and get rid of this. There's no reason for this kind of perf optimization here.
>
> > Source/WebCore/dom/PseudoElement.cpp:134
> > + renderer = new (arena) RenderTextFragment(document(), static_cast<const TextContentData*>(content)->text().impl());
>
> Too bad we don't have toTextContentData(), those tend to be cleaner/safer than raw static_casts, as they ASSERT and are fewer chars...
Yeah, after this lands I'll create a patch that does that.
>
> > Source/WebCore/dom/PseudoElement.h:58
> > +inline PassRefPtr<PseudoElement> PseudoElement::create(Element* parent, PseudoId pseudoId)
>
> I don't think you need to mark things in headers as inline? I thought that was implicit?
>
> Also, why declare this after the class instead of inline in teh declaration?
You must have inline or you'll get duplicate symbol declarations I think. We always put it in there. It should just be in the class though (I fixed it).
>
> > Source/WebCore/rendering/HitTestResult.cpp:265
> > + if (n && n->isPseudoElement())
> > + n = n->parentOrHostNode();
>
> How does shadow avoid this?
Shadow does some magic for event retargeting, but it's designed to only happen during event dispatch and we need it for all kinds of things like selection or clicking links (ie you should never hit the content). We might be able to fix shadow DOM, but I'd like to wait for that.

Comment on attachment 167888[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=167888&action=review
The only question is the perf. :)
> Source/WebCore/dom/Element.cpp:1820
> + existing->setNeedsStyleRecalc();
> + existing->recalcStyle(change);
Why do we need to setNeedsStyleRecalc()? And do you need to upgrade the change to Detach or Force? It seems any style change could be a pseudo change, so we do need this setNeedsStyleRecalc or we need to pass Force. You should document this now that we understand what's going on.
> Source/WebCore/dom/PseudoElement.cpp:107
> + if (child->isText())
> + child->setStyle(renderStyle());
> + else if (child->isImage()) {
Instead add an early return, which is if (!child->isText() || !child->isImage()) with a note, saying that it must not be our rendererer so we don't manage the style.
Then ahve a "optimized" path for text.
And then fallback for images to the normal inherit path.
> Source/WebCore/dom/PseudoElement.cpp:122
> + // PseudoElements are not part of the DOM tree and don't participte in the recalcStyle flow so we must
> + // propagate the style downward manually.
note to you: YOu said you wanted to update this.

(In reply to comment #46)
> Created an attachment (id=168121) [details]
> Patch for landing
Seems webkit patch got confused when I renamed the bug or I lost the r+ bit somehow so I had to manually put in the reviewer in and fix the bug name.
Assuming this goes green on all the bots I just need a cq+. :)

(In reply to comment #50)
> I needed to roll this back because it broke inspector (and hence is likely to break the web). Try opening console and typing there - it is not editable.
>
> http://trac.webkit.org/changeset/131050
Interesting. How do I get the web inspector to open for the web inspector? This seems like it's related to editing and generated content, but I can't find any reference to contentEditable in the inspector code.

(In reply to comment #54)
> (In reply to comment #53)
> > (In reply to comment #52)
> > > Looks like this patch caused a 7.91MB increase in memory usage on the Parser/html5-full-render:Malloc test while it was in the tree. ( http://webkit-perf.appspot.com/graph.html#tests=[[6268923,2001,7288486]]&sel=none&displayrange=7&datatype=running )
> >
> > Woah that can't be right. Two RefPtrs in rare data shouldn't be causing 8mb of memory usage... how do I profile this?
>
> I looked at the HTML5 spec http://www.whatwg.org/specs/web-apps/current-work/ and it has 137447 elements and 186374 text noes.
>
> This patch means creating another 4459 Element's for the PseudoElements, or a 1.37% growth in the number of Nodes and a 3.2% growth in the number of Elements. Presumably our memory should be scaling with those factors.
>
> There must be some bug in my code, I can't imagine that Element is 1.8k per instance.
If you are developing on Mac, I suggest using Instruments.app with the "Allocations" tool and getting a full profile of loading the test file. (And make sure you're using a release build, as debug builds have quite different memory usage characteristics.)

(In reply to comment #54)
> This patch means creating another 4459 Element's for the PseudoElements, or a 1.37% growth in the number of Nodes and a 3.2% growth in the number of Elements. Presumably our memory should be scaling with those factors.
>
> There must be some bug in my code, I can't imagine that Element is 1.8k per instance.
I would guess that this is the cost of creating a ton of ElementRareDatas on nodes that otherwise wouldn't need them. ElementRareData is actually quite large. Any pages that uses a lot of generated content nodes, but otherwise doesn't hit ElementRareData, will definitely use a lot more memory.

Created attachment 168835[details]
Patch
Fix use-after-free bugs from not explictly calling detch and fix Range code that was confused by the new nodes which broke the DevTools inspector. This isn't meant for landing, I'm going to instead break this apart into parts where we can land most of it without turning it on and then land the final part to avoid having to roll it in and out repeatedly.

(In reply to comment #57)
> Created an attachment (id=168835) [details]
> Patch
>
> Fix use-after-free bugs from not explictly calling detch and fix Range code that was confused by the new nodes which broke the DevTools inspector. This isn't meant for landing, I'm going to instead break this apart into parts where we can land most of it without turning it on and then land the final part to avoid having to roll it in and out repeatedly.
I've found the leak: I keep the PseudoElement in a RefPtr in the ElementRareData and just assign 0 to it to remove the last ref, but because of TreeShared::deref() semantics even though the m_refCount is 0 the PseudoElement doesn't actually get freed because it still has a parent. Instead to follow the TreeShared design we should just forcibly delete the PseudoElements ignoring their ref count (or I guess we could unset it's parent and then deref?).
I now see a 2MB increase in memory usage on that test in Instruments and it's stable between runs so it's not leaking anymore, but it also seems to allocate 13k PseudoElements per run even though there should only be 2.5k :/

(In reply to comment #58)
> ...
> I now see a 2MB increase in memory usage on that test in Instruments and it's stable between runs so it's not leaking anymore, but it also seems to allocate 13k PseudoElements per run even though there should only be 2.5k :/
Figured this out, there really are 13k pseudos in that test's output but there's only 2.5k in the source html5.html file! There's a bug in html5-full-render.html when chunking up the source HTML using substring where it should be using substr.
html5-full-render.html:16 has (w/ chunkSize = 500000):
spec.substring(chunkIndex * chunkSize, chunkSize);
but this is wrong because substring is (from, to) so it should be:
spec.substr(chunkIndex * chunkSize, chunkSize);
to get chunks of the specified size. Right now the html5-full-render.html actually loads massive chunks of the HTML5 spec that wrap around to the start repeatedly over and over yielding the following chunk sizes:
Chunk 1: 500000
Chunk 2: 0
Chunk 3: 500000
Chunk 4: 1000000
Chunk 5: 1500000
Chunk 6: 2000000
Chunk 7: 2500000
Chunk 8: 3000000
Chunk 9: 3500000
Chunk 10: 4000000
Chunk 11: 4500000
Chunk 12: 5000000
Chunk 13: 5500000
So while each chunk was supposed to be 500000, we end up loading chunks anywhere from 2-11x that in size.
@eseidel: It looks like you were the one who landed this test with the chunking in https://trac.webkit.org/changeset/96647 I can fix the bug, but the test will become less stressful since we'll be loading 5x less chars: 33.5m vs 6.5m chars.
The good news is that there's only a ~150 byte overhead per pseudo with this patch.
What do you want to do here Eric? :)

(In reply to comment #59)
> The good news is that there's only a ~150 byte overhead per pseudo with this patch.
That's 150 bytes more than we used to use or 150 bytes total per pseudo? Would you mind instrumenting your local build to spit out the number of psuedoelements and then testing out a few major sites (e.g. test the html spec, gmail, facebook, etc)? That way we can get a sense of what sort of memory impact to expect.

(In reply to comment #61)
> (In reply to comment #59)
> > The good news is that there's only a ~150 byte overhead per pseudo with this patch.
>
> That's 150 bytes more than we used to use or 150 bytes total per pseudo? Would you mind instrumenting your local build to spit out the number of psuedoelements and then testing out a few major sites (e.g. test the html spec, gmail, facebook, etc)? That way we can get a sense of what sort of memory impact to expect.
That's 150 bytes more than we used to use for each one. Gmail is ~40, Docs is ~30 pseudos. Facebook uses 175 on first load of my timeline and uses more and more as I scroll down and content loads. Twitter uses 134 on first load and uses more as I scroll down and content loads.
So that's +25k memory for Facebook, +19k for Twitter, and about +6k for Docs and Gmail. 6500 pseudos is equal to approximately 1MB of increased RAM usage.
This is not any different than if you inserted the equivalent number of DOM nodes and touched anything that allocates rare data including: classList, childNodes, getElementsBy(TagName|ClassName) or a bunch of other things on them.
We could move them out of the RareData and into static maps to reduce memory usage, but I think it'd make more sense to do that in a separate patch after this lands.

(In reply to comment #63)
> Created an attachment (id=170532) [details]
> Patch
I posted this just to get it up there and get eyes on the changes/simplifications. I want to land this in parts so we don't need to roll the whole thing out again if something goes wrong.