Closed
Bug 1389010
Opened 7 years ago
Closed 7 years ago
Various patches to cleanup and documentation nsCSSBorderRenderer
Categories
(Core :: Layout, enhancement)
Core
Layout
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
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 10•7 years ago
|
||
mozreview-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 11•7 years ago
|
||
mozreview-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 12•7 years ago
|
||
mozreview-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 13•7 years ago
|
||
mozreview-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+
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 16•7 years ago
|
||
(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)
Assignee | ||
Comment 17•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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 19•7 years ago
|
||
mozreview-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+
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•