WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
103705
Add infrastructure for :before and :after in DOM
https://bugs.webkit.org/show_bug.cgi?id=103705
Summary
Add infrastructure for :before and :after in DOM
Elliott Sprehn
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Elliott Sprehn
Comment 1
2012-11-29 18:48:53 PST
Created
attachment 176883
[details]
Patch
Julien Chaffraix
Comment 2
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.
Elliott Sprehn
Comment 3
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.
Elliott Sprehn
Comment 4
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.
Elliott Sprehn
Comment 5
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))
Elliott Sprehn
Comment 6
2012-12-03 22:37:19 PST
Created
attachment 177418
[details]
Patch
EFL EWS Bot
Comment 7
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
Early Warning System Bot
Comment 8
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
Early Warning System Bot
Comment 9
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
Elliott Sprehn
Comment 10
2012-12-03 22:54:35 PST
Created
attachment 177419
[details]
Patch
Eric Seidel (no email)
Comment 11
2012-12-05 12:41:00 PST
Comment on
attachment 177419
[details]
Patch OK.
WebKit Review Bot
Comment 12
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
>
WebKit Review Bot
Comment 13
2012-12-05 13:01:57 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug