Closed Bug 906116 Opened 7 years ago Closed 4 years ago

replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame, etc.

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: dholbert, Assigned: chenpighead)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files)

Bug 906116 - part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 906116 - part2: Fix comment for nsChangeHint_ReconstructFrame.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 906116 - part3.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 906116 - part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 906116 - part3.3: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 906116 - part4: Remove NS_STYLE_HINT_FRAMECHANGE.
58 bytes, text/x-review-board-request
dbaron
: review+
Details 
Right now, some cases in nsStyleStruct.cpp do:
 NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
while other cases do:
 return NS_STYLE_HINT_FRAMECHANGE;

I *think* the distinction between those values is meaningless. The _FRAMECHANGE macro is defined as follows:
> 217 #define NS_STYLE_HINT_VISUAL \
> 218   nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_SyncFrameView)
> 219 #define nsChangeHint_AllReflowHints                     \
> 220   nsChangeHint(nsChangeHint_NeedReflow |                \
> 221                nsChangeHint_ClearAncestorIntrinsics |   \
> 222                nsChangeHint_ClearDescendantIntrinsics | \
> 223                nsChangeHint_NeedDirtyReflow)
> 224 #define NS_STYLE_HINT_REFLOW \
> 225   nsChangeHint(NS_STYLE_HINT_VISUAL | nsChangeHint_AllReflowHints)
> 226 #define NS_STYLE_HINT_FRAMECHANGE \
> 227   nsChangeHint(NS_STYLE_HINT_REFLOW | nsChangeHint_ReconstructFrame)

So it's got these bits set:
 nsChangeHint_RepaintFrame
 nsChangeHint_SyncFrameView
 nsChangeHint_NeedReflow
 nsChangeHint_ClearAncestorIntrinsics
 nsChangeHint_ClearDescendantIntrinsics
 nsChangeHint_NeedDirtyReflow
in addition to having the nsChangeHint_ReconstructFrame bit set.

But all those other bits are listed *above* nsChangeHint_ReconstructFrame in the enum list...
  http://mxr.mozilla.org/mozilla-central/source/layout/base/nsChangeHint.h#226
...and the documentation for nsChangeHint_ReconstructFrame in the enum list says "This subsumes all the above"
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsChangeHint.h#69

So at least according to that documentation, there shouldn't be any reason to bother setting any of those other bits when we're setting nsChangeHint_ReconstructFrame.

Filing this bug on doing a more thorough investigation and possibly ripping out NS_STYLE_HINT_FRAMECHANGE and replacing it with just nsChangeHint_ReconstructFrame.
I believe your analysis ought to be correct, modulo bugs.
The actual hint handling code in RestyleManager::ProcessRestyledFrames ignores all the other bits if nsChangeHint_ReconstructFrame is set, so I think we can largely do the replacement described, except for fixing up the nsStyle*::MaxDifference methods.

(I think that hint handling might be incorrect for nsChangeHint_ReconstructFrame | nsChangeHint_UpdateCursor, though.)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #2)
> (I think that hint handling might be incorrect for
> nsChangeHint_ReconstructFrame | nsChangeHint_UpdateCursor, though.)

Er, that's actually fine, since we call SynthesizeMouseMove from PresShell::DidDoReflow.
Summary: Investigate difference (if any) between nsChangeHint_ReconstructFrame and NS_STYLE_HINT_FRAMECHANGE → replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame, etc.
I'd like to take this if it's okay.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
I dug a little, and found that NS_STYLE_HINT_VISUAL was extended by one additional flag (nsChangeHint_SchedulePaint) after Bug 1038781. Since nsChangeHint_SchedulePaint [1] is not subsumed in nsChangeHint_ReconstructFrame [2], we can't simply replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

I thought about changing NS_STYLE_HINT_FRAMECHANGE's #define macro to "nsChangeHint_SchedulePaint | nsChangeHint_ReconstructFrame". But it won't gain us much good, and might degrade the readability instead.

Hi dbaron, due to the current situation, I'm wondering if this bug is still worth to fix?


