Follow-up from bug 524925 which fixed outline and transform properties to use UpdateOverflow. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #107) > I also suggest filing a followup bug to have a good look at CalcDifference > implementations and see which reflow hints can be converted to > UpdateOverflow. I think there's quite a few that can be converted...

Mats, can you do this? This should be some cheap and easy performance wins...

Fwiw, green on Try: https://tbpl.mozilla.org/?tree=Try&rev=5e40e8880f93

Comment on attachment 590378 [details] [diff] [review] Changes to 'text-shadow' and 'box-shadow' only needs to update the overflow areas and repaint, not reflow. Changes to border style/color/etc only needs repaint, not SyncFrameView. Review of attachment 590378 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleStruct.cpp @@ +2794,5 @@ > if (lhs == rhs) > return NS_STYLE_HINT_NONE; > > + const nsChangeHint kUpdateOverflowAndRepaintHint = > + NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame); This can be static as well as const

Comment on attachment 590380 [details] [diff] [review] Changes to '-moz-text-blink' and some 'text-decoration-style' values only need to update the overflow areas and repaint, not reflow. Review of attachment 590380 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleStruct.cpp @@ +2745,5 @@ > + return NS_STYLE_HINT_REFLOW; > + } > + > + const nsChangeHint kUpdateOverflowAndRepaintHint = > + NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame); can be static

nsTextFrame::SetSelectedRange does some reflowing to update overflow areas in case the selection drawing requires overflow area changes. I think probably nsTextFrame::UnionAdditionalOverflow and SetSelectedRange can be simplified now. Maybe that requires another bug.

Comment on attachment 590381 [details] [diff] [review] Introduce NS_STYLE_HINT_OVERFLOW as a short-hand for nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_UpdateOverflow). Review of attachment 590381 [details] [diff] [review]: ----------------------------------------------------------------- I actually prefer the overflowAndRepaint names. It's just more clear and doesn't impact the wordiness of the code much. So I'd prefer to just leave it as is (and get rid of more of those shortcuts). ::: layout/base/nsChangeHint.h @@ +143,5 @@ > nsChangeHint_ClearDescendantIntrinsics | \ > nsChangeHint_NeedDirtyReflow) > #define NS_STYLE_HINT_REFLOW \ > nsChangeHint(nsChangeHint_RepaintFrame | nsChangeHint_ReflowFrame) > +#define NS_STYLE_HINT_OVERFLOW \ NS_STYLE_HINT_UPDATE_OVERFLOW I think

I haven't done an audit myself, but one more opportunity I know about is nsStyleSVGReset::CalcDifference ... mFilter can update overflow area instead of reflowing, and mClipPath and mMask don't even need to do that (since they don't affect the overflow area).

I'd like a short-hand for this in some form, let's go with NS_STYLE_HINT_UPDATE_OVERFLOW for now since it follows the current scheme. We can re-organize / rename the nsChangeHint stuff in a separate bug. The change is just s/NS_STYLE_HINT_OVERFLOW/NS_STYLE_HINT_UPDATE_OVERFLOW/

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > nsTextFrame::SetSelectedRange does some reflowing to update overflow areas > in case the selection drawing requires overflow area changes. I think > probably nsTextFrame::UnionAdditionalOverflow and SetSelectedRange can be > simplified now. Maybe that requires another bug. Filed bug 720270.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12) > I haven't done an audit myself, but one more opportunity I know about is > nsStyleSVGReset::CalcDifference ... mFilter can update overflow area instead > of reflowing, and mClipPath and mMask don't even need to do that (since they > don't affect the overflow area). The SVG effects stuff is really complicated (to me) so I think this deserves deeper analysis. Filed bug 720272 for that. Let's take the current improvements as a first step.

OK. nsStyleDisplay::mClip shouldn't need to reflow. I can't see any more cases (other than the SVG cases).

I also moved the ReconstructFrame cases to the start, and return early if hit. layout/reftests/bugs/389224-1.html failed with this change, not sure why. I changed it to trigger the style change of the 'load' event instead of 'MozReftestInvalidate', then it works.

(In reply to Mats Palmgren [:mats] from comment #17) > layout/reftests/bugs/389224-1.html failed with this change, not sure why. > I changed it to trigger the style change of the 'load' event instead of > 'MozReftestInvalidate', then it works. That sounds like you're just hiding the bug. Please debug!

You're right, sorry. It looked like just flaky test at first but it does indeed show a real bug with the UpdateOverflow code. Same as bug 720987 if I'm not mistaken. I'll post the bug fix there and block on that.

Updated to tip since there was a conflicting change to nsStyleVisibility::CalcDifference

Identical to the last version but without the change to the test so if you already did review the code part you can just rubber-stamp this. The fix in bug 720987 makes the test pass. The problem was that the stored overflow areas is the clipped areas and there's no code to pick up the full unclipped child overflow again in the current code so the areas remained the same after the style change. Try results: https://tbpl.mozilla.org/?tree=Try&rev=4f0758b3d061

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f11aaac24dc https://hg.mozilla.org/integration/mozilla-inbound/rev/f222fbece983 https://hg.mozilla.org/integration/mozilla-inbound/rev/0246973f2513 https://hg.mozilla.org/integration/mozilla-inbound/rev/13739446fc8e https://hg.mozilla.org/integration/mozilla-inbound/rev/07bd73451aa4 https://hg.mozilla.org/integration/mozilla-inbound/rev/2cae7e5c62ae

https://hg.mozilla.org/mozilla-central/rev/2cae7e5c62ae https://hg.mozilla.org/mozilla-central/rev/07bd73451aa4 https://hg.mozilla.org/mozilla-central/rev/13739446fc8e https://hg.mozilla.org/mozilla-central/rev/0246973f2513 https://hg.mozilla.org/mozilla-central/rev/f222fbece983 https://hg.mozilla.org/mozilla-central/rev/8f11aaac24dc

These patches will be backed out in bug 724432. I intend to re-land most of it with some additional fixes here, except text-shadow which is handled by bug 723669.

What I think I'd written elsewhere: if we want to do this, I think we should: * rename UpdateOverflow to UpdateAncestorOverflow * add a new hint called UpdateSelfOverflow * implement the new hint such that we start from accumulating the element's overflow from its descendants and its on characteristics (rather than starting with the change from pre-transform overflow to post-transform overflow on the element)

Also, I'd prefer not to have NS_STYLE_HINT_UPDATE_OVERFLOW for two reasons: * we should get rid of NS_STYLE_HINT_* and use nsChangeHint_* * I'd rather actually have to have the combination written out so that it's clearer.

A good bit of the discussion about relanding this is in bug 723669.

The prerequisites for doing this are now happening in bug 984226.