Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410553002:
Show Media Router toolbar icon ephemerally for MR dialogs (Closed)
|
Created:
4 years, 2 months ago by takumif Modified:
4 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, miu+watch_chromium.org, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow Media Router toolbar icon ephemerally for MR dialogs
This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router:
1. Implement MediaRouterActionController [1]
2. Show ephemeral icon for active local routes and issues [2]
3. Show ephemeral icon for open dialog (this CL)
This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController may show or hide the toolbar action icon depending on that. The icon will be shown when there are open dialogs, local routes or issues, or when the "Always Show Icon" option is checked.
In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController.
[1] https://codereview.chromium.org/2294973002
[2] https://codereview.chromium.org/2332693003
BUG=594577
Committed: https://crrev.com/89fc2a78d69cb432b445da453fd3f6fd02d4ebfe
Cr-Commit-Position: refs/heads/master@{#431296}
Patch Set 1 : Deleted the first patch set on accident, responded to Mark's comments inline below #
Total comments: 18
Patch Set 2 : Address Mark's comments #
Total comments: 6
Patch Set 3 : Address Mark's comments #
Total comments: 51
Patch Set 4 : Rebase #Patch Set 5 : Address Mike and Mark's comments #
Total comments: 2
Patch Set 6 : Address Mike's comments #
Total comments: 1
Patch Set 7 : DISALLOW_COPY_AND_ASSIGN MockMediaRouterActionController #Messages
Total messages: 42 (23 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== . BUG= ========== to ========== Show Media Router toolbar icon ephemerally for MR dialogs This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues [2] 3. Show ephemeral icon for open dialog (this CL) This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController shows or hides the toolbar action icon depending on that. In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController. [1] https://codereview.chromium.org/2294973002 [2] https://codereview.chromium.org/2332693003 BUG=594577 ==========
Description was changed from ========== Show Media Router toolbar icon ephemerally for MR dialogs This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues [2] 3. Show ephemeral icon for open dialog (this CL) This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController shows or hides the toolbar action icon depending on that. In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController. [1] https://codereview.chromium.org/2294973002 [2] https://codereview.chromium.org/2332693003 BUG=594577 ========== to ========== Show Media Router toolbar icon ephemerally for MR dialogs This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues [2] 3. Show ephemeral icon for open dialog (this CL) This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController may show or hide the toolbar action icon depending on that. The icon will be shown when there are open dialogs, local routes or issues, or when the "Always Show Icon" option is checked. In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController. [1] https://codereview.chromium.org/2294973002 [2] https://codereview.chromium.org/2332693003 BUG=594577 ==========
takumif@chromium.org changed reviewers: + mfoltz@google.com, msw@chromium.org
takumif@chromium.org changed reviewers: + mfoltz@chromium.org - mfoltz@google.com
mfoltz@: Please take a look at all the files. msw@: Please take a look at c/b/ui/toolbar/ files. Thank you!
Overall looks good, a few comments/questions. Can you add test cases for media_router_dialog_controller after the design is finalized? https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:49: if (dialog_count > 0u) Or just if (dialog_count) https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:89: return has_local_display_route_ || has_issue_ || dialog_count_ > 0u || Ditto https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:91: return nullptr; FAIL() here, since the only usage assumes that the action is returned? https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:171: action_controller_(GetActionController(web_contents)), Slight preference for keeping a pointer to the UI Service in this class, instead of cherry picking out the action controller. https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:280: if (action_) It seems a little odd that we have to notify two objects about the dialog state. Who owns |action_|? Is it the "view" or the "model" of the media router toolbar action? In either case, it would make a little more sense to me to have only the controller get notified about the dialog, and it takes care of updating the model/view object. https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h (right): https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:81: // It's a nullptr if there is no MediaRouterUIService, such as in unit tests. It should be a nullptr only in unit tests, right? If there's a dependency between these classes and the UI Service, I would prefer using a testing factory to return a mock UI service (that returns e.g. a mock action controller) so we don't have to write null checking code in the implementation only for unit tests. Here's one way to do that: https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile.h?rcl=0...
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
On 2016/10/11 16:38:21, mark a. foltz wrote: > Overall looks good, a few comments/questions. Can you add test cases for > media_router_dialog_controller after the design is finalized? > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.cc:49: if (dialog_count > > 0u) > Or just if (dialog_count) Done. > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.cc:89: return > has_local_display_route_ || has_issue_ || dialog_count_ > 0u || > Ditto Done. > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc > (right): > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:91: return > nullptr; > FAIL() here, since the only usage assumes that the action is returned? FAIL() doesn't work inside a method that doesn't return void. Would NOTREACHED() be appropriate? > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc > (right): > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:171: > action_controller_(GetActionController(web_contents)), > Slight preference for keeping a pointer to the UI Service in this class, instead > of cherry picking out the action controller. Sorry, could you explain why that'd be better? Wouldn't it be fine to keep a pointer to just the action controller as long as we're not using anything else from the UI service? > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:280: > if (action_) > It seems a little odd that we have to notify two objects about the dialog state. > > Who owns |action_|? Is it the "view" or the "model" of the media router toolbar > action? In either case, it would make a little more sense to me to have only > the controller get notified about the dialog, and it takes care of updating the > model/view object. MRAction (per-window) needs to be notified for depressing the icon, and MRActionController (per-profile) needs to be notified for showing/hiding the icon (which instantiates MRAction). Making MRA and MRAC observe MRDCI would mean we can't decide the order of notification, and they would need to watch out for new MRDCI (per-WebContents) getting instantiated. Making MRA observe MRAC would work, but they are currently unaware of each other. > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h > (right): > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:81: > // It's a nullptr if there is no MediaRouterUIService, such as in unit tests. > It should be a nullptr only in unit tests, right? > > If there's a dependency between these classes and the UI Service, I would prefer > using a testing factory to return a mock UI service (that returns e.g. a mock > action controller) so we don't have to write null checking code in the > implementation only for unit tests. Added mock UI service and action controller to test_helper.cc. Added a ctor to MediaRouterTest that takes a boolean flag which when true, leads to instantiation of the mock objects. > > Here's one way to do that: > https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile.h?rcl=0... Thanks for reviewing. I accidentally deleted the first patch, so I've responded to your comments inline. Sorry for the inconvenience.
On 2016/10/12 at 22:55:33, takumif wrote: > On 2016/10/11 16:38:21, mark a. foltz wrote: > > Overall looks good, a few comments/questions. Can you add test cases for > > media_router_dialog_controller after the design is finalized? > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > > File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > > chrome/browser/ui/toolbar/media_router_action_controller.cc:49: if (dialog_count > > > 0u) > > Or just if (dialog_count) > > Done. > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > > chrome/browser/ui/toolbar/media_router_action_controller.cc:89: return > > has_local_display_route_ || has_issue_ || dialog_count_ > 0u || > > Ditto > > Done. > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... > > File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc > > (right): > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:91: return > > nullptr; > > FAIL() here, since the only usage assumes that the action is returned? > > FAIL() doesn't work inside a method that doesn't return void. Would NOTREACHED() be appropriate? SGTM > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc > > (right): > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:171: > > action_controller_(GetActionController(web_contents)), > > Slight preference for keeping a pointer to the UI Service in this class, instead > > of cherry picking out the action controller. > > Sorry, could you explain why that'd be better? Wouldn't it be fine to keep a pointer to just the action controller as long as we're not using anything else from the UI service? Since the only way to get the action controller is through the UI service, right now, the dependency on the UI service is hidden in the implementation. Also, you're relying on the UI service to control the lifetime of the action controller. Taking a UI Service pointer as a member declares two things: 1. The MRDC is a layer that sits on top of the UI Service. 2. The MRDC should not outlive the UI Service. Now, I don't feel super strongly about this, since right now the UI Service is just a holder for the action controller, and the dialog controller is probably only interested in the action controller and doesn't need the pointer. But in general I feel it's better to be explicit about lifetime and layering assumptions w.r.t. dependencies on services. > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:280: > > if (action_) > > It seems a little odd that we have to notify two objects about the dialog state. > > > > Who owns |action_|? Is it the "view" or the "model" of the media router toolbar > > action? In either case, it would make a little more sense to me to have only > > the controller get notified about the dialog, and it takes care of updating the > > model/view object. > > MRAction (per-window) needs to be notified for depressing the icon, and MRActionController (per-profile) needs to be notified for showing/hiding the icon (which instantiates MRAction). > > Making MRA and MRAC observe MRDCI would mean we can't decide the order of notification, and they would need to watch out for new MRDCI (per-WebContents) getting instantiated. > > Making MRA observe MRAC would work, but they are currently unaware of each other. > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h > > (right): > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:81: > > // It's a nullptr if there is no MediaRouterUIService, such as in unit tests. > > It should be a nullptr only in unit tests, right? > > > > If there's a dependency between these classes and the UI Service, I would prefer > > using a testing factory to return a mock UI service (that returns e.g. a mock > > action controller) so we don't have to write null checking code in the > > implementation only for unit tests. > > Added mock UI service and action controller to test_helper.cc. Added a ctor to MediaRouterTest that takes a boolean flag which when true, leads to instantiation of the mock objects. > > > > > Here's one way to do that: > > https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile.h?rcl=0... > > Thanks for reviewing. > I accidentally deleted the first patch, so I've responded to your comments inline. Sorry for the inconvenience.
[Hit reply early, remaining inline comments to come last] On 2016/10/17 at 18:53:05, mark a. foltz wrote: > On 2016/10/12 at 22:55:33, takumif wrote: > > On 2016/10/11 16:38:21, mark a. foltz wrote: > > > Overall looks good, a few comments/questions. Can you add test cases for > > > media_router_dialog_controller after the design is finalized? > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > > > File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > > > chrome/browser/ui/toolbar/media_router_action_controller.cc:49: if (dialog_count > > > > 0u) > > > Or just if (dialog_count) > > > > Done. > > > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/toolb... > > > chrome/browser/ui/toolbar/media_router_action_controller.cc:89: return > > > has_local_display_route_ || has_issue_ || dialog_count_ > 0u || > > > Ditto > > > > Done. > > > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... > > > File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc > > > (right): > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/views... > > > chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:91: return > > > nullptr; > > > FAIL() here, since the only usage assumes that the action is returned? > > > > FAIL() doesn't work inside a method that doesn't return void. Would NOTREACHED() be appropriate? > > SGTM > > > > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > > File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc > > > (right): > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:171: > > > action_controller_(GetActionController(web_contents)), > > > Slight preference for keeping a pointer to the UI Service in this class, instead > > > of cherry picking out the action controller. > > > > Sorry, could you explain why that'd be better? Wouldn't it be fine to keep a pointer to just the action controller as long as we're not using anything else from the UI service? > > Since the only way to get the action controller is through the UI service, right now, the dependency on the UI service is hidden in the implementation. Also, you're relying on the UI service to control the lifetime of the action controller. > > Taking a UI Service pointer as a member declares two things: > 1. The MRDC is a layer that sits on top of the UI Service. > 2. The MRDC should not outlive the UI Service. > > Now, I don't feel super strongly about this, since right now the UI Service is just a holder for the action controller, and the dialog controller is probably only interested in the action controller and doesn't need the pointer. But in general I feel it's better to be explicit about lifetime and layering assumptions w.r.t. dependencies on services. > > > > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:280: > > > if (action_) > > > It seems a little odd that we have to notify two objects about the dialog state. > > > > > > Who owns |action_|? Is it the "view" or the "model" of the media router toolbar > > > action? In either case, it would make a little more sense to me to have only > > > the controller get notified about the dialog, and it takes care of updating the > > > model/view object. > > > > MRAction (per-window) needs to be notified for depressing the icon, and MRActionController (per-profile) needs to be notified for showing/hiding the icon (which instantiates MRAction). > > > > Making MRA and MRAC observe MRDCI would mean we can't decide the order of notification, and they would need to watch out for new MRDCI (per-WebContents) getting instantiated. > > > > Making MRA observe MRAC would work, but they are currently unaware of each other. Okay. They don't really have a normal controller/model relationship it sounds like. It might be good to add a comment explaining that |action_| is instantiated by the action controller but owned by something else (the toolbar actions model?) so they both need to be notified separately. > > > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > > File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h > > > (right): > > > > > > https://codereview.chromium.org/2410553002/diff/20001/chrome/browser/ui/webui... > > > chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:81: > > > // It's a nullptr if there is no MediaRouterUIService, such as in unit tests. > > > It should be a nullptr only in unit tests, right? > > > > > > If there's a dependency between these classes and the UI Service, I would prefer > > > using a testing factory to return a mock UI service (that returns e.g. a mock > > > action controller) so we don't have to write null checking code in the > > > implementation only for unit tests. > > > > Added mock UI service and action controller to test_helper.cc. Added a ctor to MediaRouterTest that takes a boolean flag which when true, leads to instantiation of the mock objects. > > > > > > > > Here's one way to do that: > > > https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile.h?rcl=0... > > > > Thanks for reviewing. > > I accidentally deleted the first patch, so I've responded to your comments inline. Sorry for the inconvenience.
https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/test_helper.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/test_helper.cc:12: void MockPresentationSessionSuccessCallback( PresentationSessionSessionSuccessCallback is just a base::Callback with template arguments. https://cs.chromium.org/chromium/src/chrome/browser/media/router/create_prese... It would seem better to bind a function in the unit test that verifies if the callback is called, versus implementing a do-nothing callback. Check other unit tests for examples of this pattern. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/test_helper.h (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/test_helper.h:128: class MockMediaRouterActionController : public MediaRouterActionController { It feels odd that this ended up in c/b/media/router. IMO, this belongs with the class being mocked in c/b/ui/toolbar, or with the unittest that uses it if there's only one. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:24: UnregisterObserver(); // media_router::IssuesObserver. Can you add a DCHECK() here that dialogs_shown_ == 0? https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:39: // Overridden in tests. While that's true, can you also document what these methods are for in the real implementation? https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:51: // Constructor for injecting dependencies in tests. Would prefer making this public versus having to friend and forward declare mocks in the implementation to use it. I think it's good to have a constructor in the API that explicitly declares dependencies. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:169: weak_ptr_factory_(this) {} Can you DCHECK(action_controller_) here? https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc:254: EXPECT_CALL(*action_controller, OnDialogHidden()); Is neither callback invoked when the controller is destroyed? That seems like a bug. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_test.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_test.cc:5: #include "chrome/browser/ui/webui/media_router/media_router_test.h" This should probably be called "MediaRouterWebUITest" to make it clear it's for the webui unittests. Not worth fixing in this patch unless you're especially motivated though :) https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_test.cc:34: if (require_mock_ui_service_) { Nice. I would create a separate flag for require_mock_toolbar_factory so it's explicit to callers which mocks are being used.
Patchset #2 (id:100001) has been deleted
Thanks for reviewing! https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/test_helper.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/test_helper.cc:12: void MockPresentationSessionSuccessCallback( On 2016/10/17 18:57:39, mark a. foltz wrote: > PresentationSessionSessionSuccessCallback is just a base::Callback with template > arguments. > > https://cs.chromium.org/chromium/src/chrome/browser/media/router/create_prese... > > It would seem better to bind a function in the unit test that verifies if the > callback is called, versus implementing a do-nothing callback. Check other unit > tests for examples of this pattern. Moving the functions to media_router_dialog_controller_impl_unittest.cc, and making them mock functions. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/test_helper.h (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/test_helper.h:128: class MockMediaRouterActionController : public MediaRouterActionController { On 2016/10/17 18:57:39, mark a. foltz wrote: > It feels odd that this ended up in c/b/media/router. IMO, this belongs with the > class being mocked in c/b/ui/toolbar, or with the unittest that uses it if > there's only one. Moved to media_router_web_ui_test.h, where it's used. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:24: UnregisterObserver(); // media_router::IssuesObserver. On 2016/10/17 18:57:40, mark a. foltz wrote: > Can you add a DCHECK() here that dialogs_shown_ == 0? Done. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:39: // Overridden in tests. On 2016/10/17 18:57:40, mark a. foltz wrote: > While that's true, can you also document what these methods are for in the real > implementation? Done. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:51: // Constructor for injecting dependencies in tests. On 2016/10/17 18:57:40, mark a. foltz wrote: > Would prefer making this public versus having to friend and forward declare > mocks in the implementation to use it. I think it's good to have a constructor > in the API that explicitly declares dependencies. Done. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:169: weak_ptr_factory_(this) {} On 2016/10/17 18:57:40, mark a. foltz wrote: > Can you DCHECK(action_controller_) here? Done. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_unittest.cc:254: EXPECT_CALL(*action_controller, OnDialogHidden()); On 2016/10/17 18:57:40, mark a. foltz wrote: > Is neither callback invoked when the controller is destroyed? That seems like a > bug. Are you referring to the presentation session success/error callbacks? If so, then the error callback does get called. Adding an EXPECT_CALL() for it. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_test.cc (right): https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_test.cc:5: #include "chrome/browser/ui/webui/media_router/media_router_test.h" On 2016/10/17 18:57:40, mark a. foltz wrote: > This should probably be called "MediaRouterWebUITest" to make it clear it's for > the webui unittests. Not worth fixing in this patch unless you're especially > motivated though :) Done. Most of the affected files were already in this CL anyways. https://codereview.chromium.org/2410553002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_test.cc:34: if (require_mock_ui_service_) { On 2016/10/17 18:57:40, mark a. foltz wrote: > Nice. I would create a separate flag for require_mock_toolbar_factory so it's > explicit to callers which mocks are being used. ToolbarActionsModel is a dependency for the MRUIService, so we can't separate them into two flags. Also we're using real ToolbarActionsModel, not a mocked one.
LGTM Looks really good, and thanks for doing some cleanup of the test classes. https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:42: // May show the action icon if any dialog is shown, or hide it otherwise. I found this comment a bit confusing. Did you mean to say, 'Called when a Media Router dialog is shown or hidden, and updates the visibility of the action icon.' https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:195: // Hiding the dialog while there are local routes shouldn't hide the icon. Add one more test case for hiding dialog + issue shown https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:58: extensions::FeatureSwitch::ScopedOverride feature_override_; This should no longer be necessary, as Media Router is default enabled now.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:42: // May show the action icon if any dialog is shown, or hide it otherwise. On 2016/11/01 02:00:08, mark a. foltz wrote: > I found this comment a bit confusing. > > Did you mean to say, > 'Called when a Media Router dialog is shown or hidden, and updates the > visibility of the action icon.' Yeah that sounds better. Thanks. https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:195: // Hiding the dialog while there are local routes shouldn't hide the icon. On 2016/11/01 02:00:08, mark a. foltz wrote: > Add one more test case for hiding dialog + issue shown Done. https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:58: extensions::FeatureSwitch::ScopedOverride feature_override_; On 2016/11/01 02:00:09, mark a. foltz wrote: > This should no longer be necessary, as Media Router is default enabled now. Removed.
msw@, could you take a look at the c/b/ui/toolbar/ files? Thank you!
https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller_unittest.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller_unittest.cc:83: // The non-Android implementation is tested in Why doesn't this work anymore on non-android builds? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service.h:31: std::unique_ptr<MediaRouterActionController> action_controller_; Why is this now a unique_ptr? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:52: DCHECK_NE(dialog_count_, 0u); nit: DCHECK_GE? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:53: if (dialog_count_) nit: if dialog_count_ > 0 or use dialog_count_ = std::max(0, dialog_count_ - 1); https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:93: return has_local_display_route_ || has_issue_ || dialog_count_ || nit: dialog_count_ > 0 https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:84: MediaRouterAction* GetMediaRouterAction() { nit: expose MediaRouterDialogControllerImpl::action_ with a [test-only] getter? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:131: SetAlwaysShowActionPref(true); Why is this important (for a disabled test)? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:177: MediaRouterDialogControllerImpl* dialog_controller = Why is the impl subclass used here? It seems like all the functions needed are available through the MediaRouterDialogController interface. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:181: dialog_controller->ShowMediaRouterDialog(); nit: EXPECT_FALSE(ActionExists()); above this https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:190: EXPECT_FALSE(dialog_controller->IsShowingMediaRouterDialog()); To be clear, calling ExecuteAction above calls through to HideMediaRouterDialog and reduces the dialog_count_, right? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:275: // The |action_controller_| must be notified after |action_| to avoid a UI nit: Maybe override OnDialogShown in the impl to call the action first, then the base class function? ditto for OnDialogHidden? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:283: if (IsShowingMediaRouterDialog()) { Is it possible that multiple dialogs are showing when Reset is called, and OnDialogHidden should be called multiple times to reset the dialog count to 0? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:13: class BrowserWindow; nit: probably not needed; should be included from browser_with_test_window_test.h; ditto for TestingProfile https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:16: class MockMediaRouterActionController : public MediaRouterActionController { nit: move these other classes to chrome/browser/media/router/test_helper.* https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:39: std::unique_ptr<MediaRouterActionController> action_controller_; nit: avoid unique_ptr and use a plain MockMediaRouterActionController member https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:44: // |require_mock_ui_service_| defaults to false in the default ctor. optional nit: remove default ctor; add default value for require_mock_ui_service = false below. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:49: // BrowserWithTestWindowTest: optional nit: make both overrides protected or public
Just noticed one more minor thing. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Update copyright. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Update copyright.
Patchset #4 (id:160001) has been deleted
Thanks for reviewing! https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller_unittest.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller_unittest.cc:83: // The non-Android implementation is tested in On 2016/11/04 00:07:16, msw wrote: > Why doesn't this work anymore on non-android builds? The desktop version instantiates MediaRouterDialogControllerImpl that now depends on MediaRouterActionController, which requires lots of desktop-specific code to be set up, and we can't really inject a mock version of MediaRouterActionController here. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service.h:31: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/11/04 00:07:16, msw wrote: > Why is this now a unique_ptr? This was for being able to expose it through the action_controller() getter. If I kept action_controller_ a simple MediaRouterActionController, action_controller() would've had to return a reference to it, which would be const. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:52: DCHECK_NE(dialog_count_, 0u); On 2016/11/04 00:07:16, msw wrote: > nit: DCHECK_GE? Changing to DCHECK_GT. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:53: if (dialog_count_) On 2016/11/04 00:07:16, msw wrote: > nit: if dialog_count_ > 0 or use dialog_count_ = std::max(0, dialog_count_ - 1); mfoltz@'s nit was to change from dialog_count_ > 0 to this. Do you feel strongly about this? https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:93: return has_local_display_route_ || has_issue_ || dialog_count_ || On 2016/11/04 00:07:16, msw wrote: > nit: dialog_count_ > 0 Same as above. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:84: MediaRouterAction* GetMediaRouterAction() { On 2016/11/04 00:07:16, msw wrote: > nit: expose MediaRouterDialogControllerImpl::action_ with a [test-only] getter? Done. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:131: SetAlwaysShowActionPref(true); On 2016/11/04 00:07:16, msw wrote: > Why is this important (for a disabled test)? This test was disabled recently. Without this line we'd be executing the action without having the action icon on the toolbar, which doesn't reflect actual usage. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:177: MediaRouterDialogControllerImpl* dialog_controller = On 2016/11/04 00:07:16, msw wrote: > Why is the impl subclass used here? It seems like all the functions needed are > available through the MediaRouterDialogController interface. It'd be okay to use either, but only the Impl class (the desktop implementation of MRDC, as opposed to the Android implementation) has the toolbar action related code. I'm going to call the action() getter on the Impl class above, so I'll keep using the Impl class here for the sake of consistency. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:181: dialog_controller->ShowMediaRouterDialog(); On 2016/11/04 00:07:16, msw wrote: > nit: EXPECT_FALSE(ActionExists()); above this Done. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:190: EXPECT_FALSE(dialog_controller->IsShowingMediaRouterDialog()); On 2016/11/04 00:07:16, msw wrote: > To be clear, calling ExecuteAction above calls through to HideMediaRouterDialog > and reduces the dialog_count_, right? Right. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:275: // The |action_controller_| must be notified after |action_| to avoid a UI On 2016/11/04 00:07:16, msw wrote: > nit: Maybe override OnDialogShown in the impl to call the action first, then the > base class function? ditto for OnDialogHidden? Sorry, I don't quite understand this comment. (The action controller doesn't keep track of all the actions, so it can't notify the action.) https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:283: if (IsShowingMediaRouterDialog()) { On 2016/11/04 00:07:16, msw wrote: > Is it possible that multiple dialogs are showing when Reset is called, and > OnDialogHidden should be called multiple times to reset the dialog count to 0? Each MediaRouterDialogControllerImpl can have at most one dialog, so if multiple dialogs are shown, their respective MediaRouterDialogControllerImpls will together reset the count to 0. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/11/08 21:15:53, mark a. foltz wrote: > Update copyright. Done. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/11/08 21:15:53, mark a. foltz wrote: > Update copyright. Done. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:13: class BrowserWindow; On 2016/11/04 00:07:17, msw wrote: > nit: probably not needed; should be included from > browser_with_test_window_test.h; ditto for TestingProfile Removed. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:16: class MockMediaRouterActionController : public MediaRouterActionController { On 2016/11/04 00:07:16, msw wrote: > nit: move these other classes to chrome/browser/media/router/test_helper.* These classes were there originally, but were moved here because they are desktop-UI-specific. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:39: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/11/04 00:07:17, msw wrote: > nit: avoid unique_ptr and use a plain MockMediaRouterActionController member That would make the action_controller() getter return &action_controller_, which is const and wouldn't work. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:44: // |require_mock_ui_service_| defaults to false in the default ctor. On 2016/11/04 00:07:17, msw wrote: > optional nit: remove default ctor; add default value for require_mock_ui_service > = false below. I feel like a default ctor is cleaner than always having to pass in a flag. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:49: // BrowserWithTestWindowTest: On 2016/11/04 00:07:17, msw wrote: > optional nit: make both overrides protected or public Made them both protected.
https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller_unittest.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller_unittest.cc:83: // The non-Android implementation is tested in On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:16, msw wrote: > > Why doesn't this work anymore on non-android builds? > > The desktop version instantiates MediaRouterDialogControllerImpl that now > depends on MediaRouterActionController, which requires lots of desktop-specific > code to be set up, and we can't really inject a mock version of > MediaRouterActionController here. Hmm, okay, I guess... https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service.h:31: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:16, msw wrote: > > Why is this now a unique_ptr? > > This was for being able to expose it through the action_controller() getter. If > I kept action_controller_ a simple MediaRouterActionController, > action_controller() would've had to return a reference to it, which would be > const. That's only because action_controller() is const. Should the function not be const? (is it actually okay for a client to modify the MediaRouterActionController instance given a const MediaRouterUIService?) https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:53: if (dialog_count_) On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:16, msw wrote: > > nit: if dialog_count_ > 0 or use dialog_count_ = std::max(0, dialog_count_ - > 1); > > mfoltz@'s nit was to change from dialog_count_ > 0 to this. Do you feel strongly > about this? No, I think it's worse, but I don't feel too strongly. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:177: MediaRouterDialogControllerImpl* dialog_controller = On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:16, msw wrote: > > Why is the impl subclass used here? It seems like all the functions needed are > > available through the MediaRouterDialogController interface. > > It'd be okay to use either, but only the Impl class (the desktop implementation > of MRDC, as opposed to the Android implementation) has the toolbar action > related code. I'm going to call the action() getter on the Impl class above, so > I'll keep using the Impl class here for the sake of consistency. I think it's generally beneficial to use the most abstract interface possible, in both production and testing, that way clients are less likely to rely upon or fall victim to implementation-specific details. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:275: // The |action_controller_| must be notified after |action_| to avoid a UI On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:16, msw wrote: > > nit: Maybe override OnDialogShown in the impl to call the action first, then > the > > base class function? ditto for OnDialogHidden? > > Sorry, I don't quite understand this comment. (The action controller doesn't > keep track of all the actions, so it can't notify the action.) hmm, it's odd that the *dialog* controller tracks the actions, but the *action* controller doesn't... I guess some of these patterns just aren't obvious to me. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:283: if (IsShowingMediaRouterDialog()) { On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:16, msw wrote: > > Is it possible that multiple dialogs are showing when Reset is called, and > > OnDialogHidden should be called multiple times to reset the dialog count to 0? > > Each MediaRouterDialogControllerImpl can have at most one dialog, so if multiple > dialogs are shown, their respective MediaRouterDialogControllerImpls will > together reset the count to 0. Acknowledged. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:16: class MockMediaRouterActionController : public MediaRouterActionController { On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:16, msw wrote: > > nit: move these other classes to chrome/browser/media/router/test_helper.* > > These classes were there originally, but were moved here because they are > desktop-UI-specific. Move them into the cc test file or to separate files, as needed. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:39: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:17, msw wrote: > > nit: avoid unique_ptr and use a plain MockMediaRouterActionController member > > That would make the action_controller() getter return &action_controller_, which > is const and wouldn't work. Ditto to my earlier question about the correctness of a const accessor returning a mutable member... this doesn't seem right. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:44: // |require_mock_ui_service_| defaults to false in the default ctor. On 2016/11/09 04:37:27, takumif wrote: > On 2016/11/04 00:07:17, msw wrote: > > optional nit: remove default ctor; add default value for > require_mock_ui_service > > = false below. > > I feel like a default ctor is cleaner than always having to pass in a flag. A default value, as I suggested, would avoid needing to always pass a flag. https://codereview.chromium.org/2410553002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:49: TestingProfile* CreateProfile() override; nit: reorder to match BrowserWithTestWindowTest (impls too).
Patchset #6 (id:220001) has been deleted
https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service.h:31: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/11/09 20:32:33, msw wrote: > On 2016/11/09 04:37:27, takumif wrote: > > On 2016/11/04 00:07:16, msw wrote: > > > Why is this now a unique_ptr? > > > > This was for being able to expose it through the action_controller() getter. > If > > I kept action_controller_ a simple MediaRouterActionController, > > action_controller() would've had to return a reference to it, which would be > > const. > > That's only because action_controller() is const. Should the function not be > const? (is it actually okay for a client to modify the > MediaRouterActionController instance given a const MediaRouterUIService?) Ah. Yes, it is expected that MediaRouterActionController is modified, so no, action_controller() shouldn't be const. Making it non-const and removing the use of a unique_ptr. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:177: MediaRouterDialogControllerImpl* dialog_controller = On 2016/11/09 20:32:33, msw wrote: > On 2016/11/09 04:37:27, takumif wrote: > > On 2016/11/04 00:07:16, msw wrote: > > > Why is the impl subclass used here? It seems like all the functions needed > are > > > available through the MediaRouterDialogController interface. > > > > It'd be okay to use either, but only the Impl class (the desktop > implementation > > of MRDC, as opposed to the Android implementation) has the toolbar action > > related code. I'm going to call the action() getter on the Impl class above, > so > > I'll keep using the Impl class here for the sake of consistency. > > I think it's generally beneficial to use the most abstract interface possible, > in both production and testing, that way clients are less likely to rely upon or > fall victim to implementation-specific details. Got it. Using MediaRouterDialogController. Thanks for explaining. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:16: class MockMediaRouterActionController : public MediaRouterActionController { On 2016/11/09 20:32:34, msw wrote: > On 2016/11/09 04:37:27, takumif wrote: > > On 2016/11/04 00:07:16, msw wrote: > > > nit: move these other classes to chrome/browser/media/router/test_helper.* > > > > These classes were there originally, but were moved here because they are > > desktop-UI-specific. > > Move them into the cc test file or to separate files, as needed. Moving MockMediaRouterUIService into the .cc file and moving MockMediaRouterActionController into its own file. https://codereview.chromium.org/2410553002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:39: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/11/09 20:32:34, msw wrote: > On 2016/11/09 04:37:27, takumif wrote: > > On 2016/11/04 00:07:17, msw wrote: > > > nit: avoid unique_ptr and use a plain MockMediaRouterActionController member > > > > That would make the action_controller() getter return &action_controller_, > which > > is const and wouldn't work. > > Ditto to my earlier question about the correctness of a const accessor returning > a mutable member... this doesn't seem right. Fixed. https://codereview.chromium.org/2410553002/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_web_ui_test.h (right): https://codereview.chromium.org/2410553002/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_web_ui_test.h:49: TestingProfile* CreateProfile() override; On 2016/11/09 20:32:34, msw wrote: > nit: reorder to match BrowserWithTestWindowTest (impls too). Done.
lgtm with a nit. https://codereview.chromium.org/2410553002/diff/240001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/mock_media_router_action_controller.h (right): https://codereview.chromium.org/2410553002/diff/240001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/mock_media_router_action_controller.h:24: }; nit: DISALLOW_COPY_AND_ASSIGN (made private:)
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...
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 mfoltz@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2410553002/#ps260001 (title: "DISALLOW_COPY_AND_ASSIGN MockMediaRouterActionController")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Show Media Router toolbar icon ephemerally for MR dialogs This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues [2] 3. Show ephemeral icon for open dialog (this CL) This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController may show or hide the toolbar action icon depending on that. The icon will be shown when there are open dialogs, local routes or issues, or when the "Always Show Icon" option is checked. In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController. [1] https://codereview.chromium.org/2294973002 [2] https://codereview.chromium.org/2332693003 BUG=594577 ========== to ========== Show Media Router toolbar icon ephemerally for MR dialogs This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues [2] 3. Show ephemeral icon for open dialog (this CL) This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController may show or hide the toolbar action icon depending on that. The icon will be shown when there are open dialogs, local routes or issues, or when the "Always Show Icon" option is checked. In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController. [1] https://codereview.chromium.org/2294973002 [2] https://codereview.chromium.org/2332693003 BUG=594577 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Show Media Router toolbar icon ephemerally for MR dialogs This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues [2] 3. Show ephemeral icon for open dialog (this CL) This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController may show or hide the toolbar action icon depending on that. The icon will be shown when there are open dialogs, local routes or issues, or when the "Always Show Icon" option is checked. In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController. [1] https://codereview.chromium.org/2294973002 [2] https://codereview.chromium.org/2332693003 BUG=594577 ========== to ========== Show Media Router toolbar icon ephemerally for MR dialogs This CL is part 3 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues [2] 3. Show ephemeral icon for open dialog (this CL) This CL makes MRDialogControllerImpl notify MRActionController whenever a dialog is shown or hidden, and MRActionController may show or hide the toolbar action icon depending on that. The icon will be shown when there are open dialogs, local routes or issues, or when the "Always Show Icon" option is checked. In MRUIBrowserTest, this CL replaces MRAction manually instantiated during setup with one whose instantiation is initiated by MRActionController. [1] https://codereview.chromium.org/2294973002 [2] https://codereview.chromium.org/2332693003 BUG=594577 Committed: https://crrev.com/89fc2a78d69cb432b445da453fd3f6fd02d4ebfe Cr-Commit-Position: refs/heads/master@{#431296} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/89fc2a78d69cb432b445da453fd3f6fd02d4ebfe Cr-Commit-Position: refs/heads/master@{#431296} |