[1] https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/layout/base/nsChangeHint.h#152
[2] https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/layout/base/nsChangeHint.h#76
Flags: needinfo?(dbaron)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #5)
> I dug a little, and found that NS_STYLE_HINT_VISUAL was extended by one
> additional flag (nsChangeHint_SchedulePaint) after Bug 1038781. Since
> nsChangeHint_SchedulePaint [1] is not subsumed in
> nsChangeHint_ReconstructFrame [2], we can't simply replace
> NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 2ab57664bf7cafdd64e136e341527c275fc8c3aa/layout/base/nsChangeHint.h#76

The comment ("above") is out of date; it was written when ReconstructFrame was listed last.  If you look at how we *process* change hints:
http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/layout/base/RestyleManager.cpp#827
we ignore all of the other hints if ReconstructFrame is set.


I'd still like to get rid of all the NS_STYLE_HINT_* constants.
Flags: needinfo?(dbaron)
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #6)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #5)
> > I dug a little, and found that NS_STYLE_HINT_VISUAL was extended by one
> > additional flag (nsChangeHint_SchedulePaint) after Bug 1038781. Since
> > nsChangeHint_SchedulePaint [1] is not subsumed in
> > nsChangeHint_ReconstructFrame [2], we can't simply replace
> > NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.
> 
> > [2]
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 2ab57664bf7cafdd64e136e341527c275fc8c3aa/layout/base/nsChangeHint.h#76
> 
> The comment ("above") is out of date; it was written when ReconstructFrame
> was listed last.  If you look at how we *process* change hints:
> http://searchfox.org/mozilla-central/rev/
> ef24c234ed53b3ba50a1734f6b946942e4434b5b/layout/base/RestyleManager.cpp#827
> we ignore all of the other hints if ReconstructFrame is set.
> 
> 
> I'd still like to get rid of all the NS_STYLE_HINT_* constants.

Thank you for the feedback. Then, the goal would be to confirm that nsChangeHint_ReconstructFrame do subsume all other nsChangeHints. Here's a plan:

1. Simply replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame and see if anything is broken.
2. If everything goes well, we should be able to further purge unnecessary OR operation between nsChangeHint_* and nsChangeHint_ReconstructFrame.
3. Since I'm here, some coding style could be fixed as well. (e.g. brace controlled statements)
> 1. Simply replace NS_STYLE_HINT_FRAMECHANGE with
> nsChangeHint_ReconstructFrame and see if anything is broken.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc1a591d8435&selectedJob=23095507

Looks like we have a check in debug build here:
http://searchfox.org/mozilla-central/rev/261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/layout/style/nsStyleContext.cpp#1449-1450

So, if we're planning to replace NS_STYLE_HINT_FRAMECHANGE with a one flag, nsChangeHint_ReconstructFrame, and also keep the check alive, we might want to ensure nsChangeHint_ReconstructFrame is a nsChangeHint that has an enum value which does covers all the other hints.

I'm considering to change nsChangeHint_ReconstructFrame to always be the union of all the other hints.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #9)
> Looks like we have a check in debug build here:
> http://searchfox.org/mozilla-central/rev/
> 261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/layout/style/nsStyleContext.
> cpp#1449-1450
> 
> So, if we're planning to replace NS_STYLE_HINT_FRAMECHANGE with a one flag,
> nsChangeHint_ReconstructFrame, and also keep the check alive, we might want
> to ensure nsChangeHint_ReconstructFrame is a nsChangeHint that has an enum
> value which does covers all the other hints.

No, we should update the MaxDifference() methods on the various nsStyle* structs to | in the other bits that are needed rather than relying on NS_STYLE_HINT_FRAMECHANGE.

Alternatively, you could start by removing all of the NS_STYLE_HINT_FRAMECHANGE uses *other* than those in MaxDifference methods, and then later working on the MaxDifference methods.

> I'm considering to change nsChangeHint_ReconstructFrame to always be the
> union of all the other hints.

We should not do that.
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #10)
> > I'm considering to change nsChangeHint_ReconstructFrame to always be the
> > union of all the other hints.
> 
> We should not do that.

On second thought, maybe this isn't so bad for framechange.  However, for all of the other NS_STYLE_HINT_* we do not want to do that, since we can get performance gains from using only the change hints that we want.

