Closed Bug 1389010 Opened 7 years ago Closed 7 years ago

Various patches to cleanup and documentation nsCSSBorderRenderer

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(8 files)

59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
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 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
Attachment #8895734 - Flags: review?(dholbert) → review+
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
Attachment #8895735 - Flags: review?(dholbert) → review+
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
Attachment #8895736 - Flags: review?(dholbert) → review+
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.
Attachment #8895737 - Flags: review?(dholbert) → review+
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.
Attachment #8895738 - Flags: review?(dholbert) → review+
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.
Flags: needinfo?(jwatt)
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.
Attachment #8895741 - Flags: review?(dholbert) → review+
(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.
Flags: needinfo?(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 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
Attachment #8895739 - Flags: review?(dholbert) → review+
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
Attachment #8895740 - Flags: review?(dholbert) → review+
Pushed by jwatt@jwatt.org:
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
You need to log in before you can comment on or make changes to this bug.