It still falls back even there is nothing to paint.

oh, bug 1407752 already fixed it!

Bug 1407752 doesn't seem to fix completely. I'll have another patch based on the fix in Bug 1407752.

Comment on attachment 8924045 [details] Bug 1413397 - Avoid empty border's fallback. https://reviewboard.mozilla.org/r/195266/#review200436 Looks good overall, some comments and tweaks below. ::: layout/forms/nsButtonFrameRenderer.cpp:574 (Diff revision 2) > if (GetLayerState(aDisplayListBuilder, aManager, parameter) != LAYER_ACTIVE) { > return false; > } > > + if (!mBorderRenderer) { > + return true; Add a comment here that the border must be empty. (Or you can store it in a mBorderIsEmpty field in this display item and assert it here, whichever you prefer). ::: layout/generic/nsColumnSetFrame.cpp:295 (Diff revision 2) > + if (br.isSome() && !borderIsEmpty) { > aBorderRenderers.AppendElement(br.value()); > } This change seems slightly redundant; if br.isSome() is true then borderIsEmpty must be false. I'd prefer making it an assert instead: if (br.isSome()) { MOZ_ASSERT(!borderIsEmpty); ... } ::: layout/painting/nsDisplayList.cpp:5171 (Diff revision 2) > + if (mBorderIsEmpty) { > + return LAYER_ACTIVE; > + } I think we should move this down a little to after mBorderRenderer and mBorderImageRenderer are reset to Nothing(). See next comment below. ::: layout/painting/nsDisplayList.cpp:5179 (Diff revision 2) > if ((!image || > image->GetType() != eStyleImageType_Image || > image->GetType() != eStyleImageType_Gradient) && !br) { > return LAYER_NONE; > } This is a pre-existing issue, but this entire condition is unnecessary, because it is a subset of the next condition "if (!br)". Basically here we're check "if (!br && stuff)" and the next condition is "if (!br)" so it doesn't matter if "stuff" is true or false, we return LAYER_NONE in both cases. So we can drop this condition, drop the styleBorder and image local vars, and just end up with this: mBorderRenderer = Nothing(); mBorderImageRenderer = Nothing(); if (!br) { if (mBorderIsEmpty) { return LAYER_ACTIVE; } return LAYER_NONE; } ... ::: layout/painting/nsDisplayList.cpp:5229 (Diff revision 2) > + if (mBorderIsEmpty) { > + return nullptr; > + } Does this affect the WR codepath at all? It seems like we shouldn't be calling BuildLayer for WR.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8) > ::: layout/painting/nsDisplayList.cpp:5229 > (Diff revision 2) > > + if (mBorderIsEmpty) { > > + return nullptr; > > + } > > Does this affect the WR codepath at all? It seems like we shouldn't be > calling BuildLayer for WR. It doesn't affect WR codepath. But the advanced border layer may build layer. Actually WR doesn't use nsDisplayBorder::GetLayerState neither. I'll file a bug to refactor the function.

The try push linked from the MozReview looks like it has a couple of failures. Other than that the patch looks fine to me, thanks for addressing all the comments!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13) > The try push linked from the MozReview looks like it has a couple of > failures. Other than that the patch looks fine to me, thanks for addressing > all the comments! I think those failures come from nsDisplaymtdBorder! The nsDisplaymtdBorder inherits from nsDisplayBorder. I override the CreateWebRenderCommands in nsDisplaymtdBorder to fix the problem.

Pushed by ethlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a16aef250227 Avoid empty border's fallback. r=kats