So perhaps NS_STYLE_HINT_FRAMECHANGE is the lowest priority of the NS_STYLE_HINT_* to eliminate.
The comment for nsChangeHint_ReconstructFrame is out-of-date.

In RestyleManager::ProcessRestyledFrames, we actually ignore all of the other
hints if ReconstructFrame is set. The old version was written when
ReconstructFrame was listed last. So, update the comment.

Review commit: https://reviewboard.mozilla.org/r/61940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61940/
Attachment #8766210 - Attachment description: Bug 906116 - part1: replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. → Bug 906116 - part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp.
Attachment #8767476 - Flags: review?(dbaron)
Attachment #8767477 - Flags: review?(dbaron)
Attachment #8767478 - Flags: review?(dbaron)
Attachment #8767479 - Flags: review?(dbaron)
Attachment #8767480 - Flags: review?(dbaron)
Attachment #8766210 - Flags: review?(dbaron)
Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame in
nsStyle*::CalcDifference.

Review commit: https://reviewboard.mozilla.org/r/61942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61942/
Use ReconstructFrame | (any ther bits needed) to replace
NS_STYLE_HINT_FRAMECHANGE in nsStyle*::MaxDifference.

Review commit: https://reviewboard.mozilla.org/r/61944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61944/
Use ReconstructFrame to replace NS_STYLE_HINT_FRAMECHANGE in many places, such
as HTML*Element::GetAttributeChangeHint and HTMLFrameSetElement::SetAttr.

