Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693043002:
[ios clean] Add overflow buttons for ToolsMenu. (Closed)
Description[ios clean] Add overflow buttons for ToolsMenu.
-ToolsMenu adds a StackView subclass (MenuOverflowControlsStack) as a first
element when in compact widths.
-ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad
multitasking,etc.) this mimics current behavior.
-MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this
is not necessarily how it needs to be.
-MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of
adding/removing the Stack, and not the contents.
Screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgQmFzYXZsNkd2ZlU
BUG=682880
Review-Url: https://codereview.chromium.org/2693043002
Cr-Commit-Position: refs/heads/master@{#451065}
Committed: https://chromium.googlesource.com/chromium/src/+/7f21fa5131ea7281d0b990b461eb244574c3c9d4
Patch Set 1 #
Total comments: 17
Patch Set 2 : CL Feedback #
Total comments: 1
Patch Set 3 : Use RTL for EdgeInssets #
Messages
Total messages: 22 (14 generated)
Description was changed from ========== WIP Tools Menu Overflow Controls BUG=682880 ========== to ========== [ios clean] Add overflow buttons for ToolsMenu. BUG=682880 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [ios clean] Add overflow buttons for ToolsMenu. BUG=682880 ========== to ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. BUG=682880 ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, lpromero@chromium.org, marq@chromium.org, rohitrao@chromium.org
Description was changed from ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. BUG=682880 ========== to ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. BUG=682880 ==========
Patchset #1 (id:40001) has been deleted
PTAL I'd like to get your thoughts on the overall idea, and if we should re-use ToolbarButtons with a different factory in subsequent CL's. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/BUILD.gn (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/BUILD.gn:41: source_set("toolbar_components") { This might or might not be end up being used by ToolsMenu once we're "done", but I think its still worth having good source_set modularity overall. Specially since we are building this with the idea of replacing VC in the near future.
Description was changed from ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. BUG=682880 ========== to ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ BUG=682880 ==========
Description was changed from ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcUsxU09OQnRWNTQ BUG=682880 ========== to ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgQmFzYXZsNkd2ZlU BUG=682880 ==========
Description was changed from ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgQmFzYXZsNkd2ZlU BUG=682880 ========== to ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass (MenuOverflowControlsStack) as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgQmFzYXZsNkd2ZlU BUG=682880 ==========
https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/BUILD.gn (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/BUILD.gn:41: source_set("toolbar_components") { The line between toolbar_ui and toolbar_components is not clear to me. For example, a button is UI. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:88: [self closeToolsMenu:nil]; Pls comment that this closes the menu when the device is rotated, or the app is split on tablet, and ensures that you don't get an inconsistent state when device has both compact and regular widths. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h:14: @interface MenuOverflowControlsStackView : UIStackView I would rather avoid inheritance in this case. There is nothing special about MenuOverflowControlsStackView that warrants the use of inheritance. Instead, you can use a category that constructs a custom stackView. UIStackView +(instancetype)cr_menuOverflowControlsStackView. In this factory method, you can construct the buttons and also set their actions. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:126: NSMutableArray* constraintsArray = [NSMutableArray new]; Use alloc/init instead to be more consistent with our code. And generics too. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:153: [NSLayoutConstraint activateConstraints:constraintsArray]; How did the menu work previously without this constraintsArray?
https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/BUILD.gn (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/BUILD.gn:41: source_set("toolbar_components") { On 2017/02/15 04:14:50, edchin wrote: > The line between toolbar_ui and toolbar_components is not clear to me. For > example, a button is UI. I think the idea is that _components might be used by other _ui targets. Another approach would be to move all of the button-specific stuff into something like ui/buttons and have both toolbar and tools_menu depend on it. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:88: [self closeToolsMenu:nil]; I'd like to not "fake" calls to target/action methods for cases like this; this should just call [self.toolbarCommandHandler closeToolsMenu]; https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h:14: @interface MenuOverflowControlsStackView : UIStackView On 2017/02/15 04:14:50, edchin wrote: > I would rather avoid inheritance in this case. > There is nothing special about MenuOverflowControlsStackView > that warrants the use of inheritance. > Instead, you can use a category that constructs > a custom stackView. > UIStackView +(instancetype)cr_menuOverflowControlsStackView. > In this factory method, you can construct the buttons > and also set their actions. +1, this is the pattern we've been using so far. Even if we change our minds later, it's easier if we're consistent. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:126: NSMutableArray* constraintsArray = [NSMutableArray new]; On 2017/02/15 04:14:50, edchin wrote: > Use alloc/init instead to be more consistent with our code. And generics too. Usage of +new is explicitly disallowed by the Objective-C style guide: https://google.github.io/styleguide/objcguide.xml#Avoid_+new Add a MutableConstraints typedef in ui/ui_types.h to match the existing immutable Constraints type. But: While activating all of the constraints at once in a single -activateConstraints call is a bit of an optimization, it's not really worth over-complicating code for. So it's fine to call -activateConstraints twice, once in the if() and once outside.
https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/BUILD.gn (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/BUILD.gn:41: source_set("toolbar_components") { On 2017/02/15 15:06:16, marq wrote: > On 2017/02/15 04:14:50, edchin wrote: > > The line between toolbar_ui and toolbar_components is not clear to me. For > > example, a button is UI. > > I think the idea is that _components might be used by other _ui targets. > > Another approach would be to move all of the button-specific stuff into > something like ui/buttons and have both toolbar and tools_menu depend on it. Yes, that is the idea. I guess I could rename this to toolbar_components_ui for now, that would make it clear that this is UI ToolbarComponents related. If we end up reusing more buttons later on we could move this to ui/buttons WDYT? https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:88: [self closeToolsMenu:nil]; On 2017/02/15 15:06:16, marq wrote: > I'd like to not "fake" calls to target/action methods for cases like this; this > should just call [self.toolbarCommandHandler closeToolsMenu]; Done. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h:14: @interface MenuOverflowControlsStackView : UIStackView On 2017/02/15 15:06:16, marq wrote: > On 2017/02/15 04:14:50, edchin wrote: > > I would rather avoid inheritance in this case. > > There is nothing special about MenuOverflowControlsStackView > > that warrants the use of inheritance. > > Instead, you can use a category that constructs > > a custom stackView. > > UIStackView +(instancetype)cr_menuOverflowControlsStackView. > > In this factory method, you can construct the buttons > > and also set their actions. > > +1, this is the pattern we've been using so far. Even if we change our minds > later, it's easier if we're consistent. The Buttons visibility needs to be set by the ToolsMenuVCn (Stop/Reload, Starred/Unstarred), that's why I need a reference to the buttons, and that was one of the reasons why I was subclassing. If we want to keep using categories we can create associated objects, or we can keep this subclass. I had a quick chat with Ed and he thought that this might enough to keep this as a subclass, WDYT? https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:126: NSMutableArray* constraintsArray = [NSMutableArray new]; On 2017/02/15 15:06:16, marq wrote: > On 2017/02/15 04:14:50, edchin wrote: > > Use alloc/init instead to be more consistent with our code. And generics too. > > Usage of +new is explicitly disallowed by the Objective-C style guide: > https://google.github.io/styleguide/objcguide.xml#Avoid_+new > > Add a MutableConstraints typedef in ui/ui_types.h to match the existing > immutable Constraints type. > > But: While activating all of the constraints at once in a single > -activateConstraints call is a bit of an optimization, it's not really worth > over-complicating code for. So it's fine to call -activateConstraints twice, > once in the if() and once outside. Sounds good, will call -activateConstraints twice now. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:153: [NSLayoutConstraint activateConstraints:constraintsArray]; On 2017/02/15 04:14:50, edchin wrote: > How did the menu work previously without this constraintsArray? It was creating the array in place, but I changed it so it conditionally adds extra constraints for the overflow icons. Now I followed Marks advise and I'm just activating constraints twice.
https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/BUILD.gn (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/BUILD.gn:41: source_set("toolbar_components") { On 2017/02/16 00:31:39, sczs wrote: > On 2017/02/15 15:06:16, marq wrote: > > On 2017/02/15 04:14:50, edchin wrote: > > > The line between toolbar_ui and toolbar_components is not clear to me. For > > > example, a button is UI. > > > > I think the idea is that _components might be used by other _ui targets. > > > > Another approach would be to move all of the button-specific stuff into > > something like ui/buttons and have both toolbar and tools_menu depend on it. > > Yes, that is the idea. > I guess I could rename this to toolbar_components_ui for now, that would make it > clear that this is UI ToolbarComponents related. If we end up reusing more > buttons later on we could move this to ui/buttons WDYT? I'm okay with either, with a slight preference for ui/buttons if we think it will be used in more places than just toolbar. https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h (right): https://codereview.chromium.org/2693043002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_overflow_controls_stackview.h:14: @interface MenuOverflowControlsStackView : UIStackView On 2017/02/16 00:31:39, sczs wrote: > On 2017/02/15 15:06:16, marq wrote: > > On 2017/02/15 04:14:50, edchin wrote: > > > I would rather avoid inheritance in this case. > > > There is nothing special about MenuOverflowControlsStackView > > > that warrants the use of inheritance. > > > Instead, you can use a category that constructs > > > a custom stackView. > > > UIStackView +(instancetype)cr_menuOverflowControlsStackView. > > > In this factory method, you can construct the buttons > > > and also set their actions. > > > > +1, this is the pattern we've been using so far. Even if we change our minds > > later, it's easier if we're consistent. > > The Buttons visibility needs to be set by the ToolsMenuVCn (Stop/Reload, > Starred/Unstarred), that's why I need a reference to the buttons, and that was > one of the reasons why I was subclassing. > If we want to keep using categories we can create associated objects, or we can > keep this subclass. I had a quick chat with Ed and he thought that this might > enough to keep this as a subclass, WDYT? Agreed. After some more discussion, there's additional functionality in the menu overflow controls that requires subclassing.
LGTM % RTL fix. https://codereview.chromium.org/2693043002/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2693043002/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:104: [menuButton setContentEdgeInsets:UIEdgeInsetsMake(0, 10.0f, 0, 0)]; Since you are setting different left and right insets, use UIEdgeInsetsMakeDirected() from rtl_geometry.h so that left/right will be flipped in RTL. Or, if that is specifically not what should happen, add a comment indicating why.
The CQ bit was checked by sczs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org Link to the patchset: https://codereview.chromium.org/2693043002/#ps100001 (title: "Use RTL for EdgeInssets")
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": 100001, "attempt_start_ts": 1487267900743550,
"parent_rev": "4b0ea82167e72d7f04154a60714ff18ba8b65179", "commit_rev":
"7f21fa5131ea7281d0b990b461eb244574c3c9d4"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass (MenuOverflowControlsStack) as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgQmFzYXZsNkd2ZlU BUG=682880 ========== to ========== [ios clean] Add overflow buttons for ToolsMenu. -ToolsMenu adds a StackView subclass (MenuOverflowControlsStack) as a first element when in compact widths. -ToolsMenu is dismissed everytime the ToolbarLayout changes (Rotation, iPad multitasking,etc.) this mimics current behavior. -MenuOverflowControlsStack currently uses ToolbarButtons as a placeholder, but this is not necessarily how it needs to be. -MenuOverflowControlsStack UI is not yet ready, this CL is more about the logic of adding/removing the Stack, and not the contents. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgQmFzYXZsNkd2ZlU BUG=682880 Review-Url: https://codereview.chromium.org/2693043002 Cr-Commit-Position: refs/heads/master@{#451065} Committed: https://chromium.googlesource.com/chromium/src/+/7f21fa5131ea7281d0b990b461eb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7f21fa5131ea7281d0b990b461eb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
