Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1243173003:
[MediaRouter] Moved the code useful for Android to MediaRouterDialogController (Closed)
|
Created:
5 years, 5 months ago by whywhat Modified:
5 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, media-router+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@build-media-router-android Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MediaRouter] Moved the code useful for Android to MediaRouterDialogController
Moved presentation_request_ and initiator_ with initiator_observer_
BUG=519549
Committed: https://crrev.com/69b3a41fdbd8699477f770f9bec2f7b6d3073f37
Cr-Commit-Position: refs/heads/master@{#340904}
Patch Set 1 #Patch Set 2 : Fixed desktop builds #Patch Set 3 : Moved base CloseMediaRouterDialog to the end of the derived implementation method #
Total comments: 30
Patch Set 4 : Addressed comments #Patch Set 5 : Fixed compile on the bots. #Patch Set 6 : Rebase #
Total comments: 36
Patch Set 7 : Addressed comments #Patch Set 8 : Fixed the unit test #Messages
Total messages: 53 (20 generated)
avayvod@chromium.org changed reviewers: + apacible@chromium.org, haibinlu@chromium.org, imcheng@chromium.org
PTaL Jennifer, your approval is needed for the webui bit. Haibin, your approval is needed for the chrome/browser/media/router part. Derek, you were the one mainly interested in this so you own the whole review :)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fixed desktop builds
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Moved base CloseMediaRouterDialog to the end of the derived implementation method
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_dialog_controller.cc (right): https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:38: } DCHECK(dialog_controller_); https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:70: DCHECK(initiator_); DCHECK not needed as it is already done in ctor. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:74: void MediaRouterDialogController::CloseMediaRouterDialog() { isn't this function pure virtual? https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:81: initiator_observer_.reset(new InitiatorWebContentsObserver( This function doesn't create the dialog - how about naming this ObserveInitiatorWebContents? https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:85: void MediaRouterDialogController::SetPresentationRequest( nit: TakePresentationRequest ? https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_dialog_controller.h (right): https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:19: // An implementation of this interface is tied to a WebContents known as the nit: It is no longer an interface but an abstract base class. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:46: protected: nit:indent https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:59: virtual void Reset() = 0; this needs documentation. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:61: private: nit: indent https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:71: }; DISALLOW_COPY_AND_ASSIGN ? https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:186: CreateMediaRouterDialog(); I think there might have been a bad merge: 1) SetPresentationRequest should occur before CreateMediaRouterDialog just to be safe 2) The variable |media_router_dialog| is not needed 3) We don't call ActivateInitiatorWebContents() in case of false. In addition, can the definition of this method be moved to the base class? https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:213: MediaRouterDialogController::CloseMediaRouterDialog(); this should be placed above the web_ui above to preserve the call order. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:220: Profile::FromBrowserContext(initiator()->GetBrowserContext()); nit: suggest having a initiator local variable so we don't have to repeatedly call the getter method. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:257: MediaRouterDialogController::Reset(); this should be after the DCHECKs https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:286: DCHECK(initiator()); nit: suggest having a initiator local variable so we don't have to repeatedly call the getter method.
Addressed comments
PTAL Addressed Derek's comments and also some comments from Mark and Jennifer from the previous patch. Hope it still compiles :) Jennifer, there's a strange bot failure on Mac with the patchset #3, could you take a look please: http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... These seem to have run fine on other platforms and I don't see from the first glance why the initiator_observer_ would be null on Mac. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_dialog_controller.cc (right): https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:38: } On 2015/07/23 at 00:41:14, imcheng1 wrote: > DCHECK(dialog_controller_); Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:70: DCHECK(initiator_); On 2015/07/23 at 00:41:14, imcheng1 wrote: > DCHECK not needed as it is already done in ctor. Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:74: void MediaRouterDialogController::CloseMediaRouterDialog() { On 2015/07/23 at 00:41:14, imcheng1 wrote: > isn't this function pure virtual? Yep, changed that. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:81: initiator_observer_.reset(new InitiatorWebContentsObserver( On 2015/07/23 at 00:41:13, imcheng1 wrote: > This function doesn't create the dialog - how about naming this ObserveInitiatorWebContents? Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.cc:85: void MediaRouterDialogController::SetPresentationRequest( On 2015/07/23 at 00:41:14, imcheng1 wrote: > nit: TakePresentationRequest ? Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_dialog_controller.h (right): https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:46: protected: On 2015/07/23 at 00:41:14, imcheng1 wrote: > nit:indent Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:59: virtual void Reset() = 0; On 2015/07/23 at 00:41:14, imcheng1 wrote: > this needs documentation. Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:61: private: On 2015/07/23 at 00:41:14, imcheng1 wrote: > nit: indent Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_router_dialog_controller.h:71: }; On 2015/07/23 at 00:41:14, imcheng1 wrote: > DISALLOW_COPY_AND_ASSIGN ? Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:186: CreateMediaRouterDialog(); On 2015/07/23 at 00:41:14, imcheng1 wrote: > I think there might have been a bad merge: > > 1) SetPresentationRequest should occur before CreateMediaRouterDialog just to be safe > 2) The variable |media_router_dialog| is not needed > 3) We don't call ActivateInitiatorWebContents() in case of false. > > In addition, can the definition of this method be moved to the base class? Bad merge when? This matches the code on trunk as far as I can see. Changed according to your notes and moved to the base class. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:213: MediaRouterDialogController::CloseMediaRouterDialog(); On 2015/07/23 at 00:41:14, imcheng1 wrote: > this should be placed above the web_ui above to preserve the call order. I guess, although it becomes a bit fragile: 1. The base class call can't be in the beginning because it calls Reset() and deletes the dialog_observer_, so it has to be after we get the dialog pointer. 2. Once we got the dialog pointer, we can call the base class implementation which will |Reset| the state but the dialog pointer will still be valid? 3. Calling it in the end doesn't seem to have side effects but is more natural - cleanup the derived class and call the base class method. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:220: Profile::FromBrowserContext(initiator()->GetBrowserContext()); On 2015/07/23 at 00:41:14, imcheng1 wrote: > nit: suggest having a initiator local variable so we don't have to repeatedly > call the getter method. Since it's an inline getter method, I don't think it's worth it. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:220: Profile::FromBrowserContext(initiator()->GetBrowserContext()); On 2015/07/23 at 00:41:14, imcheng1 wrote: > nit: suggest having a initiator local variable so we don't have to repeatedly > call the getter method. Since it's an inline getter, I don't think there's much difference. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:257: MediaRouterDialogController::Reset(); On 2015/07/23 at 00:41:14, imcheng1 wrote: > this should be after the DCHECKs Done. https://codereview.chromium.org/1243173003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:286: DCHECK(initiator()); On 2015/07/23 at 00:41:14, imcheng1 wrote: > nit: suggest having a initiator local variable so we don't have to repeatedly call the getter method. Same as above.
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1243173003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1243173003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/07/24 at 15:21:38, commit-bot wrote: > Dry run: This issue passed the CQ dry run. PTAL!
avayvod@chromium.org changed reviewers: + mfoltz@chromium.org, pkasting@chromium.org - haibinlu@chromium.org, imcheng@chromium.org
Jennifer, please review chrome/browser/ui/webui/media_router/ Mark, please review chrome/browser/media/router/ Peter, please review chrome/browser/ui/toolbar/
mfoltz@chromium.org changed reviewers: + haibinlu@chromium.org, imcheng@chromium.org - pkasting@chromium.org
Mostly looks good, a few adjustments to the API requested. I would like Derek to look at the changes to the desktop implementation. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller.cc (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:107: DCHECK(initiator_observer_.get()); Why does the state of the observer matter for closing the dialog? I think Close() and Reset() should be no-ops if the dialog is not open. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:111: void MediaRouterDialogController::ObserveInitiatorWebContents() { Shouldn't this be called when the dialog is created to guarantee |initiator_observer_| is set? Some things in this API will break if it's not done. For example it's not not called in the Android impl. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:112: DCHECK(thread_checker_.CalledOnValidThread()); Generally I don't bother DCHECKING on non-public methods, as there's no easy way for a caller to post a task to a private/protected method on the wrong thread. (Unless the implementation is doing its own thread hopping which is not the case here) https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:118: void MediaRouterDialogController::TakePresentationRequest( I think this method can be inlined. It's a one-liner called once. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:125: MediaRouterDialogController::PassPresentationRequest() { This is a one-liner called once - consider inlining it. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller.h (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:32: static MediaRouterDialogController* GetOrCreateForWebContents( Does this need to be public or is it an implementation detail of CreateMediaRouterDialog()? https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:48: virtual bool IsShowingMediaRouterDialog() = 0; const? https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:51: virtual void CreateMediaRouterDialog() = 0; Would a client ever want to create a dialog without showing it? Perhaps this could be protected. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:53: // Shows the media router dialog modal to the initiator WebContents. (1) for consistency, "modal to |initiator_|." (2) The documentation should clarify when this should be called vs. ShowMediaRouterDialogForPresentation. (3) Why does this return void and ShowMediaRouterDialogForPresentation return bool? Is this guaranteed to succeed? https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:59: // Use MediaRouterDialogController::GetOrCreateForWebContents() to create an Or should the public API be ShowMediaRouterDialog{ForPresentation} ? https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:70: content::WebContents* initiator() { return initiator_; } Should this method be const? https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:80: scoped_ptr<InitiatorWebContentsObserver> initiator_observer_; It would be helpful to include a comment here since the type is fwd declared. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:83: // Data for dialogs created under a Presentation API context. created at the request of the Presentation API. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:84: // Passed from the caller of ShowMediaRouterDialogForPresentation(), and Passed from the caller via SMRDFP to the dialog when it is initialized.
https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:167: return !!GetMediaRouterDialog(); Does this negate itself? https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_browsertest.cc:116: EXPECT_FALSE(dialog_controller_-> GetMediaRouterDialog()); nit: Remove whitespace?
The desktop logic looks okay. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller.h (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:43: // Closes the media router dialog. Must be called from the overrides. sounds like this should be moved to protected? Or how about making this a non-virtual method, and in that method, invoke a pure virtual method (which subclasses implement to perform specific closing logic?) https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:73: virtual void Reset(); similar comment to CloseMediaRouterDialog() above - might be a good idea to have this as a nonvirtual, and call into a pure virtual method to do implementation specific cleanups.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
LGTM, feel free to TBR mechanical changes
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Thanks all for the comments (and Peter for lgtm). Mark, Jennifer, Derek, PTAL! https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller.cc (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:107: DCHECK(initiator_observer_.get()); On 2015/07/24 at 20:10:34, mark a. foltz wrote: > Why does the state of the observer matter for closing the dialog? > > I think Close() and Reset() should be no-ops if the dialog is not open. I merely moved the code that was already there. I guess the checks were in place to underline/ensure that there's no redundant calls to Reset and Close. Note that Close in the desktop implementation had a strong CHECK statement... Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:111: void MediaRouterDialogController::ObserveInitiatorWebContents() { On 2015/07/24 at 20:10:34, mark a. foltz wrote: > Shouldn't this be called when the dialog is created to guarantee |initiator_observer_| is set? Some things in this API will break if it's not done. For example it's not not called in the Android impl. My guess that it was done to avoid redundant Close calls. Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:112: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/07/24 at 20:10:34, mark a. foltz wrote: > Generally I don't bother DCHECKING on non-public methods, as there's no easy way for a caller to post a task to a private/protected method on the wrong thread. (Unless the implementation is doing its own thread hopping which is not the case here) Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:118: void MediaRouterDialogController::TakePresentationRequest( On 2015/07/24 at 20:10:34, mark a. foltz wrote: > I think this method can be inlined. It's a one-liner called once. Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.cc:125: MediaRouterDialogController::PassPresentationRequest() { On 2015/07/24 at 20:10:34, mark a. foltz wrote: > This is a one-liner called once - consider inlining it. I think this will be used by the Android implementation therefore I'll leave it. Also inlining it would mean making the request a protected member. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router_dialog_controller.h (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:43: // Closes the media router dialog. Must be called from the overrides. On 2015/07/24 at 22:09:28, imcheng1 wrote: > sounds like this should be moved to protected? Or how about making this a non-virtual method, and in that method, invoke a pure virtual method (which subclasses implement to perform specific closing logic?) Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:48: virtual bool IsShowingMediaRouterDialog() = 0; On 2015/07/24 at 20:10:35, mark a. foltz wrote: > const? Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:51: virtual void CreateMediaRouterDialog() = 0; On 2015/07/24 at 20:10:35, mark a. foltz wrote: > Would a client ever want to create a dialog without showing it? > Perhaps this could be protected. Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:53: // Shows the media router dialog modal to the initiator WebContents. On 2015/07/24 at 20:10:34, mark a. foltz wrote: > (1) for consistency, "modal to |initiator_|." Done. > > (2) The documentation should clarify when this should be called vs. ShowMediaRouterDialogForPresentation. I would like to know better :) I guess simple Show is called for mirroring? > > (3) Why does this return void and ShowMediaRouterDialogForPresentation return bool? Is this guaranteed to succeed? bool in the ForPresentation flavor is used to indicate whether the dialog was created when the method was called or not. I added the same to the simple Show method. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:59: // Use MediaRouterDialogController::GetOrCreateForWebContents() to create an On 2015/07/24 at 20:10:35, mark a. foltz wrote: > Or should the public API be ShowMediaRouterDialog{ForPresentation} ? I believe there's some advantage to be able to cache the controller and then call show/hide on it. Since this change's original purpose was code reuse and separating Android and desktop implementation I'd try to avoid major refactorings of the desktop code. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:70: content::WebContents* initiator() { return initiator_; } On 2015/07/24 at 20:10:34, mark a. foltz wrote: > Should this method be const? If I can use const content::WebContents* everywhere? Will try. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:73: virtual void Reset(); On 2015/07/24 at 22:09:28, imcheng1 wrote: > similar comment to CloseMediaRouterDialog() above - might be a good idea to have this as a nonvirtual, and call into a pure virtual method to do implementation specific cleanups. I don't think it's justified in the case of Reset and just introduces boilerplate. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:80: scoped_ptr<InitiatorWebContentsObserver> initiator_observer_; On 2015/07/24 at 20:10:34, mark a. foltz wrote: > It would be helpful to include a comment here since the type is fwd declared. Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:83: // Data for dialogs created under a Presentation API context. On 2015/07/24 at 20:10:35, mark a. foltz wrote: > created at the request of the Presentation API. Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_dialog_controller.h:84: // Passed from the caller of ShowMediaRouterDialogForPresentation(), and On 2015/07/24 at 20:10:34, mark a. foltz wrote: > Passed from the caller via SMRDFP to the dialog when it is initialized. Done. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:167: return !!GetMediaRouterDialog(); On 2015/07/24 at 20:41:07, apacible wrote: > Does this negate itself? Yes, an old way of converting pointers/integers to a bool. Changed to a more readable != nullptr. https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_browsertest.cc:116: EXPECT_FALSE(dialog_controller_-> GetMediaRouterDialog()); On 2015/07/24 at 20:41:07, apacible wrote: > nit: Remove whitespace? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1243173003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm++ thanks for addressing design comments with this patch. Looks like there are a few unit tests that need updates.
lgtm, thanks! https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/1243173003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:167: return !!GetMediaRouterDialog(); On 2015/07/28 18:52:53, whywhat wrote: > On 2015/07/24 at 20:41:07, apacible wrote: > > Does this negate itself? > > Yes, an old way of converting pointers/integers to a bool. Changed to a more > readable != nullptr. Acknowledged, thanks!
Fixed the unit test
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1243173003/140001
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 avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, mfoltz@chromium.org, apacible@chromium.org Link to the patchset: https://codereview.chromium.org/1243173003/#ps140001 (title: "Fixed the unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1243173003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1243173003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/69b3a41fdbd8699477f770f9bec2f7b6d3073f37 Cr-Commit-Position: refs/heads/master@{#340904} |
