Bug 103705 - Add infrastructure for :before and :after in DOM
: Add infrastructure for :before and :after in DOM
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Elliott Sprehn
:
Depends on:
Blocks: 95117
  Show dependency treegraph
 
Reported: 2012-11-29 18:39 PST by Elliott Sprehn
Modified: 2012-12-05 13:01 PST (History)
10 users (show)

See Also:


Attachments
Patch (55.70 KB, patch)
2012-11-29 18:48 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (54.81 KB, patch)
2012-12-03 22:37 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (54.83 KB, patch)
2012-12-03 22:54 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-11-29 18:39:11 PST
Add all supporting infrastructure for generated content in the DOM without actually removing the old generated content support so we can land a tiny patch later.
Comment 1 Elliott Sprehn 2012-11-29 18:48:53 PST
Created attachment 176883 [details]
Patch
Comment 2 Julien Chaffraix 2012-11-30 17:22:54 PST
Comment on attachment 176883 [details]
Patch

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

The direction is good, some comments.

> Source/WebCore/dom/Element.cpp:195
> +    }

I would have thought detach would have been called before or at least during (see Node::~Node) destruction. Is this really needed or am I missing something?

> Source/WebCore/dom/Element.cpp:2098
> +        if (element->renderer())
> +            ensureElementRareData()->setPseudoElement(pseudoId, element.release());
> +        else {
> +            // If attach did not create a renderer then clear the parent so the element
> +            // will be freed when this method returns.
> +            element->setParentOrHostNode(0);
> +        }

createPseudoElementIfNeeded calls pseudoElementRendererIsNeeded which is common with PseudoElement::rendererIsNeeded so aren't we guaranteed to have a renderer in this case?

> Source/WebCore/dom/Element.cpp:2111
> +    // FIXME: Disallow generated content on things with a shadow until the spec says what to do.
> +    // http://webkit.org/b/98836

Mhh, are we sure this won't break everything? Couldn't we postpone this change to after the migration as this could be - and has been - disrupting?

> Source/WebCore/dom/Element.h:500
> +    void updatePseudoElement(PseudoId, StyleChange = NoChange);

Do we really need a default parameter for the second argument?

> Source/WebCore/dom/ElementRareData.h:155
> +    element->setParentOrHostNode(0);

We should probably ASSERT that element has no siblings or all bets are off.

> Source/WebCore/dom/ElementRareData.h:161
> +    }

As discussed it would be nicer if we stored a strong reference on the pointer, just sever the parent link, detachIfNeeded and deref here.

> Source/WebCore/dom/Node.h:758
> +    virtual PseudoId customPseudoId() const { return NOPSEUDO; }

You should probably add ASSERT(hasCustomCallback()) as we don't want to start calling this function for no good reason.

> Source/WebCore/dom/PseudoElement.cpp:80
> +    return pseudoElementRendererIsNeeded(context.style());

Ideally this should always return true (and ASSERT) as we would rather avoid keeping PseudoElements around when they are not needed.

> Source/WebCore/rendering/RenderGrid.cpp:234
> +    // FIXME: Temporary hack while the new generated content system is being implemented.
> +    if (isPseudoElement())
> +        return "RenderGrid (generated)";

This is unneeded as we don't have test covering grid + generated content. I hope you finished this code before we actually do though :)

> Source/WebCore/rendering/RenderObject.cpp:1759
> +void RenderObject::setPseudoStyle(PassRefPtr<RenderStyle> pseudoStyle)

