906116 - replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame, etc.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I believe your analysis ought to be correct, modulo bugs.

Comment 2

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

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

Comment 3

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

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

Comment 4

[Jeremy Chen ([e-mail])]

I'd like to take this if it's okay.

Comment 5

[Jeremy Chen ([e-mail])]

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

Comment 6

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

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

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 906116 - part1: Ensure bracing all controlled statements in nsStyleStruct.h and nsStyleStruct.cpp.

Review commit: https://reviewboard.mozilla.org/r/61190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61190/

Comment 8

[Jeremy Chen ([e-mail])]

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

Comment 9

[Jeremy Chen ([e-mail])]

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

Comment 10

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

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

Comment 11

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

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

Comment 12

[Jeremy Chen ([e-mail])]

Attachment: Bug 906116 - part2: Fix comment for nsChangeHint_ReconstructFrame.

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/

Comment 13

[Jeremy Chen ([e-mail])]

Attachment: Bug 906116 - part3.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

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/

Comment 14

[Jeremy Chen ([e-mail])]

Attachment: Bug 906116 - part3.2: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

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/

Comment 15

[Jeremy Chen ([e-mail])]

Attachment: Bug 906116 - part3.3: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

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 16

[Jeremy Chen ([e-mail])]

Attachment: Bug 906116 - part4: Remove NS_STYLE_HINT_FRAMECHANGE.

Review commit: https://reviewboard.mozilla.org/r/61948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61948/

Comment 17

[Jeremy Chen ([e-mail])]

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/

Comment 18

[Jeremy Chen ([e-mail])]

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

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

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

Comment 20

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

Comment on attachment 8767476 [details]
Bug 906116 - part2: Fix comment for nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/61940/#review58856

Comment 21

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

Comment on attachment 8767477 [details]
Bug 906116 - part3.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/61942/#review58858

Comment 22

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

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.

Comment 23

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

Comment on attachment 8767479 [details]
Bug 906116 - part3.3: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/61946/#review58864

Comment 24

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

Comment on attachment 8767480 [details]
Bug 906116 - part4: Remove NS_STYLE_HINT_FRAMECHANGE.

https://reviewboard.mozilla.org/r/61948/#review58866

Comment 25

[Jeremy Chen ([e-mail])]

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.

Comment 26

[Jeremy Chen ([e-mail])]

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 27

[Jeremy Chen ([e-mail])]

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/

Comment 28

[Jeremy Chen ([e-mail])]

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/1-2/

Comment 29

[Jeremy Chen ([e-mail])]

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/1-2/

Comment 30

[Jeremy Chen ([e-mail])]

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/

Comment 31

[Jeremy Chen ([e-mail])]

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/1-2/

Comment 32

[Jeremy Chen ([e-mail])]

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/1-2/

Comment 33

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

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 34

[Jeremy Chen ([e-mail])]

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

Ｗill do. Thank you for the review.

Comment 35

[Jeremy Chen ([e-mail])]

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/3-4/

Comment 36

[Jeremy Chen ([e-mail])]

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/2-3/

Comment 37

[Jeremy Chen ([e-mail])]

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/2-3/

Comment 38

[Jeremy Chen ([e-mail])]

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/

Comment 39

[Jeremy Chen ([e-mail])]

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/2-3/

Comment 40

[Jeremy Chen ([e-mail])]

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/2-3/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
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

Comment 42

[Carsten Book [:Tomcat]]

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

Comment 43

[Pulsebot]

Backout by [e-mail]:
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

Comment 44

[Jeremy Chen ([e-mail])]

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

Comment 45

[Jeremy Chen ([e-mail])]

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 46

[Jeremy Chen ([e-mail])]

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 47

[Jeremy Chen ([e-mail])]

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 48

[Jeremy Chen ([e-mail])]

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 49

[Jeremy Chen ([e-mail])]

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 50

[Jeremy Chen ([e-mail])]

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/

Comment 51

[Jeremy Chen ([e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d2588e98ee
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ea9eae528e

Try looks good. Push to autoland again.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
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

Comment 53

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

Could you explain the changes that fixed the assertions?  It's not clear to me why they were needed.

Comment 54

[Jeremy Chen ([e-mail])]

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

Comment 55

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

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?

Comment 56

[Jeremy Chen ([e-mail])]

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

Comment 57

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/e34ddc2d2bfb
https://hg.mozilla.org/mozilla-central/rev/ebd3f5aaf153
https://hg.mozilla.org/mozilla-central/rev/938e52fcbe67
https://hg.mozilla.org/mozilla-central/rev/61599303fabe
https://hg.mozilla.org/mozilla-central/rev/b2969a93a3ab
https://hg.mozilla.org/mozilla-central/rev/94e6be97e007

Comment 58

[Jeremy Chen ([e-mail])]

(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

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e36d8767d57e
part3.2.1: Replace NS_STYLE_HINT_FRAMECHANGE with nsChangeHint_ReconstructFrame. r=me

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e36d8767d57e