1389010 - Various patches to cleanup and documentation nsCSSBorderRenderer

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Jonathan Watt [:jwatt]]

Reviewing the patches for bug 1356114 was a PITA because some of the code is quite messy and undocumented. In the process of figuring out the code and how we could better fix bug 1356114 I wrote a bunch of patches to clean the code up.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 1 - Rename allBordersSame to allBordersSameStyle to distinguish it from the other similar variables.

Also moves the declaration of tlBordersSame and brBordersSame to where they're used.

Review commit: https://reviewboard.mozilla.org/r/167060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167060/

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 2 - Move an early return in nsCSSBorderRenderer::DrawBorders up to avoid unnecessary work.

Review commit: https://reviewboard.mozilla.org/r/167062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167062/

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 3 - Add comments to nsCSSRenderingBorders to indicate what the heck "composite colors" are.

Review commit: https://reviewboard.mozilla.org/r/167064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167064/

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 4 - In nsCSSRenderingBorders::DrawBorders declare allBordersSolid where it is defined and make it const.

Review commit: https://reviewboard.mozilla.org/r/167066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167066/

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 5 - Assert the line type invariant at the top of nsCSSBorderRenderer::SetupDashedOptions.

Review commit: https://reviewboard.mozilla.org/r/167068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167068/

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 6 - Call AreBorderSideFinalStylesSame in nsCSSBorderRenderer's constructor and store the result as a member.

Review commit: https://reviewboard.mozilla.org/r/167070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167070/

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 7 - Call AllBordersSameWidth in nsCSSBorderRenderer's constructor and store the result as a member.

Review commit: https://reviewboard.mozilla.org/r/167072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167072/

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: Bug 1389010, part 8 - Stop abusing mAllBordersSameStyle in nsCSSBorderRenderer::DrawBorders to force drawing as separate parts.

Review commit: https://reviewboard.mozilla.org/r/167074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167074/

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8895734 [details]
Bug 1389010, part 1 - Rename allBordersSame to allBordersSameStyle to distinguish it from the other similar variables.

https://reviewboard.mozilla.org/r/167060/#review172326

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8895735 [details]
Bug 1389010, part 2 - Move an early return in nsCSSBorderRenderer::DrawBorders up to avoid unnecessary work.

https://reviewboard.mozilla.org/r/167062/#review172332

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8895736 [details]
Bug 1389010, part 3 - Add comments to nsCSSRenderingBorders to indicate what the heck "composite colors" are.

https://reviewboard.mozilla.org/r/167064/#review172334

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8895737 [details]
Bug 1389010, part 4 - In nsCSSRenderingBorders::DrawBorders declare allBordersSolid where it is defined and make it const.

https://reviewboard.mozilla.org/r/167066/#review172336

::: layout/painting/nsCSSRenderingBorders.cpp:3304
(Diff revision 1)
> -  bool hasCompositeColors;
> +  const bool hasCompositeColors;
> +  const bool allBordersSolid = AllBordersSolid(&hasCompositeColors);

"hasCompositeColors" cannot be const -- otherwise this won't compile.

AllBordersSolid() takes a bool*, not a const bool*.

So -- doing what the commit message says (making allBordersSolid const) seems fine, but I don't think you can additionally add 'const' to hasCompositeColors.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8895738 [details]
Bug 1389010, part 5 - Assert the line type invariant at the top of nsCSSBorderRenderer::SetupDashedOptions.

https://reviewboard.mozilla.org/r/167068/#review172338

