Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906253003:
Replace call sites to BorderValue functions to save the BorderValue construction cost (Closed)
|
Created:
3 years, 6 months ago by nainar Modified:
3 years, 6 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace call sites to BorderValue functions to save the BorderValue construction cost
Currently we construct BorderValues via functions
ComputedStyle::Border{Left,Right,Start,End} only to access two methods
at most (Style and Width) or perform comparison. This patch splits out
the accessors for Style and Width and adds comparison functions to
ComputedStyle to save this construction cost.
We haven't removed the callsites in Layout land that return a
BorderValue and are saving it for a future patch to reduce complexity.
BUG=628043, 721287
Review-Url: https://codereview.chromium.org/2906253003
Cr-Commit-Position: refs/heads/master@{#475896}
Committed: https://chromium.googlesource.com/chromium/src/+/9a4d00a687e0cf0ea029a68b2dd44f5814d539d0
Patch Set 1 #Patch Set 2 : Ready for review #Patch Set 3 : Redo LayoutTable/LayoutTableSection #
Total comments: 49
Patch Set 4 : rune@ renames #Patch Set 5 : rune@ renames #
Messages
Total messages: 32 (21 generated)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nainar@chromium.org changed reviewers: + shend@chromium.org
shend@, PTAL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed the issue (redid the two classes). Passes the previously failing tests locally now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
nainar@chromium.org changed reviewers: + meade@chromium.org
rune@opera.com changed reviewers: + rune@opera.com
https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:1399: 2; Weird formatting, but I guess this is what git cl format does? https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1353: EBorderStyle sbs = side == kBorderBefore ? Style()->BorderBeforeStyle() Could we use some more descriptive variable names than sbs?
rune@, I've taken a stab at some suggested names without trying to delve too much into the code. Let me know what you think of the suggestions and I will make the changes. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTable.cpp:1399: 2; Unfortunate but yup. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1353: EBorderStyle sbs = side == kBorderBefore ? Style()->BorderBeforeStyle() I can try. Some suggestions: sbs -> style_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1362: EBorderStyle rbs = side == kBorderBefore rbs -> row_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1362: EBorderStyle rbs = side == kBorderBefore rbs -> row_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1365: float rbw = side == kBorderBefore ? FirstRow()->Style()->BorderBeforeWidth() rbw -> row_border_width https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1381: EBorderStyle cbs = side == kBorderBefore cbs -> cell_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1384: float cbw = side == kBorderBefore ? primary_cell_style.BorderBeforeWidth() cbw -> cell_border_width https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1390: EBorderStyle gbs = side == kBorderBefore gbs -> col_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1393: const float gbw = side == kBorderBefore gbw -> col_border_width https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1428: EBorderStyle sbs = side == kBorderStart ? Style()->BorderStartStyle() sbs -> style_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1430: const float sbw = side == kBorderStart ? Style()->BorderStartWidth() sbw -> style_border_width https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1440: EBorderStyle gbs = side == kBorderStart ? col->Style()->BorderStartStyle() gbs -> col_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1442: const float gbw = side == kBorderStart ? col->Style()->BorderStartWidth() gbw -> col_border_width https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1463: EBorderStyle cbs = side == kBorderStart cbs -> cell_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1466: EBorderStyle rbs = side == kBorderStart rbs -> cell_parent_border_style https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1469: const float cbw = side == kBorderStart cbw -> cell_border_width https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1472: const float rbw = side == kBorderStart rbw -> cell_parent_border_width
https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1353: EBorderStyle sbs = side == kBorderBefore ? Style()->BorderBeforeStyle() On 2017/05/31 09:31:57, nainar wrote: > I can try. Some suggestions: > > sbs -> style_border_style I think the s is for section (as in LayoutTableSection), so perhaps section_border_style? https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1362: EBorderStyle rbs = side == kBorderBefore On 2017/05/31 09:31:56, nainar wrote: > rbs -> row_border_style Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1365: float rbw = side == kBorderBefore ? FirstRow()->Style()->BorderBeforeWidth() On 2017/05/31 09:31:56, nainar wrote: > rbw -> row_border_width Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1381: EBorderStyle cbs = side == kBorderBefore On 2017/05/31 09:31:56, nainar wrote: > cbs -> cell_border_style Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1384: float cbw = side == kBorderBefore ? primary_cell_style.BorderBeforeWidth() On 2017/05/31 09:31:56, nainar wrote: > cbw -> cell_border_width Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1390: EBorderStyle gbs = side == kBorderBefore On 2017/05/31 09:31:56, nainar wrote: > gbs -> col_border_style Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1393: const float gbw = side == kBorderBefore On 2017/05/31 09:31:56, nainar wrote: > gbw -> col_border_width Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1428: EBorderStyle sbs = side == kBorderStart ? Style()->BorderStartStyle() On 2017/05/31 09:31:56, nainar wrote: > sbs -> style_border_style section_border_style? https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1430: const float sbw = side == kBorderStart ? Style()->BorderStartWidth() On 2017/05/31 09:31:56, nainar wrote: > sbw -> style_border_width section_border_width? https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1440: EBorderStyle gbs = side == kBorderStart ? col->Style()->BorderStartStyle() On 2017/05/31 09:31:56, nainar wrote: > gbs -> col_border_style Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1442: const float gbw = side == kBorderStart ? col->Style()->BorderStartWidth() On 2017/05/31 09:31:56, nainar wrote: > gbw -> col_border_width Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1463: EBorderStyle cbs = side == kBorderStart On 2017/05/31 09:31:56, nainar wrote: > cbs -> cell_border_style Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1466: EBorderStyle rbs = side == kBorderStart On 2017/05/31 09:31:57, nainar wrote: > rbs -> cell_parent_border_style row_border_style? https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1469: const float cbw = side == kBorderStart On 2017/05/31 09:31:56, nainar wrote: > cbw -> cell_border_width Acknowledged. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1472: const float rbw = side == kBorderStart On 2017/05/31 09:31:56, nainar wrote: > rbw -> cell_parent_border_width row_border_width?
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rune@, Made the changes you asked for, PTAL? Thanks! https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1353: EBorderStyle sbs = side == kBorderBefore ? Style()->BorderBeforeStyle() On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:57, nainar wrote: > > I can try. Some suggestions: > > > > sbs -> style_border_style > > I think the s is for section (as in LayoutTableSection), so perhaps section_border_style? Done. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1362: EBorderStyle rbs = side == kBorderBefore On 2017/05/31 at 11:02:10, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > rbs -> row_border_style > > Acknowledged. Done. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1365: float rbw = side == kBorderBefore ? FirstRow()->Style()->BorderBeforeWidth() On 2017/05/31 at 11:02:10, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > rbw -> row_border_width > > Acknowledged. Done https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1381: EBorderStyle cbs = side == kBorderBefore On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > cbs -> cell_border_style > > Acknowledged. Done https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1384: float cbw = side == kBorderBefore ? primary_cell_style.BorderBeforeWidth() On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > cbw -> cell_border_width > > Acknowledged. Done. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1390: EBorderStyle gbs = side == kBorderBefore On 2017/05/31 at 11:02:10, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > gbs -> col_border_style > > Acknowledged. Done https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1393: const float gbw = side == kBorderBefore On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > gbw -> col_border_width > > Acknowledged. Done. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1428: EBorderStyle sbs = side == kBorderStart ? Style()->BorderStartStyle() On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > sbs -> style_border_style > > section_border_style? Done. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1430: const float sbw = side == kBorderStart ? Style()->BorderStartWidth() On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > sbw -> style_border_width > > section_border_width? Done https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1440: EBorderStyle gbs = side == kBorderStart ? col->Style()->BorderStartStyle() On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > gbs -> col_border_style > > Acknowledged. Done. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1442: const float gbw = side == kBorderStart ? col->Style()->BorderStartWidth() On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > gbw -> col_border_width > > Acknowledged. Done https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1463: EBorderStyle cbs = side == kBorderStart On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > cbs -> cell_border_style > > Acknowledged. Done. https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1466: EBorderStyle rbs = side == kBorderStart On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:57, nainar wrote: > > rbs -> cell_parent_border_style > > row_border_style? Done https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1469: const float cbw = side == kBorderStart On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > cbw -> cell_border_width > > Acknowledged. Done https://codereview.chromium.org/2906253003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1472: const float rbw = side == kBorderStart On 2017/05/31 at 11:02:09, rune wrote: > On 2017/05/31 09:31:56, nainar wrote: > > rbw -> cell_parent_border_width > > row_border_width? Done
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shend@chromium.org Link to the patchset: https://codereview.chromium.org/2906253003/#ps80001 (title: "rune@ renames")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496237377809200,
"parent_rev": "b19c80ec1d10fa885d2174ad432bc00c472d4344", "commit_rev":
"9a4d00a687e0cf0ea029a68b2dd44f5814d539d0"}
Message was sent while issue was closed.
Description was changed from
==========
Replace call sites to BorderValue functions to save the BorderValue construction
cost
Currently we construct BorderValues via functions
ComputedStyle::Border{Left,Right,Start,End} only to access two methods
at most (Style and Width) or perform comparison. This patch splits out
the accessors for Style and Width and adds comparison functions to
ComputedStyle to save this construction cost.
We haven't removed the callsites in Layout land that return a
BorderValue and are saving it for a future patch to reduce complexity.
BUG=628043, 721287
==========
to
==========
Replace call sites to BorderValue functions to save the BorderValue construction
cost
Currently we construct BorderValues via functions
ComputedStyle::Border{Left,Right,Start,End} only to access two methods
at most (Style and Width) or perform comparison. This patch splits out
the accessors for Style and Width and adds comparison functions to
ComputedStyle to save this construction cost.
We haven't removed the callsites in Layout land that return a
BorderValue and are saving it for a future patch to reduce complexity.
BUG=628043, 721287
Review-Url: https://codereview.chromium.org/2906253003
Cr-Commit-Position: refs/heads/master@{#475896}
Committed:
https://chromium.googlesource.com/chromium/src/+/9a4d00a687e0cf0ea029a68b2dd4...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9a4d00a687e0cf0ea029a68b2dd4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
