Closed Bug 1209603 Opened 9 years ago Closed 9 years ago

specific combinations of em units and dynamic style changes can cause incorrect values of font properties

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox41 - wontfix
firefox42 - affected
firefox43 - affected
firefox44 - fixed

People

(Reporter: danielpopowich, Assigned: dbaron)

References

Details

(4 keywords)

Attachments

(13 files, 2 obsolete files)

screen shot of dev console showing bad font-size computation
53.93 KB, image/png
Details
simple testcase
1.42 KB, text/html
Details
patch 1 - Add comments to RuleNodeCacheConditions.h
3.02 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 2 - Reduce calls to StyleVisibility() in WritingMode constructor
1.52 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 3 - Don't call SetFontSizeDependency for 0em
1.36 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 4 - Add reftest for bug 1209603
2.93 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 5 - Move inline method nsStyleContext::GetCachedStyleData into header file, and make it public
3.64 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 6 - Prepare to use a different meaning of mBits when cached style data pointer is null
5.66 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 7 - Add assertions that we don't ask the rule node for data when we have cached data on the style context
5.45 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 8 - Record in mBits when we have gotten a reset style struct that is cached on the rule node
11.67 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 9 - Cache inherited style structs on the style context when we found already-cached data in the rule tree
11.54 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 10 - Make PeekStyle* exact, i.e., guaranteed to return null if we haven't computed the data for this context
4.58 KB, patch
heycam
: review+
Details | Diff | Splinter Review
patch 11 - Assert that PeekStyle* results don't change during difference computation
4.29 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150629114848

Steps to reproduce:

I'm having trouble reproducing this outside of a particular page displayed on FF 41 on Windows.  You will see in the attached screen shot my dev console with an H2 element highlighted.  The HTML tag specifies a base font-size of 16px.  H2 elements have font-size 1.5em.  Yet FF41 computes the font-size to 9px!

This does not happen on FF40.  FF40 computes the font-size as expected, to 24px.

Oddly, in the Rules tab of the dev console, if I uncheck, then recheck the H2 font-size directive, the browser recomputes the font-size correctly to 24px.

Also unexpected: if I strip down my html/css to bare minimum, the problem disappears.


Actual results:

Expect font-size to compute to 24px.


Expected results:

