That assertion is not asserting anything AFAICT. http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/layout/painting/nsCSSRendering.cpp#968

Of course, the assertion fails, d'oh :( https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba3a5b5bdc7b

Comment on attachment 8851070 [details] Bug 1350401: Cleanup outline-painting-related logic. https://reviewboard.mozilla.org/r/123508/#review126030 ::: layout/painting/nsCSSRendering.cpp:968 (Diff revision 1) > - MOZ_ASSERT(ourOutline != NS_STYLE_BORDER_STYLE_NONE, > + MOZ_ASSERT(ourOutline->mOutlineStyle != NS_STYLE_BORDER_STYLE_NONE, > "shouldn't have created nsDisplayOutline item"); > > uint8_t outlineStyle = ourOutline->mOutlineStyle; Oops, this is clearly a mistake, good catch. Perhaps we should move up the declaration of |outlineStyle| instead and assert on that value?

Looks like we have multiple callers of this function these days, not just nsDisplayOutline as the assertion suggests: http://searchfox.org/mozilla-central/search?q=symbol:_ZN14nsCSSRendering12PaintOutlineEP13nsPresContextR18nsRenderingContextP8nsIFrameRK6nsRectS8_P14nsStyleContext&redirect=false perhaps we should just move the assertion to nsDisplayOutline::Paint instead? and do an early return here for 'none'?

Comment on attachment 8851070 [details] Bug 1350401: Cleanup outline-painting-related logic. Looks like mozreview is still not great at re-requesting reviews of commits with different Mozreview-Commit-ID, so doing so manually.

Also, not good at naming stuff, so any better name for the method would be appreciated :)

Comment on attachment 8851070 [details] Bug 1350401: Cleanup outline-painting-related logic. https://reviewboard.mozilla.org/r/123508/#review126104 Nice cleanup! The suggested method name seems fine, but there is a nsStyleBorder::HasVisibleStyle which looks like it does something similar so perhaps we should name this one HasVisibleStyle() too? r=mats either way, with a couple of nits below: ::: layout/generic/nsFrame.cpp:8816 (Diff revision 2) > - const uint8_t outlineStyle = outline->mOutlineStyle; > + if (!outline->ShouldPaintOutline()) { > - if (outlineStyle == NS_STYLE_BORDER_STYLE_NONE) { > return; > } > - > nscoord width = outline->GetOutlineWidth(); This line looks a bit misplaced now. Can you move it down to where |width| is used please? (Just before the Inflate call at the end of this method) ::: layout/painting/nsCSSRendering.cpp:974 (Diff revision 2) > + uint8_t outlineStyle = ourOutline->mOutlineStyle; > + nscoord width = ourOutline->GetOutlineWidth(); Same comment for these two lines.

FYI, you will probably get a conflict on the "StyleDisplay()->mAppearance" line.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a1d828bcc845 Cleanup outline-painting-related logic. r=mats

(In reply to Mats Palmgren (:mats) from comment #8) > Nice cleanup! The suggested method name seems fine, but there is a > nsStyleBorder::HasVisibleStyle which looks like it does something similar so > perhaps we should name this one HasVisibleStyle() too? I ended up not doing it since the border method doesn't really check there's a border, but just whether the style is visible (it'd be wrong to take into account the border width there AIUI, since the border width affects layout more deeply). > This line looks a bit misplaced now. Can you move it down to where |width| Right, done :) > is used please? (Just before the Inflate call at the end of this method) > > ::: layout/painting/nsCSSRendering.cpp:974 > (Diff revision 2) > > + uint8_t outlineStyle = ourOutline->mOutlineStyle; > > + nscoord width = ourOutline->GetOutlineWidth(); > > Same comment for these two lines. Done too. Thanks for the review mats!