What's the long term plan about this function? As-is this function doesn't add much and it obfuscates the call sites as all but one will just do a straight setStyle.
Comment 3 Elliott Sprehn 2012-11-30 17:41:29 PST
(In reply to comment #2)
> (From update of attachment 176883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176883&action=review
> 
> The direction is good, some comments.
> 
> > Source/WebCore/dom/Element.cpp:195
> > +    }
> 
> I would have thought detach would have been called before or at least during (see Node::~Node) destruction. Is this really needed or am I missing something?

The order is wrong, we blow away the RareData first, and then call detach() in ~Node. That's the same reason we have manual clean up logic for ElementShadow below.

> 
> > Source/WebCore/dom/Element.cpp:2098
> > +        if (element->renderer())
> > +            ensureElementRareData()->setPseudoElement(pseudoId, element.release());
> > +        else {
> > +            // If attach did not create a renderer then clear the parent so the element
> > +            // will be freed when this method returns.
> > +            element->setParentOrHostNode(0);
> > +        }
> 
> createPseudoElementIfNeeded calls pseudoElementRendererIsNeeded which is common with PseudoElement::rendererIsNeeded so aren't we guaranteed to have a renderer in this case?

No, because isChildAllowed can return false and even though rendererIsNeeded returns true we'll end up without a renderer, see NodeRenderingContext::createRendererForElementIfNeeded. This is super rare so we could just let the PseudoElement exist in those cases to make this code shorter (my original patch was like this).

> 
> > Source/WebCore/dom/Element.cpp:2111
> > +    // FIXME: Disallow generated content on things with a shadow until the spec says what to do.
> > +    // http://webkit.org/b/98836
> 
> Mhh, are we sure this won't break everything? Couldn't we postpone this change to after the migration as this could be - and has been - disrupting?

It would break everything, but this patch doesn't enable any of the new stuff so it doesn't matter. I can leave this out I guess.

> 
> > Source/WebCore/dom/Element.h:500
> > +    void updatePseudoElement(PseudoId, StyleChange = NoChange);
> 
> Do we really need a default parameter for the second argument?

Yes, see the original patch this was lifted out of. We need to call updatePseudoElement(BEFORE) and updatePseudoElement(AFTER) inside of ::attach(). It seemed really weird to require passing NoChange there.

> 
> > Source/WebCore/dom/PseudoElement.cpp:80
> > +    return pseudoElementRendererIsNeeded(context.style());
> 
> Ideally this should always return true (and ASSERT) as we would rather avoid keeping PseudoElements around when they are not needed.

You can't just return true because during a style recalc we're going to call this method in recalcStyle() when it goes through ::reattach() and we need the same logic so when you go from content: "foo" to content: none we get no renderer.

> 
> > Source/WebCore/rendering/RenderObject.cpp:1759
> > +void RenderObject::setPseudoStyle(PassRefPtr<RenderStyle> pseudoStyle)
> 
> What's the long term plan about this function? As-is this function doesn't add much and it obfuscates the call sites as all but one will just do a straight setStyle.

We would need to duplicate the setPseudoStyle logic in ImageContentData::createRenderer and also in PseudoElement::didRecalcStyle. This method abstracts the special handling for pseudo styles.
Comment 4 Elliott Sprehn 2012-12-03 22:08:19 PST
Comment on attachment 176883 [details]
Patch

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

>>> Source/WebCore/dom/Element.cpp:2098
>>> +        }
>> 
>> createPseudoElementIfNeeded calls pseudoElementRendererIsNeeded which is common with PseudoElement::rendererIsNeeded so aren't we guaranteed to have a renderer in this case?
> 
> No, because isChildAllowed can return false and even though rendererIsNeeded returns true we'll end up without a renderer, see NodeRenderingContext::createRendererForElementIfNeeded. This is super rare so we could just let the PseudoElement exist in those cases to make this code shorter (my original patch was like this).

After thinking about this it's actually better to leave those really rare render-less PseudoElement's that occur after attach() in the tree and change the above check from if (!existing->renderer()) to if (!pseudoElementRendererIsNeeded(existing->renderStyle())) so we don't have pseudo element churn. Without that doing something like frameset:before { content: "x"; } could cause us to create and destroy the PseudoElement repeatedly on style changes.
Comment 5 Elliott Sprehn 2012-12-03 22:13:19 PST
(In reply to comment #4)
> (From update of attachment 176883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176883&action=review
> 
> ... if (!existing->renderer()) to if (!pseudoElementRendererIsNeeded(existing->renderStyle())) ..

Err that's actually pseudoElementRendererIsNeeded(renderer()->getCachedPseudoStyle(pseudoId))
Comment 6 Elliott Sprehn 2012-12-03 22:37:19 PST
Created attachment 177418 [details]
Patch
Comment 7 EFL EWS Bot 2012-12-03 22:46:23 PST
Comment on attachment 177418 [details]
Patch

Attachment 177418 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15098900
Comment 8 Early Warning System Bot 2012-12-03 22:46:26 PST
Comment on attachment 177418 [details]
Patch

Attachment 177418 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15119657
Comment 9 Early Warning System Bot 2012-12-03 22:46:34 PST
Comment on attachment 177418 [details]
Patch

Attachment 177418 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15121608
Comment 10 Elliott Sprehn 2012-12-03 22:54:35 PST
Created attachment 177419 [details]
Patch
Comment 11 Eric Seidel 2012-12-05 12:41:00 PST
Comment on attachment 177419 [details]
Patch

OK.
Comment 12 WebKit Review Bot 2012-12-05 13:01:50 PST
Comment on attachment 177419 [details]
Patch

Clearing flags on attachment: 177419

Committed r136744: <http://trac.webkit.org/changeset/136744>
Comment 13 WebKit Review Bot 2012-12-05 13:01:57 PST
All reviewed patches have been landed.  Closing bug.