Font-size is computed to 9px.  See attached screen shot of dev console.
(In reply to danielpopowich from comment #0)
> I'm having trouble reproducing this outside of a particular page displayed
> on FF 41 on Windows.
> Also unexpected: if I strip down my html/css to bare minimum, the problem
> disappears.

Please attach or link to the page showing the issue.
Component: Untriaged → Layout: Text
Flags: needinfo?(danielpopowich)
Keywords: testcase-wanted
Product: Firefox → Core
This is a proprietary single-page app.  I was hoping the screenshot of the the dev console might suffice. I'll see if I can prune it down to a bare bones reproduction of the problem.
As feared, I cannot reproduce this when I strip it down to the html/css: when I do strip it down it works.  Being a consultant, writing this app for a 3rd party, I am not at liberty to share the original app where it does happen consistently.

The screenshot I originally posted, pretty much says it all: an H2, with font-size 1.5em in a document with base font-size of 16px is being computed and displayed as 9px.  Known to happen on FF41 on Win8.1 and Win7.  If, when in the Rules tab of the dev console, I uncheck, then recheck the H2 font-size rule, it recomputes it properly to 24px.

On FF40, this is NOT a problem, the H2 is properly computed and rendered at 24px.

Something very subtle was introduced in 41...
It's just hard to figure out what the something is, without a way to reproduce the problem...

Are you willing to take the time to check whether this is still a problem in a current nightly?  And if so, whether you can find which nightly first introduced the issue, using <http://mozilla.github.io/mozregression/>?
> It's just hard to figure out what the something is, without a way to reproduce the problem...

Understood.

I'll see if I can find the nightly.  I'll keep you posted.
Thanks!  You can get the current nightly at https://nightly.mozilla.org/ for what it's worth.
Narrowed it down with mozregression.  I new it was good in 40 and bad in 41, so using mozregression --list-releases, I started with this:

    mozregression --good=2015-05-11 --bad=2015-06-29

And narrowed it down to this inbound:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a6f67ef457310bef5e85d9a6f72f73735d63a92c&tochange=733b4adb414090bb7cb057a79648fca6b3ec87b6

Hope that helps.
That definitely helps.  At least now we know which change causes the problem, and it's a change that could very well have affected font-size computation!

[Tracking Requested - why for this release]: Web compat regression
Blocks: 804975
Status: UNCONFIRMED → NEW
status-firefox41: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
Ever confirmed: true
Flags: needinfo?(cam)
Keywords: regression
For that particular change, though, it's still going to be hard to figure out the problem without a testcase.

It's likely that the problem is specific to a certain sequence of dynamic style changes, not to the static situation, because the change is related to optimizing away style recomputation that we think we can skip.
Also, did you check that it's still broken in the most recent nightly?  That's simpler, and worth doing first.
I suppose it's also possible that bug 1186768 is related, since that's the only known unfixed regression from that change.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #9)
> It's likely that the problem is specific to a certain sequence of dynamic
> style changes, not to the static situation, because the change is related to
> optimizing away style recomputation that we think we can skip.

Er, sorry, I was thinking of a different bug.

This change is related to sharing style data between other parts of the page -- most likely with another element that matches the same style rules but has a different inherited font size.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #11)
> I suppose it's also possible that bug 1186768 is related, since that's the
> only known unfixed regression from that change.

I'm pretty sure it's not; bug 1186768 only happens with script level font-size adjustment in MathML (for fractions/exponents, I think).
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #10)
> Also, did you check that it's still broken in the most recent nightly? 
> That's simpler, and worth doing first.

Yes, it's still in the latest nightly.

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #9)
> For that particular change, though, it's still going to be hard to figure
> out the problem without a testcase.

As you note below, this is a dynamic style change issue...reproducing the html structure and accompanying css and loading that statically does NOT cause the problem.  

> It's likely that the problem is specific to a certain sequence of dynamic
> style changes, not to the static situation, because the change is related to
> optimizing away style recomputation that we think we can skip.

This occurs in a classic single-page app: a DIV's innerHTML is replaced over and over in a series of screens, guiding the user through an exam.  

I'll see if I can reproduce the problem in a simplified "app" that transitions from one screen to tne next where the broken H2 font-size computation occurs.
Flags: needinfo?(danielpopowich)
It just occurred to me what may be going on...I've been thinking, "9px...9px...wtf?"  When I just remembered, the previous screen in my app sets the font-size to 6px.  1.5em of that is 9px.

So, on screen 1, a div has a font-size of 6px, and on the next "screen", screen 2, the font-size is set to 1em (to pick up the parent's font-size).  The H2 on Screen 2, with font-size 1.5em is using 6px, not 16px as the font to base 1.5em upon.

I'll see if I can mock this up.
It certainly sounds like bug 804975's changes to how we decide whether to cache in the rule tree could be at fault.  Reading through what happens in ComputeFontData nothing stands out (though I think there are some problems that you're unlikely to be running in to -- bug 1186768 comment 10 and bug 1209805), so Daniel if you can manage to come up with a test that would be really helpful!
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #16)
> so Daniel if you can manage to come up with a test that would be
> really helpful!

I've been trying...I stripped down this app to what I thought would be a good example, but then: it all works!  I run both the problematic app and my stripped down sample, side by side in two tabs in the most recent nightly build, the dev console shows me the exact same markup, and the css is the same, too, yet one H2 is 24px and the other is 9px.

I'll keep trying.
Wow, this one is ellusive.  

To recap, I have something like this:

   <div id="container" style="font-size: 6px">
      <h2>a heading</h2>
   </div>

And code like this

    mydiv = document.getElementById('container');
    mydiv.style['font-size'] = '1em';

At which point the container DIV should have the font-size of its parent, which happens to be 16px.  The H2 within the container (font-size: 1.5em) ends up with calculated font-size of 9px, instead of 24px.  You see this in the screenshot I posted with the bug report.

So, I've been trying to strip down this huge app to a testcase to demonstrate the problem and thus far have been unsuccessful...it works just fine in my stripped down testcases, no matter how exact I get the code, relative to the app's code.

So I decided to run my app with the dev console debugger...stepping through lines of code, at the point in the code where the font-size of the container is set to 1em, IT WORKS.

Does that help anyone narrow down where this could be happening?  The font-size does not properly compute to 24px in the running app, but when stepping through code in the debugger, it does.

wtf?
Recent regression, tracking.

Cam, any chance you can fix that for the 42 release? Thanks
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
tracking-firefox41: ? → -
tracking-firefox42: ? → +
tracking-firefox43: ? → +
tracking-firefox44: --- → +
(In reply to Kohei Yoshino [:kohei] from comment #19)
> Posted the site compatibility doc.
> https://www.fxsitecompat.com/en-US/docs/2015/css-cascading-may-go-wrong-when-
> style-is-dynamically-updated/

I think that post is somewhat misleading; it makes it sound like this is a somewhat general and understood problem, yet it's a very specific problem that is so far known to affect only one site that isn't publicly visible.

(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Recent regression, tracking.
> 
> Cam, any chance you can fix that for the 42 release? Thanks

I don't think there's enough information in the bug to fix the problem, since we don't yet have a site where we can observe the problem happening.
Flags: needinfo?(kohei.yoshino)
[Tracking Requested - why for this release]:  Changing tracking+ back to tracking? because I don't think we should be tracking a bug that, after hitting release, has been observed by only a single person, and can't be seen on any public sites.
tracking-firefox42: + → ?
tracking-firefox43: + → ?
tracking-firefox44: + → ?
Summary: bad css cascade in version 41 → unknown font-size CSS computation regression from bug 804975
> Does that help anyone narrow down where this could be happening?

Sadly, no.  All that tells us is that stopping in a debugger lets some style changes happen before others whereas without the stop they would happen as part of the same restyling pass...
I have slightly updated the compat doc description.
https://github.com/fxsitecompat/www.fxsitecompat.com/commit/7e27f89
Flags: needinfo?(kohei.yoshino)
One other comment:  having one of us debug the actual site where it's happening doesn't require that you give the whole world access to that site, if there's a way you could give a few of us access (e.g., sending credentials by email to me and/or to heycam) so that we can see the real site.
I've requested my client allow me to give limited access to my code to mozilla devs to debug this problem.  I await their reply.
I have received permission from my client to share the code with mozilla devs.

Please reply if you will be working on this, and I will email you necessary credentials and instructions.

Thanks!
I have the site in question.

So far I've gotten to the point that the style context was reparented by nsStyleContext::MoveTo, in this code in ElementRestyler::Restyle:

  if (result == eRestyleResult_Stop) {
    MOZ_ASSERT(mFrame->StyleContext() == oldContext,
               "frame should have been left with its old style context");

    nsIFrame* unused;
    nsStyleContext* newParent = mFrame->GetParentStyleContext(&unused);
    if (oldContext->GetParent() != newParent) {
      // If we received eRestyleResult_Stop, then the old style context was
      // left on mFrame.  Since we ended up restyling our parent, change
      // this old style context to point to its new parent.
      LOG_RESTYLE("moving style context %p from old parent %p to new parent %p",
                  oldContext.get(), oldContext->GetParent(), newParent);
      // We keep strong references to the new parent around until the end
      // of the restyle, in case:
      //   (a) we swapped structs between the old and new parent,
      //   (b) some descendants of the old parent are not getting restyled
      //       (which is the reason for the existence of
      //       ClearCachedInheritedStyleDataOnDescendants),
      //   (c) something under ProcessPendingRestyles (which notably is called
      //       *before* ClearCachedInheritedStyleDataOnDescendants is called
      //       on the old context) causes the new parent to be destroyed, thus
      //       destroying its owned structs, and
      //   (d) something under ProcessPendingRestyles then wants to use of those
      //       now destroyed structs (through the old parent's descendants).
      mSwappedStructOwners.AppendElement(newParent);
      oldContext->MoveTo(newParent);
    }

even though its old parent and new parent have different font sizes.  Haven't yet figured out why *that* happens.
OK, I think I've worked out much of went wrong now.

The basic problem is that we do the initial computation of the bad nsStyleFont struct *inside* of the CalcStyleDifference call, while doing the PeekStyleMargin on the *old* context.  Since DO_STRUCT_DIFFERENCE(Font) comes before DO_STRUCT_DIFFERENCE(Margin), we don't report the font struct difference.

Based on what I've seen so far in gdb, I think I can write a simple testcase for the problem, but I haven't done so yet.



A few other things I think we should do:
 * make 0em not set a font-size dependency
 * improve the comments in RuleNodeCacheCondtions to better indicate that we only use conditions for reset structs, and that conditions depend only on inherited structs, and this is important since otherwise we'd end up with infinite recursion
 * improve the comments in RuleNodeCacheConditions to make it clear that the conditions refer to the element's own font-size and writing-mode
Attached file simple testcase (obsolete) — 
Slightly simpler, and fixing some incorrect comments
Attachment #8672849 - Attachment is obsolete: true
Attached file simple testcase — 
fixed another misleading comment
Attachment #8672858 - Attachment is obsolete: true
Flags: needinfo?(cam)
I guess I see two different approaches to fixing the underlying bug here:

 (1) make nsStyleContext::CalcStyleDifference loop over all the reset structs at the beginning before it does the DO_STRUCT_DIFFERENCE calls, so that we have all the structs cached.  (And probably add assertions at the end that the set of cached structs is the same as at the start... which we should probably do for either option!)  This would make things a little bit slower, but it's easy.

 (2) Try to make PeekStyle* more exact, so that it knows that we didn't get rule-node cached structs for *this* style context.  This could make things a little faster, but I'm not quite sure how I'd want to do it.
heycam came up with a third option, which is certainly better than (1):

  (3) Change PeekStyle* so that the condition checks we do inside of it (in GetStyle* on nsRuleNode, with aComputeData false) bail if they'd have to  compute a new struct (i.e., use PeekStyleData equivalent)



Also, since my previous explanation of the problem wasn't clear to him, probably worth including my re-explanation of the bug here:

The problem here is that we DO_STRUCT_DIFFERENCE(Margin) after we DO_STRUCT_DIFFERENCE(Font), and the DO_STRUCT_DIFFERENCE(Margin) calls PeekStyleMargin which evaluates the conditions on the rule node, and evaluating the condition forces the font struct to be computed.  Then we report no-style-change, keep the context and move it to the new parent, with a font struct that's now incorrect for the new parent.  This is since conditions introduced the problem that PeekStyleData can cause a *different* previously-not-computed struct to be computed.
(3) can actually be even simpler, since we always cache inherited structs on the style context, and also always cache structs that required conditions on the style context.  So we can make PeekStyleData a bit more accurate through those changes, but still not exact.
And heycam points out a possible approach for (2) might be to use the style context's mBits -- to give it the meaning we need when the stored struct is null.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #37)
> And heycam points out a possible approach for (2) might be to use the style
> context's mBits -- to give it the meaning we need when the stored struct is
> null.

The only thing I noticed that would need to be updated due to the new mBits meaning is nsStyleContext::HasCachedInheritedStyleData.
Keywords: testcase-wanted → testcase
Summary: unknown font-size CSS computation regression from bug 804975 → specific combinations of em units and dynamic style changes can cause incorrect values of font properties
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Component: Layout: Text → CSS Parsing and Computation
Version: 41 Branch → Trunk
I may make an attempt to make sense of:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=833c3c37daa6&newProject=try&newRevision=98d531d40668
although only for the tests I'm familiar with, given that it's not labeled with "higher is better" and "lower is better".
This should be a bit of an optimization for cases where '0em' is the
only 'em' unit in a style struct.
Attachment #8674680 - Flags: review?(cam)
Moving it to the header allows its use by another method in the header
file, in patch 6.

Making it public allows its use in assertions in nsRuleNode in patch 7.
Attachment #8674682 - Flags: review?(cam)
We currently only use the style struct bits in mBits when the style
context has the relevant struct cached.  The bit being set indicates
whether or not the context owns the struct.

This patch conditions the necessary tests on a cached struct being
present so that we can use (for reset structs, i.e., those with
non-inherited properties) mBits to mean something different when the
cached storage is null.
Attachment #8674683 - Flags: review?(cam)
I'm a little worried about the performance of the change to
nsRuleNode::GetStyle*, which sets a bit on the style context every time
a struct getter goes through it.  It's not obvious how that compares to
the performance benefit from patch 10.
Attachment #8674685 - Flags: review?(cam)
This means we obey the invariant that if we've requested an inherited
struct on a context, that struct will be cached on the style context.  I
believe bug 527977 intended to do make us obey this invariant, but it
missed the case where nsRuleNode::GetStyle* found cached data already on
the rule node, and the case where nsRuleNode::WalkRuleTree found a
usable struct higher in the rule tree.

Without this change, patch 10 will not function correctly for inherited
structs when we encounter this case, and will cause assertions in
dom/base/test/test_bug560780.html due to triggering style change hints
on text nodes that inherited a color struct from a parent on whose rule
node the struct was stored.  (It may also have caused some of the other
test failures.)

This should be a clear performance improvement, since the path that's
being slowed down by the added work in this patch will, with the patch,
now only execute once because of that work.
Attachment #8674686 - Flags: review?(cam)
This fixes the bug because it prevents a cache conditions check for a
later PeekStyle* in nsStyleContext::CalcStyleDifference from computing a
struct that was null when we checked it earlier in CalcStyleDifference.
This, in turn, could allow the old context to be retained (and
reparented to the new parent) even though it now has incorrect data in
the font or visibility struct that was computed while checking the
conditions for another struct.

This should also improve performance in some cases of style changes on
not-yet-presented frames because we have fewer change hints to process.
Attachment #8674687 - Flags: review?(cam)
This assertion catches the condition that led to the bug.

I confirmed that without patch 10 on this bug, the assert fires on the
reftest added in patch 4, but does not fire on slight modifications of
that testcase that don't show the bug.
Attachment #8674688 - Flags: review?(cam)
Attachment #8674678 - Flags: review?(cam) → review+
Attachment #8674679 - Flags: review?(cam) → review+
Attachment #8674680 - Flags: review?(cam) → review+
Attachment #8674681 - Flags: review?(cam) → review+
Comment on attachment 8674682 [details] [diff] [review]
patch 5 - Move inline method nsStyleContext::GetCachedStyleData into header file, and make it public

Review of attachment 8674682 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleContext.h
@@ +455,5 @@
>  
> +  /**
> +   * Return style data that is currently cached on the style context.
> +   * Only returns the structs we cache ourselves; never consults the
> +   * ruletree.

Nit: "rule tree" is probably the canonical spelling, if you want to correct it while moving it.

@@ +457,5 @@
> +   * Return style data that is currently cached on the style context.
> +   * Only returns the structs we cache ourselves; never consults the
> +   * ruletree.
> +   *
> +   * For use only in "internal" use in nsStyleContext and nsRuleNode.

I'm not sure that sentence parses properly.  Maybe:

  For "internal" use only in nsStyleContext and nsRuleNode.
Attachment #8674682 - Flags: review?(cam) → review+
Comment on attachment 8674683 [details] [diff] [review]
patch 6 - Prepare to use a different meaning of mBits when cached style data pointer is null

Review of attachment 8674683 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleContext.h
@@ +289,2 @@
>     */
> +  bool HasCachedInheritedStyleData(nsStyleStructID aSID) {

Should we rename this?  "Inherited" sounds misleading if it can come from the rule node too.  Maybe "HasCachedDependentStyleData"?

@@ +615,5 @@
>    nsResetStyleData*       mCachedResetData; // Cached reset style data.
>    nsInheritedStyleData    mCachedInheritedData; // Cached inherited style data
> +
> +  // mBits stores a number of things:
> +  //  - For all structs (those with inherited properties), when they are

Should the " (those with inherited properties)" be removed, and in patch 8 added back along with doing s/all/inherited/?
Attachment #8674683 - Flags: review?(cam) → review+
Attachment #8674684 - Flags: review?(cam) → review+
Comment on attachment 8674685 [details] [diff] [review]
patch 8 - Record in mBits when we have gotten a reset style struct that is cached on the rule node

Review of attachment 8674685 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.cpp
@@ +2350,5 @@
>      PropagateDependentBit(aSID, ruleNode, startStruct);
> +    if (isReset) {
> +      // Record that we have asked for this struct on this context, but
> +      // it is not cached on the context.
> +      aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));

Why don't we also need to do this for the |return parentStruct;| later in WalkRuleTree?  I can believe that we don't need to record these bits on the root rule node, but if we deliberately avoid doing that just to save unnecessary work, better note that in the comment on mBits.
Comment on attachment 8674686 [details] [diff] [review]
patch 9 - Cache inherited style structs on the style context when we found already-cached data in the rule tree

Review of attachment 8674686 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.h
@@ +902,5 @@
> +        /* the context by our caller) as not being owned by the context. */   \
> +        /* Normally this would be aContext->AddStyleBit(), but aContext is */ \
> +        /* an incomplete type here, so we work around that with a param. */   \
> +        aContextStyleBits |= NS_STYLE_INHERIT_BIT(name_);                     \
> +        /* Our caller will cache the data on the style context. */            \

I think this change should be in part 8?
Attachment #8674686 - Flags: review?(cam) → review+
Comment on attachment 8674687 [details] [diff] [review]
patch 10 - Make PeekStyle* exact, i.e., guaranteed to return null if we haven't computed the data for this context

Review of attachment 8674687 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleContext.h
@@ +532,5 @@
>          static_cast<nsStyle##name_*>(                                   \
>            mCachedInheritedData.mStyleStructs[eStyleStruct_##name_]);    \
>        if (cachedData) /* Have it cached already, yay */                 \
>          return cachedData;                                              \
> +      if (!aComputeData) {                                              \

I'm not sure if aComputeData is appropriately named, and after this patch even less so, but I'm not sure what a better name is.
Attachment #8674687 - Flags: review?(cam) → review+
Attachment #8674688 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #55)
> Review of attachment 8674685 [details] [diff] [review]:
> > +    if (isReset) {
> > +      // Record that we have asked for this struct on this context, but
> > +      // it is not cached on the context.
> > +      aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
> 
> Why don't we also need to do this for the |return parentStruct;| later in
> WalkRuleTree?  I can believe that we don't need to record these bits on the
> root rule node, but if we deliberately avoid doing that just to save
> unnecessary work, better note that in the comment on mBits.

The |return parentStruct;| later in WalkRuleTree is right next to a SetStyle call, i.e., it's a case where the cached style data is non-null, so we use the old meaning of mBits.  It's also right next to an existing AddStyleBit call.


(In reply to Cameron McCormack (:heycam) from comment #56)
> Review of attachment 8674686 [details] [diff] [review]:
> ::: layout/style/nsRuleNode.h
> > +        /* the context by our caller) as not being owned by the context. */   \
> > +        /* Normally this would be aContext->AddStyleBit(), but aContext is */ \
> > +        /* an incomplete type here, so we work around that with a param. */   \
> > +        aContextStyleBits |= NS_STYLE_INHERIT_BIT(name_);                     \
> > +        /* Our caller will cache the data on the style context. */            \
> 
> I think this change should be in part 8?

No, this is in the nsRuleNode::GetStyle##name_ defined by STYLE_STRUCT_INHERITED, so it belongs in patch 9, which changes things only for inherited structs.  The nsRuleNode::GetStyle##name_ defined by STYLE_STRUCT_RESET is patched in part 8, which changes things only for reset structs.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #58)
> The |return parentStruct;| later in WalkRuleTree is right next to a SetStyle
> call, i.e., it's a case where the cached style data is non-null, so we use
> the old meaning of mBits.  It's also right next to an existing AddStyleBit
> call.

Right, of course.

> (In reply to Cameron McCormack (:heycam) from comment #56)
> > Review of attachment 8674686 [details] [diff] [review]:
> > ::: layout/style/nsRuleNode.h
> > > +        /* the context by our caller) as not being owned by the context. */   \
> > > +        /* Normally this would be aContext->AddStyleBit(), but aContext is */ \
> > > +        /* an incomplete type here, so we work around that with a param. */   \
> > > +        aContextStyleBits |= NS_STYLE_INHERIT_BIT(name_);                     \
> > > +        /* Our caller will cache the data on the style context. */            \
> > 
> > I think this change should be in part 8?
> 
> No, this is in the nsRuleNode::GetStyle##name_ defined by
> STYLE_STRUCT_INHERITED, so it belongs in patch 9, which changes things only
> for inherited structs.  The nsRuleNode::GetStyle##name_ defined by
> STYLE_STRUCT_RESET is patched in part 8, which changes things only for reset
> structs.

I see.
Attachment #8674685 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #54)
> patch 6 - Prepare to use a different meaning of mBits when cached style data
> pointer is null
> 
> Review of attachment 8674683 [details] [diff] [review]:

> > +
> > +  // mBits stores a number of things:
> > +  //  - For all structs (those with inherited properties), when they are
> 
> Should the " (those with inherited properties)" be removed, and in patch 8
> added back along with doing s/all/inherited/?

This still applies to all structs even after patch 8.  I'll just remove the parenthetical, which was based on my own misunderstanding early in the work of writing the patch series.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #35)
> heycam came up with a third option, which is certainly better than (1):
> 
>   (3) Change PeekStyle* so that the condition checks we do inside of it (in
> GetStyle* on nsRuleNode, with aComputeData false) bail if they'd have to 
> compute a new struct (i.e., use PeekStyleData equivalent)

Now I was thinking about this a little more (great time to have second thoughts), and wondering if maybe this would have been better.  It would have just involved passing aComputeData through to nsConditionalResetStyleData::GetStyleData, I think.  And I'm not sure the benefits of making PeekStyleData exact are worth constantly setting bits in mBits.

(And yes, aComputeData isn't a great name after this series, and it's entirely vestigial for nsRuleNode::GetStyle##name_ defined by STYLE_STRUCT_INHERITED.)
I am happy with that approach if you want to change to it in a followup...
Depends on: 1217829
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: