Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889483002:
[ios clean] Creates ToolsMenu model. (Closed)
|
Created:
3 years, 7 months ago by sczs Modified:
3 years, 7 months ago CC:
chromium-reviews, marq+scrutinize_chromium.org, lpromero+watch_chromium.org, ios-reviews+clean_chromium.org, ios-reviews_chromium.org, rohitrao (ping after 24h) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios clean] Creates ToolsMenu model.
Creates a new Model since re-using the old one would require too much work and
not a lot of gain. (Will expand this in the Design Doc, but Marq agreed with me that it
wasn't worth it).
This follows the pattern of the old Model but simplifies it since we won't have as many
different cases as before.
Most items are hidden, and some have been left visible just to show how it works.
TabSwitcher Menu Screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U
Web Menu Screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk
BUG=682880
Review-Url: https://codereview.chromium.org/2889483002
Cr-Commit-Position: refs/heads/master@{#474131}
Committed: https://chromium.googlesource.com/chromium/src/+/87a692a678babe78f8b2ddde3cf6bcc276aea8d4
Patch Set 1 #
Total comments: 17
Patch Set 2 : CL Feedback #Patch Set 3 : Rebase #Patch Set 4 : Move TODO comment. #
Messages
Total messages: 30 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [ios clean] Creates ToolsMenu model. BUG=682880 ========== to ========== [ios clean] Creates ToolsMenu model. TabSwitcher Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, rohitrao@google.com
Description was changed from ========== [ios clean] Creates ToolsMenu model. TabSwitcher Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ========== to ========== [ios clean] Creates ToolsMenu model. Creates a new Model since re-using the old one would require too much work and not a lot of gain. (Will expand this on the Desig Doc, but Marq agreed that it wasn't worth it). This follows the pattern of the old Model but simplifies it since we won't have as many different cases as before. Most items are hidden, and some have been left visible just to show how it works. TabSwitcher Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ==========
sczs@chromium.org changed reviewers: + rohitrao@chromium.org - rohitrao@google.com
sczs@chromium.org changed reviewers: - rohitrao@chromium.org
Hey Ed, could you PTAL. CCing Rohit in case he wants to give a look or has any tips! https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/BUILD.gn (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/BUILD.gn:21: "//ios/chrome/browser/ui/tools_menu", I will cleanup this on a following CL. The constants file needs to be moved to shared https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (left): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:12: #import "ios/clean/chrome/browser/ui/commands/find_in_page_visibility_commands.h" Just some cleanup https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:38: TEST_F(ToolsMediatorTest, TestHideOverFlowControls) { I plan on adding more asserts to these tests over time. Specially once we are not hiding items because they are not implemented yet.
On 2017/05/16 20:31:04, sczs wrote: > Hey Ed, could you PTAL. > > CCing Rohit in case he wants to give a look or has any tips! > > https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... > File ios/clean/chrome/browser/ui/tools/BUILD.gn (right): > > https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tools/BUILD.gn:21: > "//ios/chrome/browser/ui/tools_menu", > I will cleanup this on a following CL. The constants file needs to be moved to > shared > > https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... > File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (left): > > https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:12: #import > "ios/clean/chrome/browser/ui/commands/find_in_page_visibility_commands.h" > Just some cleanup > > https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... > File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): > > https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:38: > TEST_F(ToolsMediatorTest, TestHideOverFlowControls) { > I plan on adding more asserts to these tests over time. Specially once we are > not hiding items because they are not implemented yet. Friendly ping :o)
sczs@chromium.org changed reviewers: + lpromero@chromium.org
Adding lpromero@ since he's back :)
Description was changed from ========== [ios clean] Creates ToolsMenu model. Creates a new Model since re-using the old one would require too much work and not a lot of gain. (Will expand this on the Desig Doc, but Marq agreed that it wasn't worth it). This follows the pattern of the old Model but simplifies it since we won't have as many different cases as before. Most items are hidden, and some have been left visible just to show how it works. TabSwitcher Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ========== to ========== [ios clean] Creates ToolsMenu model. Creates a new Model since re-using the old one would require too much work and not a lot of gain. (Will expand this in the Design Doc, but Marq agreed with me that it wasn't worth it). This follows the pattern of the old Model but simplifies it since we won't have as many different cases as before. Most items are hidden, and some have been left visible just to show how it works. TabSwitcher Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ==========
lgtm with comments. Please change the description to expand to "screenshot". https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/BUILD.gn (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/BUILD.gn:21: "//ios/chrome/browser/ui/tools_menu", On 2017/05/16 20:31:03, sczs wrote: > I will cleanup this on a following CL. The constants file needs to be moved to > shared Can you either add a comment in the code, or link here in the review to a WIP CL? https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.mm (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:25: // Checks if a specific Menu Item should be visibile for the current *visible https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:75: // We first check if the item should be visible on the current Incognito Nit: avoid "we". Just remove it, here. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:94: // We then check if the item should be visibile for the current Toolbar state. Idem. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:105: return NO; Should you remove the default case, so that this breaks compilation when a new case is added? https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_menu_model.h (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_menu_model.h:11: const int kToolsMenuNumberOfItems = 16; Can't this be possibly computed from |itemsModelList|? Also, how come it is not just declared here and defined in the implementation? https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_menu_model.h:32: const NSInteger kVisibleAlways = 0; Why isn't this an NS_OPTIONS too?
Description was changed from ========== [ios clean] Creates ToolsMenu model. Creates a new Model since re-using the old one would require too much work and not a lot of gain. (Will expand this in the Design Doc, but Marq agreed with me that it wasn't worth it). This follows the pattern of the old Model but simplifies it since we won't have as many different cases as before. Most items are hidden, and some have been left visible just to show how it works. TabSwitcher Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu SS: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ========== to ========== [ios clean] Creates ToolsMenu model. Creates a new Model since re-using the old one would require too much work and not a lot of gain. (Will expand this in the Design Doc, but Marq agreed with me that it wasn't worth it). This follows the pattern of the old Model but simplifies it since we won't have as many different cases as before. Most items are hidden, and some have been left visible just to show how it works. TabSwitcher Menu Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ==========
Thanks for the review Louis. Addressed all your comments. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/BUILD.gn (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/BUILD.gn:21: "//ios/chrome/browser/ui/tools_menu", On 2017/05/23 09:00:43, lpromero wrote: > On 2017/05/16 20:31:03, sczs wrote: > > I will cleanup this on a following CL. The constants file needs to be moved to > > shared > > Can you either add a comment in the code, or link here in the review to a WIP > CL? Done. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.mm (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:25: // Checks if a specific Menu Item should be visibile for the current On 2017/05/23 09:00:43, lpromero wrote: > *visible Done. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:75: // We first check if the item should be visible on the current Incognito On 2017/05/23 09:00:43, lpromero wrote: > Nit: avoid "we". Just remove it, here. Done. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:94: // We then check if the item should be visibile for the current Toolbar state. On 2017/05/23 09:00:43, lpromero wrote: > Idem. Done. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:105: return NO; On 2017/05/23 09:00:44, lpromero wrote: > Should you remove the default case, so that this breaks compilation when a new > case is added? I was trying to do this, but without the default case I still need to return something outside the switch, which defeats the purpose. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_menu_model.h (right): https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_menu_model.h:11: const int kToolsMenuNumberOfItems = 16; On 2017/05/23 09:00:44, lpromero wrote: > Can't this be possibly computed from |itemsModelList|? Also, how come it is not > just declared here and defined in the implementation? This constant is only used here in the header so we can initialize the Array. I'm using the sizeOfArray macro on the mediator in order to get the Array size. I tried hiding this on the implementation file but I can't use the macro. https://codereview.chromium.org/2889483002/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_menu_model.h:32: const NSInteger kVisibleAlways = 0; On 2017/05/23 09:00:44, lpromero wrote: > Why isn't this an NS_OPTIONS too? You're right got too stuck on the previous Model and didn't changed this. Done.
The CQ bit was checked by sczs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2889483002/#ps40001 (title: "CL Feedback")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sczs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2889483002/#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
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2889483002/#ps80001 (title: "Move TODO comment.")
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": 80001, "attempt_start_ts": 1495575088424010,
"parent_rev": "1cf16502586c8e7c6ba9e9293bda196c04ac4b0e", "commit_rev":
"87a692a678babe78f8b2ddde3cf6bcc276aea8d4"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Creates ToolsMenu model. Creates a new Model since re-using the old one would require too much work and not a lot of gain. (Will expand this in the Design Doc, but Marq agreed with me that it wasn't worth it). This follows the pattern of the old Model but simplifies it since we won't have as many different cases as before. Most items are hidden, and some have been left visible just to show how it works. TabSwitcher Menu Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 ========== to ========== [ios clean] Creates ToolsMenu model. Creates a new Model since re-using the old one would require too much work and not a lot of gain. (Will expand this in the Design Doc, but Marq agreed with me that it wasn't worth it). This follows the pattern of the old Model but simplifies it since we won't have as many different cases as before. Most items are hidden, and some have been left visible just to show how it works. TabSwitcher Menu Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgRjJvNnJJYkhKN1U Web Menu Screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgSENLR1J2T3RxRlk BUG=682880 Review-Url: https://codereview.chromium.org/2889483002 Cr-Commit-Position: refs/heads/master@{#474131} Committed: https://chromium.googlesource.com/chromium/src/+/87a692a678babe78f8b2ddde3cf6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/87a692a678babe78f8b2ddde3cf6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
