528038 - [FIX]"top" changes not handled correctly when inherited

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

See attached self-explaining testcase.  This is a regression from bug 502288.  Now that a hint on an ancestor (in this case "reflow yourself") no longer guarantees the equivalent hint on the descendant (because we don't automatically reflow all descendants), the optimization to skip comparing structs if the rulenode didn't change in nsStyleContext::CalcStyleDifference is no longer valid.

It seems to me that a valid optimization would skipping structs if the result of possibly comparing them is subsumed in the hint we already have.  Or something along those lines.  But that would cause us to rediff nsStylePosition on all descendants of something with a top/left change, since the change hint wouldn't include the repaint hint that nsStylePosition can in theory produce...

Any other bright ideas?

Not quite sure whether this should block 1.9.2, but it's certainly a layout regression from 1.9.1.

Comment 1

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

When we propagate hints to the descendant reresolve as "already have these hints on an ancestor", shouldn't we just not propagate the hints when the same hint on the child isn't subsumed by what the ancestor did?  (i.e., not propagate NeedReflow when NeedDirtyReflow wasn't set, and not propagate ClearAncestorIntrinsics when ClearDescendantIntrinsics wasn't set)

Comment 2

[Boris Zbarsky [:bzbarsky]]

We already do those; see the very start of ReResolveStyleContext.  But nsStyleContext::CalcStyleDifference skips compares just based on the rulenode, independent of hints.

Comment 3

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

Can we only do the compare if the rulenode is the same if the hints are such that we have a hint on an ancestor that needs to be repeated for a descendant (i.e., do the compare if either the rulenode changed or we hit one of those cases)?

Comment 4

[Boris Zbarsky [:bzbarsky]]

That's what I was talking about in comment 0, yes.  I'll try to see what I can do to make it sane.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 6

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

>+                // For aMinHint we can use NS_STYLE_HINT_FRAMECHANGE to
>+                // optimize away as much as possible.
>                 styleChange =
>-                  oldExtraContext->CalcStyleDifference(newExtraContext);
>+                  oldExtraContext->CalcStyleDifference(newExtraContext,
>+                                                       nsChangeHint(0));

The comment doesn't seem to match the code.

Why don't you need to make more intrusive changes to ReParentStyleContext too?

Comment 7

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

Boy... this is also a bit confusing that the bitsNeedingForcedCompare actually doesn't need to be propagated to descendants since it's all about 'inherit'.  It took me a bit of confusion while reading ReResolveStyleContext to remember that.  Maybe worth some comments?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> The comment doesn't seem to match the code.

Er, yes.  That comment is left over from a previous iteration of the patch.  Will fix.

> Why don't you need to make more intrusive changes to ReParentStyleContext too?

Because ReParentStyleContext does not in fact do any change hint processing.  It ignores the return value of CalcStyleDifference, apart from asserting that it doesn't include framechange.

> Maybe worth some comments?

Will add.  I wish this stuff were less confusing, for sure; maybe the new setup for this can be cleaner...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with better comments

I also added a test for tables and a change to the provider frame stuff to make sure that test keeps passing even if we remove the top/etc inherit on the outer table frame from ua.css.

Comment 10

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

Comment on attachment 412766 [details] [diff] [review]
Patch with better comments

>+  // We don't need to propagate bitsNeedingForcedCompare to our own
>+  // descendants, since they're all about changes that we inherit from

should say something like "beyond removing them from aMinChange",
since that effect does propagate to descendants, and needs to.

>+      //
>+      // Combine aMinChange and bitsNeedingForcedCompare, so we pass
>+      // in the original value of aMinChange
>       assumeDifferenceHint = ReResolveStyleContext(aPresContext, providerFrame,
>                                                    aParentContent, aChangeList,
>-                                                   aMinChange, PR_FALSE);
>+                                                   NS_CombineHint(aMinChange,
>+                                                                  bitsNeedingForcedCompare),

Doesn't this regress the original bug for which you added the code to
remove from aMinChange?  If not, why not?  (I'd have expected just not
to see this change.)

>+    aBitsNeedingCompare = nsChangeHint(NS_STYLE_HINT_FRAMECHANGE |
>+                                       nsChangeHint_UpdateCursor);

We ought to have a constant for "all change hint bits".

>+      /* It seems like we could do better here with */                        \
>+      /* MaxDifference() maybe */                                             \

I once (August 2007) wrote a patch to use MaxDifference in that code and
found something wrong with it.  Though maybe the only problem was that
it didn't compile in DEBUG builds, as I noted in
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/984eec7d581f/unapplied.style-calcdifference-hints

r=dbaron with those changes, although I'd like an answer to the
aMinChange issue

Comment 11

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

Though maybe the aMinChange doesn't actually *need* to propagate since we remove the same bits every time.  It still makes more sense to me thinking that it does...

Comment 12

[Boris Zbarsky [:bzbarsky]]

I thought about this overnight some more, and maybe the bit-propagation does need to happen to general descendants, because things inheriting from us might not be our kids in the _frame_ tree (which is what this is traversing).  I'll try to write some tests and see what things look like.  And answer the questions from comment 10.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, the tests for table pseudos fail.  Working on a better fix.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So fundamentally, we are now (in my last patch in this bug) using aMinChange to propagate two pieces of information:

1) Information about what the parent plans to do (so that we can optimize away
   doing redundant work).
2) Information about what styles might have changed on the parent (so that we
   know those styles might have changed on us too).

If the change due to item #2 might cause us to do work that isn't covered by item #1, then we need to make sure to include said change in our change hint.

Now item #1 needs to only be propagated from a parent frame to its immediate child frame, but item #2 needs to be propagated to all descendants who inherit style from the parent.

Comment 15

[Boris Zbarsky [:bzbarsky]]

And worse yet, item #2 might need to be propagated to siblings or their kids (e.g. ib splits or captions).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Better fix

More testcases (three of which the previous patch still failed), and a fix that just doesn't try to propagate information item #2.  Instead, we always compute the change for structs that might contribute a hint that won't get effectively propagated to everything in sight.  That's the padding, margin, display, and position structs.  The good news is that these should usually be cached in the ruletree, so the change computation will often optimize away via the struct pointer equality compare if the rulenode hasn't changed.  So this isn't really any slower (and yes, I did measure in a testcase in which we effectively reparent a whole bunch of style contexts).

I can add asserts based on MaxDifference, but all I could assert is that if MaxDifference doesn't include reflow bits then ForceCompare should be false.  If you think that's useful, I'll add it.

Comment 17

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

Comment on attachment 412940 [details] [diff] [review]
Better fix

r=dbaron

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/32f2d602e5b7

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.2 branch merge

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/66e6b88c8f87