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.

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.

Comment on attachment 8858719 [details] Bug 1344082 - Divided nsDisplayTableBorderBackground into nsDisplayTableBorder and nsDisplayTableBackground. https://reviewboard.mozilla.org/r/130752/#review133582

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.

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.

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.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5135b8c25fbf6ce8fceff0a8ffcdcba1fdc5c745

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