Bug 103705

Created attachment 176883 [details] Patch

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.

(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 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.

(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))

Created attachment 177418 [details] Patch

Comment on attachment 177418 [details] Patch Attachment 177418 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15098900

Comment on attachment 177418 [details] Patch Attachment 177418 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15119657

Comment on attachment 177418 [details] Patch Attachment 177418 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15121608

Created attachment 177419 [details] Patch

Comment on attachment 177419 [details] Patch OK.

Comment on attachment 177419 [details] Patch Clearing flags on attachment: 177419 Committed r136744: <http://trac.webkit.org/changeset/136744>

All reviewed patches have been landed. Closing bug.