Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817203003:
[ios clean] Functionality for menu overflow navigation buttons (Closed)
Description[ios clean] Functionality for menu overflow navigation buttons
Adds functionality to stop and reload buttons in the ToolsMenu overflow stack.
Some refactoring.
BUG=682880
Review-Url: https://codereview.chromium.org/2817203003
Cr-Commit-Position: refs/heads/master@{#466384}
Committed: https://chromium.googlesource.com/chromium/src/+/fc73346d1a81f7c76c4fbb567439082f6e00e8c3
Patch Set 1 #Patch Set 2 : Refactor #
Total comments: 7
Patch Set 3 : CL Feedback #Patch Set 4 : Rebase #
Total comments: 1
Messages
Total messages: 28 (18 generated)
sczs@chromium.org changed reviewers: + edchin@chromium.org, lpromero@chromium.org, marq@chromium.org
PTAL
Description was changed from ========== [ios clean] Functionality for menu overflow navigation buttons BUG=682880 ========== to ========== [ios clean] Functionality for menu overflow navigation buttons Adds functionality to stop and reload buttons in the ToolsMenu overflow stack. Some refactoring. BUG=682880 ==========
lgtm https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:108: addTarget:nil This uses nil-targeting but not the others. Consider cleaning the discrepancy. https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:137: [self.dispatcher showFindInPage]; Should this also close the tools menu if opened?
https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:137: [self.dispatcher showFindInPage]; On 2017/04/18 09:34:11, lpromero wrote: > Should this also close the tools menu if opened? It does already; all of the regular menu buttons have -closeToolMenu as their first action. https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:144: [self closeToolsMenu]; Please follow the pattern of the other menu controls and have the buttons send -closeToolsMenu as a separate action (added before the specific button actions).
I see. Thanks! https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:137: [self.dispatcher showFindInPage]; On 2017/04/18 12:11:23, marq wrote: > On 2017/04/18 09:34:11, lpromero wrote: > > Should this also close the tools menu if opened? > > It does already; all of the regular menu buttons have -closeToolMenu as their > first action. Acknowledged.
PTAL Setting the dispatcher as the action target as Louis suggested in another CL. https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:108: addTarget:nil On 2017/04/18 09:34:11, lpromero wrote: > This uses nil-targeting but not the others. Consider cleaning the discrepancy. Good catch. Done. https://codereview.chromium.org/2817203003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:144: [self closeToolsMenu]; On 2017/04/18 12:11:23, marq wrote: > Please follow the pattern of the other menu controls and have the buttons send > -closeToolsMenu as a separate action (added before the specific button actions). 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: 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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
Ping marq@ after rebase.
lgtm https://codereview.chromium.org/2817203003/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): https://codereview.chromium.org/2817203003/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:113: if ([view isKindOfClass:[ToolbarButton class]]) { Technically you don't need this check, you can just use ObjCCast, which will return nil if the object isn't the right class, so the -addTarget call will no-op. But this is fine as-is.
On 2017/04/21 16:30:36, marq wrote: > lgtm > > https://codereview.chromium.org/2817203003/diff/60001/ios/clean/chrome/browse... > File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right): > > https://codereview.chromium.org/2817203003/diff/60001/ios/clean/chrome/browse... > ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:113: if ([view > isKindOfClass:[ToolbarButton class]]) { > Technically you don't need this check, you can just use ObjCCast, which will > return nil if the object isn't the right class, so the -addTarget call will > no-op. > > But this is fine as-is. Thanks for the tip Mark, will keep it in mind.
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/2817203003/#ps60001 (title: "Rebase")
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": 60001, "attempt_start_ts": 1492796637681730,
"parent_rev": "1af8d6abdae50ae8b8df88c7796a9d042461b4d0", "commit_rev":
"fc73346d1a81f7c76c4fbb567439082f6e00e8c3"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] Functionality for menu overflow navigation buttons Adds functionality to stop and reload buttons in the ToolsMenu overflow stack. Some refactoring. BUG=682880 ========== to ========== [ios clean] Functionality for menu overflow navigation buttons Adds functionality to stop and reload buttons in the ToolsMenu overflow stack. Some refactoring. BUG=682880 Review-Url: https://codereview.chromium.org/2817203003 Cr-Commit-Position: refs/heads/master@{#466384} Committed: https://chromium.googlesource.com/chromium/src/+/fc73346d1a81f7c76c4fbb567439... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/fc73346d1a81f7c76c4fbb567439... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
