Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579613002:
[Media Router Action] Hide action back in the overflow menu after popping it out (Closed)
Description[Media Router Action] Hide action back in the overflow menu after popping it out
In macOS, when the user right clicks on the media router action while it's in
the toolbar overflow menu, it gets popped out onto the toolbar to show its
context menu. This change makes the action go back into the overflow menu once
the context menu is closed, which will be consistent with the behavior of
extension actions.
BUG=670585
Committed: https://crrev.com/05d325bc6eacf649f14b1fddce78e1268c8b6e89
Cr-Commit-Position: refs/heads/master@{#441253}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change unit test to interactive UI test #
Total comments: 2
Patch Set 3 : Address Devlin's comment #
Total comments: 9
Patch Set 4 : Address Mike and Jennifer's comments #Patch Set 5 : Modify a comment #
Total comments: 2
Patch Set 6 : Address Robert's comment #
Messages
Total messages: 35 (19 generated)
Description was changed from ========== Overflow bug BUG= ========== to ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed. BUG=670585 ==========
Description was changed from ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed. BUG=670585 ========== to ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed. BUG=670585 ==========
Description was changed from ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed. BUG=670585 ========== to ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed, which will be consistent with the behavior of extension actions. BUG=670585 ==========
Description was changed from ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed, which will be consistent with the behavior of extension actions. BUG=670585 ========== to ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed, which will be consistent with the behavior of extension actions. BUG=670585 ==========
Patchset #1 (id:1) has been deleted
takumif@chromium.org changed reviewers: + apacible@chromium.org, msw@chromium.org, rdevlin.cronin@chromium.org
Please take a look, thank you!
https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:381: EXPECT_CALL(mock_actions_bar, popped_out_action()) I'm not a big fan of using mocks like this, because it really just tests the code, rather than the behavior (and the code isn't particularly interesting). In the end, it becomes a change detection test, which is more of a negative. If you want to add a test for this, we have some interactive ui tests that test similar behavior for extensions in chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm - maybe we could use that type of pattern? https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right): https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.h:200: virtual void UndoPopOut(); I'd like to avoid this if possible; see also comment in the unittest.
Patchset #2 (id:40001) has been deleted
On 2016/12/15 22:25:26, Devlin wrote: > https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... > File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): > > https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_unittest.cc:381: > EXPECT_CALL(mock_actions_bar, popped_out_action()) > I'm not a big fan of using mocks like this, because it really just tests the > code, rather than the behavior (and the code isn't particularly interesting). > In the end, it becomes a change detection test, which is more of a negative. > > If you want to add a test for this, we have some interactive ui tests that test > similar behavior for extensions in > chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm - > maybe we could use that type of pattern? > > https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... > File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right): > > https://codereview.chromium.org/2579613002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/toolbar_actions_bar.h:200: virtual void UndoPopOut(); > I'd like to avoid this if possible; see also comment in the unittest. BrowserActionButtonUiTest.ContextMenusOnMainAndOverflow overlaps a lot with how I wanted to test, so I reused the common parts. Please let me know if this approach seems alright, or if there's a better way to do it. Thanks!
much better! lgtm https://codereview.chromium.org/2579613002/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2579613002/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:142: if (toolbar_actions_bar_ && nit: can toolbar_actions_bar_ ever be null here? (It can for ExtensionActionViewController, but I don't think it can for this.)
https://codereview.chromium.org/2579613002/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2579613002/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:142: if (toolbar_actions_bar_ && On 2016/12/20 16:40:41, Devlin wrote: > nit: can toolbar_actions_bar_ ever be null here? (It can for > ExtensionActionViewController, but I don't think it can for this.) Right, it wouldn't be null here. We do DCHECK for it in the ctor. Removed.
lgtm with minor test file comments. https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:97: // Simulates a clicks on the action button in the overflow menu, and runs nit: "a click" https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:275: // passing in runLoop.QuitClosure() is not sufficient here.) Consider clarifying this comment, since the code does indeed use runLoop.QuitClosure() below, so I'm not sure what this means about it being insufficient. That said, and you simply moved this code, and I don't know Cocoa well (I hope you're not relying on a competent review from me in this file).
lgtm https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:287: // action button, not the overflow's. Since Cocoa doesn't support running nit: suggested phrasing - "The menu opened on the main bar's action button rather than the overflow's since Cocoa does not support running a menu within a menu."
Patchset #4 (id:100001) has been deleted
takumif@chromium.org changed reviewers: + rsesek@chromium.org
rsesek@, please take a look at browser_action_button_interactive_uitest.mm. rdcronin@, please take a look at the comment below regarding runLoop.QuitClosure(). Thanks! https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:97: // Simulates a clicks on the action button in the overflow menu, and runs On 2016/12/20 20:26:14, msw wrote: > nit: "a click" Fixed. https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:275: // passing in runLoop.QuitClosure() is not sufficient here.) On 2016/12/20 20:26:14, msw wrote: > Consider clarifying this comment, since the code does indeed use > runLoop.QuitClosure() below, so I'm not sure what this means about it being > insufficient. That said, and you simply moved this code, and I don't know Cocoa > well (I hope you're not relying on a competent review from me in this file). I think this is referring to the fact that we need to use MenuWatcher. Devlin, do you have suggestions for changing the wording? https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:287: // action button, not the overflow's. Since Cocoa doesn't support running On 2016/12/20 20:36:08, apacible wrote: > nit: suggested phrasing - "The menu opened on the main bar's action button > rather than the overflow's since Cocoa does not support running a menu within a > menu." Done.
https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:275: // passing in runLoop.QuitClosure() is not sufficient here.) On 2016/12/28 20:48:29, takumif wrote: > On 2016/12/20 20:26:14, msw wrote: > > Consider clarifying this comment, since the code does indeed use > > runLoop.QuitClosure() below, so I'm not sure what this means about it being > > insufficient. That said, and you simply moved this code, and I don't know > Cocoa > > well (I hope you're not relying on a competent review from me in this file). > > I think this is referring to the fact that we need to use MenuWatcher. Devlin, > do you have suggestions for changing the wording? IIRC, Cocoa menus are such that we couldn't do: SendMouseEvents(quit_closure); runLoop.Run(); // Test menu sync here And this comment is trying to explain that. Maybe rephrase to: // Due to the blocking nature of Cocoa menus, we can't test the menu // synchronously here by quitting the run loop when it opens, and // instead need to test through a callback. or something? Unfortunately I don't have enough knowledge of the inner workings of Cocoa menus to offer much more - I just remember it was a right pain to find a way of doing this, and most things I tried failed.
still lgtm https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:275: // passing in runLoop.QuitClosure() is not sufficient here.) On 2016/12/28 20:58:55, Devlin wrote: > On 2016/12/28 20:48:29, takumif wrote: > > On 2016/12/20 20:26:14, msw wrote: > > > Consider clarifying this comment, since the code does indeed use > > > runLoop.QuitClosure() below, so I'm not sure what this means about it being > > > insufficient. That said, and you simply moved this code, and I don't know > > Cocoa > > > well (I hope you're not relying on a competent review from me in this file). > > > > I think this is referring to the fact that we need to use MenuWatcher. Devlin, > > do you have suggestions for changing the wording? > > IIRC, Cocoa menus are such that we couldn't do: > SendMouseEvents(quit_closure); > runLoop.Run(); > // Test menu sync here > > And this comment is trying to explain that. Maybe rephrase to: > // Due to the blocking nature of Cocoa menus, we can't test the menu > // synchronously here by quitting the run loop when it opens, and > // instead need to test through a callback. > or something? Unfortunately I don't have enough knowledge of the inner workings > of Cocoa menus to offer much more - I just remember it was a right pain to find > a way of doing this, and most things I tried failed. Thanks for clarifying! The comment is fine as-is, but yours is even better.
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:275: // passing in runLoop.QuitClosure() is not sufficient here.) On 2016/12/28 20:58:55, Devlin wrote: > On 2016/12/28 20:48:29, takumif wrote: > > On 2016/12/20 20:26:14, msw wrote: > > > Consider clarifying this comment, since the code does indeed use > > > runLoop.QuitClosure() below, so I'm not sure what this means about it being > > > insufficient. That said, and you simply moved this code, and I don't know > > Cocoa > > > well (I hope you're not relying on a competent review from me in this file). > > > > I think this is referring to the fact that we need to use MenuWatcher. Devlin, > > do you have suggestions for changing the wording? > > IIRC, Cocoa menus are such that we couldn't do: > SendMouseEvents(quit_closure); > runLoop.Run(); > // Test menu sync here > > And this comment is trying to explain that. Maybe rephrase to: > // Due to the blocking nature of Cocoa menus, we can't test the menu > // synchronously here by quitting the run loop when it opens, and > // instead need to test through a callback. > or something? Unfortunately I don't have enough knowledge of the inner workings > of Cocoa menus to offer much more - I just remember it was a right pain to find > a way of doing this, and most things I tried failed. Thank you for the explanation! Updating the comment.
Comment LGTM. The deal with Cocoa menus is that when they are opened, a nested run loop is started by AppKit in a limited mode. This is what cause the "blocking nature" and so any tests of the menu need to be done in a callback that runs in that nested loop. https://codereview.chromium.org/2579613002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:400: ToolbarActionsBar* actions_bar = nit: while under_scores are correct for this function, the variables above are camelCase, and this file is generally camelCase (even though the correct style is under_scores), so let's be consistent here.
The CQ bit was checked by takumif@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...
Thanks for reviewing and explaining why the blocking happens! https://codereview.chromium.org/2579613002/diff/160001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm (right): https://codereview.chromium.org/2579613002/diff/160001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm:400: ToolbarActionsBar* actions_bar = On 2017/01/03 21:42:22, Robert Sesek wrote: > nit: while under_scores are correct for this function, the variables above are > camelCase, and this file is generally camelCase (even though the correct style > is under_scores), so let's be consistent here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, apacible@chromium.org, msw@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2579613002/#ps180001 (title: "Address Robert's 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": 180001, "attempt_start_ts": 1483486216893230,
"parent_rev": "c76f4e5686a5ea89a7695acb353e494f18df9505", "commit_rev":
"fc28977c4bf5bdea3118a32908cba57ec6a6966a"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed, which will be consistent with the behavior of extension actions. BUG=670585 ========== to ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed, which will be consistent with the behavior of extension actions. BUG=670585 Review-Url: https://codereview.chromium.org/2579613002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed, which will be consistent with the behavior of extension actions. BUG=670585 Review-Url: https://codereview.chromium.org/2579613002 ========== to ========== [Media Router Action] Hide action back in the overflow menu after popping it out In macOS, when the user right clicks on the media router action while it's in the toolbar overflow menu, it gets popped out onto the toolbar to show its context menu. This change makes the action go back into the overflow menu once the context menu is closed, which will be consistent with the behavior of extension actions. BUG=670585 Committed: https://crrev.com/05d325bc6eacf649f14b1fddce78e1268c8b6e89 Cr-Commit-Position: refs/heads/master@{#441253} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/05d325bc6eacf649f14b1fddce78e1268c8b6e89 Cr-Commit-Position: refs/heads/master@{#441253} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
