Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810603002:
[ios clean] Adds ToolsMenu to TabGrid (Closed)
Description[ios clean] Adds ToolsMenu to TabGrid
The button with the three dots will now show the ToolsMenu instead of Settings.
This is the first step in order to change the content depending on the context. For that
reason we can see the navigation buttons in the screenshot, but didn't make
those changes in this CL.
FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the
"Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL,
not sure yet what broke it but I will fix it and create a test for that.
Screenshot:
https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing
BUG=682880
Review-Url: https://codereview.chromium.org/2810603002
Cr-Commit-Position: refs/heads/master@{#464535}
Committed: https://chromium.googlesource.com/chromium/src/+/1348142cf8663a4e48249e9c0497c14ed91aa0b5
Patch Set 1 #
Total comments: 30
Patch Set 2 : CL Feedback #
Total comments: 12
Patch Set 3 : Clean up tasks. #Patch Set 4 : Rebase #
Total comments: 6
Messages
Total messages: 28 (13 generated)
Description was changed from ========== [ios] Adds ToolsMenu to TabGrid BUG=682880 ========== to ========== [ios] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't want to make those changes on the same CL as well. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, lpromero@chromium.org, marq@chromium.org
Description was changed from ========== [ios] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't want to make those changes on the same CL as well. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ========== to ========== [ios] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't want to make those changes on the same CL as well. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ==========
Description was changed from ========== [ios] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't want to make those changes on the same CL as well. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ========== to ========== [ios] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't make those changes in this CL. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ==========
Description was changed from ========== [ios] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't make those changes in this CL. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ========== to ========== [ios clean] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't make those changes in this CL. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ==========
PTAL, specially edchin@ since this CL includes a lot of your files.
https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:77: [dispatcher startDispatchingToTarget:self nit: Feel free to extract to method to mirror the good code structure you started. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:129: [self.browser->dispatcher() Extract to method (like you did for registering) to mirror code structure. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:161: - (void)registerForToolsMenuCommands { Move this to a private section at the end. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:182: #pragma mark - SettingsCommands Should these disappear from this file due to this change? https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:19: @property(nonatomic, strong) UIStackView* toolbarContent; Since this is added as a subview, it can be weak. This is preferred by marq. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:41: self.toolbarContent.translatesAutoresizingMaskIntoConstraints = NO; Use direct access in partially constructed states. Not sure if this is required in our style guide. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:60: [self.toolbarContent.arrangedSubviews lastObject]); Feel free to move the stackview construction and buttons to this file so that you have access to the button directly. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h:25: id<SettingsCommands, TabGridCommands, ToolsMenuCommands> Probably requires an update to showcase. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:113: if (!key) { This logic requires some comments. It's not intuitive. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:114: if ([self.toolbar conformsToProtocol:@protocol(ZoomTransitionDelegate)]) { Is it necessary to check conformance and reinterpret when we know for sure that TabGridToolbar conforms? https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:129: - (CGRect)frameForMenuPresentation:(UIPresentationController*)presentation { If this code exists in multiple places, maybe we should have a utils method for this code. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:136: menuRect.origin = CGPointMake(50, 50); Magic numbers? https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/ui_button+cr_tab_grid.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/ui_button+cr_tab_grid.mm:7: #import "ios/clean/chrome/browser/ui/actions/settings_actions.h" Replace with tools_menu_actions.
https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:41: self.toolbarContent.translatesAutoresizingMaskIntoConstraints = NO; On 2017/04/10 05:50:57, edchin wrote: > Use direct access in partially constructed states. Not sure if this is required > in our style guide. It's not explicitly defined, but it's a widely-accepted best practice. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:114: if ([self.toolbar conformsToProtocol:@protocol(ZoomTransitionDelegate)]) { On 2017/04/10 05:50:57, edchin wrote: > Is it necessary to check conformance and reinterpret when we know for sure that > TabGridToolbar conforms? It shouldn't be.
PTAL https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:77: [dispatcher startDispatchingToTarget:self On 2017/04/10 05:50:56, edchin wrote: > nit: Feel free to extract to method to mirror the good code structure you > started. I extracted to method because we need to register twice. I think this might be pre-mature refactoring/optimization for these other cases that are only called once. On the other hand, I can completely understand if you want to keep this tidy, so let me know and I'll gladly create methods for these other registrations. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:129: [self.browser->dispatcher() On 2017/04/10 05:50:57, edchin wrote: > Extract to method (like you did for registering) to mirror code structure. Same as above. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:161: - (void)registerForToolsMenuCommands { On 2017/04/10 05:50:57, edchin wrote: > Move this to a private section at the end. Done. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:182: #pragma mark - SettingsCommands On 2017/04/10 05:50:57, edchin wrote: > Should these disappear from this file due to this change? For now TabGrid will still be in charge of displaying Settings, at least for the case when it displays its ToolsMenu. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:19: @property(nonatomic, strong) UIStackView* toolbarContent; On 2017/04/10 05:50:57, edchin wrote: > Since this is added as a subview, it can be weak. This is preferred by marq. Done. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:41: self.toolbarContent.translatesAutoresizingMaskIntoConstraints = NO; On 2017/04/10 11:36:39, marq wrote: > On 2017/04/10 05:50:57, edchin wrote: > > Use direct access in partially constructed states. Not sure if this is > required > > in our style guide. > > It's not explicitly defined, but it's a widely-accepted best practice. Done. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:60: [self.toolbarContent.arrangedSubviews lastObject]); On 2017/04/10 05:50:57, edchin wrote: > Feel free to move the stackview construction and buttons to this file so that > you have access to the button directly. Acknowledged. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h:25: id<SettingsCommands, TabGridCommands, ToolsMenuCommands> On 2017/04/10 05:50:57, edchin wrote: > Probably requires an update to showcase. Good catch. Done. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:113: if (!key) { On 2017/04/10 05:50:57, edchin wrote: > This logic requires some comments. It's not intuitive. Done. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:114: if ([self.toolbar conformsToProtocol:@protocol(ZoomTransitionDelegate)]) { On 2017/04/10 11:36:39, marq wrote: > On 2017/04/10 05:50:57, edchin wrote: > > Is it necessary to check conformance and reinterpret when we know for sure > that > > TabGridToolbar conforms? > > It shouldn't be. Removed this check. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:129: - (CGRect)frameForMenuPresentation:(UIPresentationController*)presentation { On 2017/04/10 05:50:57, edchin wrote: > If this code exists in multiple places, maybe we should have a utils method for > this code. Definitely, I think thats out of the scope of this CL, but we should discuss on how to create utils files/methods for different purposes. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:136: menuRect.origin = CGPointMake(50, 50); On 2017/04/10 05:50:57, edchin wrote: > Magic numbers? Agree, this is Mark original code. Once we move to a utils file we can get rid of them. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/ui_button+cr_tab_grid.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/ui_button+cr_tab_grid.mm:7: #import "ios/clean/chrome/browser/ui/actions/settings_actions.h" On 2017/04/10 05:50:57, edchin wrote: > Replace with tools_menu_actions. There's no ToolsMenuActions, instead of creating another temporal file I'm grouping both Settings related UI Responder Chain actions on a single file marked as HACK
https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:77: [dispatcher startDispatchingToTarget:self On 2017/04/11 18:47:59, sczs wrote: > On 2017/04/10 05:50:56, edchin wrote: > > nit: Feel free to extract to method to mirror the good code structure you > > started. > > I extracted to method because we need to register twice. I think this might be > pre-mature refactoring/optimization for these other cases that are only called > once. On the other hand, I can completely understand if you want to keep this > tidy, so let me know and I'll gladly create methods for these other > registrations. I understand that you extracted the other methods for DRY. Code parallel structure is just as an important reason to extract to method. Please extract this as well. https://codereview.chromium.org/2810603002/diff/1/ios/clean/chrome/browser/ui... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:129: [self.browser->dispatcher() On 2017/04/11 18:47:59, sczs wrote: > On 2017/04/10 05:50:57, edchin wrote: > > Extract to method (like you did for registering) to mirror code structure. > > Same as above. Register/deregister should have a mirrored structure. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:33: ToolsMenuCommands, Pls alphabetize. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:101: DCHECK([childCoordinator isKindOfClass:[ToolsCoordinator class]] || Pls alphabetize. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm (right): https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:36: _toolbarContent = [UIStackView cr_tabGridToolbarStackView]; The ivar is a weak reference so this will not retain. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:59: [self.toolbarContent.arrangedSubviews lastObject]); arrangedSubviews is already an array of UIViews. You don't need to cast to UIButton since all you need is the bounds from the view. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:61: return rect; nit: Use single line instead of two. return [view convertRect:menuButton.bounds fromView:menuButton]; https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h (right): https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h:22: MenuPresentationDelegate> Pls alphabetize.
Addressed the comments and did the refactor. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:33: ToolsMenuCommands, On 2017/04/12 09:13:37, edchin wrote: > Pls alphabetize. Done. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:101: DCHECK([childCoordinator isKindOfClass:[ToolsCoordinator class]] || On 2017/04/12 09:13:37, edchin wrote: > Pls alphabetize. Done. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm (right): https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:36: _toolbarContent = [UIStackView cr_tabGridToolbarStackView]; On 2017/04/12 09:13:37, edchin wrote: > The ivar is a weak reference so this will not retain. Done. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:59: [self.toolbarContent.arrangedSubviews lastObject]); On 2017/04/12 09:13:37, edchin wrote: > arrangedSubviews is already an array of UIViews. You don't need to cast to > UIButton since all you need is the bounds from the view. Done. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.mm:61: return rect; On 2017/04/12 09:13:37, edchin wrote: > nit: Use single line instead of two. > return [view convertRect:menuButton.bounds fromView:menuButton]; Done. https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h (right): https://codereview.chromium.org/2810603002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h:22: MenuPresentationDelegate> On 2017/04/12 09:13:37, edchin wrote: > Pls alphabetize. Done.
lgtm
The CQ bit was checked by edchin@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sczs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org Link to the patchset: https://codereview.chromium.org/2810603002/#ps60001 (title: "Rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sczs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry, late to the party. No problem ack some changes, but if you find some useful, feel free to follow up in a new CL. LGTM https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:124: [self deRegisterFromToolsMenuCommands]; s/deRe/dere? s/From/For? https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:225: forSelector:@selector(showToolsMenu)]; Nit: you might be able to use the protocol based API: startDispatchingToTarget:forProtocol: https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm (right): https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:113: // If key is nil we return a CGRect based on the toolbar position, if not it Avoid we :) https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:117: return [self.toolbar rectForZoomWithKey:key inView:view]; Should you formalize as WithKey:nil? https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:127: - (CGRect)frameForMenuPresentation:(UIPresentationController*)presentation { Is this the method you wanted to put in a UI util? What about creating a DefaultMenuPresentationDelegate or MenuPresenter object that implements a method like -frameForMenuPresentation:withBounds:. This view controller could then asks its MenuPresenter object to do the computation. https://codereview.chromium.org/2810603002/diff/60001/ios/showcase/tab_grid/s... File ios/showcase/tab_grid/sc_tab_grid_coordinator.mm (right): https://codereview.chromium.org/2810603002/diff/60001/ios/showcase/tab_grid/s... ios/showcase/tab_grid/sc_tab_grid_coordinator.mm:31: @protocol(SettingsCommands), @protocol(TabGridCommands), Are the settings commands still needed here?
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492115434524080,
"parent_rev": "6d16ea5a2687375c5a28f31e8a119f754ce84f3c", "commit_rev":
"1348142cf8663a4e48249e9c0497c14ed91aa0b5"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't make those changes in this CL. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 ========== to ========== [ios clean] Adds ToolsMenu to TabGrid The button with the three dots will now show the ToolsMenu instead of Settings. This is the first step in order to change the content depending on the context. For that reason we can see the navigation buttons in the screenshot, but didn't make those changes in this CL. FYI: The settings window will work from the "TabGrid" ToolsMenu, but not the "Toolbar" ToolsMenu. This is currently happening and is not being caused by this CL, not sure yet what broke it but I will fix it and create a test for that. Screenshot: https://drive.google.com/file/d/0Byo6-Nuda2jgTURCSDFoUUNJVEU/view?usp=sharing BUG=682880 Review-Url: https://codereview.chromium.org/2810603002 Cr-Commit-Position: refs/heads/master@{#464535} Committed: https://chromium.googlesource.com/chromium/src/+/1348142cf8663a4e48249e9c0497... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1348142cf8663a4e48249e9c0497...
Message was sent while issue was closed.
On 2017/04/13 20:43:57, lpromero wrote: > Sorry, late to the party. No problem ack some changes, but if you find some > useful, feel free to follow up in a new CL. > > LGTM > > https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... > File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right): > > https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:124: [self > deRegisterFromToolsMenuCommands]; > s/deRe/dere? > s/From/For? > > https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:225: > forSelector:@selector(showToolsMenu)]; > Nit: you might be able to use the protocol based API: > startDispatchingToTarget:forProtocol: > > https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... > File ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm (right): > > https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:113: // If key > is nil we return a CGRect based on the toolbar position, if not it > Avoid we :) > > https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:117: return > [self.toolbar rectForZoomWithKey:key inView:view]; > Should you formalize as WithKey:nil? > > https://codereview.chromium.org/2810603002/diff/60001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:127: - > (CGRect)frameForMenuPresentation:(UIPresentationController*)presentation { > Is this the method you wanted to put in a UI util? > > What about creating a DefaultMenuPresentationDelegate or MenuPresenter object > that implements a method like -frameForMenuPresentation:withBounds:. This view > controller could then asks its MenuPresenter object to do the computation. > > https://codereview.chromium.org/2810603002/diff/60001/ios/showcase/tab_grid/s... > File ios/showcase/tab_grid/sc_tab_grid_coordinator.mm (right): > > https://codereview.chromium.org/2810603002/diff/60001/ios/showcase/tab_grid/s... > ios/showcase/tab_grid/sc_tab_grid_coordinator.mm:31: > @protocol(SettingsCommands), @protocol(TabGridCommands), > Are the settings commands still needed here? Thanks Louis! Will definitely address these on a followup/related CL. |
