Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2908623004:
[ios clean] Toolbar displays total number of tabs. (Closed)
Description[ios clean] Toolbar displays total number of tabs.
-ToolbarMediator is now a WebStateListObserver and updates the consumer with the
current number of Webstates (tabs), these will be displayed on the TabStrip
Toolbar button.
-Adds text support to ToolbarButton so the text is displayed on top of the image.
-Adds a new consumer protocol method for setting up the number of tabs, which
ToolbarVC conforms to.
-Adds ToolbarMediator unittests related to WebStateList changes.
Screenshot 3 Tabs:
https://drive.google.com/open?id=0Byo6-Nuda2jgSXZuWDFzenU4c0U
Screenshot 15 Tabs:
https://drive.google.com/open?id=0Byo6-Nuda2jgUkpmN0RVQ1h4RWM
BUG=683793
Review-Url: https://codereview.chromium.org/2908623004
Cr-Commit-Position: refs/heads/master@{#476025}
Committed: https://chromium.googlesource.com/chromium/src/+/9a182ab9892e7f016117751a60a02b93e4ddcaec
Patch Set 1 #Patch Set 2 : Minor comment changes. #
Total comments: 31
Patch Set 3 : Rebase #Patch Set 4 : CL Feedback and changes Font to bold. #
Total comments: 10
Patch Set 5 : Improve comments rename variables. #
Total comments: 2
Patch Set 6 : Add comment. #Messages
Total messages: 39 (27 generated)
Description was changed from ========== [ios clean] Toolbar displays total number of tabs. BUG=683793 ========== to ========== [ios clean] Toolbar displays total number of tabs. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, marq@chromium.org, rohitrao@chromium.org
sczs@chromium.org changed required reviewers: + edchin@chromium.org
Description was changed from ========== [ios clean] Toolbar displays total number of tabs. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ========== to ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs) so these can be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ==========
Description was changed from ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs) so these can be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ========== to ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs) so these can be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds a new consumer protocol method for setting up the number of tabs, which ToolbarVC conforms to. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ==========
The CQ bit was checked by sczs@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...
Description was changed from ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs) so these can be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds a new consumer protocol method for setting up the number of tabs, which ToolbarVC conforms to. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ========== to ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs), these will be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds a new consumer protocol method for setting up the number of tabs, which ToolbarVC conforms to. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ==========
PTAL. Even though the CL touches plenty of files, it shouldn't take long to review. Thanks!
lgtm: there's some minor comments, and a call for another CL with related changes. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm:24: [[button titleLabel] setFont:[UIFont systemFontOfSize:11]]; Is there some predefined font size that can be used? https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h:21: - (void)setNumberOfTabs:(int)numberOfTabs; nit: what's more common? numberOfThings or thingsCount? No need to change anything. Just food for thought. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:44: - (void)setWebState:(web::WebState*)webState { Perhaps in another CL, please implement listening to the webStateList and updating on active web state changes. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.h (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.h:23: @property(nonatomic, assign) web::WebState* webState; Perhaps in another CL: This webState should be taken off the public interface since you're now able to listen to active WS updates from the WSL. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm:112: [self.consumer setNumberOfTabs:_webStateList->count()]; If the webStateList is set to nil, wouldn't you want to set the tab count to zero, instead of leaving it at some existing number? Consider DCHECK(webStateList). I don't think we have this pattern yet, but I feel it is better than convoluted logic that checks for nullability.
The CQ bit was checked by edchin@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/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm:24: [[button titleLabel] setFont:[UIFont systemFontOfSize:11]]; On 2017/05/27 16:25:06, edchin wrote: > Is there some predefined font size that can be used? We should either duplicate/refactor the logic in chrome/browser/ui/toolbar_controller.mm for the stackButton, or we should decide how we will handle this with Dynamic Type. I'd prefer that we move away from quick placeholder implementations like this. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm:37: self.imageView.center = center; Looks like this is an exact copy of the ToolbarCenteredButton implementation from ToolbarController (which is fine), with the omission of the AlignRectToPixel() call -- any reason for not including that? https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_constants.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_constants.mm:22: const CGFloat kToolbarButtonTitleHighlightedColor = 0x4285F4; Neither of these are 'colors' in the usual sense -- the first is (I'm guessing) a white value, and the second is an RGB triplet packed into a hex integer. Please be clear and consistent in naming and implementation, and document in the public API how the values of these constants should be interpreted. (I'm fine with using the packed-hex values, but if we do that, then kToolbarButtonTitleNormalColor should be 0x858585). https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h:20: // Updates the toolbar with the current number of total webstates. Nit: Total number of tabs. The consumer doesn't know anything about web states. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:44: - (void)setWebState:(web::WebState*)webState { On 2017/05/27 16:25:06, edchin wrote: > Perhaps in another CL, please implement listening to the webStateList > and updating on active web state changes. We need to discuss what our overall approach is to this. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm:112: [self.consumer setNumberOfTabs:_webStateList->count()]; On 2017/05/27 16:25:06, edchin wrote: > If the webStateList is set to nil, wouldn't you want to set the tab count to > zero, instead of leaving it at some existing number? Consider > DCHECK(webStateList). I don't think we have this pattern yet, but I feel it is > better than convoluted logic that checks for nullability. Yes, I think it's correct to declare that setting a nullptr WSL here is a programming error, and enforce that with a DCHECK at the start of the method. (Consider: if the WSL is empty, what toolbar is this mediator talking to?) https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:47: // Explicitly dissconnect the mediator so there won't be any WebStateList typo: disconnect newline before this comment? https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:49: ~ToolbarMediatorTest() override { [mediator_ disconnect]; } I think we only use one-liner method implementations in trivial getters/setters defined in headers. Prefer to have line breaks and indentation for all methods inside implementation files. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:199: InsertWebState(3); Mild nit: if for some reason kNumberOfWebStates is changed to be < 3, this will DCHECK. Maybe just use 0? https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:171: setTitleColor:UIColorFromRGB(kToolbarButtonTitleHighlightedColor, 1.0) I think you can omit the alpha (1.0) parameter if it's the default (which is 1.0). https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:336: setTitle:[NSString stringWithFormat:@"%d", numberOfTabs] Duplicate or refactor the logic from [ToolbarController setTabCount:] to include font sizing, and handling of large tab counts.
Description was changed from ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs), these will be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds a new consumer protocol method for setting up the number of tabs, which ToolbarVC conforms to. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgNGdEanp6MndlWnc BUG=683793 ========== to ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs), these will be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds a new consumer protocol method for setting up the number of tabs, which ToolbarVC conforms to. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot 3 Tabs: https://drive.google.com/open?id=0Byo6-Nuda2jgSXZuWDFzenU4c0U Screenshot 15 Tabs: https://drive.google.com/open?id=0Byo6-Nuda2jgUkpmN0RVQ1h4RWM BUG=683793 ==========
Thank for all the feedback. I've addressed it all, PTAL. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm:24: [[button titleLabel] setFont:[UIFont systemFontOfSize:11]]; On 2017/05/29 11:04:36, marq (ping after 24h) wrote: > On 2017/05/27 16:25:06, edchin wrote: > > Is there some predefined font size that can be used? > > We should either duplicate/refactor the logic in > chrome/browser/ui/toolbar_controller.mm for the stackButton, or we should decide > how we will handle this with Dynamic Type. I'd prefer that we move away from > quick placeholder implementations like this. Moved this to the VC. Because of the UI constraints I think it will be difficult to handle this with dynamic type. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_button.mm:37: self.imageView.center = center; On 2017/05/29 11:04:36, marq (ping after 24h) wrote: > Looks like this is an exact copy of the ToolbarCenteredButton implementation > from ToolbarController (which is fine), with the omission of the > AlignRectToPixel() call -- any reason for not including that? Great catch. I missed it somehow. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_constants.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_constants.mm:22: const CGFloat kToolbarButtonTitleHighlightedColor = 0x4285F4; On 2017/05/29 11:04:36, marq (ping after 24h) wrote: > Neither of these are 'colors' in the usual sense -- the first is (I'm guessing) > a white value, and the second is an RGB triplet packed into a hex integer. > Please be clear and consistent in naming and implementation, and document in the > public API how the values of these constants should be interpreted. > > (I'm fine with using the packed-hex values, but if we do that, then > kToolbarButtonTitleNormalColor should be 0x858585). Sounds good. If we want to have consistency and not that many constants I think using the Hex values is the best choice. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h:20: // Updates the toolbar with the current number of total webstates. On 2017/05/29 11:04:36, marq (ping after 24h) wrote: > Nit: Total number of tabs. The consumer doesn't know anything about web states. Done. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_consumer.h:21: - (void)setNumberOfTabs:(int)numberOfTabs; On 2017/05/27 16:25:06, edchin wrote: > nit: what's more common? numberOfThings or thingsCount? No need to change > anything. Just food for thought. You're right, tabCount is much better! Changed it :) https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:44: - (void)setWebState:(web::WebState*)webState { On 2017/05/29 11:04:36, marq (ping after 24h) wrote: > On 2017/05/27 16:25:06, edchin wrote: > > Perhaps in another CL, please implement listening to the webStateList > > and updating on active web state changes. > > We need to discuss what our overall approach is to this. Acknowledged. I agree that this is something that we need to discuss. Sylvain had some good observations that Mark seemed to agree with. So we can see how the de-phantomization goes and we can revisit this. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.h (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.h:23: @property(nonatomic, assign) web::WebState* webState; On 2017/05/27 16:25:06, edchin wrote: > Perhaps in another CL: > This webState should be taken off the public interface > since you're now able to listen to active WS updates > from the WSL. Thanks for pointing this out! Will follow the Phantom browsers discussion and update this if necessary. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator.mm:112: [self.consumer setNumberOfTabs:_webStateList->count()]; On 2017/05/29 11:04:36, marq (ping after 24h) wrote: > On 2017/05/27 16:25:06, edchin wrote: > > If the webStateList is set to nil, wouldn't you want to set the tab count to > > zero, instead of leaving it at some existing number? Consider > > DCHECK(webStateList). I don't think we have this pattern yet, but I feel it is > > better than convoluted logic that checks for nullability. > > Yes, I think it's correct to declare that setting a nullptr WSL here is a > programming error, and enforce that with a DCHECK at the start of the method. > > (Consider: if the WSL is empty, what toolbar is this mediator talking to?) The main problem is that we are explicitly calling self.webStateList = nullptr on disconnect. Without this we get the following observer_list failed check during testing: FATAL:observer_list.h(338)] Check failed: ObserverListBase<ObserverType>::size() == 0U (1 vs. 0) (If I try to set the backing variable to nullptr I still get this same check fail) I think Ed and Rohit got to the bottom of this, and that's why TabGridMediator also checks for a nil webStateList The second issue is that TabCoordinator unit tests are currently not initializing a WebStateList, so they would also trigger a DCHECK(webStateList). We also need to update those. For these reasons I would recommend leaving this as is and we can revisit this issue for both this mediator and TabGridMediator on a separate CL. I will add a TODO for now. If you don't agree with this approach please let me know, I'm open to any ideas :) https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:47: // Explicitly dissconnect the mediator so there won't be any WebStateList On 2017/05/29 11:04:37, marq (ping after 24h) wrote: > typo: disconnect > > newline before this comment? Done. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:49: ~ToolbarMediatorTest() override { [mediator_ disconnect]; } On 2017/05/29 11:04:37, marq (ping after 24h) wrote: > I think we only use one-liner method implementations in trivial getters/setters > defined in headers. Prefer to have line breaks and indentation for all methods > inside implementation files. Done. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:199: InsertWebState(3); On 2017/05/29 11:04:37, marq (ping after 24h) wrote: > Mild nit: if for some reason kNumberOfWebStates is changed to be < 3, this will > DCHECK. Maybe just use 0? Done. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:171: setTitleColor:UIColorFromRGB(kToolbarButtonTitleHighlightedColor, 1.0) On 2017/05/29 11:04:37, marq (ping after 24h) wrote: > I think you can omit the alpha (1.0) parameter if it's the default (which is > 1.0). Done. https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:336: setTitle:[NSString stringWithFormat:@"%d", numberOfTabs] On 2017/05/29 11:04:37, marq (ping after 24h) wrote: > Duplicate or refactor the logic from [ToolbarController setTabCount:] to include > font sizing, and handling of large tab counts. Done.
The CQ bit was checked by sczs@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/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:49: ~ToolbarMediatorTest() override { [mediator_ disconnect]; } On 2017/05/30 00:18:21, sczs wrote: > On 2017/05/29 11:04:37, marq (ping after 24h) wrote: > > I think we only use one-liner method implementations in trivial > getters/setters > > defined in headers. Prefer to have line breaks and indentation for all methods > > inside implementation files. > > Done. Not done? https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:226: // Set the buttons constraints priority to UILayoutPriorityDefaultHigh so Super nit: "button constraint priority" or "button constraints' priority". https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:342: // Update the text shown in the |stackButton_|. Note that the button's title Update the comments and variable names to match the language used in this class. We don't have a "stack view", so referring to a "stack button" is unclear. https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:431: - (UIFont*)fontForSize:(int)size { What's the utility in this method?
Thanks for reviewing. PTAL https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm (right): https://codereview.chromium.org/2908623004/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_mediator_unittest.mm:49: ~ToolbarMediatorTest() override { [mediator_ disconnect]; } On 2017/05/30 09:47:03, marq (ping after 24h) wrote: > On 2017/05/30 00:18:21, sczs wrote: > > On 2017/05/29 11:04:37, marq (ping after 24h) wrote: > > > I think we only use one-liner method implementations in trivial > > getters/setters > > > defined in headers. Prefer to have line breaks and indentation for all > methods > > > inside implementation files. > > > > Done. > > Not done? I was pretty sure I had made this change. Now I realize that git cl format formats it this way. https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:226: // Set the buttons constraints priority to UILayoutPriorityDefaultHigh so On 2017/05/30 09:47:04, marq (ping after 24h) wrote: > Super nit: "button constraint priority" or "button constraints' priority". Done. Thanks for pointing these comments mistakes out! https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:342: // Update the text shown in the |stackButton_|. Note that the button's title On 2017/05/30 09:47:04, marq (ping after 24h) wrote: > Update the comments and variable names to match the language used in this class. > We don't have a "stack view", so referring to a "stack button" is unclear. Done. https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:431: - (UIFont*)fontForSize:(int)size { On 2017/05/30 09:47:04, marq (ping after 24h) wrote: > What's the utility in this method? I guess the only utility is to change the font size, I copied it from toolbar_controller. I'm not liking it either, will remove.
The CQ bit was checked by sczs@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.
LGTM, one additional comment suggestion only! https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:226: // Set the buttons constraints priority to UILayoutPriorityDefaultHigh so On 2017/05/30 18:14:12, sczs wrote: > On 2017/05/30 09:47:04, marq (ping after 24h) wrote: > > Super nit: "button constraint priority" or "button constraints' priority". > > Done. Thanks for pointing these comments mistakes out! No problem; I know I can be picky about language (about English, anyway), but most of the time they really are nitpicks. https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:431: - (UIFont*)fontForSize:(int)size { On 2017/05/30 18:14:12, sczs wrote: > On 2017/05/30 09:47:04, marq (ping after 24h) wrote: > > What's the utility in this method? > > I guess the only utility is to change the font size, I copied it from > toolbar_controller. I'm not liking it either, will remove. Yeah, I think it's alway worth asking if utility methods, *especially* one-liners, are worth the extra cost in readability. Sometimes they very much are, but it's always worth thinking about. https://codereview.chromium.org/2908623004/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:350: tabStripButtonTitle = @":)"; Just for clarity, I'd add a comment here to the effect of: "As an easter egg, show a smiley face instead of the count if the user has more than 99 tabs open".
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, marq@chromium.org Link to the patchset: https://codereview.chromium.org/2908623004/#ps100001 (title: "Add comment.")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496260298674060,
"parent_rev": "d10fc6ad638e7cd58576fd378ee4fcf1481c0ed3", "commit_rev":
"9a182ab9892e7f016117751a60a02b93e4ddcaec"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs), these will be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds a new consumer protocol method for setting up the number of tabs, which ToolbarVC conforms to. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot 3 Tabs: https://drive.google.com/open?id=0Byo6-Nuda2jgSXZuWDFzenU4c0U Screenshot 15 Tabs: https://drive.google.com/open?id=0Byo6-Nuda2jgUkpmN0RVQ1h4RWM BUG=683793 ========== to ========== [ios clean] Toolbar displays total number of tabs. -ToolbarMediator is now a WebStateListObserver and updates the consumer with the current number of Webstates (tabs), these will be displayed on the TabStrip Toolbar button. -Adds text support to ToolbarButton so the text is displayed on top of the image. -Adds a new consumer protocol method for setting up the number of tabs, which ToolbarVC conforms to. -Adds ToolbarMediator unittests related to WebStateList changes. Screenshot 3 Tabs: https://drive.google.com/open?id=0Byo6-Nuda2jgSXZuWDFzenU4c0U Screenshot 15 Tabs: https://drive.google.com/open?id=0Byo6-Nuda2jgUkpmN0RVQ1h4RWM BUG=683793 Review-Url: https://codereview.chromium.org/2908623004 Cr-Commit-Position: refs/heads/master@{#476025} Committed: https://chromium.googlesource.com/chromium/src/+/9a182ab9892e7f016117751a60a0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9a182ab9892e7f016117751a60a0...
Message was sent while issue was closed.
https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:226: // Set the buttons constraints priority to UILayoutPriorityDefaultHigh so On 2017/05/31 09:47:26, marq (ping after 24h) wrote: > On 2017/05/30 18:14:12, sczs wrote: > > On 2017/05/30 09:47:04, marq (ping after 24h) wrote: > > > Super nit: "button constraint priority" or "button constraints' priority". > > > > Done. Thanks for pointing these comments mistakes out! > > No problem; I know I can be picky about language (about English, anyway), but > most of the time they really are nitpicks. Please keep doing that, is the best way to learn :) https://codereview.chromium.org/2908623004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:431: - (UIFont*)fontForSize:(int)size { On 2017/05/31 09:47:26, marq (ping after 24h) wrote: > On 2017/05/30 18:14:12, sczs wrote: > > On 2017/05/30 09:47:04, marq (ping after 24h) wrote: > > > What's the utility in this method? > > > > I guess the only utility is to change the font size, I copied it from > > toolbar_controller. I'm not liking it either, will remove. > > Yeah, I think it's alway worth asking if utility methods, *especially* > one-liners, are worth the extra cost in readability. Sometimes they very much > are, but it's always worth thinking about. Acknowledged. https://codereview.chromium.org/2908623004/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right): https://codereview.chromium.org/2908623004/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:350: tabStripButtonTitle = @":)"; On 2017/05/31 09:47:26, marq (ping after 24h) wrote: > Just for clarity, I'd add a comment here to the effect of: "As an easter egg, > show a smiley face instead of the count if the user has more than 99 tabs open". Done. |