Review commit: https://reviewboard.mozilla.org/r/61946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61946/
Comment on attachment 8766210 [details]
Bug 906116 - part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61190/diff/1-2/
(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #6)

> I'd still like to get rid of all the NS_STYLE_HINT_* constants.

Sure, we should be able to eliminate rest of the NS_STYLE_HINT_* constants through similar steps used in this bug. 

I'm not planning to do all the work here. After this bug, maybe me (or someone) can refer to this bug and directly eliminate rest of the NS_STYLE_HINT_* constants in bug 1077851.
Comment on attachment 8766210 [details]
Bug 906116 - part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp.

https://reviewboard.mozilla.org/r/61190/#review58854
Attachment #8766210 - Flags: review?(dbaron) → review+
Attachment #8767476 - Flags: review?(dbaron) → review+
Comment on attachment 8767477 [details]
Bug 906116 - part3.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/61942/#review58858
Attachment #8767477 - Flags: review?(dbaron) → review+
Comment on attachment 8767478 [details]
Bug 906116 - part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/61944/#review58860

::: layout/style/nsStyleStruct.h:1438
(Diff revision 1)
> -    return NS_STYLE_HINT_FRAMECHANGE |
> +    return nsChangeHint_ReconstructFrame |
> +           nsChangeHint_RepaintFrame |
> +           nsChangeHint_SyncFrameView |
> +           nsChangeHint_SchedulePaint |
> +           nsChangeHint_NeedReflow |
> +           nsChangeHint_ReflowChangesSizeOrPosition |
> +           nsChangeHint_ClearAncestorIntrinsics |
> +           nsChangeHint_ClearDescendantIntrinsics |
> +           nsChangeHint_NeedDirtyReflow |

I think it's better to write these in the way that they're written in the CalcDifference methods.

For example, in this nsStyleList case, it should probably be written as
nsChangeHint_ReconstructFrame |
NS_STYLE_HINT_REFLOW |
NS_STYLE_HINT_VISUAL;

Then we should fix up the other parts as we switch to appropriate bits replacing NS_STYLE_HINT_REFLOW and NS_STYLE_HINT_VISUAL.

(I don't think it needs nsChangeHint_NeutralChange.)

This applies to most/all of the changes in this patch.
Attachment #8767478 - Flags: review?(dbaron) → review-
Comment on attachment 8767479 [details]
Bug 906116 - part3.3: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/61946/#review58864
Attachment #8767479 - Flags: review?(dbaron) → review+
Attachment #8767480 - Flags: review?(dbaron) → review+
https://reviewboard.mozilla.org/r/61944/#review58860

> I think it's better to write these in the way that they're written in the CalcDifference methods.
> 
> For example, in this nsStyleList case, it should probably be written as
> nsChangeHint_ReconstructFrame |
> NS_STYLE_HINT_REFLOW |
> NS_STYLE_HINT_VISUAL;
> 
> Then we should fix up the other parts as we switch to appropriate bits replacing NS_STYLE_HINT_REFLOW and NS_STYLE_HINT_VISUAL.
> 
> (I don't think it needs nsChangeHint_NeutralChange.)
> 
> This applies to most/all of the changes in this patch.

You're right. For those nsStyle* that do not have nsChangeHint_NeutralChange in its CalcDifference, we should be able to elimate nsChangeHint_NeutralChange from its MaxDifference as well.

Will address your comment and elimate unnecessary use of nsChangeHint_NeutralChange in next version.
https://reviewboard.mozilla.org/r/61944/#review58860

> You're right. For those nsStyle* that do not have nsChangeHint_NeutralChange in its CalcDifference, we should be able to elimate nsChangeHint_NeutralChange from its MaxDifference as well.
> 
> Will address your comment and elimate unnecessary use of nsChangeHint_NeutralChange in next version.

s/elimate/eliminate
Comment on attachment 8766210 [details]
Bug 906116 - part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61190/diff/2-3/
Attachment #8767478 - Flags: review- → review?(dbaron)
Comment on attachment 8767478 [details]
Bug 906116 - part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61944/diff/1-2/
Attachment #8767478 - Flags: review?(dbaron) → review+
Comment on attachment 8767478 [details]
Bug 906116 - part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/61944/#review59142

r=dbaron with the following two methods fixed:

::: layout/style/nsStyleStruct.h:2778
(Diff revision 2)
> -    return NS_STYLE_HINT_FRAMECHANGE;
> +    return nsChangeHint_ReconstructFrame |
> +           NS_STYLE_HINT_REFLOW;

This should just be nsChangeHint_ReconstructFrame, since nsStyleTable::CalcDifference produces no other hints.

::: layout/style/nsStyleStruct.h:3095
(Diff revision 2)
>    }
>  
>    nsChangeHint CalcDifference(const nsStyleUserInterface& aNewData) const;
>    static nsChangeHint MaxDifference() {
> -    return NS_STYLE_HINT_FRAMECHANGE |
> +    return nsChangeHint_ReconstructFrame |
> +           NS_STYLE_HINT_REFLOW |

Instead of NS_STYLE_HINT_REFLOW, this should be:
 nsChangeHint_NeedReflow |
 nsChangeHint_NeedDirtyReflow |
 NS_STYLE_HINT_VISUAL
since that's what nsStyleUserInterface::CalcDifference produces
Comment on attachment 8767478 [details]
Bug 906116 - part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61944/diff/2-3/
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f4e9c57ee8e
part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/d84c0edb6912
part2: Fix comment for nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/17d17aeec1fa
part3.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/34c54dbb1b7d
part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/8c1f9996a7d6
part3.3: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/1ec6e0357c42
part4: Remove NS_STYLE_HINT_FRAMECHANGE. r=dbaron
backout for bustage/crashes like  Assertion failure: NS_IsHintSubset(nsStyleUserInterface::DifferenceAlwaysHandledForDescendants(), nsStyleUserInterface::MaxDifference()), at c:\builds\moz2_slave\autoland-w64-d-000000000000000\build\src\obj-firefox\layout\style\nsStyleStructList.h:65
Flags: needinfo?(jeremychen)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a2cec4fd398
Backed out changeset 1ec6e0357c42 
https://hg.mozilla.org/integration/autoland/rev/19b7de94186b
Backed out changeset 8c1f9996a7d6 
https://hg.mozilla.org/integration/autoland/rev/b76169d840fc
Backed out changeset 34c54dbb1b7d 
https://hg.mozilla.org/integration/autoland/rev/7de0043e22d6
Backed out changeset 17d17aeec1fa 
https://hg.mozilla.org/integration/autoland/rev/b4cd9f6f9187
Backed out changeset d84c0edb6912 
https://hg.mozilla.org/integration/autoland/rev/a7d6bb9e7d12
Backed out changeset 0f4e9c57ee8e for bustage on a CLOSED TREE
(In reply to Carsten Book [:Tomcat] from comment #42)
> backout for bustage/crashes like  Assertion failure:
> NS_IsHintSubset(nsStyleUserInterface::
> DifferenceAlwaysHandledForDescendants(),
> nsStyleUserInterface::MaxDifference()), at
> c:\builds\moz2_slave\autoland-w64-d-000000000000000\build\src\obj-
> firefox\layout\style\nsStyleStructList.h:65

Already have a patch. Would re-push after a positive try result.
Flags: needinfo?(jeremychen)
Comment on attachment 8766210 [details]
Bug 906116 - part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61190/diff/4-5/
Comment on attachment 8767476 [details]
Bug 906116 - part2: Fix comment for nsChangeHint_ReconstructFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61940/diff/3-4/
Comment on attachment 8767477 [details]
Bug 906116 - part3.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61942/diff/3-4/
Comment on attachment 8767478 [details]
Bug 906116 - part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61944/diff/3-4/
Comment on attachment 8767479 [details]
Bug 906116 - part3.3: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61946/diff/3-4/
Comment on attachment 8767480 [details]
Bug 906116 - part4: Remove NS_STYLE_HINT_FRAMECHANGE.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61948/diff/3-4/
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e34ddc2d2bfb
part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/ebd3f5aaf153
part2: Fix comment for nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/938e52fcbe67
part3.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/61599303fabe
part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/b2969a93a3ab
part3.3: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/94e6be97e007
part4: Remove NS_STYLE_HINT_FRAMECHANGE. r=dbaron
Could you explain the changes that fixed the assertions?  It's not clear to me why they were needed.
The assertion added in Bug 897763, http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/layout/style/nsStyleContext.cpp#1445-1446, is for checking that nsStyle*:DifferenceAlwaysHandledForDescendants should always be a subset of nsStyle*::MaxDifference. So, I fixed the assertions by adding nsChangeHints which are in nsStyle*::DifferenceAlwaysHandledForDescendants but not in nsStyle*::MaxDifference. This explains the changes I made in https://reviewboard.mozilla.org/r/61944/diff/3-4/.
I think the right fix there would have been to change the DifferenceAlwaysHandledForDescendants methods on nsStyleTable and nsStyleUserInterface.  In nsStyleTable it should just be:
    // CalcDifference never returns the reflow hints that are sometimes
    // handled for descendants at all.
    return nsChangeHint(0);
but in nsStyleUserInterface it would still need to return the nsChangeHint_NeedReflow hint.

Could you fix that in a followup patch?
Flags: needinfo?(jeremychen)
(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #55)
> I think the right fix there would have been to change the
> DifferenceAlwaysHandledForDescendants methods on nsStyleTable and
> nsStyleUserInterface.  In nsStyleTable it should just be:
>     // CalcDifference never returns the reflow hints that are sometimes
>     // handled for descendants at all.
>     return nsChangeHint(0);
> but in nsStyleUserInterface it would still need to return the
> nsChangeHint_NeedReflow hint.
> 
> Could you fix that in a followup patch?

Yes, I'd like to.
Keep the ni for now. Will clear ni after I fix this.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #56)
> (In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain
> patch) from comment #55)
> > I think the right fix there would have been to change the
> > DifferenceAlwaysHandledForDescendants methods on nsStyleTable and
> > nsStyleUserInterface.  In nsStyleTable it should just be:
> >     // CalcDifference never returns the reflow hints that are sometimes
> >     // handled for descendants at all.
> >     return nsChangeHint(0);
> > but in nsStyleUserInterface it would still need to return the
> > nsChangeHint_NeedReflow hint.
> > 
> > Could you fix that in a followup patch?
> 
> Yes, I'd like to.
> Keep the ni for now. Will clear ni after I fix this.

Wait for positive try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ec96ca983b9
Flags: needinfo?(jeremychen)
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e36d8767d57e
part3.2.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=me
You need to log in before you can comment on or make changes to this bug.