Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1139203003:
[Media Router] MediaRouterUI + WebUI handler implementation. (Closed)
|
Created:
5 years, 7 months ago by imcheng (use chromium acct) Modified:
5 years, 6 months ago Reviewers:
Wez CC:
chromium-reviews, posciak+watch_chromium.org, media-router+watch_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] MediaRouterUI + WebUI handler implementation.
This patch has two parts:
- MediaRouterWebUIMessageHandler handles calls coming
from WebUI/JS. Also sends up-to-date data (e.g. MediaSinks)
to WebUI.
- MediaRouterUI interfaces with the MediaRouter to handle calls
from the WebUI (such as CreateRoute) and listen for sink /
route updates.
Implemented handler logic for the following from Media Router WebUI:
OnGetInitialSettings
OnCreateRoute
OnActOnIssue - skeleton impl
OnCloseRoute
OnCloseDialog
Implemented WebUI logic for the following (to update the UI):
UpdateSinks
UpdateRoutes
UpdateCastModes
AddRoute
UpdateIssue - skeleton impl
Some Issues related APIs are left unimplemented until more resources
have been upstreamed.
BUG=464216, 464208
Committed: https://crrev.com/bcb07449ce226e9263cf4f2637a1656377782e17
Cr-Commit-Position: refs/heads/master@{#331905}
Patch Set 1 #Patch Set 2 : Fix naming #Patch Set 3 : fix compile #Patch Set 4 : Rebase #
Total comments: 118
Patch Set 5 : Addressed wez's comments + git cl format #Patch Set 6 : fix compile' #
Total comments: 8
Patch Set 7 : Addressed wez's 2nd set of comments #Patch Set 8 : Rebase #Patch Set 9 : add back use of MediaRouterMojoImpl #
Messages
Total messages: 12 (3 generated)
imcheng@google.com changed reviewers: + wez@chromium.org
https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:39: // Requests a media route from |source| to |sink_id|. nit: Rephrase the comment to match the request->create renaming. Does this method now create a new route, or potentially re-connect an existing one? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:25: // Observes when issue to show in UI has been updated. nit: Can we word this to be clearer; this class calls to refresh the UI when any Issue is updated? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:26: class UIIssuesObserver : public IssuesObserver { nit: UiIssuesObserver https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:40: // Not owned by this class. Rather than "not owned by" I'd suggest something like "Reference back to the owning MediaRouterUI instance." here and below. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:69: ui_initialized_(false), nit: is_initialized_? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:81: html_source.release()); nit: Blank line after this, for consistency. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:88: /* TODO(imcheng): Uncomment once Kevin's MediaRouterMojoImpl patch landed. What's the bug # for that patch? Does this class make sense with a null |router_|..? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:95: routes_observer_.reset(new UIMediaRoutesObserver(router_, this)); nit: Suggest a comment on this block "Register for Issue and MediaRoute updates."? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:129: return DoCreateRoute(sink_id, GetPreferredCastMode(cast_modes_)); Where does cast_modes_ ever get set..? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:132: bool MediaRouterUI::CreateRouteWithCastModeOverride( Why do we need the word "override"? i.e. not CreateRouteWithCastMode? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:135: // |cast_mode_override| == |GetPreferredCastMode(cast_modes_)|. Not sure what this comment is intended to achieve? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:149: // TODO(imcheng): Uncomment once Kevin's MediaRouterMojoImpl patch landed. See above re bug #. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:176: // the MediaSource in MediaRouterUI if an update occurred between the user It's not clear what "the MediaSource in MediaRouterUI" means - do you mean "the source details displayed in the UI" or something else? Why is the fact that the source may differ relevant to this code - are you just trying to explain the reason for calling GetSourceForCastMode(), for example? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:185: base::Bind(&MediaRouterUI::OnRouteResponseReceived, nit: Indentation - consider git cl format. :) https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:25: class MediaRouterUI : public ConstrainedWebDialogUI, nit: MediaRouterUi would be the preferred name according to style-guide, though I notice that Chromium is pretty inconsistent where UI is concerned; feel free to ignore my UI->Ui comments elsewhere. :-/ https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:34: override; Do we expect callers to invoke this? If not then consider moving it to the private section; although the interface inheritance must always be public, it's OK to "hide" interface methods that callers shouldn't need to think about. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:36: // Called by |routes_observer_| when the set of active routes has changed. Would it make sense for this to be private and for the observer helper sub-component to be a friend of this class? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:42: // Notifies that the UI has been initialized. nit: Indentation? (BTW, you know about git cl format, right?) https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:42: // Notifies that the UI has been initialized. Not clear from this comment who/what is being notified? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:43: void UIInitialized(); nit: CamelCase applies even to acronyms & abbreviations, so UI->Ui here. OnUiInitialized? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:46: // determined by the preferred MediaCastMode. It's unclear in this context what "using MediaSink |sink_id|" means - do you mean that we create a route from source content to |sink_id|, using the preferred MediaCastMode (i.e. fling, tab-mirroring, etc)? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:49: // Returns true if a route request is successfully submitted. Since a "success" result only indicates that the request has been made, not that it's successfully completed, suggest rephrases this to "Returns false if unable to request the route." then briefly describe what happens when the route request completes. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:50: // One of the Init* functions must have been called before. There are no Init* functions...? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:54: // determined by |cast_mode_override|. There seems to be some confusion between "mode" and "source" terminology here. :( https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:56: // One of the Init* functions must have been called before. See above re Init* functions? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:63: // If the UI is already initialized, notifies |handler_| to update the UI. nit: What does it do if the UI is not initialized? Suggest rewording, e.g. "Updates the UI to display the new |issue|. Ignored if the UI is not yet initialized." https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:66: // Calls MediaRouter to clear the given issue. nit: What does it mean to "clear" the issue? Is that documented elsewhere? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:70: // initially loaded. (i.e., user has not selected a cast mode override yet) nit: You mean "e.g.", not "i.e.", I think? "e.g." means "for example", where "i.e." means "in other words". https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:80: // Callback from Media Router for a route creation request. nit: "Callback passed to MediaRouter to receive response to route creation requests." https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:84: bool DoCreateRoute(const MediaSinkId& sink_id, MediaCastMode cast_mode); Do we need this? Why can't CreateRoute() just call CreateRouteWithCastMode() with the preferred route, and have that contain the implementation? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:90: scoped_ptr<IssuesObserver> issues_observer_; nit: Add a comment to this block of members to explain that they are non-null while we're registered to receive notifications from those components. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:93: // Keeps track of whether initial settings have been requested. initial settings for what? requested by whom? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:94: bool ui_initialized_; nit: is_initialized_? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:96: // Set to |true| if there is a pending route request for this UI. Does this basically mean it's true when we're trying to connect to the device to start an activity? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:103: std::string source_host_; Do we need this variable if DEFAULT mode is not yet supported? Or can we add in in the later CL that adds that mode? Do we have a bug # for that work? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:107: // Does not own this object. This comment's pretty ambiguous - are you saying |router_| doesn't own |this|, or |this| doesn't own |router_|, or something else? Isn't this just a bare reference back to the global MediaRouter instance? Is the guarantee simply that that will never be torn down until the browser shuts down? What guarantees that |this| will be torn down before that point? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:24: // Callback names. Are these actually callback names, or event/notification names? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:31: void SetSinksListValue( nit: As below, can you just have this accept the sinks and return a scoped_ptr<base::ListValue>? It means one of your call-sites loses the stack allocation optimization, but since the ListValue will be heap allocating stuff internally, I doubt that that's a significant impact. (Famous last words...) https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:36: for (const MediaSinkWithCastModes& sink_with_cast_modes : sinks) { nit: Suggest adding some blank lines in here to break the loop contents up into logical chunks, for readability. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:49: void SetMediaRouteDictionary( This name's not very descriptive - suggest e.g. MediaRouteDetailsToValue() and have it return the DictionaryValue rather than modifying an existing one, since that should work fine with your two current call-sites. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:59: base::ListValue* value) { See comments on preceding methods re naming and scoped_ptr<> result https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:72: base::ListValue* value) { Same here. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:136: nit: Why the blank line here - you don't have one in the preceding methods? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:141: DVLOG(1) << "AddRoute"; nit: Why does AddRoute deserve a DVLOG but not the other updates? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:184: // Note that it is possible for this source to be different from What is "this source"? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:197: << "valid sink ID. Aborting."; nit: Consider folding this into the "Unable to extract args" check. Is there also a restriction on valid values for |cast_mode_num|? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:211: // before sending to MRPM, e.g. low-fps-mirror, user-override. Can you re-word this comment to be clearer - I think you're siggesting that we should add configuration of mode-specific parameters here...? Do we have a bug # for this follow-up work? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:214: // Users explicitly selected cast mode. nit: Users->User https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:223: // TODO(imcheng): Display error in UI. Do we have a bug filed for this follow-up work? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:230: Still doesn't look to be implemented..? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:236: NOTIMPLEMENTED(); Looks like this should be removed? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:248: if (dialog_closing_) How can we get OnCloseDialog if it's already closing? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:34: // Updates the list of sinks in the dialog. None of these comments actually add anything; they pretty much mirror the method names. Suggest a single comment "Methods to update the status displayed by the dialog." and then have all these methods in one block, with the exception of UpdateIssue, which needs the ownership of |issue| documenting. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:48: void UpdateIssue(const Issue* issue); Why const Issue* rather than const Issue& ?
PTAL https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:39: // Requests a media route from |source| to |sink_id|. On 2015/05/20 17:51:45, Wez wrote: > nit: Rephrase the comment to match the request->create renaming. > > Does this method now create a new route, or potentially re-connect an existing > one? Done. It only creates a new route. We will have a separate function for join. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:25: // Observes when issue to show in UI has been updated. On 2015/05/20 17:51:45, Wez wrote: > nit: Can we word this to be clearer; this class calls to refresh the UI when any > Issue is updated? Done according to the document in IssuesObserver. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:26: class UIIssuesObserver : public IssuesObserver { On 2015/05/20 17:51:45, Wez wrote: > nit: UiIssuesObserver Acknowledged. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:40: // Not owned by this class. On 2015/05/20 17:51:46, Wez wrote: > Rather than "not owned by" I'd suggest something like "Reference back to the > owning MediaRouterUI instance." here and below. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:69: ui_initialized_(false), On 2015/05/20 17:51:45, Wez wrote: > nit: is_initialized_? See other comment. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:81: html_source.release()); On 2015/05/20 17:51:45, Wez wrote: > nit: Blank line after this, for consistency. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:88: /* TODO(imcheng): Uncomment once Kevin's MediaRouterMojoImpl patch landed. On 2015/05/20 17:51:45, Wez wrote: > What's the bug # for that patch? > > Does this class make sense with a null |router_|..? If it's null then it will just crash at some point (DCHECK at the observers). It should be fine since nobody is using it yet. Alternatively I can wait for https://codereview.chromium.org/1143603004/ to land first and then uncomment this before landing this patch. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:95: routes_observer_.reset(new UIMediaRoutesObserver(router_, this)); On 2015/05/20 17:51:45, Wez wrote: > nit: Suggest a comment on this block "Register for Issue and MediaRoute > updates."? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:129: return DoCreateRoute(sink_id, GetPreferredCastMode(cast_modes_)); On 2015/05/20 17:51:46, Wez wrote: > Where does cast_modes_ ever get set..? In the Init* functions to be upstreamed separately. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:132: bool MediaRouterUI::CreateRouteWithCastModeOverride( On 2015/05/20 17:51:45, Wez wrote: > Why do we need the word "override"? i.e. not CreateRouteWithCastMode? See other comment. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:135: // |cast_mode_override| == |GetPreferredCastMode(cast_modes_)|. On 2015/05/20 17:51:45, Wez wrote: > Not sure what this comment is intended to achieve? It's for later when we pass in an override flag to the CreateRoute request. If user explicitly chose a cast mode and it is equal to the preferred cast mode, then we don't consider it as an override. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:149: // TODO(imcheng): Uncomment once Kevin's MediaRouterMojoImpl patch landed. On 2015/05/20 17:51:45, Wez wrote: > See above re bug #. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:176: // the MediaSource in MediaRouterUI if an update occurred between the user On 2015/05/20 17:51:45, Wez wrote: > It's not clear what "the MediaSource in MediaRouterUI" means - do you mean "the > source details displayed in the UI" or something else? > > Why is the fact that the source may differ relevant to this code - are you just > trying to explain the reason for calling GetSourceForCastMode(), for example? This describes a case that is theoretically possible but occurs very rarely in practice, where the MediaSource intended by the user is different than the MediaSource selected here. This is because the mapping from MediaCastMode to MediaSource is not static over the lifetime of the UI. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:185: base::Bind(&MediaRouterUI::OnRouteResponseReceived, On 2015/05/20 17:51:46, Wez wrote: > nit: Indentation - consider git cl format. :) Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:25: class MediaRouterUI : public ConstrainedWebDialogUI, On 2015/05/20 17:51:46, Wez wrote: > nit: MediaRouterUi would be the preferred name according to style-guide, though > I notice that Chromium is pretty inconsistent where UI is concerned; feel free > to ignore my UI->Ui comments elsewhere. :-/ Acknowledged. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:34: override; On 2015/05/20 17:51:46, Wez wrote: > Do we expect callers to invoke this? If not then consider moving it to the > private section; although the interface inheritance must always be public, it's > OK to "hide" interface methods that callers shouldn't need to think about. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:36: // Called by |routes_observer_| when the set of active routes has changed. On 2015/05/20 17:51:46, Wez wrote: > Would it make sense for this to be private and for the observer helper > sub-component to be a friend of this class? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:42: // Notifies that the UI has been initialized. On 2015/05/20 17:51:46, Wez wrote: > nit: Indentation? > > (BTW, you know about git cl format, right?) Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:42: // Notifies that the UI has been initialized. On 2015/05/20 17:51:46, Wez wrote: > Not clear from this comment who/what is being notified? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:43: void UIInitialized(); On 2015/05/20 17:51:46, Wez wrote: > nit: CamelCase applies even to acronyms & abbreviations, so UI->Ui here. > > OnUiInitialized? Acknowledged. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:46: // determined by the preferred MediaCastMode. On 2015/05/20 17:51:46, Wez wrote: > It's unclear in this context what "using MediaSink |sink_id|" means - do you > mean that we create a route from source content to |sink_id|, using the > preferred MediaCastMode (i.e. fling, tab-mirroring, etc)? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:49: // Returns true if a route request is successfully submitted. On 2015/05/20 17:51:46, Wez wrote: > Since a "success" result only indicates that the request has been made, not that > it's successfully completed, suggest rephrases this to "Returns false if unable > to request the route." then briefly describe what happens when the route request > completes. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:50: // One of the Init* functions must have been called before. On 2015/05/20 17:51:47, Wez wrote: > There are no Init* functions...? Init* functions are not upstreamed yet. I will remove this comment for now. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:54: // determined by |cast_mode_override|. On 2015/05/20 17:51:46, Wez wrote: > There seems to be some confusion between "mode" and "source" terminology here. > :( A cast mode maps to a source in the context of a MediaRouterUI instance. I changed the comments here. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:56: // One of the Init* functions must have been called before. On 2015/05/20 17:51:46, Wez wrote: > See above re Init* functions? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:63: // If the UI is already initialized, notifies |handler_| to update the UI. On 2015/05/20 17:51:46, Wez wrote: > nit: What does it do if the UI is not initialized? Suggest rewording, e.g. > "Updates the UI to display the new |issue|. Ignored if the UI is not yet > initialized." Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:66: // Calls MediaRouter to clear the given issue. On 2015/05/20 17:51:47, Wez wrote: > nit: What does it mean to "clear" the issue? Is that documented elsewhere? Yes, the MediaRouter API that's called by this is here: https://codereview.chromium.org/1143603004/diff/60001/chrome/browser/media/ro... It means action has been taken to resolve the issue and can be removed from the issue reporter's (i.e., component extension) record. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:70: // initially loaded. (i.e., user has not selected a cast mode override yet) On 2015/05/20 17:51:46, Wez wrote: > nit: You mean "e.g.", not "i.e.", I think? "e.g." means "for example", where > "i.e." means "in other words". It's really neither. So I will just reword it. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:80: // Callback from Media Router for a route creation request. On 2015/05/20 17:51:46, Wez wrote: > nit: "Callback passed to MediaRouter to receive response to route creation > requests." Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:84: bool DoCreateRoute(const MediaSinkId& sink_id, MediaCastMode cast_mode); On 2015/05/20 17:51:46, Wez wrote: > Do we need this? Why can't CreateRoute() just call CreateRouteWithCastMode() > with the preferred route, and have that contain the implementation? I think it's a bit more explicit this way. In the future we may also pass in a flag to the create route request that specifies a cast mode override was chosen. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:90: scoped_ptr<IssuesObserver> issues_observer_; On 2015/05/20 17:51:46, Wez wrote: > nit: Add a comment to this block of members to explain that they are non-null > while we're registered to receive notifications from those components. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:93: // Keeps track of whether initial settings have been requested. On 2015/05/20 17:51:46, Wez wrote: > initial settings for what? > requested by whom? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:94: bool ui_initialized_; On 2015/05/20 17:51:46, Wez wrote: > nit: is_initialized_? There are a couple of things to keep track of initialized here: this instance and the ui. This tracks the latter. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:96: // Set to |true| if there is a pending route request for this UI. On 2015/05/20 17:51:46, Wez wrote: > Does this basically mean it's true when we're trying to connect to the device to > start an activity? Yes. Since this class interfaces with media router, the documentation here uses media router terminology. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:103: std::string source_host_; On 2015/05/20 17:51:46, Wez wrote: > Do we need this variable if DEFAULT mode is not yet supported? Or can we add in > in the later CL that adds that mode? Do we have a bug # for that work? I can remove this for now. DEFAULT mode is already supported in private repo. I just wanted to upstream that separately. It uses the same bug # as this one since it's just part of the MediaRouterUI implementation. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:107: // Does not own this object. On 2015/05/20 17:51:46, Wez wrote: > This comment's pretty ambiguous - are you saying |router_| doesn't own |this|, > or |this| doesn't own |router_|, or something else? Isn't this just a bare > reference back to the global MediaRouter instance? Is the guarantee simply that > that will never be torn down until the browser shuts down? What guarantees that > |this| will be torn down before that point? How about "Not owned by this instance."? MediaRouter is a browser context keyed service. This instance is backed by a WebContents. Thus this will be destroyed before MediaRouter. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:24: // Callback names. On 2015/05/20 17:51:47, Wez wrote: > Are these actually callback names, or event/notification names? These are JS function names. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:31: void SetSinksListValue( On 2015/05/20 17:51:47, Wez wrote: > nit: As below, can you just have this accept the sinks and return a > scoped_ptr<base::ListValue>? It means one of your call-sites loses the stack > allocation optimization, but since the ListValue will be heap allocating stuff > internally, I doubt that that's a significant impact. (Famous last words...) Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:36: for (const MediaSinkWithCastModes& sink_with_cast_modes : sinks) { On 2015/05/20 17:51:47, Wez wrote: > nit: Suggest adding some blank lines in here to break the loop contents up into > logical chunks, for readability. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:49: void SetMediaRouteDictionary( On 2015/05/20 17:51:47, Wez wrote: > This name's not very descriptive - suggest e.g. MediaRouteDetailsToValue() and > have it return the DictionaryValue rather than modifying an existing one, since > that should work fine with your two current call-sites. Renamed to RouteToValue and similarly for others. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:59: base::ListValue* value) { On 2015/05/20 17:51:47, Wez wrote: > See comments on preceding methods re naming and scoped_ptr<> result Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:72: base::ListValue* value) { On 2015/05/20 17:51:47, Wez wrote: > Same here. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:136: On 2015/05/20 17:51:47, Wez wrote: > nit: Why the blank line here - you don't have one in the preceding methods? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:141: DVLOG(1) << "AddRoute"; On 2015/05/20 17:51:47, Wez wrote: > nit: Why does AddRoute deserve a DVLOG but not the other updates? Added DVLOG(2) to public methods. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:184: // Note that it is possible for this source to be different from On 2015/05/20 17:51:47, Wez wrote: > What is "this source"? Removed comment as it is duplicated with MRUI. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:197: << "valid sink ID. Aborting."; On 2015/05/20 17:51:47, Wez wrote: > nit: Consider folding this into the "Unable to extract args" check. > > Is there also a restriction on valid values for |cast_mode_num|? Yes. It is checked below. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:211: // before sending to MRPM, e.g. low-fps-mirror, user-override. On 2015/05/20 17:51:47, Wez wrote: > Can you re-word this comment to be clearer - I think you're siggesting that we > should add configuration of mode-specific parameters here...? > > Do we have a bug # for this follow-up work? Done. Added tracking bug. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:214: // Users explicitly selected cast mode. On 2015/05/20 17:51:47, Wez wrote: > nit: Users->User Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:223: // TODO(imcheng): Display error in UI. On 2015/05/20 17:51:47, Wez wrote: > Do we have a bug filed for this follow-up work? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:230: On 2015/05/20 17:51:47, Wez wrote: > Still doesn't look to be implemented..? Reverted. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:236: NOTIMPLEMENTED(); On 2015/05/20 17:51:47, Wez wrote: > Looks like this should be removed? Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:248: if (dialog_closing_) On 2015/05/20 17:51:47, Wez wrote: > How can we get OnCloseDialog if it's already closing? If we can manage to call it again before it is actually closed. One example is navigating to chrome://media-router and clicking on the close button. While it's not meaningful to do that, it's not really worth it to (and not sure if we can) handle this special case. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:34: // Updates the list of sinks in the dialog. On 2015/05/20 17:51:47, Wez wrote: > None of these comments actually add anything; they pretty much mirror the method > names. Suggest a single comment "Methods to update the status displayed by the > dialog." and then have all these methods in one block, with the exception of > UpdateIssue, which needs the ownership of |issue| documenting. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:48: void UpdateIssue(const Issue* issue); On 2015/05/20 17:51:47, Wez wrote: > Why const Issue* rather than const Issue& ? nullptr is used to indicate there are no more issues.
LGTM w/ nits. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:88: /* TODO(imcheng): Uncomment once Kevin's MediaRouterMojoImpl patch landed. On 2015/05/20 22:01:12, imcheng wrote: > On 2015/05/20 17:51:45, Wez wrote: > > What's the bug # for that patch? > > > > Does this class make sense with a null |router_|..? > > If it's null then it will just crash at some point (DCHECK at the observers). It > should be fine since nobody is using it yet. Alternatively I can wait for > https://codereview.chromium.org/1143603004/ to land first and then uncomment > this before landing this patch. Acknowledged. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:129: return DoCreateRoute(sink_id, GetPreferredCastMode(cast_modes_)); On 2015/05/20 22:01:12, imcheng wrote: > On 2015/05/20 17:51:46, Wez wrote: > > Where does cast_modes_ ever get set..? > > In the Init* functions to be upstreamed separately. Acknowledged. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:135: // |cast_mode_override| == |GetPreferredCastMode(cast_modes_)|. On 2015/05/20 22:01:12, imcheng wrote: > On 2015/05/20 17:51:45, Wez wrote: > > Not sure what this comment is intended to achieve? > > It's for later when we pass in an override flag to the CreateRoute request. If > user explicitly chose a cast mode and it is equal to the preferred cast mode, > then we don't consider it as an override. Is the distinction important? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:176: // the MediaSource in MediaRouterUI if an update occurred between the user On 2015/05/20 22:01:12, imcheng wrote: > On 2015/05/20 17:51:45, Wez wrote: > > It's not clear what "the MediaSource in MediaRouterUI" means - do you mean > "the > > source details displayed in the UI" or something else? > > > > Why is the fact that the source may differ relevant to this code - are you > just > > trying to explain the reason for calling GetSourceForCastMode(), for example? > > This describes a case that is theoretically possible but occurs very rarely in > practice, where the MediaSource intended by the user is different than the > MediaSource selected here. This is because the mapping from MediaCastMode to > MediaSource is not static over the lifetime of the UI. OK, so I think you need to make clear that what you're describing is basically a rarely-encountered bug that will mean the user doesn't get the behaviour they expected. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:107: // Does not own this object. On 2015/05/20 22:01:12, imcheng wrote: > On 2015/05/20 17:51:46, Wez wrote: > > This comment's pretty ambiguous - are you saying |router_| doesn't own |this|, > > or |this| doesn't own |router_|, or something else? Isn't this just a bare > > reference back to the global MediaRouter instance? Is the guarantee simply > that > > that will never be torn down until the browser shuts down? What guarantees > that > > |this| will be torn down before that point? > > How about "Not owned by this instance."? > > MediaRouter is a browser context keyed service. This instance is backed by a > WebContents. Thus this will be destroyed before MediaRouter. I'd suggest "Cached pointer to the MediaRouter for our BrowserContext.?" - WDYT? https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:141: DVLOG(1) << "AddRoute"; On 2015/05/20 22:01:13, imcheng wrote: > On 2015/05/20 17:51:47, Wez wrote: > > nit: Why does AddRoute deserve a DVLOG but not the other updates? > > Added DVLOG(2) to public methods. Acknowledged. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:248: if (dialog_closing_) On 2015/05/20 22:01:14, imcheng wrote: > On 2015/05/20 17:51:47, Wez wrote: > > How can we get OnCloseDialog if it's already closing? > > If we can manage to call it again before it is actually closed. One example is > navigating to chrome://media-router and clicking on the close button. While it's > not meaningful to do that, it's not really worth it to (and not sure if we can) > handle this special case. Acknowledged. https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:177: // reuqest and when this function is called. typo:request https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:134: // TODO(imcheng): Implement conversion from Issue to dictionary object. Bug #? https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:230: // TODO(imcheng): Implement. Bug#? https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:38: // Does not take ownership of |issue|. Note that |issue| can be nullptr. nit: Clarify _why_ |issue| may be nullptr.
https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:135: // |cast_mode_override| == |GetPreferredCastMode(cast_modes_)|. On 2015/05/21 22:58:36, Wez wrote: > On 2015/05/20 22:01:12, imcheng wrote: > > On 2015/05/20 17:51:45, Wez wrote: > > > Not sure what this comment is intended to achieve? > > > > It's for later when we pass in an override flag to the CreateRoute request. If > > user explicitly chose a cast mode and it is equal to the preferred cast mode, > > then we don't consider it as an override. > > Is the distinction important? Probably. We don't want to pass in that flag if the user didn't actually mean to override the source to be routed. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:176: // the MediaSource in MediaRouterUI if an update occurred between the user On 2015/05/21 22:58:36, Wez wrote: > On 2015/05/20 22:01:12, imcheng wrote: > > On 2015/05/20 17:51:45, Wez wrote: > > > It's not clear what "the MediaSource in MediaRouterUI" means - do you mean > > "the > > > source details displayed in the UI" or something else? > > > > > > Why is the fact that the source may differ relevant to this code - are you > > just > > > trying to explain the reason for calling GetSourceForCastMode(), for > example? > > > > This describes a case that is theoretically possible but occurs very rarely in > > practice, where the MediaSource intended by the user is different than the > > MediaSource selected here. This is because the mapping from MediaCastMode to > > MediaSource is not static over the lifetime of the UI. > > OK, so I think you need to make clear that what you're describing is basically a > rarely-encountered bug that will mean the user doesn't get the behaviour they > expected. Done. https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/1139203003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:107: // Does not own this object. On 2015/05/21 22:58:37, Wez wrote: > On 2015/05/20 22:01:12, imcheng wrote: > > On 2015/05/20 17:51:46, Wez wrote: > > > This comment's pretty ambiguous - are you saying |router_| doesn't own > |this|, > > > or |this| doesn't own |router_|, or something else? Isn't this just a bare > > > reference back to the global MediaRouter instance? Is the guarantee simply > > that > > > that will never be torn down until the browser shuts down? What guarantees > > that > > > |this| will be torn down before that point? > > > > How about "Not owned by this instance."? > > > > MediaRouter is a browser context keyed service. This instance is backed by a > > WebContents. Thus this will be destroyed before MediaRouter. > > I'd suggest "Cached pointer to the MediaRouter for our BrowserContext.?" - WDYT? Ok. But I replaced "our" with "this instance's". https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_ui.cc:177: // reuqest and when this function is called. On 2015/05/21 22:58:37, Wez wrote: > typo:request Done. https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:134: // TODO(imcheng): Implement conversion from Issue to dictionary object. On 2015/05/21 22:58:37, Wez wrote: > Bug #? Done. https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:230: // TODO(imcheng): Implement. On 2015/05/21 22:58:37, Wez wrote: > Bug#? Done. https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h (right): https://codereview.chromium.org/1139203003/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h:38: // Does not take ownership of |issue|. Note that |issue| can be nullptr. On 2015/05/21 22:58:37, Wez wrote: > nit: Clarify _why_ |issue| may be nullptr. Done.
The CQ bit was checked by imcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/1139203003/#ps160001 (title: "add back use of MediaRouterMojoImpl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139203003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bcb07449ce226e9263cf4f2637a1656377782e17 Cr-Commit-Position: refs/heads/master@{#331905}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/1162693002/ by dmurph@chromium.org. The reason for reverting is: Causing failures: https://code.google.com/p/chromium/issues/detail?id=493525. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
