Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444923003:
Some more pre-material cleanups (Closed)
|
Created:
4 years, 1 month ago by Evan Stade Modified:
4 years, 1 month ago CC:
chromium-reviews, tfarina, James Su, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSome more pre-material cleanups
BUG=648281
Committed: https://crrev.com/3d5fe31add4e90541a5164216f9a16b6cf0c7086
Cr-Commit-Position: refs/heads/master@{#428603}
Patch Set 1 #Patch Set 2 : remove pre-material layout constants #Patch Set 3 : also remove some layout constants #
Total comments: 21
Patch Set 4 : pkasting review #Messages
Total messages: 44 (27 generated)
The CQ bit was checked by estade@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_chromeos_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 estade@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...
estade@chromium.org changed reviewers: + groby@chromium.org, pkasting@chromium.org
+groby for cocoa, +pkasting for the rest
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 estade@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...
+comments for Peter https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... File chrome/browser/ui/layout_constants.cc (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:14: const int kLocationBarBubbleFontVerticalPadding[] = {2, 4}; what do we think should happen to the layout constants here, especially the ones that are the same in normal and hybrid? Or the ones like kFindBarVerticalOffset which are only used in one place, i.e. browser_view_layout.cc https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:15: const int kLocationBarBubbleHorizontalPadding[] = {0, 0}; this one definitely seems like it should just die https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:25: #if defined(OS_MACOSX) the mac/non mac distinction is no longer important https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:34: const int kToolbarLocationBarRightPadding[] = {4, 8}; perhaps this can/should be combined with ToolbarStandardSpacing now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/infobars/infobar_controller.mm (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/infobars/infobar_controller.mm:78: // TODO(ellyjones): Remove this constant. Remove how? https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/infob... File chrome/browser/ui/infobar_container_delegate.cc (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/infob... chrome/browser/ui/infobar_container_delegate.cc:90: bar_target_height != -1 ? bar_target_height : kDefaultBarTargetHeight; Nit: Reverse condition and arms so second arm does not mentally read like a double-negative https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... File chrome/browser/ui/layout_constants.cc (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:14: const int kLocationBarBubbleFontVerticalPadding[] = {2, 4}; On 2016/10/24 19:09:40, Evan Stade wrote: > what do we think should happen to the layout constants here, especially the ones > that are the same in normal and hybrid? Or the ones like kFindBarVerticalOffset > which are only used in one place, i.e. browser_view_layout.cc Things that are only used in one place, whether the same or different between normal and hybrid, I would define in that file. Things that are the same between normal and hybrid, but used in multiple places, I would define publicly in some relevant class as a public constant. Not sure how many things are "different between normal and hybrid, and used in multiple places". For now I'd leave those here. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:15: const int kLocationBarBubbleHorizontalPadding[] = {0, 0}; On 2016/10/24 19:09:41, Evan Stade wrote: > this one definitely seems like it should just die Yep. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:25: #if defined(OS_MACOSX) On 2016/10/24 19:09:40, Evan Stade wrote: > the mac/non mac distinction is no longer important Yup, rip out https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:34: const int kToolbarLocationBarRightPadding[] = {4, 8}; On 2016/10/24 19:09:40, Evan Stade wrote: > perhaps this can/should be combined with ToolbarStandardSpacing now? Probably. In general I'd try to combine constants when it makes sense to do so. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (left): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:58: static bool initialized = false; Argh, this code was dumb. https://codereview.chromium.org/2444923003/diff/40001/ui/base/material_design... File ui/base/material_design/material_design_controller.h (right): https://codereview.chromium.org/2444923003/diff/40001/ui/base/material_design... ui/base/material_design/material_design_controller.h:27: }; Probably long term we remove these and replace with some sort of hybrid-mode bool?
On 2016/10/24 23:49:56, Peter Kasting wrote: > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/infobars/infobar_controller.mm (right): > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/infobars/infobar_controller.mm:78: // TODO(ellyjones): > Remove this constant. > Remove how? > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/infob... > File chrome/browser/ui/infobar_container_delegate.cc (right): > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/infob... > chrome/browser/ui/infobar_container_delegate.cc:90: bar_target_height != -1 ? > bar_target_height : kDefaultBarTargetHeight; > Nit: Reverse condition and arms so second arm does not mentally read like a > double-negative > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... > File chrome/browser/ui/layout_constants.cc (right): > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... > chrome/browser/ui/layout_constants.cc:14: const int > kLocationBarBubbleFontVerticalPadding[] = {2, 4}; > On 2016/10/24 19:09:40, Evan Stade wrote: > > what do we think should happen to the layout constants here, especially the > ones > > that are the same in normal and hybrid? Or the ones like > kFindBarVerticalOffset > > which are only used in one place, i.e. browser_view_layout.cc > > Things that are only used in one place, whether the same or different between > normal and hybrid, I would define in that file. > > Things that are the same between normal and hybrid, but used in multiple places, > I would define publicly in some relevant class as a public constant. > > Not sure how many things are "different between normal and hybrid, and used in > multiple places". For now I'd leave those here. > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... > chrome/browser/ui/layout_constants.cc:15: const int > kLocationBarBubbleHorizontalPadding[] = {0, 0}; > On 2016/10/24 19:09:41, Evan Stade wrote: > > this one definitely seems like it should just die > > Yep. > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... > chrome/browser/ui/layout_constants.cc:25: #if defined(OS_MACOSX) > On 2016/10/24 19:09:40, Evan Stade wrote: > > the mac/non mac distinction is no longer important > > Yup, rip out > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... > chrome/browser/ui/layout_constants.cc:34: const int > kToolbarLocationBarRightPadding[] = {4, 8}; > On 2016/10/24 19:09:40, Evan Stade wrote: > > perhaps this can/should be combined with ToolbarStandardSpacing now? > > Probably. In general I'd try to combine constants when it makes sense to do so. > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/toolb... > File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (left): > > https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/toolbar_actions_bar.cc:58: static bool initialized = > false; > Argh, this code was dumb. > > https://codereview.chromium.org/2444923003/diff/40001/ui/base/material_design... > File ui/base/material_design/material_design_controller.h (right): > > https://codereview.chromium.org/2444923003/diff/40001/ui/base/material_design... > ui/base/material_design/material_design_controller.h:27: }; > Probably long term we remove these and replace with some sort of hybrid-mode > bool? FWIW, cocoa is pretty much OK, except for the same question Peter had. Since it's just two constants changing, RS LGTM % clarifying what is supposed to happen at the TODO.
https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/infobars/infobar_controller.mm (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/infobars/infobar_controller.mm:78: // TODO(ellyjones): Remove this constant. On 2016/10/24 23:49:56, Peter Kasting wrote: > Remove how? I don't know exactly how (not familiar enough with cocoa), but I imagine it involves updated the .xib? Or perhaps using [self frame].origin.y and [subview frame].origin.y? This is just causing things to be vertically centered, it doesn't seem like that should require two constants. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/infob... File chrome/browser/ui/infobar_container_delegate.cc (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/infob... chrome/browser/ui/infobar_container_delegate.cc:90: bar_target_height != -1 ? bar_target_height : kDefaultBarTargetHeight; On 2016/10/24 23:49:56, Peter Kasting wrote: > Nit: Reverse condition and arms so second arm does not mentally read like a > double-negative Done. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... File chrome/browser/ui/layout_constants.cc (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:14: const int kLocationBarBubbleFontVerticalPadding[] = {2, 4}; On 2016/10/24 23:49:56, Peter Kasting wrote: > On 2016/10/24 19:09:40, Evan Stade wrote: > > what do we think should happen to the layout constants here, especially the > ones > > that are the same in normal and hybrid? Or the ones like > kFindBarVerticalOffset > > which are only used in one place, i.e. browser_view_layout.cc > > Things that are only used in one place, whether the same or different between > normal and hybrid, I would define in that file. > > Things that are the same between normal and hybrid, but used in multiple places, > I would define publicly in some relevant class as a public constant. > > Not sure how many things are "different between normal and hybrid, and used in > multiple places". For now I'd leave those here. this plan soudns fine, but for now (for the purposes of smallish, atomic changes) I intend to leave everything here https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:15: const int kLocationBarBubbleHorizontalPadding[] = {0, 0}; On 2016/10/24 23:49:56, Peter Kasting wrote: > On 2016/10/24 19:09:41, Evan Stade wrote: > > this one definitely seems like it should just die > > Yep. Done. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:25: #if defined(OS_MACOSX) On 2016/10/24 23:49:56, Peter Kasting wrote: > On 2016/10/24 19:09:40, Evan Stade wrote: > > the mac/non mac distinction is no longer important > > Yup, rip out Done. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (left): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:58: static bool initialized = false; On 2016/10/24 23:49:56, Peter Kasting wrote: > Argh, this code was dumb. Acknowledged. https://codereview.chromium.org/2444923003/diff/40001/ui/base/material_design... File ui/base/material_design/material_design_controller.h (right): https://codereview.chromium.org/2444923003/diff/40001/ui/base/material_design... ui/base/material_design/material_design_controller.h:27: }; On 2016/10/24 23:49:56, Peter Kasting wrote: > Probably long term we remove these and replace with some sort of hybrid-mode > bool? I briefly discussed with Terry, and it's my feeling that the long term fate of this class (assuming it's going to boil down to just normal vs hybrid) would be to moved somehow to chrome/browser. I don't think anything outside of there makes a distinction between normal and hybrid.
The CQ bit was checked by estade@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.
ping Peter
Sorry, LGTM https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... File chrome/browser/ui/layout_constants.cc (right): https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:14: const int kLocationBarBubbleFontVerticalPadding[] = {2, 4}; On 2016/10/25 16:22:57, Evan Stade wrote: > On 2016/10/24 23:49:56, Peter Kasting wrote: > > On 2016/10/24 19:09:40, Evan Stade wrote: > > > what do we think should happen to the layout constants here, especially the > > ones > > > that are the same in normal and hybrid? Or the ones like > > kFindBarVerticalOffset > > > which are only used in one place, i.e. browser_view_layout.cc > > > > Things that are only used in one place, whether the same or different between > > normal and hybrid, I would define in that file. > > > > Things that are the same between normal and hybrid, but used in multiple > places, > > I would define publicly in some relevant class as a public constant. > > > > Not sure how many things are "different between normal and hybrid, and used in > > multiple places". For now I'd leave those here. > > this plan soudns fine, but for now (for the purposes of smallish, atomic > changes) I intend to leave everything here I'm cool with that. You've been cleaning all the rest of the MD stuff up, so I assume you will take care of it eventually. https://codereview.chromium.org/2444923003/diff/40001/chrome/browser/ui/layou... chrome/browser/ui/layout_constants.cc:34: const int kToolbarLocationBarRightPadding[] = {4, 8}; On 2016/10/24 23:49:56, Peter Kasting wrote: > On 2016/10/24 19:09:40, Evan Stade wrote: > > perhaps this can/should be combined with ToolbarStandardSpacing now? > > Probably. In general I'd try to combine constants when it makes sense to do so. Do later?
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/2444923003/#ps60001 (title: "pkasting review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
estade@chromium.org changed reviewers: + sadrul@chromium.org
+sadrul for two line change in ui/base/material_design
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by estade@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.
(also, please CQ if lgty)
The CQ bit was checked by sadrul@chromium.org
lgtm lgtm. cq'ing
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Some more pre-material cleanups BUG=648281 ========== to ========== Some more pre-material cleanups BUG=648281 Committed: https://crrev.com/3d5fe31add4e90541a5164216f9a16b6cf0c7086 Cr-Commit-Position: refs/heads/master@{#428603} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3d5fe31add4e90541a5164216f9a16b6cf0c7086 Cr-Commit-Position: refs/heads/master@{#428603}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2464063002/ by kjellander@chromium.org. The reason for reverting is: Speculative revert since being the only commit in the first of a long series of flaky failures in interactive_ui_tests on Mac10.10 Tests: https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/8428. |
