Right now, we return NS_STYLE_HINT_VISUAL in a lot of places in nsStyleStruct.cpp where really there's just a color or text-decorations changing (things which just require a simple repaint). Ostensibly, we *should* be able to just return nsChangeHint_RepaintFrame for these sorts of changes. Quoting its documentation: > 20 // change was visual only (e.g., COLOR=) > 21 // Invalidates all descendant frames (including following > 22 // placeholders to out-of-flow frames). In contrast, NS_STYLE_HINT_VISUAL is a bitfield which includes that change hint as well as two more: * nsChangeHint_SyncFrameView, which does some view-specific stuff. (tn says it triggers a call to nsContainerFrame::SyncFrameViewProperties, which just does z-index stuff and visibility hidden for iframes). This doesn't seem like something we need for a color . * nsChangeHint_SchedulePaint, which is documented as being "useful if something has changed which will be invalidated by DLBI." (This is not the case for e.g. a color or text-decoration change - those aren't covered by DLBI.) So I think most NS_STYLE_HINT_VISUAL usages are overkill, and we can simply return nsChangeHint_RepaintFrame. (We already do this for e.g. "outline-color" changes. I don't see why "border-color", "text-decoration", etc. are any different.) [Note: my motivation in filing this bug is that, in bug 1194480, I want to add an early-return to nsStyleBorder::CalcDifference which returns nsChangeHint_RepaintFrame combined with a few other bits. This early-return will only be valid if it subsumes return statements after it -- and it won't technically subsume those return statements right now, because they use NS_STYLE_HINT_VISUAL which has these other (arguably-unnecessary) bits.]

Here's a first pass at a fix, just replacing instances where I'm fairly sure it makes sense. (There are a few cases I'm unsure about and am hence leaving untouched, e.g. mUserSelect in nsStyleUIReset::CalcDifference. I think this is fine; this doesn't have to be a complete fix, and it's probably better that it's not a complete fix from a granular-regression-tracking standpoint.) Try run to see if this breaks anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fc26c56e486

Try run is looking good. Making a few minor tweaks: - adjusted a few MaxDifference() impls where NS_STYLE_HINT_VISUAL was part of the returned value but can no longer be returned from CalcDifference. (Other MaxDifference() methods return strictly-stronger values like NS_STYLE_HINT_REFLOW and hence don't need modifying.) - Added back nsChangeHint_SchedulePaint [part of NS_STYLE_HINT_VISUAL] at one spot, for text-decoration-line, since I think it might actually be needed there (because we're updating the overflow regions with nsChangeHint_UpdateSubtreeOverflow and hence would benefit from an invalidating paint). Try run for updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e320952cdeeb

Comment on attachment 8653076 [details] [diff] [review] fix v2 (heycam, if you're comfortable reviewing this, great! If not, please punt to dbaron, possibly with 'needinfo' if he's still in no-review-requests mode.)

Happy to look at this, but it will probably have to be next week (when dbaron and I are both back).

Sure, no rush. Thanks! [CC'ing dbaron so that this is on his radar, too.]

Try run from comment 2 is 99% done and looks green (aside from a few intermittent failures) - hooray!

Comment on attachment 8653076 [details] [diff] [review] fix v2 Review of attachment 8653076 [details] [diff] [review]: ----------------------------------------------------------------- While looking through nsStyleStruct.cpp I saw bz's comments on CalcShadowDifference. Could you file a bug on that? We should fix that up to return nsChangeHint_RepaintFrame where appropriate. ::: layout/style/nsStyleStruct.cpp @@ +548,5 @@ > } > > // Note that mBorderStyle stores not only the border style but also > // color-related flags. Given that we've already done an mComputedBorder > + // comparison, border-style differences can only lead to a visual hint. So I'd probably update "visual" to "repaint" (and in the previous comment too). @@ +3344,5 @@ > uint8_t lineStyle = GetDecorationStyle(); > uint8_t otherLineStyle = aOther.GetDecorationStyle(); > if (mTextDecorationLine != aOther.mTextDecorationLine || > lineStyle != otherLineStyle) { > // Repaint for other style decoration lines because they must be in This comment looks slightly inaccurate (before your change here, too). Mind updating it? @@ +3351,2 @@ > NS_UpdateHint(hint, nsChangeHint_UpdateSubtreeOverflow); > + NS_UpdateHint(hint, nsChangeHint_SchedulePaint); We presumably need to do this because decoration changes can cause new visual overflow rects. This makes me wonder whether we ever care about the nsFont::decorations difference checked for in CalcFontDifference.

(In reply to Cameron McCormack (:heycam) from comment #7) > While looking through nsStyleStruct.cpp I saw bz's comments on > CalcShadowDifference. Could you file a bug on that? We should fix that up > to return nsChangeHint_RepaintFrame where appropriate. Good news! Bug 1194480 already covers that, and I've got a patch over there. (Though there's an assertion-failure I need to debug before that can land.) > I'd probably update "visual" to "repaint" (and in the previous comment too). Cool, fixed both. > > // Repaint for other style decoration lines because they must be in > > This comment looks slightly inaccurate (before your change here, too). Mind > updating it? I think you're talking about how the comment implies that all changes must be inside of our overflow rect (if I'm reading it correctly), but on the other hand we actually send a UpdateSubtreeOverflow change-hint [and now SchedulePaint as well] -- is that what you're pointing out? If so: yes, I think the comment needs updating to match reality. Looks like that change (sending the UpdateSubtreeOverflow change hint) happened here, for the record: http://hg.mozilla.org/mozilla-central/diff/75618ce20f68/layout/style/nsStyleStruct.cpp I'll take a look and update that comment to something that matches current reality before landing.

Here's the updated patch. Try run to be sure this still doesn't break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50f27d1778e7

Disregard prev. Try run link; here's one that actually requests unit tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=720c2b0b422b