Add webrender support for TableBCBorder

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ethlin, Assigned: mtseng)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

For the first round DisplayItem Conversion, we should add WR support for TableBorderBackground. I set the assignee to myself for tracking.
The rendering in nsDisplayTableBorderBackground is pretty complicated. I would like to take this bug as a meta bug and it's easier to divide the work.
Depends on: 1346141
It may be worth finishing bug 929484 before doing this work.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> It may be worth finishing bug 929484 before doing this work.

Thanks. After bug 929484, we don't need to convert TableBorderBackground directly. The TableBorderBackground will be split into TableBorderCollapse, Background. I'll see how to convert TableBorderCollapse to wr display item.
Depends on: 929484
Depends on: 1348755
Assignee: ethlin → mtseng
Comment on attachment 8858719 [details]
Bug 1344082 - Divided nsDisplayTableBorderBackground into nsDisplayTableBorder and nsDisplayTableBackground.

https://reviewboard.mozilla.org/r/130752/#review133582
Attachment #8858719 - Flags: review?(matt.woodrow) → review+
Attachment #8858720 - Flags: review?(matt.woodrow) → review?(mstange)
Comment on attachment 8858720 [details]
Bug 1344082 - Create webrender commands for nsDisplayTableBorderCollapse.

https://reviewboard.mozilla.org/r/130754/#review134008

I did a first pass of a review, I'm going to continue reviewing tomorrow.

::: layout/tables/nsTableFrame.h:314
(Diff revision 1)
>  
>    void AddBCDamageArea(const mozilla::TableArea& aValue);
>    bool BCRecalcNeeded(nsStyleContext* aOldStyleContext,
>                          nsStyleContext* aNewStyleContext);
>    void PaintBCBorders(DrawTarget& aDrawTarget, const nsRect& aDirtyRect);
> +  void CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,

Let's rename this to CreateWebRenderCommandsForBCBorders. Otherwise, the reader might assume that the webrender commands are for painting the whole table.

::: layout/tables/nsTableFrame.cpp:7260
(Diff revision 1)
> +                                     nscolor& aBorderColor,
> +                                     nscolor& aBGColor,
> +                                     nsRect& aBorderRect,
> +                                     int32_t& aAppUnitsPerDevPixel,
> +                                     uint8_t& aStartBevelSide,
> +                                     nscoord& aStartBevelOffset,
> +                                     uint8_t& aEndBevelSide,
> +                                     nscoord& aEndBevelOffset)

Please create a struct for these and return it as a return value, not as a series of outparameters.
Summary: Add webrender support for TableBorderBackground → Add webrender support for TableBCBorder
Comment on attachment 8858720 [details]
Bug 1344082 - Create webrender commands for nsDisplayTableBorderCollapse.

https://reviewboard.mozilla.org/r/130754/#review137000

::: layout/tables/nsTableFrame.h:317
(Diff revision 2)
>                          nsStyleContext* aNewStyleContext);
>    void PaintBCBorders(DrawTarget& aDrawTarget, const nsRect& aDirtyRect);
> +  void CreateWebRenderCommandsForBCBorders(mozilla::wr::DisplayListBuilder& aBuilder,
> +                                           nsTArray<mozilla::layers::WebRenderParentCommand>& aParentCommands,
> +                                           mozilla::layers::WebRenderDisplayItemLayer* aLayer,
> +                                           const nsPoint& aPt);

What's the meaning of aPt? It would be nice to find a more predictive name, or to add a comment about the meaning of this argument.

::: layout/tables/nsTableFrame.cpp:7646
(Diff revision 2)
> +BCInlineDirSeg::CreateWebRenderCommands(BCPaintBorderIterator& aIter,
> +                                        wr::DisplayListBuilder& aBuilder,
> +                                        nsTArray<layers::WebRenderParentCommand>& aParentCommands,
> +                                        layers::WebRenderDisplayItemLayer* aLayer,
> +                                        const nsPoint& aPt)

This method seems to be incomplete. It doesn't respect m{Start,End}Bevel{Side,Offset}. It also doesn't seem to respect mBGColor but maybe mBGColor is now unused after the changes in bug 929484.

Please add a comment about the cases it doesn't handle.

::: layout/tables/nsTableFrame.cpp:7665
(Diff revision 2)
> +  NS_FOR_CSS_SIDES(i) {
> +    wrSide[i] = wr::ToWrBorderSide(ToDeviceColor(param->mBorderColor), NS_STYLE_BORDER_STYLE_NONE);
> +  }
> +  wrSide[eSideTop] = wr::ToWrBorderSide(ToDeviceColor(param->mBorderColor), param->mBorderStyle);
> +
> +  WrBorderRadius borderRadius = wr::ToWrBorderRadius(LayerSize(0, 0),

Let's rename this variable to borderRadii because borderWidths is also plural.

Can you initialize it using
WrBorderRadius borderRadii = { {0, 0},  {0, 0},  {0, 0},  {0, 0} };
?
Not sure if that improves things, your call.

::: layout/tables/nsTableFrame.cpp:7669
(Diff revision 2)
> +  WrBorderWidths borderWidths = wr::ToWrBorderWidths(transformedRect.Height(),
> +                                                     transformedRect.Height(),
> +                                                     transformedRect.Height(),
> +                                                     transformedRect.Height());

Using height for all sides is quite confusing to me. This needs a comment at least.
Attachment #8858720 - Flags: review?(mstange) → review+
Comment on attachment 8858720 [details]
Bug 1344082 - Create webrender commands for nsDisplayTableBorderCollapse.

https://reviewboard.mozilla.org/r/130754/#review137012

Looks good except for the missing comments about incompleteness. All my comments about BCInlineDirSeg::CreateWebRenderCommands also apply for the BCBlockDirSeg.
Comment on attachment 8858720 [details]
Bug 1344082 - Create webrender commands for nsDisplayTableBorderCollapse.

https://reviewboard.mozilla.org/r/130754/#review137000

> What's the meaning of aPt? It would be nice to find a more predictive name, or to add a comment about the meaning of this argument.

Renamed it to aOffsetToReferenceFrame.

> This method seems to be incomplete. It doesn't respect m{Start,End}Bevel{Side,Offset}. It also doesn't seem to respect mBGColor but maybe mBGColor is now unused after the changes in bug 929484.
> 
> Please add a comment about the cases it doesn't handle.

I added a comment about we don't support bevel side and offset currently. I don't mentioned mBGColor since bug 929484 is fixed.

> Let's rename this variable to borderRadii because borderWidths is also plural.
> 
> Can you initialize it using
> WrBorderRadius borderRadii = { {0, 0},  {0, 0},  {0, 0},  {0, 0} };
> ?
> Not sure if that improves things, your call.

done

> Using height for all sides is quite confusing to me. This needs a comment at least.

Added a comment about this.
Part 1 is no need because bug 929484 is landed.
Attachment #8858719 - Attachment is obsolete: true
Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c06102eacf2d337fc9922ed85148aad128018e


R7 and mda1 should be intermittent which is not related to this patch.
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/0ea1bcbd5466
Create webrender commands for nsDisplayTableBorderCollapse. r=mstange
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1422393
You need to log in before you can comment on or make changes to this bug.