569006 - Fast-path checks for :before/:after if we're not reresolving style on kids

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

With the fix for bug 494117, he before/after probing becomes a bigger bottleneck in reresolve (esp. on pages that actually use them and have lots of rules around).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 448160 [details] [diff] [review]
Like so

>+  // If changes to the "content" property ever stop triggering framechanges,
>+  // the optimization that skips ::before/::after probing in
>+  // ReResolveStyleContext will need to be adjusted for cases when we dropped
>+  // the pseudo due to empty content.

I don't think I understand this comment.  Could you make it clearer?

Also, maybe put a blank line after it since it's related to the whole function and not just the code right below it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I don't think I understand this comment.  Could you make it clearer?

How is this:

  // In ReResolveStyleContext we assume that if there's no existing ::before
  // or ::after and we don't have to restyle children of the node then we
  // can't end up with a ::before or ::after due to the restyle of the node
  // itself.  That's not quite true, but the only exception to the above is
  // when the "content" property of the node changes.  Since the code here
  // triggers a frame change on the node in that case, the optimization in
  // ReResolveStyleContext is ok.  But if we ever change this code to not
  // reconstruct frames on changes to the "content" property, then we will
  // need to revisit the optimization in ReResolveStyleContext.

> Also, maybe put a blank line after it

Done.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Yeah, I understand the comment now.

But maybe you mean to say when the 'content' property of the *pseudo-element* changes, we reframe the element.  Also, CSS properties generally have 'quotes' rather than "quotes".

Comment 5

[Boris Zbarsky [:bzbarsky]]

No, I meant what I said.  If the 'content' property of the pseudo-element changes directly, that means that the selectors matching that pseudo-element have changed, and we will have the "restyle your kids" bit set in ReResolveStyleContext.  The case that depends on the reframe behavior of 'content' is something like this:

  div { content: "foo"; }
  div::before { content: inherit; }

where things will work out because the _div_ gets reframed.  Perhaps I should say:

  That's not quite true, but the only exception to the above is
  when the "content" property of the node changes and the pseudo-element
  inherits the changed value.

or so?

> Also, CSS properties generally have 'quotes' rather than "quotes".

Will do.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

ok, sounds good.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8d309d7e0746