Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(685)

Issue 2970273002: [Media Router] Remove the extension ID getter from MRMojoImpl (Closed)

Created:
3 years, 5 months ago by takumif
Modified:
3 years, 5 months ago
Reviewers:
imcheng, sheretov
CC:
chromium-reviews, imcheng+watch_chromium.org, Aaron Boodman, chfremer+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, feature-media-reviews_chromium.org, darin (slow to review), mfoltz+watch_chromium.org, qsr+mojo_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Media Router] Remove the extension ID getter from MRMojoImpl This CL removes the media_route_provider_extension_id() getter from MediaRouterMojoImpl, which was previously being accessed by static-casting MediaRouter into MediaRouterMojoImpl. The getter in EventPageRequestManager is now used. It seems that we cannot simply hard-code the ID, since it's different between dev and release builds of the component extension. BUG=597778 BUG=727993 Review-Url: https://codereview.chromium.org/2970273002 Cr-Commit-Position: refs/heads/master@{#485439} Committed: https://chromium.googlesource.com/chromium/src/+/bae32143ddf7c4cf69f0f71e260a194648e0b27b

Patch Set 1 #

Patch Set 2 : . #

Total comments: 4

Patch Set 3 : Address Derek's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -31 lines) Patch
M chrome/browser/media/router/mojo/media_router_mojo_impl.h View 1 2 chunks +1 line, -4 lines 0 comments Download
M chrome/browser/media/router/mojo/media_router_mojo_impl.cc View 1 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/ui/toolbar/media_router_contextual_menu.cc View 2 chunks +10 lines, -10 lines 0 comments Download
M chrome/browser/ui/webui/cast/cast_ui.cc View 1 2 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_ui.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_ui.cc View 1 3 chunks +7 lines, -5 lines 0 comments Download

Messages

Total messages: 23 (17 generated)
takumif
Please take a look at: imcheng: overall sheretov: cast_ui.cc Thanks!
3 years, 5 months ago (2017-07-06 20:22:55 UTC) #5
imcheng
lgtm https://codereview.chromium.org/2970273002/diff/20001/chrome/browser/media/router/mojo/media_router_mojo_impl.h File chrome/browser/media/router/mojo/media_router_mojo_impl.h (left): https://codereview.chromium.org/2970273002/diff/20001/chrome/browser/media/router/mojo/media_router_mojo_impl.h#oldcode117 chrome/browser/media/router/mojo/media_router_mojo_impl.h:117: // TODO(crbug.com/597778): Remove this getter. Seems this patch ...
3 years, 5 months ago (2017-07-07 21:15:43 UTC) #6
takumif
https://codereview.chromium.org/2970273002/diff/20001/chrome/browser/media/router/mojo/media_router_mojo_impl.h File chrome/browser/media/router/mojo/media_router_mojo_impl.h (left): https://codereview.chromium.org/2970273002/diff/20001/chrome/browser/media/router/mojo/media_router_mojo_impl.h#oldcode117 chrome/browser/media/router/mojo/media_router_mojo_impl.h:117: // TODO(crbug.com/597778): Remove this getter. On 2017/07/07 21:15:43, imcheng ...
3 years, 5 months ago (2017-07-07 21:58:58 UTC) #9
sheretov
LGTM for chrome/browser/ui/webui
3 years, 5 months ago (2017-07-10 20:57:03 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2970273002/40001
3 years, 5 months ago (2017-07-10 23:17:36 UTC) #20
commit-bot: I haz the power
3 years, 5 months ago (2017-07-10 23:24:28 UTC) #23
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/bae32143ddf7c4cf69f0f71e260a...

Powered by Google App Engine
This is Rietveld 408576698