Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696263002:
Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton (Closed)
DescriptionRefactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton.
ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric.
BUG=652024, 687349
Review-Url: https://codereview.chromium.org/2696263002
Cr-Commit-Position: refs/heads/master@{#456078}
Committed: https://chromium.googlesource.com/chromium/src/+/c34b8926fd843300658dcdaec4c69c6967cf86c6
Patch Set 1 #Patch Set 2 : Reduced ViewsDelegate layout/metric functions to only two functions #
Total comments: 7
Patch Set 3 : Fix unit_tests #Patch Set 4 : Moved enums out of ViewsDelegate #Patch Set 5 : Added GetDefaultSpacingMetric to ViewsDelegate to start process of hiding layout constants #
Total comments: 3
Patch Set 6 : Fix unit tests to use a ViewsDelegate to guard against crashes #
Total comments: 1
Patch Set 7 : Move TestViewsDelegate to actual test from the ancestor harness #Patch Set 8 : Prefer embedded initialization over heap allocation for TestViewsDelegate #
Total comments: 33
Patch Set 9 : Moved GetDefaultSpacingMetric to ChromeViewsDelegate Refactor ReloadButtonTest to better conform to… #Patch Set 10 : Merged with latest changes. #
Total comments: 2
Patch Set 11 : Rename SpacingMetric to DistanceMetric. Removed ICON_TO_TEXT in favor of existing element. #Patch Set 12 : Use 'related control spacing' constant for icon-to-text spacing #
Total comments: 11
Patch Set 13 : Some final nits #Patch Set 14 : Used ifdef instead of duplicated code for delegate initialization #Messages
Total messages: 116 (69 generated)
Description was changed from ========== MD-ify the icon-to-text spacing for checkbox and radiobutton BUG=652024 ========== to ========== MD-ify the icon-to-text spacing for checkbox and radiobutton BUG=652024,687349 ==========
Description was changed from ========== MD-ify the icon-to-text spacing for checkbox and radiobutton BUG=652024,687349 ========== to ========== MD-ify the icon-to-text spacing for checkbox and radiobutton. Refactor ViewsDelegate. ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric. BUG=652024,687349 ==========
Description was changed from ========== MD-ify the icon-to-text spacing for checkbox and radiobutton. Refactor ViewsDelegate. ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric. BUG=652024,687349 ========== to ========== Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton. ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric. BUG=652024,687349 ==========
kylixrd@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org
This CL covers refactoring of the ViewsDelegate in which all the various separate metric functions are coalesced into a couple of functions which use an enumeration for the metric. Separately, the spacing between the icon and the text on LabelButton (and subsequently Checkbox and RadioButton) to 16dip for MD.
The CQ bit was checked by kylixrd@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
I only looked at ui/views https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... ui/views/layout/layout_constants.h:88: constexpr int kIconTextSpacing = 5; Isn't the goal to effectively get rid of this file? If we don't, folks will still use it and miss routing through ViewsDelegate. Can you move the constant to views_delegate.cc? It looks like the constant is referenced in Harmony, can it call to ViewsDelegate? https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate... ui/views/views_delegate.h:91: enum class InsetsMetric { optional: move this and SpacingMetric outside of ViewsDelegate class (still in this file) so that code like: views_delegate->GetInsetsMetric(ViewsDelegate::InsetsMetric::BUBBLE_DIALOG); is: views_delegate->GetInsetsMetric(InsetsMetric::BUBBLE_DIALOG); Typing ViewsDelegate, or worse yet views::ViewDelegate:: all the time is rather tedious.
The CQ bit was checked by kylixrd@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/2696263002/diff/20001/ui/views/layout/layout_... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... ui/views/layout/layout_constants.h:88: constexpr int kIconTextSpacing = 5; On 2017/02/17 20:50:46, sky wrote: > Isn't the goal to effectively get rid of this file? If we don't, folks will > still use it and miss routing through ViewsDelegate. Can you move the constant > to views_delegate.cc? It looks like the constant is referenced in Harmony, can > it call to ViewsDelegate? That would then be a circular reference, which would lead to an infinite loop or stack overflow. The goal is to eliminate the LayoutDelegate and incorporate it all into ViewsDelegate. This would then resolve the circular reference. https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate... ui/views/views_delegate.h:91: enum class InsetsMetric { On 2017/02/17 20:50:46, sky wrote: > optional: move this and SpacingMetric outside of ViewsDelegate class (still in > this file) so that code like: > > views_delegate->GetInsetsMetric(ViewsDelegate::InsetsMetric::BUBBLE_DIALOG); > > is: > > views_delegate->GetInsetsMetric(InsetsMetric::BUBBLE_DIALOG); > > Typing ViewsDelegate, or worse yet views::ViewDelegate:: all the time is rather > tedious. I added them based on the precedent set by ProcessMenuAcceleratorResult and AppbarAutohideEdge above. Should they also be moved outside ViewsDelegate as well?
https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... ui/views/layout/layout_constants.h:88: constexpr int kIconTextSpacing = 5; On 2017/02/17 21:40:51, kylix_rd wrote: > On 2017/02/17 20:50:46, sky wrote: > > Isn't the goal to effectively get rid of this file? If we don't, folks will > > still use it and miss routing through ViewsDelegate. Can you move the constant > > to views_delegate.cc? It looks like the constant is referenced in Harmony, can > > it call to ViewsDelegate? > > That would then be a circular reference, which would lead to an infinite loop or > stack overflow. > > The goal is to eliminate the LayoutDelegate and incorporate it all into > ViewsDelegate. This would then resolve the circular reference. I was hoping you could call directly to the base implementation. https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate... ui/views/views_delegate.h:91: enum class InsetsMetric { On 2017/02/17 21:40:51, kylix_rd wrote: > On 2017/02/17 20:50:46, sky wrote: > > optional: move this and SpacingMetric outside of ViewsDelegate class (still in > > this file) so that code like: > > > > views_delegate->GetInsetsMetric(ViewsDelegate::InsetsMetric::BUBBLE_DIALOG); > > > > is: > > > > views_delegate->GetInsetsMetric(InsetsMetric::BUBBLE_DIALOG); > > > > Typing ViewsDelegate, or worse yet views::ViewDelegate:: all the time is > rather > > tedious. > > I added them based on the precedent set by ProcessMenuAcceleratorResult and > AppbarAutohideEdge above. Should they also be moved outside ViewsDelegate as > well? For enums that are used extensively, like I think this one will be, I side with making it less verbose to type. That isn't the case with ProcessMenuAcceleratorResult .
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... File ui/views/layout/layout_constants.h (right): https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... ui/views/layout/layout_constants.h:88: constexpr int kIconTextSpacing = 5; On 2017/02/17 22:39:03, sky wrote: > On 2017/02/17 21:40:51, kylix_rd wrote: > > On 2017/02/17 20:50:46, sky wrote: > > > Isn't the goal to effectively get rid of this file? If we don't, folks will > > > still use it and miss routing through ViewsDelegate. Can you move the > constant > > > to views_delegate.cc? It looks like the constant is referenced in Harmony, > can > > > it call to ViewsDelegate? > > > > That would then be a circular reference, which would lead to an infinite loop > or > > stack overflow. > > > > The goal is to eliminate the LayoutDelegate and incorporate it all into > > ViewsDelegate. This would then resolve the circular reference. > > I was hoping you could call directly to the base implementation. Yes. And I'm not a huge fan of how LayoutDelegate and ViewsDelegate have different ideas of when they want to return a number versus a gfx::Insets. I think this is primarily because we've been building LayoutDelegate to return single numbers, and the existing APIs there do that, but we probably want to return real Insets from it anyway. I would make the goal for the initial ViewsDelegate refactoring CL to not have any overlap between ViewsDelegate and LayoutDelegate constants. If views_delegate.h declares a metric, then that metric (or a component thereof) probably should not appear in layout_delegate.h. The most straightforward way to achieve this would probably be to create a views-level LayoutDelegate, containing all the metrics needed in views/, with a Chrome-side subclass extending it for further Chrome-specific metrics, and then move all of this entirely out of ViewsDelegate. (This is bug 687349 comment 3.) The other way is what the bug originally describes, which is to nuke the metric-related functions from LayoutDelegate (but leave it around for other things), and move them into ChromeViewsDelegate, splitting the enum into two pieces. Neither one of these is the work of five minutes to implement, which is why I hadn't gotten to this yet :)
The CQ bit was checked by kylixrd@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.
Moved enum out of the ViewsDelegate class up to the views namespace. Merged with latest ViewsDelegate changes.
On 2017/02/17 21:40:51, kylix_rd wrote: > https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... > File ui/views/layout/layout_constants.h (right): > > https://codereview.chromium.org/2696263002/diff/20001/ui/views/layout/layout_... > ui/views/layout/layout_constants.h:88: constexpr int kIconTextSpacing = 5; > On 2017/02/17 20:50:46, sky wrote: > > Isn't the goal to effectively get rid of this file? If we don't, folks will > > still use it and miss routing through ViewsDelegate. Can you move the constant > > to views_delegate.cc? It looks like the constant is referenced in Harmony, can > > it call to ViewsDelegate? > > That would then be a circular reference, which would lead to an infinite loop or > stack overflow. > > The goal is to eliminate the LayoutDelegate and incorporate it all into > ViewsDelegate. This would then resolve the circular reference. How about making ChromeViewsDelegate have a function to get the default value and HarmonyLayoutDelegate calls this rather than using kIconTextSpacing from views? My reasoning here is that exposing kIconTextSpacing as public is encouraging people to use it, when you don't want that. > > https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate.h > File ui/views/views_delegate.h (right): > > https://codereview.chromium.org/2696263002/diff/20001/ui/views/views_delegate... > ui/views/views_delegate.h:91: enum class InsetsMetric { > On 2017/02/17 20:50:46, sky wrote: > > optional: move this and SpacingMetric outside of ViewsDelegate class (still in > > this file) so that code like: > > > > views_delegate->GetInsetsMetric(ViewsDelegate::InsetsMetric::BUBBLE_DIALOG); > > > > is: > > > > views_delegate->GetInsetsMetric(InsetsMetric::BUBBLE_DIALOG); > > > > Typing ViewsDelegate, or worse yet views::ViewDelegate:: all the time is > rather > > tedious. > > I added them based on the precedent set by ProcessMenuAcceleratorResult and > AppbarAutohideEdge above. Should they also be moved outside ViewsDelegate as > well?
On 2017/02/23 15:13:40, kylix_rd wrote: > Moved enum out of the ViewsDelegate class up to the views namespace. Merged with > latest ViewsDelegate changes. It looks like this doesn't implement what I suggested, which might be fine, if you think it makes sense to do this as a stepping-stone patch and that in a followup CL. But I'd like to hear more about whether you agree or disagree with the proposal and what your longer-term plan is.
On 2017/02/23 22:45:48, Peter Kasting wrote: > On 2017/02/23 15:13:40, kylix_rd wrote: > > Moved enum out of the ViewsDelegate class up to the views namespace. Merged > with > > latest ViewsDelegate changes. > > It looks like this doesn't implement what I suggested, which might be fine, if > you think it makes sense to do this as a stepping-stone patch and that in a > followup CL. But I'd like to hear more about whether you agree or disagree with > the proposal and what your longer-term plan is. Correct. This is more about starting the process of coalescing the constants and multiple functions into a handful of general functions with an enum selector. The first suggestions in https://bugs.chromium.org/p/chromium/issues/detail?id=687349 would require a unsafe or static down-cast within an added static function to return the proper ChromeViewsDelegate type. The second suggestion is less extensible because many of the various metrics are only used in the higher level chrome/browser code. It would also require moving more MD knowledge into the views-layer, which sky@ had indicated should not be there (although when MD is the default, the layout will cease to be switchable... at least until the next UI rework).
The CQ bit was checked by kylixrd@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...
Latest patch adds GetDefaultSpacingMetric() static function per suggestion from sky@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/layout_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/layout_delegate.cc:62: return views::ViewsDelegate::GetDefaultSpacingMetric( I would rather see this code call ChromeViewsDelegate::Get()->GetDefaultSpacingMetric(). Where GetDefaultSpacingMetric() is private in ChromeViewsDelegate and this class is a friend of ChromeViewsDelegate. That way GetDefaultSpacingMetric() is not public and can't be accidentally called.
https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/layout_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/layout_delegate.cc:62: return views::ViewsDelegate::GetDefaultSpacingMetric( On 2017/02/24 18:31:13, sky wrote: > I would rather see this code call > ChromeViewsDelegate::Get()->GetDefaultSpacingMetric(). Where > GetDefaultSpacingMetric() is private in ChromeViewsDelegate and this class is a > friend of ChromeViewsDelegate. That way GetDefaultSpacingMetric() is not public > and can't be accidentally called. I placed the function on ViewsDelegate as a static since it may need to be called in cases where there is no ViewsDelegate instance setup. Some of the unit-tests fail with access violations or seg-faults (the ViewsDelegate singleton instance is nullptr). For instance, the LabelButton control does this... I suppose it could fall back to a literal constant if ViewsDelegate::GetInstance() returns nullptr.
On 2017/02/24 14:07:25, kylix_rd wrote: > On 2017/02/23 22:45:48, Peter Kasting wrote: > > On 2017/02/23 15:13:40, kylix_rd wrote: > > > Moved enum out of the ViewsDelegate class up to the views namespace. Merged > > with > > > latest ViewsDelegate changes. > > > > It looks like this doesn't implement what I suggested, which might be fine, if > > you think it makes sense to do this as a stepping-stone patch and that in a > > followup CL. But I'd like to hear more about whether you agree or disagree > with > > the proposal and what your longer-term plan is. > > Correct. This is more about starting the process of coalescing the constants and > multiple functions into a handful of general functions with an enum selector. > The first suggestions in > https://bugs.chromium.org/p/chromium/issues/detail?id=687349 would require a > unsafe or static down-cast within an added static function to return the proper > ChromeViewsDelegate type. I don't know that I follow this. Wouldn't that only be true if ChromeViewsDelegate was the Source Of Truth for this stuff, and ViewsDelegate was trying to cast to it (rather than the other way around)? I think I'm not following you. Feel free to pseudocode to make things clearer. > The second suggestion is less extensible because many > of the various metrics are only used in the higher level chrome/browser code. It > would also require moving more MD knowledge into the views-layer, which sky@ had > indicated should not be there (although when MD is the default, the layout will > cease to be switchable... at least until the next UI rework). Again, I'm confused by this. The idea would be that there would be LayoutDelegate and ChromeLayoutDelegate (just like with ViewsDelegate), and only the latter would expose APIs not relevant to the core views/ code. https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/layout_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/layout_delegate.cc:62: return views::ViewsDelegate::GetDefaultSpacingMetric( On 2017/02/24 19:04:20, kylix_rd wrote: > On 2017/02/24 18:31:13, sky wrote: > > I would rather see this code call > > ChromeViewsDelegate::Get()->GetDefaultSpacingMetric(). Where > > GetDefaultSpacingMetric() is private in ChromeViewsDelegate and this class is > a > > friend of ChromeViewsDelegate. That way GetDefaultSpacingMetric() is not > public > > and can't be accidentally called. > > I placed the function on ViewsDelegate as a static since it may need to be > called in cases where there is no ViewsDelegate instance setup. Some of the > unit-tests fail with access violations or seg-faults (the ViewsDelegate > singleton instance is nullptr). > > For instance, the LabelButton control does this... I suppose it could fall back > to a literal constant if ViewsDelegate::GetInstance() returns nullptr. We should fix the tests so there is always a ViewsDelegate instance. That's much nicer than trying to check for and handle null in various cases, which is why we've started ripping out all the null checks of this everywhere.
On 2017/02/24 20:41:03, Peter Kasting wrote: > On 2017/02/24 14:07:25, kylix_rd wrote: > > On 2017/02/23 22:45:48, Peter Kasting wrote: > > > On 2017/02/23 15:13:40, kylix_rd wrote: > > > > Moved enum out of the ViewsDelegate class up to the views namespace. > Merged > > > with > > > > latest ViewsDelegate changes. > > > > > > It looks like this doesn't implement what I suggested, which might be fine, > if > > > you think it makes sense to do this as a stepping-stone patch and that in a > > > followup CL. But I'd like to hear more about whether you agree or disagree > > with > > > the proposal and what your longer-term plan is. > > > > Correct. This is more about starting the process of coalescing the constants > and > > multiple functions into a handful of general functions with an enum selector. > > The first suggestions in > > https://bugs.chromium.org/p/chromium/issues/detail?id=687349 would require a > > unsafe or static down-cast within an added static function to return the > proper > > ChromeViewsDelegate type. > > I don't know that I follow this. Wouldn't that only be true if > ChromeViewsDelegate was the Source Of Truth for this stuff, and ViewsDelegate > was trying to cast to it (rather than the other way around)? > > I think I'm not following you. Feel free to pseudocode to make things clearer. ViewsDelegate::GetInstance() returns a ViewsDelegate* instance. Even if the actual instance were of ChromeViewsDelegate, GetMetric would be the version which takes the ViewsMetric enum. You could not pass a ChromeMetric element to it without using a static_cast at the call site. The other way would be to down-cast the ViewsDelegate* instance to ChromeViewsDelegate* and then call *that* version of GetMetric. > > The second suggestion is less extensible because many > > of the various metrics are only used in the higher level chrome/browser code. > It > > would also require moving more MD knowledge into the views-layer, which sky@ > had > > indicated should not be there (although when MD is the default, the layout > will > > cease to be switchable... at least until the next UI rework). > > Again, I'm confused by this. The idea would be that there would be > LayoutDelegate and ChromeLayoutDelegate (just like with ViewsDelegate), and only > the latter would expose APIs not relevant to the core views/ code. Should this use the same approach as mentioned in the description of the aforementioned issue, then it suffers same problem I outlined above. > https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/harmony/layout_delegate.cc (right): > > https://codereview.chromium.org/2696263002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/harmony/layout_delegate.cc:62: return > views::ViewsDelegate::GetDefaultSpacingMetric( > On 2017/02/24 19:04:20, kylix_rd wrote: > > On 2017/02/24 18:31:13, sky wrote: > > > I would rather see this code call > > > ChromeViewsDelegate::Get()->GetDefaultSpacingMetric(). Where > > > GetDefaultSpacingMetric() is private in ChromeViewsDelegate and this class > is > > a > > > friend of ChromeViewsDelegate. That way GetDefaultSpacingMetric() is not > > public > > > and can't be accidentally called. > > > > I placed the function on ViewsDelegate as a static since it may need to be > > called in cases where there is no ViewsDelegate instance setup. Some of the > > unit-tests fail with access violations or seg-faults (the ViewsDelegate > > singleton instance is nullptr). > > > > For instance, the LabelButton control does this... I suppose it could fall > back > > to a literal constant if ViewsDelegate::GetInstance() returns nullptr. > > We should fix the tests so there is always a ViewsDelegate instance. That's > much nicer than trying to check for and handle null in various cases, which is > why we've started ripping out all the null checks of this everywhere. I'll need to research how to make that happen. The ViewsDelegate instance would need to be explicitly instantiated... someplace.
On 2017/02/24 21:15:01, kylix_rd wrote: > On 2017/02/24 20:41:03, Peter Kasting wrote: > > On 2017/02/24 14:07:25, kylix_rd wrote: > > > On 2017/02/23 22:45:48, Peter Kasting wrote: > > > > On 2017/02/23 15:13:40, kylix_rd wrote: > > > > > Moved enum out of the ViewsDelegate class up to the views namespace. > > Merged > > > > with > > > > > latest ViewsDelegate changes. > > > > > > > > It looks like this doesn't implement what I suggested, which might be > fine, > > if > > > > you think it makes sense to do this as a stepping-stone patch and that in > a > > > > followup CL. But I'd like to hear more about whether you agree or > disagree > > > with > > > > the proposal and what your longer-term plan is. > > > > > > Correct. This is more about starting the process of coalescing the constants > > and > > > multiple functions into a handful of general functions with an enum > selector. > > > The first suggestions in > > > https://bugs.chromium.org/p/chromium/issues/detail?id=687349 would require a > > > unsafe or static down-cast within an added static function to return the > > proper > > > ChromeViewsDelegate type. > > > > I don't know that I follow this. Wouldn't that only be true if > > ChromeViewsDelegate was the Source Of Truth for this stuff, and ViewsDelegate > > was trying to cast to it (rather than the other way around)? > > > > I think I'm not following you. Feel free to pseudocode to make things > clearer. > > ViewsDelegate::GetInstance() returns a ViewsDelegate* instance. Even if the > actual instance were of ChromeViewsDelegate, GetMetric would be the version > which takes the ViewsMetric enum. You could not pass a ChromeMetric element to > it without using a static_cast at the call site. Correct. views/ code would use ViewsDelegate::GetInstance(). I was thinking chrome/ code would use a newly-added ChromeViewsDelegate::GetInstance() method. Then each side would get the appropriate type, which would help ensure they were restricted to the correct enum values. > > We should fix the tests so there is always a ViewsDelegate instance. That's > > much nicer than trying to check for and handle null in various cases, which is > > why we've started ripping out all the null checks of this everywhere. > > I'll need to research how to make that happen. The ViewsDelegate instance would > need to be explicitly instantiated... someplace. Take a look at TestViewsDelegate and who instantiates it. It looks like some core test helpers like ViewEventTestBase already do.
The CQ bit was checked by kylixrd@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by kylixrd@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by kylixrd@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
On 2017/02/24 23:15:55, Peter Kasting wrote: > > > We should fix the tests so there is always a ViewsDelegate instance. That's > > > much nicer than trying to check for and handle null in various cases, which > is > > > why we've started ripping out all the null checks of this everywhere. > > > > I'll need to research how to make that happen. The ViewsDelegate instance > would > > need to be explicitly instantiated... someplace. > > Take a look at TestViewsDelegate and who instantiates it. It looks like some > core test helpers like ViewEventTestBase already do. I got the tests that were failing which didn't have a ViewsDelegate instance to now work. However, the Android builds are failing because the implementation of TestViewsDelegate isn't being linked in. I'm not sure which, if any, implementation that would be.
On 2017/02/27 21:54:49, kylix_rd wrote: > On 2017/02/24 23:15:55, Peter Kasting wrote: > > > > We should fix the tests so there is always a ViewsDelegate instance. > That's > > > > much nicer than trying to check for and handle null in various cases, > which > > is > > > > why we've started ripping out all the null checks of this everywhere. > > > > > > I'll need to research how to make that happen. The ViewsDelegate instance > > would > > > need to be explicitly instantiated... someplace. > > > > Take a look at TestViewsDelegate and who instantiates it. It looks like some > > core test helpers like ViewEventTestBase already do. > > I got the tests that were failing which didn't have a ViewsDelegate instance to > now work. However, the Android builds are failing because the implementation of > TestViewsDelegate isn't being linked in. I'm not sure which, if any, > implementation that would be. Android doesn't use views. Based on the link error some file is trying to use TestViewsDelegate now that shouldn't be. Maybe https://codereview.chromium.org/2696263002/diff/140001/chrome/test/base/chrom... ?
https://codereview.chromium.org/2696263002/diff/140001/chrome/test/base/chrom... File chrome/test/base/chrome_render_view_host_test_harness.h (right): https://codereview.chromium.org/2696263002/diff/140001/chrome/test/base/chrom... chrome/test/base/chrome_render_view_host_test_harness.h:34: std::unique_ptr<views::TestViewsDelegate> views_delegate_; Yeah, pretty sure this can't be here, as RVH tests are broader than just views/.
On 2017/02/27 23:44:33, sky wrote: > On 2017/02/27 21:54:49, kylix_rd wrote: > > On 2017/02/24 23:15:55, Peter Kasting wrote: > > > > > We should fix the tests so there is always a ViewsDelegate instance. > > That's > > > > > much nicer than trying to check for and handle null in various cases, > > which > > > is > > > > > why we've started ripping out all the null checks of this everywhere. > > > > > > > > I'll need to research how to make that happen. The ViewsDelegate instance > > > would > > > > need to be explicitly instantiated... someplace. > > > > > > Take a look at TestViewsDelegate and who instantiates it. It looks like > some > > > core test helpers like ViewEventTestBase already do. > > > > I got the tests that were failing which didn't have a ViewsDelegate instance > to > > now work. However, the Android builds are failing because the implementation > of > > TestViewsDelegate isn't being linked in. I'm not sure which, if any, > > implementation that would be. > > Android doesn't use views. Based on the link error some file is trying to use > TestViewsDelegate now that shouldn't be. Maybe > https://codereview.chromium.org/2696263002/diff/140001/chrome/test/base/chrom... > ? It is interesting that the unit-tests on Android seem to be instantiating a LabelButton. Am I not reading the code correctly?
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by kylixrd@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...
On 2017/02/28 15:08:26, kylix_rd wrote: > On 2017/02/27 23:44:33, sky wrote: > > On 2017/02/27 21:54:49, kylix_rd wrote: > > > On 2017/02/24 23:15:55, Peter Kasting wrote: > > > > > > We should fix the tests so there is always a ViewsDelegate instance. > > > That's > > > > > > much nicer than trying to check for and handle null in various cases, > > > which > > > > is > > > > > > why we've started ripping out all the null checks of this everywhere. > > > > > > > > > > I'll need to research how to make that happen. The ViewsDelegate > instance > > > > would > > > > > need to be explicitly instantiated... someplace. > > > > > > > > Take a look at TestViewsDelegate and who instantiates it. It looks like > > some > > > > core test helpers like ViewEventTestBase already do. > > > > > > I got the tests that were failing which didn't have a ViewsDelegate instance > > to > > > now work. However, the Android builds are failing because the implementation > > of > > > TestViewsDelegate isn't being linked in. I'm not sure which, if any, > > > implementation that would be. > > > > Android doesn't use views. Based on the link error some file is trying to use > > TestViewsDelegate now that shouldn't be. Maybe > > > https://codereview.chromium.org/2696263002/diff/140001/chrome/test/base/chrom... > > ? > > It is interesting that the unit-tests on Android seem to be instantiating a > LabelButton. Am I not reading the code correctly? I think I was reading the code incorrectly. My mistake.
Fixed Android build failure and change initialization of the TestViewsDelegate from heap allocated to embedded in the test class.
The CQ bit was checked by kylixrd@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.
https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/reload_button_unittest.cc (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:5: #include "chrome/browser/ui/views/toolbar/reload_button.h" newline between 5/6 (see style guide for details). https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:29: private: Style guide says private section should be last (after protected, which is after public). https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... File content/public/test/test_renderer_host.cc (right): https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... content/public/test/test_renderer_host.cc:268: rvh_test_enabler_.reset(new RenderViewHostTestEnabler()); Revert this change? https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:237: static int GetDefaultSpacingMetric(SpacingMetric metric); Now that you've made it so ViewsDelegate is always created is this still needed?
https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.cc:48: views::SpacingMetric::RELATED_VERTICAL_CONTROL); Seems weird that a HORIZONTAL-type metric here would return a value from a VERTICAL-type metric. https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:52: // associated text. This includes checkboxes and radio buttons. I'm confused. How is this different than RELATED_LABEL_HORIZONTAL_SPACING, covers "an item such as an icon or checkbox and a label related to it"? It seems like we shouldn't have both of these. Unless they really mean completely different things and should be clarified, let's collapse, even if that causes a behavior change. Note: If this stays, alphabetize. https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/reload_button_unittest.cc (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:34: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... File content/public/test/test_renderer_host.cc (right): https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... content/public/test/test_renderer_host.cc:268: rvh_test_enabler_.reset(new RenderViewHostTestEnabler()); On 2017/02/28 20:11:52, sky wrote: > Revert this change? Didn't chromium-dev have a long thread about this a while back and how usually people should use the () form unless they really, really knew what they were doing? I can't seem to find it in gmail search though. https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... File ui/views/views_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.cc:24: constexpr int kIconTextSpacing = 5; Nit: Can be defined in the function below that uses this https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:68: // The margins that should be applied around a bubble dialog. What is a "bubble dialog"? Is that what the UX folks are trying to rename a popover? How is that different from a non-bubble dialog? I mostly ask all this because "bubble" is a confusing term in our codebase and I'm hoping to excise it, or at least prevent it spreading. https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:72: enum class SpacingMetric { Nit: I suggest naming these to closely align with the layout_constants names. https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:85: // Returns the default minimum width of a dialog button. Nit: No "Returns" (2 places) https://codereview.chromium.org/2696263002/diff/200001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2696263002/diff/200001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:58: int spacing = ViewsDelegate::GetInstance() Doesn't seem like you need to add null-checks. I suspect this is due to not rebasing properly, since we used to have things like these, but they got removed. (4 places) https://codereview.chromium.org/2696263002/diff/200001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:137: : kRelatedButtonHSpacing; More bad merging? https://codereview.chromium.org/2696263002/diff/200001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:316: // this can be deleted when harmony is always on. Another bad merge, I think
https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... File content/public/test/test_renderer_host.cc (right): https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... content/public/test/test_renderer_host.cc:268: rvh_test_enabler_.reset(new RenderViewHostTestEnabler()); On 2017/03/01 06:32:36, Peter Kasting wrote: > On 2017/02/28 20:11:52, sky wrote: > > Revert this change? > > Didn't chromium-dev have a long thread about this a while back and how usually > people should use the () form unless they really, really knew what they were > doing? I can't seem to find it in gmail search though. I don't remember that, but it's certainly possible I msised it. If the () is preferable, keep it. Although for my knowledge could you give me the quick synopsis as why the () is preferable?
The CQ bit was checked by kylixrd@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/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.cc:48: views::SpacingMetric::RELATED_VERTICAL_CONTROL); On 2017/03/01 06:32:36, Peter Kasting wrote: > Seems weird that a HORIZONTAL-type metric here would return a value from a > VERTICAL-type metric. Oops. My mistake. Will be fixed. https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:52: // associated text. This includes checkboxes and radio buttons. On 2017/03/01 06:32:36, Peter Kasting wrote: > I'm confused. How is this different than RELATED_LABEL_HORIZONTAL_SPACING, > covers "an item such as an icon or checkbox and a label related to it"? > > It seems like we shouldn't have both of these. Unless they really mean > completely different things and should be clarified, let's collapse, even if > that causes a behavior change. > > Note: If this stays, alphabetize. The original constant, private to the label_button.cc module, was kSpacing with a value of 5. RELATED_LABEL_HORIZONTAL_SPACING would eventually resolve to kItemLabelSpacing which has a value of 10. I added a new constant since it was different from kItemLabelSpacing. kItemLabelSpacing is clearly confusing and is used in a different context. A quick search of the source seems to show that its use isn't what it appears to say it's for. https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/reload_button_unittest.cc (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:5: #include "chrome/browser/ui/views/toolbar/reload_button.h" On 2017/02/28 20:11:52, sky wrote: > newline between 5/6 (see style guide for details). Interesting... this was changed by git cl format, which I would have assumed would do the right thing. https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:5: #include "chrome/browser/ui/views/toolbar/reload_button.h" On 2017/02/28 20:11:52, sky wrote: > newline between 5/6 (see style guide for details). Done. https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:29: private: On 2017/02/28 20:11:52, sky wrote: > Style guide says private section should be last (after protected, which is after > public). It seems that there is a chicken-and-egg problem then. Placing the TestViewsDelegate field lexically after the ReloadButton field causes the ReloadButton to be constructed prior to the TestViewsDelegate. This will lead to a crash in the construction of the ReloadButton. The solution would be to move the ReloadButton field to the private section and then add a getter function returning a ReloadButton& to the protected section and refactor the tests which reference the reload_ field. https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:34: }; On 2017/03/01 06:32:36, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... File content/public/test/test_renderer_host.cc (right): https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... content/public/test/test_renderer_host.cc:268: rvh_test_enabler_.reset(new RenderViewHostTestEnabler()); On 2017/03/01 06:32:36, Peter Kasting wrote: > On 2017/02/28 20:11:52, sky wrote: > > Revert this change? > > Didn't chromium-dev have a long thread about this a while back and how usually > people should use the () form unless they really, really knew what they were > doing? I can't seem to find it in gmail search though. That is likely correct because IIRC, this was changed by running git cl format. https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:68: // The margins that should be applied around a bubble dialog. On 2017/03/01 06:32:36, Peter Kasting wrote: > What is a "bubble dialog"? Is that what the UX folks are trying to rename a > popover? How is that different from a non-bubble dialog? Yes, I think the UX folks are using "popover" as a synonym for "bubble dialog". AFICT, the bubbles are used for the pop-overs related to the content error/state glyphs which appear in the OmniBox such as the Register Protocol Handler glyph, blocked flash content, etc... > I mostly ask all this because "bubble" is a confusing term in our codebase and > I'm hoping to excise it, or at least prevent it spreading. It's used in the various "bubble_dialog_xxxx" modules, so I kept the naming consistent. If the goal is to excise "bubble" from the codebase, that should be a focused, single purpose effort separate from the goals here. https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:72: enum class SpacingMetric { On 2017/03/01 06:32:36, Peter Kasting wrote: > Nit: I suggest naming these to closely align with the layout_constants names. Some of those constant names contain the word "SPACING" which creates a naming redundancy. Using an enum class to provide the categorical context along with the specific metric seems more clear and less redundant. I don't want to have a name such as SpacingMetric::ICON_TEXT_SPACING. That looks like; "Department of Redundancy Department" https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:85: // Returns the default minimum width of a dialog button. On 2017/03/01 06:32:36, Peter Kasting wrote: > Nit: No "Returns" (2 places) Done. https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:237: static int GetDefaultSpacingMetric(SpacingMetric metric); On 2017/02/28 20:11:52, sky wrote: > Now that you've made it so ViewsDelegate is always created is this still needed? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(have not looked at new patch, on the run) https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:52: // associated text. This includes checkboxes and radio buttons. On 2017/03/01 17:33:27, kylix_rd wrote: > On 2017/03/01 06:32:36, Peter Kasting wrote: > > I'm confused. How is this different than RELATED_LABEL_HORIZONTAL_SPACING, > > covers "an item such as an icon or checkbox and a label related to it"? > > > > It seems like we shouldn't have both of these. Unless they really mean > > completely different things and should be clarified, let's collapse, even if > > that causes a behavior change. > > > > Note: If this stays, alphabetize. > > The original constant, private to the label_button.cc module, was kSpacing with > a value of 5. RELATED_LABEL_HORIZONTAL_SPACING would eventually resolve to > kItemLabelSpacing which has a value of 10. I added a new constant since it was > different from kItemLabelSpacing. Right. My hope was to either decide we could change the existing behavior and unify both as either 5 or 10, or else to understand the necessary difference well enough to make the names and comments really clear so that it would be obvious what these would actually be used for. I'm OK with either route, the first is preferable :) https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/reload_button_unittest.cc (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:29: private: On 2017/03/01 17:33:27, kylix_rd wrote: > On 2017/02/28 20:11:52, sky wrote: > > Style guide says private section should be last (after protected, which is > after > > public). > > It seems that there is a chicken-and-egg problem then. Placing the > TestViewsDelegate field lexically after the ReloadButton field causes the > ReloadButton to be constructed prior to the TestViewsDelegate. This will lead to > a crash in the construction of the ReloadButton. > > The solution would be to move the ReloadButton field to the private section and > then add a getter function returning a ReloadButton& to the protected section > and refactor the tests which reference the reload_ field. I was gonna leave a comment about how this test really shouldn't have a protected data member, but I didn't want to burden you with unnecessary code cleanup in the midst of trying to land this. But... yeah, that seems like the correct way to solve :) https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... File content/public/test/test_renderer_host.cc (right): https://codereview.chromium.org/2696263002/diff/200001/content/public/test/te... content/public/test/test_renderer_host.cc:268: rvh_test_enabler_.reset(new RenderViewHostTestEnabler()); On 2017/03/01 17:07:16, sky wrote: > On 2017/03/01 06:32:36, Peter Kasting wrote: > > On 2017/02/28 20:11:52, sky wrote: > > > Revert this change? > > > > Didn't chromium-dev have a long thread about this a while back and how usually > > people should use the () form unless they really, really knew what they were > > doing? I can't seem to find it in gmail search though. > > I don't remember that, but it's certainly possible I msised it. If the () is > preferable, keep it. Although for my knowledge could you give me the quick > synopsis as why the () is preferable? From memory, () zero-inits certain objects and no-() leaves the members as garbage? https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:68: // The margins that should be applied around a bubble dialog. On 2017/03/01 17:33:27, kylix_rd wrote: > On 2017/03/01 06:32:36, Peter Kasting wrote: > > I mostly ask all this because "bubble" is a confusing term in our codebase and > > I'm hoping to excise it, or at least prevent it spreading. > > It's used in the various "bubble_dialog_xxxx" modules, so I kept the naming > consistent. If the goal is to excise "bubble" from the codebase, that should be > a focused, single purpose effort separate from the goals here. That's fair. https://codereview.chromium.org/2696263002/diff/200001/ui/views/views_delegat... ui/views/views_delegate.h:72: enum class SpacingMetric { On 2017/03/01 17:33:27, kylix_rd wrote: > On 2017/03/01 06:32:36, Peter Kasting wrote: > > Nit: I suggest naming these to closely align with the layout_constants names. > > Some of those constant names contain the word "SPACING" which creates a naming > redundancy. Using an enum class to provide the categorical context along with > the specific metric seems more clear and less redundant. I don't want to have a > name such as SpacingMetric::ICON_TEXT_SPACING. That looks like; "Department of > Redundancy Department" That's fine, I didn't mean you had to copy names exactly. Dropping SPACING or something is OK. (Although perhaps the problem is that this enum is "SpacingMetric" instead of "IntegralMetric" or the like, so the fix is really to rename the enum to just mean "some integer value"). But having e.g. RELATED_HORIZONTAL_BUTTON here versus RELATED_BUTTON_HORIZONTAL[_SPACING] is a problem because it changes both the alphabetization and, more importantly, the meaning. A "horizontal button" versus a "related button" are different things :)
The CQ bit was checked by kylixrd@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...
On 2017/03/01 23:32:24, Peter Kasting wrote: > (have not looked at new patch, on the run) > That's fine, I didn't mean you had to copy names exactly. Dropping SPACING or > something is OK. (Although perhaps the problem is that this enum is > "SpacingMetric" instead of "IntegralMetric" or the like, so the fix is really to > rename the enum to just mean "some integer value"). > > But having e.g. RELATED_HORIZONTAL_BUTTON here versus > RELATED_BUTTON_HORIZONTAL[_SPACING] is a problem because it changes both the > alphabetization and, more importantly, the meaning. A "horizontal button" > versus a "related button" are different things :) All the constants related to the SpacingMetric are, in fact, the "spacing" between some element. Because the enum is SpacingMetric, it should be clear that RELATED_HORIZONTAL_BUTTON has to do with "spacing" of some kind. Since it is an "enum class" the "SpacingMetric::" qualifier is required.
On 2017/03/02 18:11:05, kylix_rd wrote: > On 2017/03/01 23:32:24, Peter Kasting wrote: > > (have not looked at new patch, on the run) > > That's fine, I didn't mean you had to copy names exactly. Dropping SPACING or > > something is OK. (Although perhaps the problem is that this enum is > > "SpacingMetric" instead of "IntegralMetric" or the like, so the fix is really > to > > rename the enum to just mean "some integer value"). > > > > But having e.g. RELATED_HORIZONTAL_BUTTON here versus > > RELATED_BUTTON_HORIZONTAL[_SPACING] is a problem because it changes both the > > alphabetization and, more importantly, the meaning. A "horizontal button" > > versus a "related button" are different things :) > > All the constants related to the SpacingMetric are, in fact, the "spacing" > between some element. Because the enum is SpacingMetric, it should be clear that > RELATED_HORIZONTAL_BUTTON has to do with "spacing" of some kind. Since it is an > "enum class" the "SpacingMetric::" qualifier is required. Ok... I take some of that back. Some of the items are not spacing between elements, but rather sizes of elements. In my defense, those came in later as changes were being made to the ViewsDelegate. In those cases, maybe a SizingMetric enum should be added and a different "int GetSizingMetric(SizingMetric metric)" function is added?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/02 18:14:29, kylix_rd wrote: > On 2017/03/02 18:11:05, kylix_rd wrote: > > On 2017/03/01 23:32:24, Peter Kasting wrote: > > > (have not looked at new patch, on the run) > > > That's fine, I didn't mean you had to copy names exactly. Dropping SPACING > or > > > something is OK. (Although perhaps the problem is that this enum is > > > "SpacingMetric" instead of "IntegralMetric" or the like, so the fix is > really > > to > > > rename the enum to just mean "some integer value"). > > > > > > But having e.g. RELATED_HORIZONTAL_BUTTON here versus > > > RELATED_BUTTON_HORIZONTAL[_SPACING] is a problem because it changes both the > > > alphabetization and, more importantly, the meaning. A "horizontal button" > > > versus a "related button" are different things :) > > > > All the constants related to the SpacingMetric are, in fact, the "spacing" > > between some element. Because the enum is SpacingMetric, it should be clear > that > > RELATED_HORIZONTAL_BUTTON has to do with "spacing" of some kind. Sure, but my point was that it sounds as if "horizontal" here modifies "button", not "spacing". RELATED_BUTTON_HORIZONTAL, by contrast, makes it more clear that "horizontal" isn't about "button". When I renamed the layout delegate values I tried to pick consistent and clear names, so my bias is towards continuing to use those and simply dropping redundant parts of the names. > Ok... I take some of that back. Some of the items are not spacing between > elements, but rather sizes of elements. In my defense, those came in later as > changes were being made to the ViewsDelegate. In those cases, maybe a > SizingMetric enum should be added and a different "int > GetSizingMetric(SizingMetric metric)" function is added? I don't think we should have two different enums for "spacing constants" versus "sizing constants", because they're both really the size of something (size of a button, size of a gap), and because it just doesn't seem like a distinction that makes readers' lives clearer and easier. Hence the suggestion to just have one for integers, one for insets, etc. I mean, honestly, if it were possible to express in C++, I'd have some sort of single "GetMetric" that returns a T, where T changes type depending on what you ask for.
On 2017/03/02 20:17:46, Peter Kasting wrote: > On 2017/03/02 18:14:29, kylix_rd wrote: > > On 2017/03/02 18:11:05, kylix_rd wrote: > > > On 2017/03/01 23:32:24, Peter Kasting wrote: > > > > (have not looked at new patch, on the run) > > > > That's fine, I didn't mean you had to copy names exactly. Dropping > SPACING > > or > > > > something is OK. (Although perhaps the problem is that this enum is > > > > "SpacingMetric" instead of "IntegralMetric" or the like, so the fix is > > really > > > to > > > > rename the enum to just mean "some integer value"). > > > > > > > > But having e.g. RELATED_HORIZONTAL_BUTTON here versus > > > > RELATED_BUTTON_HORIZONTAL[_SPACING] is a problem because it changes both > the > > > > alphabetization and, more importantly, the meaning. A "horizontal button" > > > > versus a "related button" are different things :) > > > > > > All the constants related to the SpacingMetric are, in fact, the "spacing" > > > between some element. Because the enum is SpacingMetric, it should be clear > > that > > > RELATED_HORIZONTAL_BUTTON has to do with "spacing" of some kind. > > Sure, but my point was that it sounds as if "horizontal" here modifies "button", > not "spacing". RELATED_BUTTON_HORIZONTAL, by contrast, makes it more clear that > "horizontal" isn't about "button". Oh, I see what you mean. Mild dyslexia revealed those as being the same except without "_SPACING". > When I renamed the layout delegate values I tried to pick consistent and clear > names, so my bias is towards continuing to use those and simply dropping > redundant parts of the names. Agreed. > > Ok... I take some of that back. Some of the items are not spacing between > > elements, but rather sizes of elements. In my defense, those came in later as > > changes were being made to the ViewsDelegate. In those cases, maybe a > > SizingMetric enum should be added and a different "int > > GetSizingMetric(SizingMetric metric)" function is added? > > I don't think we should have two different enums for "spacing constants" versus > "sizing constants", because they're both really the size of something (size of a > button, size of a gap), and because it just doesn't seem like a distinction that > makes readers' lives clearer and easier. Hence the suggestion to just have one > for integers, one for insets, etc. Would that not then require the individual names to return to indicating whether something is "spacing" or "size"? > I mean, honestly, if it were possible to express in C++, I'd have some sort of > single "GetMetric" that returns a T, where T changes type depending on what you > ask for. Generic virtual methods are an interesting concept :).
On 2017/03/02 21:39:43, kylix_rd wrote: > On 2017/03/02 20:17:46, Peter Kasting wrote: > > On 2017/03/02 18:14:29, kylix_rd wrote: > > > Ok... I take some of that back. Some of the items are not spacing between > > > elements, but rather sizes of elements. In my defense, those came in later > as > > > changes were being made to the ViewsDelegate. In those cases, maybe a > > > SizingMetric enum should be added and a different "int > > > GetSizingMetric(SizingMetric metric)" function is added? > > > > I don't think we should have two different enums for "spacing constants" > versus > > "sizing constants", because they're both really the size of something (size of > a > > button, size of a gap), and because it just doesn't seem like a distinction > that > > makes readers' lives clearer and easier. Hence the suggestion to just have > one > > for integers, one for insets, etc. > > Would that not then require the individual names to return to indicating whether > something is "spacing" or "size"? Yes, which is why the layout delegate names mention _SPACING or _PADDING or _WIDTH or whatever. I'm anticipating your response will be that if we're going to have this in the name anyway, why not just have it in the API name and not the enum value? But I view SPACING and PADDING and MARGIN as distinct, just as I view WIDTH and HEIGHT as distinct, and if we're going to be making those distinctions clear somewhere anyway (as I think we should) we might as well not have a partial distinction in the function name and also a distinction in the enum name. Clearer to place all the distinctions in one naming spot. Or at least, that's how I've been thinking about it so far.
https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.h (right): https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.h:52: // associated text. This includes checkboxes and radio buttons. On 2017/03/01 23:32:23, Peter Kasting wrote: > On 2017/03/01 17:33:27, kylix_rd wrote: > > On 2017/03/01 06:32:36, Peter Kasting wrote: > > > I'm confused. How is this different than RELATED_LABEL_HORIZONTAL_SPACING, > > > covers "an item such as an icon or checkbox and a label related to it"? > > > > > > It seems like we shouldn't have both of these. Unless they really mean > > > completely different things and should be clarified, let's collapse, even if > > > that causes a behavior change. > > > > > > Note: If this stays, alphabetize. > > > > The original constant, private to the label_button.cc module, was kSpacing > with > > a value of 5. RELATED_LABEL_HORIZONTAL_SPACING would eventually resolve to > > kItemLabelSpacing which has a value of 10. I added a new constant since it was > > different from kItemLabelSpacing. > > Right. My hope was to either decide we could change the existing behavior and > unify both as either 5 or 10, or else to understand the necessary difference > well enough to make the names and comments really clear so that it would be > obvious what these would actually be used for. I'm OK with either route, the > first is preferable :) After looking at the use of RELATED_LABEL_HORIZONTAL_SPACING, I think we should: * Change its value to 5 in pre-Harmony * Not add ICON_TEXT_SPACING * Use RELATED_LABEL_HORIZONTAL_SPACING for cases that would have used ICON_TEXT_SPACING * Change website_settings_popup_view.cc:kPermissionImageSpacing to use this constant as well https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate.cc:27: return ChromeViewsDelegate::GetInstance()->GetDefaultSpacingMetric( I wonder if it would make more sense to just leave the direct views::kConstant usage here and avoid introducing GetDefaultSpacingMetric() at all. Yes, it duplicates the constant usages between ViewsDelegate and here. But if the subsequent plan is to hoist LayoutDelegate to the views layer and remove these accessors from ViewsDelegate entirely, then we'll just have to put those constants back in here anyway. https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/reload_button_unittest.cc (right): https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/reload_button_unittest.cc:31: ReloadButton& reload() { return reload_; } Nit: I suggest returning a pointer rather than a non-const ref, partly because non-const refs make me nervous, but partly because half the callers below want to take the address anyway.
On 2017/03/03 00:05:55, Peter Kasting wrote: > https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... > File chrome/browser/ui/views/harmony/layout_delegate.h (right): > > https://codereview.chromium.org/2696263002/diff/200001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/layout_delegate.h:52: // associated text. This > includes checkboxes and radio buttons. > On 2017/03/01 23:32:23, Peter Kasting wrote: > > On 2017/03/01 17:33:27, kylix_rd wrote: > > > On 2017/03/01 06:32:36, Peter Kasting wrote: > > > > I'm confused. How is this different than > RELATED_LABEL_HORIZONTAL_SPACING, > > > > covers "an item such as an icon or checkbox and a label related to it"? > > > > > > > > It seems like we shouldn't have both of these. Unless they really mean > > > > completely different things and should be clarified, let's collapse, even > if > > > > that causes a behavior change. > > > > > > > > Note: If this stays, alphabetize. > > > > > > The original constant, private to the label_button.cc module, was kSpacing > > with > > > a value of 5. RELATED_LABEL_HORIZONTAL_SPACING would eventually resolve to > > > kItemLabelSpacing which has a value of 10. I added a new constant since it > was > > > different from kItemLabelSpacing. > > > > Right. My hope was to either decide we could change the existing behavior and > > unify both as either 5 or 10, or else to understand the necessary difference > > well enough to make the names and comments really clear so that it would be > > obvious what these would actually be used for. I'm OK with either route, the > > first is preferable :) > > After looking at the use of RELATED_LABEL_HORIZONTAL_SPACING, I think we should: > * Change its value to 5 in pre-Harmony > * Not add ICON_TEXT_SPACING > * Use RELATED_LABEL_HORIZONTAL_SPACING for cases that would have used > ICON_TEXT_SPACING > * Change website_settings_popup_view.cc:kPermissionImageSpacing to use this > constant as well Won't the existing uses of the pre-Harmony kItemLabelSpacing constant now be out of spec and thus be seen as a regression? At least until Harmony is switched on permanently. I don't quite have enough historical context to make that call, so I'll rely on your perspective. > https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... > File chrome/browser/ui/views/harmony/layout_delegate.cc (right): > > https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/layout_delegate.cc:27: return > ChromeViewsDelegate::GetInstance()->GetDefaultSpacingMetric( > I wonder if it would make more sense to just leave the direct views::kConstant > usage here and avoid introducing GetDefaultSpacingMetric() at all. > > Yes, it duplicates the constant usages between ViewsDelegate and here. But if > the subsequent plan is to hoist LayoutDelegate to the views layer and remove > these accessors from ViewsDelegate entirely, then we'll just have to put those > constants back in here anyway. This is a bit counter to what sky@ has been pushing for; to stop adding new publicly visible kConstants and favor using the ViewsLayout (or eventually LayoutDelegate). Otherwise there is the risk of others using the new constants before we get the refactoring work done. This would unnecessarily increase the effort. > https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/reload_button_unittest.cc (right): > > https://codereview.chromium.org/2696263002/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/reload_button_unittest.cc:31: ReloadButton& > reload() { return reload_; } > Nit: I suggest returning a pointer rather than a non-const ref, partly because > non-const refs make me nervous, but partly because half the callers below want > to take the address anyway. OK.
The CQ bit was checked by kylixrd@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.
On 2017/03/03 14:43:04, kylix_rd wrote: > On 2017/03/03 00:05:55, Peter Kasting wrote: > > After looking at the use of RELATED_LABEL_HORIZONTAL_SPACING, I think we > should: > > * Change its value to 5 in pre-Harmony > > * Not add ICON_TEXT_SPACING > > * Use RELATED_LABEL_HORIZONTAL_SPACING for cases that would have used > > ICON_TEXT_SPACING > > * Change website_settings_popup_view.cc:kPermissionImageSpacing to use this > > constant as well > > Won't the existing uses of the pre-Harmony kItemLabelSpacing constant now be out > of spec and thus be seen as a regression? You assume we had a spec when building those :). In general, I'm not scared of behavior changes for pre-Harmony as long as they don't feel too broken. I didn't think about changing the pre-Harmony value by changing kItemLabelSpacing from 10 to 5 -- I was thinking about changing RELATED_LABEL_HORIZONTAL_SPACING from 10 to 5 and only looked at uses of that. But you're right that the saner way would be to change the underlying constant, which would have larger effects. Looking at kItemLabelSpacing makes things a bit more awkward. Basically, we currently have two cases, "related" and "unrelated" labels, and we're using this for both. (Here I use "related" for something like a checkbox to its label, and "unrelated" for something like a label that asks "Do you want to proceed" to an adjacent button that says "OK".) Arguably, these should all get turned into the kRelatedControlHorizontalSpacing and kUnrelatedControlHorizontalSpacing values, which are 8 and 12. Then in Harmony, we should probably make these 8 and 16, in keeping with your and my feeling that checkbox-to-label spacing should be 8 DIP rather than 16. (These values are currently 16 and 16. Changing this will probably require converting a lot of the existing "related control" uses in the codebase to "unrelated control", in keeping with my declaration a couple weeks back about interpreting "related" narrowly.) This would let us kill both kItemLabelSpacing and the icon-to-text value. The question is, even if all this makes sense, what's the right path to implement it, especially so as not to block this CL forever? I would say, make this particular usage use RELATED_CONTROL_HORIZONTAL_SPACING, which will end up being less of a change in pre-Harmony than making it use the "related label" value, and, for now, no change in Harmony. Then I'll file bugs for the other stuff. > > I wonder if it would make more sense to just leave the direct views::kConstant > > usage here and avoid introducing GetDefaultSpacingMetric() at all. > > > > Yes, it duplicates the constant usages between ViewsDelegate and here. But if > > the subsequent plan is to hoist LayoutDelegate to the views layer and remove > > these accessors from ViewsDelegate entirely, then we'll just have to put those > > constants back in here anyway. > > This is a bit counter to what sky@ has been pushing for; to stop adding new > publicly visible kConstants and favor using the ViewsLayout (or eventually > LayoutDelegate). Otherwise there is the risk of others using the new constants > before we get the refactoring work done. This would unnecessarily increase the > effort. There's two parts here: adding new kConstants, and adding new uses of the existing kConstants. I'm a fan of not adding new kConstants. Which, at this point, this CL avoids doing. My proposal would introduce a duplicate set of uses of the existing kConstants compared to the plumbing that routes LayoutDelegate back to ViewsDelegate. It is hard for me to know how to best balance the tradeoffs here, but that seems unlikely to cause major problems, and likely to be less work to clean up in subsequent patches to remove all this stuff from ViewsDelegate entirely (and ultimately remove the kConstants entirely and define them directly in layout_delegate.cc). However, maybe I'm missing something.
On 2017/03/04 00:24:40, Peter Kasting wrote: > I would say, make this particular usage use RELATED_CONTROL_HORIZONTAL_SPACING, > which will end up being less of a change in pre-Harmony than making it use the > "related label" value, and, for now, no change in Harmony. Then I'll file bugs > for the other stuff. https://bugs.chromium.org/p/chromium/issues/detail?id=698446
On 2017/03/04 00:24:40, Peter Kasting wrote: > On 2017/03/03 14:43:04, kylix_rd wrote: > > On 2017/03/03 00:05:55, Peter Kasting wrote: > > > After looking at the use of RELATED_LABEL_HORIZONTAL_SPACING, I think we > > should: > > > * Change its value to 5 in pre-Harmony > > > * Not add ICON_TEXT_SPACING > > > * Use RELATED_LABEL_HORIZONTAL_SPACING for cases that would have used > > > ICON_TEXT_SPACING > > > * Change website_settings_popup_view.cc:kPermissionImageSpacing to use this > > > constant as well > > > > Won't the existing uses of the pre-Harmony kItemLabelSpacing constant now be > out > > of spec and thus be seen as a regression? > > You assume we had a spec when building those :). In general, I'm not scared of > behavior changes for pre-Harmony as long as they don't feel too broken. > > I didn't think about changing the pre-Harmony value by changing > kItemLabelSpacing from 10 to 5 -- I was thinking about changing > RELATED_LABEL_HORIZONTAL_SPACING from 10 to 5 and only looked at uses of that. > But you're right that the saner way would be to change the underlying constant, > which would have larger effects. > > Looking at kItemLabelSpacing makes things a bit more awkward. Basically, we > currently have two cases, "related" and "unrelated" labels, and we're using this > for both. (Here I use "related" for something like a checkbox to its label, and > "unrelated" for something like a label that asks "Do you want to proceed" to an > adjacent button that says "OK".) > > Arguably, these should all get turned into the kRelatedControlHorizontalSpacing > and kUnrelatedControlHorizontalSpacing values, which are 8 and 12. Then in > Harmony, we should probably make these 8 and 16, in keeping with your and my > feeling that checkbox-to-label spacing should be 8 DIP rather than 16. (These > values are currently 16 and 16. Changing this will probably require converting > a lot of the existing "related control" uses in the codebase to "unrelated > control", in keeping with my declaration a couple weeks back about interpreting > "related" narrowly.) > > This would let us kill both kItemLabelSpacing and the icon-to-text value. > > The question is, even if all this makes sense, what's the right path to > implement it, especially so as not to block this CL forever? I would rather get this CL committed soon since it will be a blueprint for subsequent changes. There have already been a couple of ViewsDelegate changes since I've started which I've had to merge by removing the added method and incorporating it into the GetDistanceMetric() method and enum. After that, I'll then be able to focus more on these inconsistencies. > I would say, make this particular usage use RELATED_CONTROL_HORIZONTAL_SPACING, > which will end up being less of a change in pre-Harmony than making it use the > "related label" value, and, for now, no change in Harmony. Then I'll file bugs > for the other stuff. This I can do now.
The CQ bit was checked by kylixrd@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...
This patch should incorporate the initial changes suggested by pkasting@. The rest will be a separate CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ui/views LGTM
LGTM https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate_win.cc (right): https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate_win.cc:112: : in_autohide_edges_callback_(false), weak_factory_(this) { For this function, it looks like the only reason it needs to be in this file is because of the member references to win-only members. But as a consequence, we need to duplicate the body of the function and the file-local |views_delegate| object into this file. I suggest removing this, and instead in the header that declares these members, using default member initializers to avoid the need to give a member initializer list in any C++ file. https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate_win.cc:117: ChromeViewsDelegate::~ChromeViewsDelegate() { For these two functions, the implementations look the same as in the cross-platform class. Why do they have to be pulled out to the _win file? https://codereview.chromium.org/2696263002/diff/280001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/280001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.cc:219: margins_ = views_delegate Nit: Just assume |views_delegate| is non-null (2 places) https://codereview.chromium.org/2696263002/diff/280001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/280001/ui/views/views_delegat... ui/views/views_delegate.h:69: BUBBLE_DIALOG, Nit: Alphabetize? (both enums)
The CQ bit was checked by kylixrd@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/2696263002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate_win.cc (right): https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate_win.cc:112: : in_autohide_edges_callback_(false), weak_factory_(this) { On 2017/03/09 05:52:11, Peter Kasting wrote: > For this function, it looks like the only reason it needs to be in this file is > because of the member references to win-only members. But as a consequence, we > need to duplicate the body of the function and the file-local |views_delegate| > object into this file. I suggest removing this, and instead in the header that > declares these members, using default member initializers to avoid the need to > give a member initializer list in any C++ file. Can weak_factory_ be initialized with "this" using the default member initializers? Because of the difference constructor and that it references views_delegate, the other methods which also reference it must also be moved since views_delegate has no public linkage. I added the GetInstance() static and the views_delegate reference in order to avoid performing a very unsafe static_cast<> of the ViewsDelegate::GetInstance() return value. Having separate ChromeViewsDelegate::GetInstance() ensures that it will actually return an instance of ChromeViewsDelegate or nullptr if it weren't constructed. https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate_win.cc:117: ChromeViewsDelegate::~ChromeViewsDelegate() { On 2017/03/09 05:52:11, Peter Kasting wrote: > For these two functions, the implementations look the same as in the > cross-platform class. Why do they have to be pulled out to the _win file? Since views_delegate is also defined in chrome_views_delegate.cc, it doesn't have public linkage (to ensure it is only obtained via the GetInstance() method). The constructor for the Windows version of the delegate must also initialize some extra fields and initialize the views_delegate global. It looks like the chrome_views_delegate_xxx.cc modules were added in order to minimize the #ifdef's within chrome_views_delegate.cc. https://codereview.chromium.org/2696263002/diff/280001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.cc (right): https://codereview.chromium.org/2696263002/diff/280001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.cc:219: margins_ = views_delegate On 2017/03/09 05:52:11, Peter Kasting wrote: > Nit: Just assume |views_delegate| is non-null (2 places) Done. https://codereview.chromium.org/2696263002/diff/280001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2696263002/diff/280001/ui/views/views_delegat... ui/views/views_delegate.h:69: BUBBLE_DIALOG, On 2017/03/09 05:52:11, Peter Kasting wrote: > Nit: Alphabetize? (both enums) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate_win.cc (right): https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate_win.cc:112: : in_autohide_edges_callback_(false), weak_factory_(this) { On 2017/03/09 15:02:31, kylix_rd wrote: > On 2017/03/09 05:52:11, Peter Kasting wrote: > > For this function, it looks like the only reason it needs to be in this file > is > > because of the member references to win-only members. But as a consequence, > we > > need to duplicate the body of the function and the file-local |views_delegate| > > object into this file. I suggest removing this, and instead in the header > that > > declares these members, using default member initializers to avoid the need to > > give a member initializer list in any C++ file. > > Can weak_factory_ be initialized with "this" using the default member > initializers? It should be able to, AFAIK. Such initializers are basically expressions that run in the context of the constructor when it executes, unless they're overridden by an explicit member initializer. > Because of the difference constructor and that it references > views_delegate, the other methods which also reference it must also be moved > since views_delegate has no public linkage. Ah, right. That answers my other question. Yes, I'd try to do "= this" in the header, to avoid the need for all this. If that doesn't work, I'd use an #ifdef in the cross-platform .cc, since on balance that seems better than having to hoist so much other duplicated stuff to this file.
The CQ bit was checked by kylixrd@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...
This should address the last concern. Went with a small #if..#else block in chrome_views_delegate.cc and removed the constructor definition from chrome_views_delegate_win.cc. https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate_win.cc (right): https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate_win.cc:112: : in_autohide_edges_callback_(false), weak_factory_(this) { On 2017/03/09 18:26:48, Peter Kasting wrote: > On 2017/03/09 15:02:31, kylix_rd wrote: > > On 2017/03/09 05:52:11, Peter Kasting wrote: > > > For this function, it looks like the only reason it needs to be in this file > > is > > > because of the member references to win-only members. But as a consequence, > > we > > > need to duplicate the body of the function and the file-local > |views_delegate| > > > object into this file. I suggest removing this, and instead in the header > > that > > > declares these members, using default member initializers to avoid the need > to > > > give a member initializer list in any C++ file. > > > > Can weak_factory_ be initialized with "this" using the default member > > initializers? > > It should be able to, AFAIK. Such initializers are basically expressions that > run in the context of the constructor when it executes, unless they're > overridden by an explicit member initializer. > > > Because of the difference constructor and that it references > > views_delegate, the other methods which also reference it must also be moved > > since views_delegate has no public linkage. > > Ah, right. That answers my other question. > > Yes, I'd try to do "= this" in the header, to avoid the need for all this. If > that doesn't work, I'd use an #ifdef in the cross-platform .cc, since on balance > that seems better than having to hoist so much other duplicated stuff to this > file. weak_factory_ takes "this" as a constructor parameter, so a simple assignment won't work (tried it). It will work with in_autohide_edges_callback_, but since weak_factory_ must still be handled in the constructor initializer list, it might as well remain there as well. I've moved the duplicated code back to chrome_views_delegate.cc and merely added an #if..#else around the different constructor definition and initializers.
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 kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2696263002/#ps320001 (title: "Used ifdef instead of duplicated code for delegate initialization")
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": 320001, "attempt_start_ts": 1489163432493290,
"parent_rev": "282f59a44a9c93d74226da46cbf7536fbd840346", "commit_rev":
"c34b8926fd843300658dcdaec4c69c6967cf86c6"}
Message was sent while issue was closed.
Description was changed from ========== Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton. ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric. BUG=652024,687349 ========== to ========== Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton. ViewsDelegate should have the layout related functions coalesced into a handful of functions which take an enum element for each metric. BUG=652024,687349 Review-Url: https://codereview.chromium.org/2696263002 Cr-Commit-Position: refs/heads/master@{#456078} Committed: https://chromium.googlesource.com/chromium/src/+/c34b8926fd843300658dcdaec4c6... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001) as https://chromium.googlesource.com/chromium/src/+/c34b8926fd843300658dcdaec4c6...
Message was sent while issue was closed.
https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/chrome_views_delegate_win.cc (right): https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/chrome_views_delegate_win.cc:112: : in_autohide_edges_callback_(false), weak_factory_(this) { On 2017/03/10 15:10:40, kylix_rd wrote: > On 2017/03/09 18:26:48, Peter Kasting wrote: > > On 2017/03/09 15:02:31, kylix_rd wrote: > > > On 2017/03/09 05:52:11, Peter Kasting wrote: > > > > For this function, it looks like the only reason it needs to be in this > file > > > is > > > > because of the member references to win-only members. But as a > consequence, > > > we > > > > need to duplicate the body of the function and the file-local > > |views_delegate| > > > > object into this file. I suggest removing this, and instead in the header > > > that > > > > declares these members, using default member initializers to avoid the > need > > to > > > > give a member initializer list in any C++ file. > > > > > > Can weak_factory_ be initialized with "this" using the default member > > > initializers? > > > > It should be able to, AFAIK. Such initializers are basically expressions that > > run in the context of the constructor when it executes, unless they're > > overridden by an explicit member initializer. > > > > > Because of the difference constructor and that it references > > > views_delegate, the other methods which also reference it must also be moved > > > since views_delegate has no public linkage. > > > > Ah, right. That answers my other question. > > > > Yes, I'd try to do "= this" in the header, to avoid the need for all this. If > > that doesn't work, I'd use an #ifdef in the cross-platform .cc, since on > balance > > that seems better than having to hoist so much other duplicated stuff to this > > file. > > weak_factory_ takes "this" as a constructor parameter, so a simple assignment > won't work (tried it). Oh, due to the constructor being marked explicit? If so it might have worked to do "= base::WeakPtrFactory<ChromeViewsDelegate>(this)". But that's kind of ugly. Up to you if you think it would be a win and want to try it in a future CL :)
Message was sent while issue was closed.
On 2017/03/10 20:44:54, Peter Kasting wrote: > https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... > File chrome/browser/ui/views/chrome_views_delegate_win.cc (right): > > https://codereview.chromium.org/2696263002/diff/280001/chrome/browser/ui/view... > chrome/browser/ui/views/chrome_views_delegate_win.cc:112: : > in_autohide_edges_callback_(false), weak_factory_(this) { > On 2017/03/10 15:10:40, kylix_rd wrote: > > On 2017/03/09 18:26:48, Peter Kasting wrote: > > > On 2017/03/09 15:02:31, kylix_rd wrote: > > > > On 2017/03/09 05:52:11, Peter Kasting wrote: > > > > > For this function, it looks like the only reason it needs to be in this > > file > > > > is > > > > > because of the member references to win-only members. But as a > > consequence, > > > > we > > > > > need to duplicate the body of the function and the file-local > > > |views_delegate| > > > > > object into this file. I suggest removing this, and instead in the > header > > > > that > > > > > declares these members, using default member initializers to avoid the > > need > > > to > > > > > give a member initializer list in any C++ file. > > > > > > > > Can weak_factory_ be initialized with "this" using the default member > > > > initializers? > > > > > > It should be able to, AFAIK. Such initializers are basically expressions > that > > > run in the context of the constructor when it executes, unless they're > > > overridden by an explicit member initializer. > > > > > > > Because of the difference constructor and that it references > > > > views_delegate, the other methods which also reference it must also be > moved > > > > since views_delegate has no public linkage. > > > > > > Ah, right. That answers my other question. > > > > > > Yes, I'd try to do "= this" in the header, to avoid the need for all this. > If > > > that doesn't work, I'd use an #ifdef in the cross-platform .cc, since on > > balance > > > that seems better than having to hoist so much other duplicated stuff to > this > > > file. > > > > weak_factory_ takes "this" as a constructor parameter, so a simple assignment > > won't work (tried it). > > Oh, due to the constructor being marked explicit? > > If so it might have worked to do "= > base::WeakPtrFactory<ChromeViewsDelegate>(this)". But that's kind of ugly. Up > to you if you think it would be a win and want to try it in a future CL :) Yes. Really ugly :). I might try it merely for my own edification. Not a fan of the #if either, but I find that a little more palatable. |
