Closed Bug 1413397 Opened 6 years ago Closed 6 years ago

Empty border should not fall back

Categories

(Core :: Graphics: WebRender, defect, P1)

Product:
Core
Component:
Graphics: WebRender
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [wr-mvp])

Attachments

(1 file, 1 obsolete file)

Bug 1413397 - Avoid empty border's fallback.
59 bytes, text/x-review-board-request
kats
: review+
Details 
It still falls back even there is nothing to paint.
Blocks: 1407744
Whiteboard: [wr-mvp] [triage]
Summary: Avoid nsDisplayFieldSetBorder fallback on Gmail → Empty border should not fall back
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
oh, bug 1407752 already fixed it!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Attachment #8924043 - Flags: review?(bugmail)
Attachment #8924045 - Flags: review?(bugmail)
Bug 1407752 doesn't seem to fix completely. I'll have another patch based on the fix in Bug 1407752.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attachment #8924043 - Attachment is obsolete: true
Attachment #8924043 - Flags: review?(bugmail)
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.
Attachment #8924045 - Flags: review?(bugmail) → review+
(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
Blocks: 1414211
https://hg.mozilla.org/mozilla-central/rev/a16aef250227
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.