Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2926533002:
Remove references to ui/views/layout/layout_constants.h (Closed)
|
Created:
3 years, 6 months ago by ananta Modified:
3 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove references to ui/views/layout/layout_constants.h
Replace references to constants in ui/views/layout/layout_constants.h with their
equivalents using ChromeLayoutProvider.
BUG=691897
Review-Url: https://codereview.chromium.org/2926533002
Cr-Commit-Position: refs/heads/master@{#477424}
Committed: https://chromium.googlesource.com/chromium/src/+/8c0bf389d78991280270f383043405f0098a9a85
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address review comments #Patch Set 3 : Fix spelling #
Total comments: 4
Patch Set 4 : Address next round of comments. #Patch Set 5 : Fix redness #Patch Set 6 : Fix dumb error leading to redness #
Total comments: 3
Patch Set 7 : Use horizontal constants for width. #Messages
Total messages: 34 (22 generated)
ananta@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:47: views::INSETS_DIALOG_CONTENTS); For this file, wait on https://chromium-review.googlesource.com/c/515902/ to land, then pass INSETS_DIALOG_CONTENTS to the BoxLayout directly. https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:74: columns->AddPaddingColumn(1, unrelated_vertical_spacing); This uses a vertical constant for a horizontal value, which can't be right. Replace with a horizontal constant. https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:92: layout->StartRowWithPadding(0, column_set_id, 0, unrelated_vertical_spacing); You used the LARGE variant of this above, should probably use it here https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:183: DISTANCE_UNRELATED_CONTROL_VERTICAL), kMaxContentWidth); This uses a vertical constant for a horizontal value, which can't be right. Replace with a horizontal constant. https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:188: // Bullet labels need to take into acocunt the size of the bullet. Nit: While here: can you fix the "account" typo?
https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:74: columns->AddPaddingColumn(1, unrelated_vertical_spacing); On 2017/06/06 03:34:33, Peter Kasting wrote: > This uses a vertical constant for a horizontal value, which can't be right. > Replace with a horizontal constant. Replaced with DISTANCE_UNRELATED_CONTROL_HORIZONTAL https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:92: layout->StartRowWithPadding(0, column_set_id, 0, unrelated_vertical_spacing); On 2017/06/06 03:34:33, Peter Kasting wrote: > You used the LARGE variant of this above, should probably use it here Thanks done. https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:183: DISTANCE_UNRELATED_CONTROL_VERTICAL), kMaxContentWidth); On 2017/06/06 03:34:33, Peter Kasting wrote: > This uses a vertical constant for a horizontal value, which can't be right. > Replace with a horizontal constant. Done. Replaced with DISTANCE_UNRELATED_CONTROL_HORIZONTAL https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/sad... chrome/browser/ui/views/sad_tab_view.cc:188: // Bullet labels need to take into acocunt the size of the bullet. On 2017/06/06 03:34:33, Peter Kasting wrote: > Nit: While here: can you fix the "account" typo? Done.
https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_cleaner_dialog_win.cc (right): https://codereview.chromium.org/2926533002/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_cleaner_dialog_win.cc:47: views::INSETS_DIALOG_CONTENTS); On 2017/06/06 03:34:33, Peter Kasting wrote: > For this file, wait on https://chromium-review.googlesource.com/c/515902/ to > land, then pass INSETS_DIALOG_CONTENTS to the BoxLayout directly. Sure reverted this change
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2926533002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2926533002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:69: columns->AddPaddingColumn(1, unrelated_vertical_spacing); This is also a horizontal amount. https://codereview.chromium.org/2926533002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:184: DISTANCE_UNRELATED_CONTROL_HORIZONTAL), kMaxContentWidth); The amount here has to equal the sum of the widths of the two padding columns above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2926533002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2926533002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:69: columns->AddPaddingColumn(1, unrelated_vertical_spacing); On 2017/06/06 03:47:01, Peter Kasting wrote: > This is also a horizontal amount. Yes. I should have noticed it earlier. https://codereview.chromium.org/2926533002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:184: DISTANCE_UNRELATED_CONTROL_HORIZONTAL), kMaxContentWidth); On 2017/06/06 03:47:01, Peter Kasting wrote: > The amount here has to equal the sum of the widths of the two padding columns > above. Sorry for missing this one as well. I missed the * 2 while changing the constant.
The CQ bit was checked by ananta@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@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 ananta@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm sorry to keep bouncing this back to you. OTOH, I guess this reinforces the idea of checking the existing semantics very carefully while converting, since they might be wrong? https://codereview.chromium.org/2926533002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2926533002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:108: column_set->AddPaddingColumn(1, unrelated_vertical_spacing); These are more horizontal values (here and below).
https://codereview.chromium.org/2926533002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2926533002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:108: column_set->AddPaddingColumn(1, unrelated_vertical_spacing); On 2017/06/06 19:46:00, Peter Kasting wrote: > These are more horizontal values (here and below). I had a draft comment here which I forgot to send out asking about these. The documentation specifically says width https://cs.chromium.org/chromium/src/ui/views/layout/grid_layout.cc?type=cs&q...
https://codereview.chromium.org/2926533002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/2926533002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/sad_tab_view.cc:108: column_set->AddPaddingColumn(1, unrelated_vertical_spacing); On 2017/06/06 20:08:43, ananta wrote: > On 2017/06/06 19:46:00, Peter Kasting wrote: > > These are more horizontal values (here and below). > > I had a draft comment here which I forgot to send out asking about these. The > documentation specifically says width > https://cs.chromium.org/chromium/src/ui/views/layout/grid_layout.cc?type=cs&q... I updated the patchset to use the horizontal constants.
The CQ bit was checked by ananta@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...
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 ananta@chromium.org
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": 120001, "attempt_start_ts": 1496785644615170,
"parent_rev": "5c265a308a311a7d65ab37c10cf84830ddea72d5", "commit_rev":
"8c0bf389d78991280270f383043405f0098a9a85"}
Message was sent while issue was closed.
Description was changed from ========== Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 ========== to ========== Remove references to ui/views/layout/layout_constants.h Replace references to constants in ui/views/layout/layout_constants.h with their equivalents using ChromeLayoutProvider. BUG=691897 Review-Url: https://codereview.chromium.org/2926533002 Cr-Commit-Position: refs/heads/master@{#477424} Committed: https://chromium.googlesource.com/chromium/src/+/8c0bf389d78991280270f3830434... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8c0bf389d78991280270f3830434... |