::: layout/painting/nsCSSRenderingBorders.cpp:1631
(Diff revision 1)
>  void
>  nsCSSBorderRenderer::SetupDashedOptions(StrokeOptions* aStrokeOptions,
>                                          Float aDash[2],
>                                          mozilla::Side aSide,
>                                          Float aBorderLength, bool isCorner)
>  {
> +  MOZ_ASSERT(mBorderStyles[aSide] == NS_STYLE_BORDER_STYLE_DASHED ||
> +             mBorderStyles[aSide] == NS_STYLE_BORDER_STYLE_DOTTED,
> +             "Style should be dashed or dotted.");
> +

I assume you've verified (by code inspection) that all callers actually enforce this condition before they call this method.

Comment 14

[Daniel Holbert [:dholbert]]

I'm not immediately clear on why parts 6 and 7 make sense.

Does nsCSSBorderRenderer::DrawBorders() get called multiple times on the same instance?  If so, then I can see the patches making sense, to avoid recomputation.  But if not, I don't see why it's any more more sensible for this to be a member var vs. a more-localized-to-its-usage local var.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8895741 [details]
Bug 1389010, part 8 - Stop abusing mAllBordersSameStyle in nsCSSBorderRenderer::DrawBorders to force drawing as separate parts.

https://reviewboard.mozilla.org/r/167074/#review172344

This looks fine -- though in the perhaps-unlikely event that parts 6 and 7 are unneeded (per comment 14 -- not sure), then you'll want s/mAllBordersSameStyle/allBordersSameStyle/ in the commit message of course.

Comment 16

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> I assume you've verified (by code inspection) that all callers actually
> enforce this condition before they call this method.

Indeed.

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> I'm not immediately clear on why parts 6 and 7 make sense.
> 
> Does nsCSSBorderRenderer::DrawBorders() get called multiple times on the
> same instance?  If so, then I can see the patches making sense, to avoid
> recomputation.  But if not, I don't see why it's any more more sensible for
> this to be a member var vs. a more-localized-to-its-usage local var.

I have later patches (not cleaned up and attached yet) where I factor out some of the DrawBorders code into a separate function, but then I end up wanting the information returned by AreBorderSideFinalStylesSame and AllBordersSameWidth in both that factored out function and in DrawBorders.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8895739 [details]
Bug 1389010, part 6 - Call AreBorderSideFinalStylesSame in nsCSSBorderRenderer's constructor and store the result as a member.

https://reviewboard.mozilla.org/r/167070/#review174256

ok, r=me

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8895740 [details]
Bug 1389010, part 7 - Call AllBordersSameWidth in nsCSSBorderRenderer's constructor and store the result as a member.

https://reviewboard.mozilla.org/r/167072/#review174258

r=me

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfcc8657b725
part 1 - Rename allBordersSame to allBordersSameStyle to distinguish it from the other similar variables. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f0c27b4101
part 2 - Move an early return in nsCSSBorderRenderer::DrawBorders up to avoid unnecessary work. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b73c875c30a6
part 3 - Add comments to nsCSSRenderingBorders to indicate what the heck "composite colors" are. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaee7e790488
part 4 - In nsCSSRenderingBorders::DrawBorders declare allBordersSolid where it is defined and make it const. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/430be457b43b
part 5 - Assert the line type invariant at the top of nsCSSBorderRenderer::SetupDashedOptions. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca14dc22d579
part 6 - Call AreBorderSideFinalStylesSame in nsCSSBorderRenderer's constructor and store the result as a member. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b0eecb3ab5
part 7 - Call AllBordersSameWidth in nsCSSBorderRenderer's constructor and store the result as a member. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/360cd29a8a04
part 8 - Stop abusing mAllBordersSameStyle in nsCSSBorderRenderer::DrawBorders to force drawing as separate parts. r=dholbert

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfcc8657b725
https://hg.mozilla.org/mozilla-central/rev/68f0c27b4101
https://hg.mozilla.org/mozilla-central/rev/b73c875c30a6
https://hg.mozilla.org/mozilla-central/rev/eaee7e790488
https://hg.mozilla.org/mozilla-central/rev/430be457b43b
https://hg.mozilla.org/mozilla-central/rev/ca14dc22d579
https://hg.mozilla.org/mozilla-central/rev/a1b0eecb3ab5
https://hg.mozilla.org/mozilla-central/rev/360cd29a8a04