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

Issue 2450433002: Let CastMediaBlocker observe MediaSession instead of WebContents (Closed)

Created:
4 years, 1 month ago by Zhiqiang Zhang (Slow)
Modified:
4 years, 1 month ago
Reviewers:
derekjchow1, halliwell
CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Let CastMediaBlocker observe MediaSession instead of WebContents This CL is part of a larger effort of decoupling MediaSession from WebContents. BUG=658678

Patch Set 1 #

Patch Set 2 : fixed tests #

Total comments: 8

Patch Set 3 : fixed test helper #

Patch Set 4 : fixed Linux build, truely... #

Patch Set 5 : addressed comments #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -95 lines) Patch
M chromecast/browser/cast_media_blocker.h View 1 2 3 4 2 chunks +9 lines, -18 lines 3 comments Download
M chromecast/browser/cast_media_blocker.cc View 1 2 3 4 7 chunks +14 lines, -22 lines 0 comments Download
M chromecast/browser/test/cast_media_blocker_test.cc View 1 2 3 4 12 chunks +64 lines, -54 lines 0 comments Download
M chromecast/browser/test/chromecast_browser_test_helper_default.cc View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download

Depends on Patchset:

Messages

Total messages: 9 (3 generated)
Zhiqiang Zhang (Slow)
PTAL https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast_media_blocker.cc File chromecast/browser/cast_media_blocker.cc (left): https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast_media_blocker.cc#oldcode18 chromecast/browser/cast_media_blocker.cc:18: base::Bind(&CastMediaBlocker::Resume, base::Unretained(this))) {} Are these callbacks used for ...
4 years, 1 month ago (2016-10-24 18:29:32 UTC) #3
derekjchow1
https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast_media_blocker.cc File chromecast/browser/cast_media_blocker.cc (left): https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast_media_blocker.cc#oldcode18 chromecast/browser/cast_media_blocker.cc:18: base::Bind(&CastMediaBlocker::Resume, base::Unretained(this))) {} On 2016/10/24 18:29:32, Zhiqiang Zhang wrote: ...
4 years, 1 month ago (2016-10-24 18:50:33 UTC) #4
Zhiqiang Zhang (Slow)
PTAL Got approval on the MediaSession-WebContents decoupling. We are good to go. https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast_media_blocker.cc File chromecast/browser/cast_media_blocker.cc ...
4 years, 1 month ago (2016-10-28 10:36:53 UTC) #5
derekjchow1
lgtm, but you'll need owners approval in chromecast/ to submit this change. Luke, could you ...
4 years, 1 month ago (2016-10-28 16:46:17 UTC) #7
halliwell
lgtm % nit https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast_media_blocker.h File chromecast/browser/cast_media_blocker.h (right): https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast_media_blocker.h#newcode21 chromecast/browser/cast_media_blocker.h:21: content::WebContentsObserver { This inheritance seems pointless ...
4 years, 1 month ago (2016-10-28 17:33:29 UTC) #8
derekjchow1
4 years, 1 month ago (2016-10-28 17:35:41 UTC) #9
https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:21: content::WebContentsObserver {
On 2016/10/28 17:33:29, halliwell wrote:
> This inheritance seems pointless now.  Derek, are you planning to handle the
> TODO here more or less immediately?

Yes, I requested this so I could synchronize changing our internal builds with
the public one.

Powered by Google App Engine
This is Rietveld 408576698