Issue 1159113006: [Android] A prototype of the interactive media notification.

Issue 1159113006: [Android] A prototype of the interactive media notification. (Closed)

Created:
5 years, 6 months ago by whywhat

Modified:
5 years, 5 months ago

Reviewers:
Ted C, qinmin, jochen (gone - plz use gerrit), mlamouri (slow - plz ping)

CC:
chromium-reviews, davve, jam, philipj_slow, richt

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[Android] A prototype of the interactive media notification. MediaSession notifies WebContentsDelegate when to show/hide the controls. ChromeTab's delegate implementation uses NotificationMediaPlaybackControls for the notification. Notification is implemented similar to the Cast one. WebContentsDelegate gets the necessary information from MediaSession via MediaControlsDelegate interface provided by WebContents. BUG=470600 TEST=Manual Committed: https://crrev.com/41634b16099cde386dc1090423f566bd4624070c Cr-Commit-Position: refs/heads/master@{#337844}

Patch Set 1 #

Patch Set 2 : Specify the right suspension type, use Cast notification layout, hide the notification when state c… #

Patch Set 3 : Only show broad domain and remove the listener #

Patch Set 4 : Updated notification style, fixed some issues. #

Patch Set 5 : Added browser tests #

Total comments: 50

Patch Set 6 : Addressed comments and nits #

Total comments: 2

Patch Set 7 : Fixed a few missed nits #

Total comments: 18

Patch Set 8 : Fixed Mounir's comments #

Total comments: 26

Patch Set 9 : Addressed the comments #

Total comments: 10

Patch Set 10 : Fixed tests and Min's nits #

Total comments: 51

Patch Set 11 : Fixed smaller comments from Ted #

Patch Set 12 : Forgot to upload a few style fixes #

Patch Set 13 : Forgot to add a file #

Patch Set 14 : Rebased the change #

Patch Set 15 : Moved the plumbing from WebContentsDelegate to WebContentsObserver #

Patch Set 16 : Fixed compile errors for chrome_public_apk #

Patch Set 17 : Updated the browser tests #

Patch Set 18 : Fixed compile on non-Android. Fixed incidental diffs. #

Patch Set 19 : Added some null checks and hiding the notification when the tab is destroyed. #

Total comments: 6

Patch Set 20 : Fixed Min's nits #

Total comments: 31

Patch Set 21 : Addressed Ted's comments #

Total comments: 7

Patch Set 22 : Removed unnecessary clear() call #

Patch Set 23 : Updated the paddings for the notification layout according to the UX feedback #

Total comments: 1

Patch Set 24 : Moved OnMediaSessionStateChanged to WebContentsImpl #

Created: 5 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1213 lines, -311 lines)			Patch
M	chrome/android/java/AndroidManifest.xml	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	1 chunk	+2 lines, -0 lines	0 comments	Download
A +	chrome/android/java/res/layout/playback_notification_bar.xml	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	4 chunks	+16 lines, -39 lines	0 comments	Download
M	chrome/android/java/res/values-v17/styles.xml	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+7 lines, -1 line	0 comments	Download
M	chrome/android/java/res/values-v21/styles.xml	View	1 2 3 4 5	1 chunk	+6 lines, -1 line	0 comments	Download
M	chrome/android/java/res/values/values.xml	View	1 2 3 4 5	1 chunk	+3 lines, -0 lines	0 comments	Download
A	chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+89 lines, -0 lines	0 comments	Download
A	chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackListener.java	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+20 lines, -0 lines	0 comments	Download
A	chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+129 lines, -0 lines	0 comments	Download
A	chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+269 lines, -0 lines	0 comments	Download
M	chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	3 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/android/java/strings/android_chrome_strings.grd	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	2 chunks	+14 lines, -0 lines	0 comments	Download
M	chrome/android/shell/java/AndroidManifest.xml.jinja2	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+4 lines, -0 lines	0 comments	Download
M	content/browser/android/web_contents_observer_proxy.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/browser/android/web_contents_observer_proxy.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+10 lines, -0 lines	0 comments	Download
M	content/browser/media/android/browser_media_player_manager.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+2 lines, -1 line	0 comments	Download
M	content/browser/media/android/media_session.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	5 chunks	+29 lines, -12 lines	0 comments	Download
M	content/browser/media/android/media_session.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	7 chunks	+44 lines, -6 lines	0 comments	Download
M	content/browser/media/android/media_session_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	4 chunks	+481 lines, -249 lines	0 comments	Download
M	content/browser/web_contents/web_contents_android.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+4 lines, -0 lines	0 comments	Download
M	content/browser/web_contents/web_contents_android.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	1 chunk	+8 lines, -0 lines	0 comments	Download
M	content/browser/web_contents/web_contents_impl.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	1 chunk	+4 lines, -0 lines	0 comments	Download
M	content/browser/web_contents/web_contents_impl.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	2 chunks	+20 lines, -0 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	2 chunks	+12 lines, -0 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+8 lines, -0 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content_public/browser/WebContents.java	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+10 lines, -0 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content_public/browser/WebContentsObserver.java	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+7 lines, -0 lines	0 comments	Download
M	content/public/browser/web_contents.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	2 chunks	+6 lines, -1 line	0 comments	Download
M	content/public/browser/web_contents_observer.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+4 lines, -0 lines	0 comments	Download

Dependent Patchsets:

Issue 1215713021 Patch 1

Issue 1215713021 Patch 20001

Messages

Total messages: 41 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mlamouri (slow - plz ping)

A few comments from a first pass. I haven't looked at everything very closely. https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java/res/layout/remote_notification_bar.xml ...

5 years, 6 months ago (2015-06-18 16:43:23 UTC) #5

whywhat

git cl upload is still doing something so I will update the CL when it ...

5 years, 6 months ago (2015-06-19 16:00:35 UTC) #6

mlamouri (slow - plz ping)

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java (right): https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java#newcode29 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:29: * The eTLD+1 of the media On 2015/06/19 at ...

5 years, 6 months ago (2015-06-22 14:34:17 UTC) #7

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java
(right):

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:29:
* The eTLD+1 of the media
On 2015/06/19 at 16:00:34, whywhat wrote:
> On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > I believe this is the origin, not eTLD+1.
> > 
> > Also, not the origin of the media but the origin of the top frame of the tab
containing the media.
> 
> Well, the origin doesn't have a simple definition, right?
https://tools.ietf.org/html/rfc6454#section-3.2
> 
> Since I get it from originForDisplay though, I could name it that.
> Yes, it's tab's/page's origin for display.

Origin has a fairly simple definition:
scheme + subdomains + domain + port.

Maybe getOriginForDisplay doesn't show the port?

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:529:
String originForDisplay = getUrl();
On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> That means that we show the origin of the top frame even if the sound comes
from an iframe, right? Is that what we want?

You didn't address that, did you? Maybe we should ask rolfe@ what she thinks
about it? I guess the user shouldn't be confused if they see "someband.com is
playing" while actually soundcloud.com is playing but I prefer to make sure.

https://codereview.chromium.org/1159113006/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/1159113006/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_web_contents_delegate_android.cc:368: jboolean
jis_paused = delegate->IsPaused();
On 2015/06/19 at 16:00:34, whywhat wrote:
> On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > If I understand correctly, the only reason why you have this IsPaused() call
is because you will call ShowMediaControls() to signal that there is a
play/pause state change, right? It seems that we could generalize that behaviour
by having simply a UpdateMediaControls() and have the method check with the
MediaControlsDelegate what should be done. Alternatively, we could have an
object hanging from WebContentsDelegate that would represent the Media Controls
so the MediaSession could directly tell it to Show(), Hide(), Update(), etc.
> 
> So why would you want to split Show and Update?
> This method currently does exactly what you describe as the first option. For
the alternative, WebContentsDelegate works like that object now with
MediaSession calling it directly when it needs to show/hide the notification.
> 
> I'd like to minimize cross components and Java/C++ interface so would like to
avoid Show/UpdateTitle/UpdateState/UpdateIcon/Hide style interface.

What would you think of having UpdateMediaControls() then have the
WebContentsDelegate use the MediaControlsDelegate to check whether the controls
should be paused, shown, etc. It might be a better signal if we want later to
update the title, etc. Instead of Show/Hide and Show being used for updates too,
we have one Update signal. WDYT?

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:533: if
(is_media_related_action)
On 2015/06/19 at 16:00:34, whywhat wrote:
> On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > When do you end up in that situation?
> 
> So it happens when I go to the notification and pause a single player media
session. The player's ReleaseMediaResources is called since the tab is invisible
and player is in the paused state (caused by the notification). Then it would
get removed from the session and the notification would get hidden.
> 
> See
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
where this is called with is_media_related_action is false.
> 
> I don't think we need to remove the notification if the player's resources
were released for some reason - we should update the status of the player in the
notification though and let the user try to resume it.

Ok.

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
content/browser/media/android/media_session.cc:13: 
On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> nit: remove empty line

double nit: you didn't do it.

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
content/browser/media/android/media_session.cc:123: }
On 2015/06/19 at 16:00:34, whywhat wrote:
> On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > Shouldn't that be in the ::OnSuspend(bool) method?
> 
> OnSuspend(bool) is called from the controls so it seems strange - the controls
would update the session and the session would update the controls in response.

Hmm, I see. Could you rename OnSuspend() to OnSuspendInternal() then? The intent
was that one method would bridge to the other, this is no longer the case so we
should avoid confusion with the name.

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
File content/browser/media/android/media_session.h (left):

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
content/browser/media/android/media_session.h:69: protected:
On 2015/06/19 at 16:00:34, whywhat wrote:
> On 2015/06/18 at 16:43:23, Mounir Lamouri wrote:
> > I had that so we had a very specific layer exposed to tests instead of
exposing the entire class. Why did you feel it wasn't needed anymore?
> 
> If you declare a friend class it has access to all class members. protected
doesn't have any effect here. These methods could be moved to the test class
actually :)

May I give you my resignation? :)

https://codereview.chromium.org/1159113006/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java
(right):

https://codereview.chromium.org/1159113006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:29:
* The origin (scheme + host) of the tab containing the media
Does it include the port? If not, just say "origin (without port)".

mlamouri (slow - plz ping)

Some more comments on the Java code. https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java (right): https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java#newcode18 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java:18: public abstract ...

5 years, 6 months ago (2015-06-23 14:58:57 UTC) #8

whywhat

5 years, 6 months ago (2015-06-23 19:39:11 UTC) #10

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java
(right):

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:29:
* The eTLD+1 of the media
On 2015/06/22 at 14:34:17, Mounir Lamouri wrote:
> On 2015/06/19 at 16:00:34, whywhat wrote:
> > On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > > I believe this is the origin, not eTLD+1.
> > > 
> > > Also, not the origin of the media but the origin of the top frame of the
tab containing the media.
> > 
> > Well, the origin doesn't have a simple definition, right?
https://tools.ietf.org/html/rfc6454#section-3.2
> > 
> > Since I get it from originForDisplay though, I could name it that.
> > Yes, it's tab's/page's origin for display.
> 
> Origin has a fairly simple definition:
> scheme + subdomains + domain + port.
> 
> Maybe getOriginForDisplay doesn't show the port?

It doesn't show the default port for https.

Just for the sake of argument (I changed the name to |origin|)

Still, the origin of a URI can be a globally unique identifier instead:
https://tools.ietf.org/html/rfc6454#section-4
E.g. for file or data URIs (which can still play audio and have our notification
show up).

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1159113006/diff/80001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:529:
String originForDisplay = getUrl();
On 2015/06/22 at 14:34:17, Mounir Lamouri wrote:
> On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > That means that we show the origin of the top frame even if the sound comes
from an iframe, right? Is that what we want?
> 
> You didn't address that, did you? Maybe we should ask rolfe@ what she thinks
about it? I guess the user shouldn't be confused if they see "someband.com is
playing" while actually soundcloud.com is playing but I prefer to make sure.

It would be confusing for the user to see "youtube.com" if they're on
"facebook.com". The page is what the user cares about I think. Let's ask Rebecca
indeed.

https://codereview.chromium.org/1159113006/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/1159113006/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_web_contents_delegate_android.cc:368: jboolean
jis_paused = delegate->IsPaused();
On 2015/06/22 at 14:34:17, Mounir Lamouri wrote:
> On 2015/06/19 at 16:00:34, whywhat wrote:
> > On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > > If I understand correctly, the only reason why you have this IsPaused()
call is because you will call ShowMediaControls() to signal that there is a
play/pause state change, right? It seems that we could generalize that behaviour
by having simply a UpdateMediaControls() and have the method check with the
MediaControlsDelegate what should be done. Alternatively, we could have an
object hanging from WebContentsDelegate that would represent the Media Controls
so the MediaSession could directly tell it to Show(), Hide(), Update(), etc.
> > 
> > So why would you want to split Show and Update?
> > This method currently does exactly what you describe as the first option.
For the alternative, WebContentsDelegate works like that object now with
MediaSession calling it directly when it needs to show/hide the notification.
> > 
> > I'd like to minimize cross components and Java/C++ interface so would like
to avoid Show/UpdateTitle/UpdateState/UpdateIcon/Hide style interface.
> 
> What would you think of having UpdateMediaControls() then have the
WebContentsDelegate use the MediaControlsDelegate to check whether the controls
should be paused, shown, etc. It might be a better signal if we want later to
update the title, etc. Instead of Show/Hide and Show being used for updates too,
we have one Update signal. WDYT?

Per offline discussion we agreed that there're two distinct use cases:
- when media session changes its active state (so that the embedder could
start/stop listening to media keys, setup/tear down the lock screen and software
keys via the notification, etc)
- when something relevant to the visible ui changes, e.g. title, icon, playback
state, etc

I think we could have a single method still to notify of any state change and
the delegate would then determine what changed and do the appropriate action.

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
content/browser/media/android/media_session.cc:13: 
On 2015/06/22 at 14:34:17, Mounir Lamouri wrote:
> On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > nit: remove empty line
> 
> double nit: you didn't do it.

Do you check the diff that you commented on? :)

https://codereview.chromium.org/1159113006/diff/80001/content/browser/media/a...
content/browser/media/android/media_session.cc:123: }
Ok.

On 2015/06/22 at 14:34:17, Mounir Lamouri wrote:
> On 2015/06/19 at 16:00:34, whywhat wrote:
> > On 2015/06/18 at 16:43:22, Mounir Lamouri wrote:
> > > Shouldn't that be in the ::OnSuspend(bool) method?
> > 
> > OnSuspend(bool) is called from the controls so it seems strange - the
controls would update the session and the session would update the controls in
response.
> 
> Hmm, I see. Could you rename OnSuspend() to OnSuspendInternal() then? The
intent was that one method would bridge to the other, this is no longer the case
so we should avoid confusion with the name.

https://codereview.chromium.org/1159113006/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java
(right):

https://codereview.chromium.org/1159113006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:29:
* The origin (scheme + host) of the tab containing the media
On 2015/06/22 at 14:34:17, Mounir Lamouri wrote:
> Does it include the port? If not, just say "origin (without port)".

It doesn't if the port is the default. Should just say the origin. Also, need to
switch it to use FormatUrlForSecurityDisplay.

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java:18:
public abstract class MediaPlaybackControls implements MediaPlaybackListener {
On 2015/06/23 at 14:58:56, Mounir Lamouri wrote:
> I think having that class here is great but are there plans to actually use it
outside of the consumer you are adding here?

You're right.

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java:47:
if (equal(mMediaInfo, mediaInfo)) return;
On 2015/06/23 at 14:58:56, Mounir Lamouri wrote:
> nit: mMediaInfo.equals(mediaInfo)

This would cause NPE if mMediaInfo is null.

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackControls.java:91:
private static boolean equal(Object a, Object b) {
On 2015/06/23 at 14:58:56, Mounir Lamouri wrote:
> Why do you need that?

See above :) I think we need a more generic equal method somewhere in
ApiCompatibility (see my Presentation API CL).

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackListener.java
(right):

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackListener.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2015/06/23 at 14:58:56, Mounir Lamouri wrote:
> 2015

Ack.

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackListener.java:10:
public interface MediaPlaybackListener {
On 2015/06/23 at 14:58:56, Mounir Lamouri wrote:
> I think you should only keep this interface if there is an obvious other
consumer. Otherwise, it sounds like a lot of indirection.

Ack.

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaPlaybackListener.java:15:
void onPlaybackStateChanged(boolean isPaused);
On 2015/06/23 at 14:58:56, Mounir Lamouri wrote:
> Maybe an int with final values would be better? PAUSE/PLAY.
onPlaybackStateChanged(true) doesn't mean anything unless you go look at that
file.

You suggested that I change from PlaybackState enum-like constants to a boolean
:) Now you don't like it here.
Maybe I should have onResumed/onPaused.

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:28:
*
On 2015/06/23 at 14:58:57, Mounir Lamouri wrote:
> nit: remove that line?

Done.

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:242:
.setContent(createContentView())
On 2015/06/23 at 14:58:57, Mounir Lamouri wrote:
> Why not using setContentTitle() and setContentText() instead?

It's how we achieve the desired consistent layout for Cast. We have a specific
view that has its own layout for title and text and icon and buttons. Otherwise,
it will look more native I guess but somewhat different on each Android version.
Need to run this through Rebecca after getting some screenshots :)

https://codereview.chromium.org/1159113006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:305:
getService().getPendingIntent(ListenerService.ACTION_ID_PLAY));
On 2015/06/23 at 14:58:56, Mounir Lamouri wrote:
> We might want to consider using addAction() in order to be compatible with
Android Wear.

Ack.

mlamouri (slow - plz ping)

lgtm with the comments applied. Thanks! :) https://codereview.chromium.org/1159113006/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java (right): https://codereview.chromium.org/1159113006/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java#newcode53 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:53: * @param ...

5 years, 6 months ago (2015-06-24 16:15:03 UTC) #12

whywhat

https://codereview.chromium.org/1159113006/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java (right): https://codereview.chromium.org/1159113006/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java#newcode53 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:53: * @param other the source. On 2015/06/24 at 16:15:02, ...

5 years, 6 months ago (2015-06-24 18:36:16 UTC) #13

qinmin

https://codereview.chromium.org/1159113006/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java (right): https://codereview.chromium.org/1159113006/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java#newcode16 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:16: public String title; final, and same for variables below ...

5 years, 6 months ago (2015-06-24 19:10:20 UTC) #14

whywhat

PTAL https://codereview.chromium.org/1159113006/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java (right): https://codereview.chromium.org/1159113006/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java#newcode16 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:16: public String title; On 2015/06/24 at 19:10:20, qinmin ...

5 years, 6 months ago (2015-06-25 10:10:52 UTC) #15

qinmin

https://codereview.chromium.org/1159113006/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1159113006/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java#newcode359 chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:359: private static boolean equal(Object a, Object b) { On ...

5 years, 5 months ago (2015-06-25 15:08:43 UTC) #16

whywhat

5 years, 5 months ago (2015-06-25 16:25:07 UTC) #17

Ted C

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java (right): https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java#newcode42 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:42: boolean isPaused, these won't all fit on a single ...

5 years, 5 months ago (2015-06-26 00:43:55 UTC) #18

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java
(right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:42:
boolean isPaused,
these won't all fit on a single line?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:34:
public static interface Listener {
I would call this MediaPlaybackListener so it is easier to grok out of context
of this file.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:56:
public static final int ACTION_ID_PLAY = 0;
I'm confused why there is this mapping?

Why not just

private static final String PLAY_ACTION =
        "NotificationMediaPlaybackControls.ListenerService.PLAY";
private static final String PAUSE_ACTION =
        "NotificationMediaPlaybackControls.ListenerService.PAUSE";

Why store the pending intents?  why not just generate them as needed?

This isn't going to be a very frequent occurrence, so that seems overkill.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:109:
getOrCreate(getApplicationContext()).onPlaybackStateChanged(false);
why getOrCreate here but you use sInstance above.  is the above a potential NPE
case?

I think if sInstance is null, you should treat it in the intent == null case at
the top.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:120:
private static NotificationMediaPlaybackControls sInstance = null;
null is the default, so don't need it.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:121:
private static final Object LOCK = new Object();
move the static finals above the plain ol' static.

I would consider moving these above the class definitions above.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:122:
private static final int MSG_UPDATE_NOTIFICATION = 100;
I would call this MSG_ID_UPDATE_...

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:130:
public static NotificationMediaPlaybackControls getOrCreate(Context context) {
Seems like a few places in the Tab logic would benefit from just get w/o create
(i.e. if hiding and one doesn't exist).

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:146:
sInstance.mService = service;
assert mService == null?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:162:
private Set<Listener> mListeners;
Should this be an observerList?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:163:
private MediaInfo mMediaInfo;
Can there ever only be one media info shown?  If so, we should have asserts in
place ensuring that.  Same for listeners?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:166:
this.mContext = context;
"this." is unnecessary

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:183:

should you be setting mMediaInfo = null here?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:194:
public void onPlaybackStateChanged(boolean isPaused) {
there seems to be a fair amount of public methods that aren't accessed from
outside this file.  Please reduce the scope as needed and ensure that all public
methods have javadoc.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:211:
Notification getNotification() {
why getNotification instead of just referencing mNotification (since it seems to
be scoped to just this file), same for getService() and getContext()

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:219:
private void onMediaInfoChanged() {
only called one place, inline it

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:242:
private void destroyNotification() {
For the MediaNotificationService, we found destroy to be pretty unreliable on
the service if swiped away.

So we had to call MediaNotificationService#clearMediaNotifications from
onDeferredStartup, so you might
need to do something like that as well.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:292:
if (mediaInfo == null) return;
Should you be destroying the notification in this case?  Should this ever
happen?  if not, assert here.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:357:
if (mListeners == null) mListeners = new CopyOnWriteArraySet<Listener>();
I would just initialize this in the constructor.  ensuring it is always
non-null.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1604: <message
name="IDS_MEDIA_PLAYBACK_NOTIFICATION_PLAYING_FOR_MEDIA" desc="Notification text
when playing media locally. [CHAR LIMIT=40]">
why locally?  where else would you be playing it?

We should probably clarify what type of notification this is going to be used
for.  Will it be a toast, etc..?  We should get more context to the translators.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1605: Playing “<ph
name="MEDIA_TITLE">%1$s<ex>Psy - Gangnam Style - YouTube</ex></ph>”
are those intentionally fancy " "'s?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:98:
public class ChromeTab extends Tab {
ChromeTab is going away, you should probably just add these to Tab.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:366:
private NotificationMediaPlaybackControls.Listener mControlsListener =
I'd rather not see this logic within tab.  I think we should pull it out as a
tabhelper of sorts and have it do this stuff.

I also don't think the methods you're calling right below belong in the web
contents delegate, so I think whatever extracted class should contain them (as
they only need the webcontents anyway, not any of the delegate functionality).

The class could be a webcontentsobserver as well, so it would know when to
delete itself.  And based on my comment about all the update paths potentially
going through the webcontentsobserver, it could maybe be isolated entirely
outside of this file.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:929:
notificationControls.hide(getId());
should this be removing the listener as well?

Should the listener be associated with the id (passed in the show(...) method
maybe) such that it gets removed when you hide an id?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/shell/j...
File chrome/android/shell/java/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/shell/j...
chrome/android/shell/java/AndroidManifest.xml.jinja2:221: <service
android:name="org.chromium.chrome.browser.media.ui.NotificationMediaPlaybackControl$ListenerService"
/>
exported="false" here?

https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...
content/public/browser/web_contents_delegate.h:410: virtual void
UpdateMediaControls(WebContents* web_contents) {}
Why does this exist here?

WebContentsObserver already has methods MediaStartedPlaying & MediaPaused.  I'm
not a big fan of having this exist in the delegate and the observer for very
similar functionality.

I would rather us make the observer more robust to ensure it covers the cases
you are trying to accomplish (or just ensuring the two above methods are called
when appropriate).

whywhat

Fixed the minor comments and replied with a suggestion and question about moving the logic ...

5 years, 5 months ago (2015-06-26 19:29:32 UTC) #19

Fixed the minor comments and replied with a suggestion and question about moving
the logic from Tab's WebContents delegate to some WebContentsObserver.

PTAL.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java
(right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaInfo.java:42:
boolean isPaused,
On 2015/06/26 at 00:43:53, Ted C wrote:
> these won't all fit on a single line?

Even in 80 characters :D

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:34:
public static interface Listener {
On 2015/06/26 at 00:43:53, Ted C wrote:
> I would call this MediaPlaybackListener so it is easier to grok out of context
of this file.

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:56:
public static final int ACTION_ID_PLAY = 0;
On 2015/06/26 at 00:43:54, Ted C wrote:
> I'm confused why there is this mapping?
> 
> Why not just
> 
> private static final String PLAY_ACTION =
>         "NotificationMediaPlaybackControls.ListenerService.PLAY";
> private static final String PAUSE_ACTION =
>         "NotificationMediaPlaybackControls.ListenerService.PAUSE";
> 
> Why store the pending intents?  why not just generate them as needed?
> 
> This isn't going to be a very frequent occurrence, so that seems overkill.

The notification may be recreated every second or so when/if we add a progress
bar to it.
Also, stripping the prefix from the intent's action is an easy way to dismiss
all actions we don't care about.
I agree it's an overkill for now. Updated the CL.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:109:
getOrCreate(getApplicationContext()).onPlaybackStateChanged(false);
On 2015/06/26 at 00:43:54, Ted C wrote:
> why getOrCreate here but you use sInstance above.  is the above a potential
NPE case?
> 
> I think if sInstance is null, you should treat it in the intent == null case
at the top.

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:120:
private static NotificationMediaPlaybackControls sInstance = null;
On 2015/06/26 at 00:43:54, Ted C wrote:
> null is the default, so don't need it.

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:121:
private static final Object LOCK = new Object();
On 2015/06/26 at 00:43:54, Ted C wrote:
> move the static finals above the plain ol' static.
> 
> I would consider moving these above the class definitions above.

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:122:
private static final int MSG_UPDATE_NOTIFICATION = 100;
On 2015/06/26 at 00:43:53, Ted C wrote:
> I would call this MSG_ID_UPDATE_...

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:130:
public static NotificationMediaPlaybackControls getOrCreate(Context context) {
On 2015/06/26 at 00:43:54, Ted C wrote:
> Seems like a few places in the Tab logic would benefit from just get w/o
create (i.e. if hiding and one doesn't exist).

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:146:
sInstance.mService = service;
On 2015/06/26 at 00:43:54, Ted C wrote:
> assert mService == null?

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:162:
private Set<Listener> mListeners;
On 2015/06/26 at 00:43:54, Ted C wrote:
> Should this be an observerList?

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:163:
private MediaInfo mMediaInfo;
On 2015/06/26 at 00:43:54, Ted C wrote:
> Can there ever only be one media info shown?  If so, we should have asserts in
place ensuring that.  Same for listeners?

Currently, yes and yes (one media info, one listener).
However, the hide request can come from the previous tab after the current one
set its own.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:166:
this.mContext = context;
On 2015/06/26 at 00:43:54, Ted C wrote:
> "this." is unnecessary

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:183:

On 2015/06/26 at 00:43:54, Ted C wrote:
> should you be setting mMediaInfo = null here?

done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:194:
public void onPlaybackStateChanged(boolean isPaused) {
On 2015/06/26 at 00:43:55, Ted C wrote:
> there seems to be a fair amount of public methods that aren't accessed from
outside this file.  Please reduce the scope as needed and ensure that all public
methods have javadoc.

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:292:
if (mediaInfo == null) return;
On 2015/06/26 at 00:43:54, Ted C wrote:
> Should you be destroying the notification in this case?  Should this ever
happen?  if not, assert here.

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:357:
if (mListeners == null) mListeners = new CopyOnWriteArraySet<Listener>();
On 2015/06/26 at 00:43:54, Ted C wrote:
> I would just initialize this in the constructor.  ensuring it is always
non-null.

Done.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1604: <message
name="IDS_MEDIA_PLAYBACK_NOTIFICATION_PLAYING_FOR_MEDIA" desc="Notification text
when playing media locally. [CHAR LIMIT=40]">
On 2015/06/26 at 00:43:55, Ted C wrote:
> why locally?  where else would you be playing it?

There're messages for remote media playback (aka Cast) above these.

> 
> We should probably clarify what type of notification this is going to be used
for.  Will it be a toast, etc..?  We should get more context to the translators.

I think we should use toast, etc in the description and notification should
refer to an Android notification. That seems to be consistent with other strings
around this one.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1605: Playing “<ph
name="MEDIA_TITLE">%1$s<ex>Psy - Gangnam Style - YouTube</ex></ph>”
On 2015/06/26 at 00:43:55, Ted C wrote:
> are those intentionally fancy " "'s?

Yes, same way as we do for Cast so that the tab title could be separated from
the rest of the text.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:98:
public class ChromeTab extends Tab {
On 2015/06/26 at 00:43:55, Ted C wrote:
> ChromeTab is going away, you should probably just add these to Tab.

Ack.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:366:
private NotificationMediaPlaybackControls.Listener mControlsListener =
On 2015/06/26 at 00:43:55, Ted C wrote:
> I'd rather not see this logic within tab.  I think we should pull it out as a
tabhelper of sorts and have it do this stuff.
> 
> I also don't think the methods you're calling right below belong in the web
contents delegate, so I think whatever extracted class should contain them (as
they only need the webcontents anyway, not any of the delegate functionality).
> 
> The class could be a webcontentsobserver as well, so it would know when to
delete itself.  And based on my comment about all the update paths potentially
going through the webcontentsobserver, it could maybe be isolated entirely
outside of this file.


Ack. I'll need a tab id though to be able to come back to the tab when the
notification is touched. How do I do this from WebContentsObserver?
MediaNotificationService sits fine here and I'm writing its replacement for
media playback case essentially.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:929:
notificationControls.hide(getId());
On 2015/06/26 at 00:43:55, Ted C wrote:
> should this be removing the listener as well?
> 
> Should the listener be associated with the id (passed in the show(...) method
maybe) such that it gets removed when you hide an id?

I've put the listener to MediaInfo so now there's only one listener that has a
corresponding tabId.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/shell/j...
File chrome/android/shell/java/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/shell/j...
chrome/android/shell/java/AndroidManifest.xml.jinja2:221: <service
android:name="org.chromium.chrome.browser.media.ui.NotificationMediaPlaybackControl$ListenerService"
/>
On 2015/06/26 at 00:43:55, Ted C wrote:
> exported="false" here?

Done.

https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...
content/public/browser/web_contents_delegate.h:410: virtual void
UpdateMediaControls(WebContents* web_contents) {}
On 2015/06/26 at 00:43:55, Ted C wrote:
> Why does this exist here?
> 
> WebContentsObserver already has methods MediaStartedPlaying & MediaPaused. 
I'm not a big fan of having this exist in the delegate and the observer for very
similar functionality.

So these methods are only used for Chromecast as far as I can see and we don't
really bind the media controls notification to a single player playing/paused.
So MediaSession from media_session.cc would call something like
WebContents::MediaSessionChanged() and that would notify the
WebContentsObservers that would use the MediaControlsDelegate to get what they
need and update the controls.

WDYT?
Where would I initialize such an observer in Chrome for Android?

> 
> I would rather us make the observer more robust to ensure it covers the cases
you are trying to accomplish (or just ensuring the two above methods are called
when appropriate).

Ted C

@jam -- I'm looking for your thoughts on how best we should be adding this ...

5 years, 5 months ago (2015-06-30 04:56:13 UTC) #21

@jam -- I'm looking for your thoughts on how best we should be adding this
plumbing with regards to the content layer.

This file is the one where the discussion is going on:
https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...

My understanding is:
1.) MediaSession needs to update the chrome layer to display a notification to
control the media
https://codereview.chromium.org/1159113006/diff/260001/content/browser/media/...

2.) The chrome layer needs a way to communicate back to the content layer to
pause/play the media:

Control interface in content/public:
https://codereview.chromium.org/1159113006/diff/260001/content/public/browser...

Which is exposed in:
https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...

------

At one extreme, I think both sides could be registered as SupportsUserData, but
that seems like might be too obscure.

Otherwise, just trying to figure out whether the methods should be exposed on
WebContentsObserver or WebContentsDelegate.

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java
(right):

https://codereview.chromium.org/1159113006/diff/180001/chrome/android/java_st...
chrome/android/java_staging/src/org/chromium/chrome/browser/tab/ChromeTab.java:366:
private NotificationMediaPlaybackControls.Listener mControlsListener =
On 2015/06/26 19:29:32, whywhat wrote:
> On 2015/06/26 at 00:43:55, Ted C wrote:
> > I'd rather not see this logic within tab.  I think we should pull it out as
a
> tabhelper of sorts and have it do this stuff.
> > 
> > I also don't think the methods you're calling right below belong in the web
> contents delegate, so I think whatever extracted class should contain them (as
> they only need the webcontents anyway, not any of the delegate functionality).
> > 
> > The class could be a webcontentsobserver as well, so it would know when to
> delete itself.  And based on my comment about all the update paths potentially
> going through the webcontentsobserver, it could maybe be isolated entirely
> outside of this file.
> 
> 
> Ack. I'll need a tab id though to be able to come back to the tab when the
> notification is touched. How do I do this from WebContentsObserver?
> MediaNotificationService sits fine here and I'm writing its replacement for
> media playback case essentially.

Per my follow up comment, there is no reason you can't create a specialized
observer that takes a Tab (or you can use a TabObserver if you prefer).

I wouldn't take MediaNotificationService as a good example of what to follow
with regards to how it integrates with the Tab.  It is littered all over the
file and is definitely an example of something we'd like to see rewritten and
extracted from the Tab.

https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/1159113006/diff/180001/content/public/browser...
content/public/browser/web_contents_delegate.h:410: virtual void
UpdateMediaControls(WebContents* web_contents) {}
On 2015/06/26 19:29:32, whywhat wrote:
> On 2015/06/26 at 00:43:55, Ted C wrote:
> > Why does this exist here?
> > 
> > WebContentsObserver already has methods MediaStartedPlaying & MediaPaused. 
> I'm not a big fan of having this exist in the delegate and the observer for
very
> similar functionality.
> 
> So these methods are only used for Chromecast as far as I can see and we don't
> really bind the media controls notification to a single player playing/paused.
> So MediaSession from media_session.cc would call something like
> WebContents::MediaSessionChanged() and that would notify the
> WebContentsObservers that would use the MediaControlsDelegate to get what they
> need and update the controls.

The problem I have with it being in both WebContentsDelegate and
WebContentsObserver is that embedders of the content layer have no real idea of
when to use one or the other.

If the existing ones are in WebContentsObserver, I would rather be consistent
and add further clarification of when to use one over the other.

> 
> WDYT?
> Where would I initialize such an observer in Chrome for Android?

You would initialize it in Tab/ChromeTab.  There are already a few examples of
an self contained observer that is a good example of what I'm referring to.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

That is really the example of how I would like to see the chrome side
implemented.

Even if the WebContentsDelegate approach is what needs to be done, you could add
a TabObserver method to still keep the bulk of the code out of the tab logic
itself.

> 
> > 
> > I would rather us make the observer more robust to ensure it covers the
cases
> you are trying to accomplish (or just ensuring the two above methods are
called
> when appropriate).
>

whywhat

I moved the logic out to MediaSessionTabHelper and changed the plumbing to WebContentsObserver instead of ...

5 years, 5 months ago (2015-06-30 21:06:34 UTC) #22

whywhat

I updated the browser tests to check that WebContentsObserver is called when needed. PTAL.

5 years, 5 months ago (2015-07-02 19:30:28 UTC) #23

qinmin

lgtm lgtm % nits I will leave the WebContentsObserver/WebContentsDelegate issue to jam https://codereview.chromium.org/1159113006/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java ...

5 years, 5 months ago (2015-07-04 19:30:46 UTC) #24

whywhat

PTAL https://codereview.chromium.org/1159113006/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java (right): https://codereview.chromium.org/1159113006/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java#newcode206 chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:206: // destroyNotification(). This is done to avoid the ...

5 years, 5 months ago (2015-07-06 17:38:24 UTC) #25

Ted C

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java#newcode48 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:48: int tabId = mTab == null ? Tab.INVALID_TAB_ID : ...

5 years, 5 months ago (2015-07-06 22:18:29 UTC) #26

whywhat

PTAL! https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java#newcode48 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:48: int tabId = mTab == null ? Tab.INVALID_TAB_ID ...

5 years, 5 months ago (2015-07-07 15:44:58 UTC) #27

PTAL!

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:48:
int tabId = mTab == null ? Tab.INVALID_TAB_ID : mTab.getId();
On 2015/07/06 at 22:18:28, Ted C wrote:
> you want to call super.destroy() as well.

Oops, didn't expect any of the WebContentsObserver methods to be anything but
no-op :)

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:63:
Log.e(TAG, "Wrong syntax for the tab URL when playing media. Showing the URL.");
On 2015/07/06 at 22:18:28, Ted C wrote:
> I would say, "Unable to parse the origin from the URL.  Showing the full URL
instead."

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:79:
if (mWebContents != null && mWebContentsObserver != null) {
On 2015/07/06 at 22:18:28, Ted C wrote:
> here, just check mWebContentsObserver != null and then call .destroy() on it.

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:91:
// Usually means that tab's web contents changed.
On 2015/07/06 at 22:18:28, Ted C wrote:
> this also happens when you navigate to a native page or to a renderer drawn
page (if on a native page).

Isn't this included in "web contents changed" as in changed from/to null?

> 
> You could just do:
> 
> if (mWebContents == tab.getWebContents()) return;
> 
> Then you do not need to destroy and recreate it.

Done. I still need to detach and attach the observer to it though, right? Why
would the web contents be equal when onContentChanged is called? Is it called
anytime some state of WebContents changes?

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:96:
mWebContents.addObserver(mWebContentsObserver);
Also wasn't needed - WebContentsObserver adds itself to the WebContents.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:114:
mTab.addObserver(mTabObserver);
On 2015/07/06 at 22:18:28, Ted C wrote:
> For safety sake, I would do
> 
> if (tab.getWebContents() != null) {
>     // create web contents observer here too...
> }
> 
> Then you can handle the case where the tab has been initialized prior to
calling this.

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:29:
private static final int MSG_ID_UPDATE_NOTIFICATION = 100;
On 2015/07/06 at 22:18:28, Ted C wrote:
> not used

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:61:
public void onDestroy() {
On 2015/07/06 at 22:18:29, Ted C wrote:
> should you call super.onDestroy() here as well?

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:169:
if (mMediaInfo == null || (tabId != Tab.INVALID_TAB_ID && tabId !=
mMediaInfo.tabId)) {
On 2015/07/06 at 22:18:28, Ted C wrote:
> If you have a problem with a dangling notification (i.e. chrome killed due to
OOM without calling proper service shutdown), then mMediaInfo will be null when
you call this on startup.
> 
> What happens if you have a notification and you just do kill <chrome browser
PID> from adb shell?  Does the notification get cleared?
> 
> I think if you get INVALID_TAB_ID, you should skip any other checks.  Or it
would probably just be clearer to have an explicit clearNotification that does
not take a param and calls destroyNotification explicitly.

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:195:
NotificationCompat.Builder notificationBuilder =
On 2015/07/06 at 22:18:28, Ted C wrote:
> I would just inline this in updateNotification.  You can just check if
mNotification is null there and create it.

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:214:
private final Context getContext() {
On 2015/07/06 at 22:18:28, Ted C wrote:
> remove this and just inline mContext everywhere.

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:221:
mMediaInfo.origin != null ? mMediaInfo.origin : "");
On 2015/07/06 at 22:18:28, Ted C wrote:
> You should probably have some Unknown URL or something if there is no origin
right?

Done.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:274:
manager.notify(R.id.media_playback_notification, mNotification);
On 2015/07/06 at 22:18:28, Ted C wrote:
> I would use a notification namespace here (and it's needed in cancel).
> 
>
https://developer.android.com/reference/android/support/v4/app/NotificationMa...,
int, android.app.Notification)
> 
> Should be fine to use the id, but the namespace should guarantee no conflicts
in case someone else was using an incrementing number outside of the id
allocation.

So we actually don't need NotificationManager at all it seems - startForeground
and stopSelf show and hide notifications for the service already. These don't
take namespace arguments though :(

When I added a namespace to notify/cancel, I've got two identical notifications,
one coming from the manager with the namespace and another one from
startForeground...

https://codereview.chromium.org/1159113006/diff/380001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1159113006/diff/380001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:509: 
On 2015/07/06 at 22:18:29, Ted C wrote:
> remove blank line?

Done.

whywhat

+jochen, cc:jam (OOO) Jochen, could you take a look at this cl instead of John? ...

5 years, 5 months ago (2015-07-07 15:59:31 UTC) #29

Ted C

Overall looks good...one question about the need for clear, but otherwise seems ready to go. ...

5 years, 5 months ago (2015-07-07 17:11:19 UTC) #30

Overall looks good...one question about the need for clear, but otherwise seems
ready to go.

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:91:
// Usually means that tab's web contents changed.
On 2015/07/07 15:44:57, whywhat wrote:
> On 2015/07/06 at 22:18:28, Ted C wrote:
> > this also happens when you navigate to a native page or to a renderer drawn
> page (if on a native page).
> 
> Isn't this included in "web contents changed" as in changed from/to null?

No, the webcontents only changes for a tab when you prerender and it
gets swapped in (maybe reader mode as well), but going to a native
page doesn't actually change the web contents.

> 
> > 
> > You could just do:
> > 
> > if (mWebContents == tab.getWebContents()) return;
> > 
> > Then you do not need to destroy and recreate it.
> 
> Done. I still need to detach and attach the observer to it though, right? Why
> would the web contents be equal when onContentChanged is called? Is it called
> anytime some state of WebContents changes?

Per my comment above, the web contents actually changes very infrequently and
onContentChanged is really onTabView changed which can either be from a new
ContentViewCore or switching to a native page such as the NTP.

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:118:
sInstance.clearNotification();
Hmm...with startForeground is this actually necessary?  I've never used that API
before and maybe that is the secret sauce that ensures notifications are
actually cleaned up when the service dies in android.

Have you tested killing the browser process via ADB?  Does the notification go
away?

Pre-L if you swipe away a tabbed mode activity from recents does it go away? 
Same in L (if you have only a single document or are in tabbed mode)?

Since the clear call is gated on sInstance being non-null, this will only work
if you had previously called show on an notification, which will probably never
be the case on startup where this is called.

I'm just thinking that maybe you don't actually need this at all, but you should
test all possible cases of dismissing/killing chrome to verify that hypothesis
(although as written I don't see this actually doing anything on startup and
would need to be changed to not do an sInstance check).

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:227:
if (mNotification == null) {
should you move this down below the two early returns below?

https://codereview.chromium.org/1159113006/diff/380002/content/browser/media/...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1159113006/diff/380002/content/browser/media/...
content/browser/media/android/media_session.cc:137: return audio_focus_state_ !=
State::Suspended &&
Why does Suspended determine whether it is controllable?  Maybe that is just
unclear by the naming and at first glance, it being TemporarilySuspended doesn't
really clarify that was something that a user could have done.

Might be worth adding some documentation to the states to help the likes of me
understand this kind of implications in the future.

whywhat

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java#newcode91 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:91: // Usually means that tab's web contents changed. On ...

5 years, 5 months ago (2015-07-07 19:19:04 UTC) #31

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:91:
// Usually means that tab's web contents changed.
On 2015/07/07 at 17:11:19, Ted C wrote:
> On 2015/07/07 15:44:57, whywhat wrote:
> > On 2015/07/06 at 22:18:28, Ted C wrote:
> > > this also happens when you navigate to a native page or to a renderer
drawn
> > page (if on a native page).
> > 
> > Isn't this included in "web contents changed" as in changed from/to null?
> 
> No, the webcontents only changes for a tab when you prerender and it
> gets swapped in (maybe reader mode as well), but going to a native
> page doesn't actually change the web contents.
> 
> > 
> > > 
> > > You could just do:
> > > 
> > > if (mWebContents == tab.getWebContents()) return;
> > > 
> > > Then you do not need to destroy and recreate it.
> > 
> > Done. I still need to detach and attach the observer to it though, right?
Why
> > would the web contents be equal when onContentChanged is called? Is it
called
> > anytime some state of WebContents changes?
> 
> Per my comment above, the web contents actually changes very infrequently and
onContentChanged is really onTabView changed which can either be from a new
ContentViewCore or switching to a native page such as the NTP.

I think I understand now but just to be clear - there's nothing to fix here in
the code? The case when web contents is the same happens more often than not, so
having this early return is important.

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:118:
sInstance.clearNotification();
On 2015/07/07 at 17:11:19, Ted C wrote:
> Hmm...with startForeground is this actually necessary?  I've never used that
API before and maybe that is the secret sauce that ensures notifications are
actually cleaned up when the service dies in android.
> 
> Have you tested killing the browser process via ADB?  Does the notification go
away?
> 
> Pre-L if you swipe away a tabbed mode activity from recents does it go away? 
Same in L (if you have only a single document or are in tabbed mode)?
> 
> Since the clear call is gated on sInstance being non-null, this will only work
if you had previously called show on an notification, which will probably never
be the case on startup where this is called.
> 
> I'm just thinking that maybe you don't actually need this at all, but you
should test all possible cases of dismissing/killing chrome to verify that
hypothesis (although as written I don't see this actually doing anything on
startup and would need to be changed to not do an sInstance check).

I seem not to need it in DeferredStartupHandler. All the tests you mentioned
were fine. However, one case doesn't work as I'd expect - if in tabbed activity
mode I close/swipe the tab away in the tab switcher, the notification stays
until the Undo snackbar disappears.

Do you have an idea what event should I listen for to catch this case?

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:227:
if (mNotification == null) {
On 2015/07/07 at 17:11:19, Ted C wrote:
> should you move this down below the two early returns below?

Done.

https://codereview.chromium.org/1159113006/diff/380002/content/browser/media/...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1159113006/diff/380002/content/browser/media/...
content/browser/media/android/media_session.cc:137: return audio_focus_state_ !=
State::Suspended &&
On 2015/07/07 at 17:11:19, Ted C wrote:
> Why does Suspended determine whether it is controllable?  Maybe that is just
unclear by the naming and at first glance, it being TemporarilySuspended doesn't
really clarify that was something that a user could have done.
> 
> Might be worth adding some documentation to the states to help the likes of me
understand this kind of implications in the future.

So for the Content type, "Active" and "SuspendedTemporarily" states correspond
to playing and paused accordingly. "Suspended" means stopped.
I guess the terminology comes from the WebAudio's AudioContext. Mounir would
know better :)

Ted C

lgtm https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java#newcode91 chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:91: // Usually means that tab's web contents changed. ...

5 years, 5 months ago (2015-07-07 20:04:12 UTC) #33

lgtm

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1159113006/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:91:
// Usually means that tab's web contents changed.
On 2015/07/07 19:19:04, whywhat wrote:
> On 2015/07/07 at 17:11:19, Ted C wrote:
> > On 2015/07/07 15:44:57, whywhat wrote:
> > > On 2015/07/06 at 22:18:28, Ted C wrote:
> > > > this also happens when you navigate to a native page or to a renderer
> drawn
> > > page (if on a native page).
> > > 
> > > Isn't this included in "web contents changed" as in changed from/to null?
> > 
> > No, the webcontents only changes for a tab when you prerender and it
> > gets swapped in (maybe reader mode as well), but going to a native
> > page doesn't actually change the web contents.
> > 
> > > 
> > > > 
> > > > You could just do:
> > > > 
> > > > if (mWebContents == tab.getWebContents()) return;
> > > > 
> > > > Then you do not need to destroy and recreate it.
> > > 
> > > Done. I still need to detach and attach the observer to it though, right?
> Why
> > > would the web contents be equal when onContentChanged is called? Is it
> called
> > > anytime some state of WebContents changes?
> > 
> > Per my comment above, the web contents actually changes very infrequently
and
> onContentChanged is really onTabView changed which can either be from a new
> ContentViewCore or switching to a native page such as the NTP.
> 
> I think I understand now but just to be clear - there's nothing to fix here in
> the code? The case when web contents is the same happens more often than not,
so
> having this early return is important.

Correct, nothing to change.  Just wanted to fully explain it.

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java
(right):

https://codereview.chromium.org/1159113006/diff/380002/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/NotificationMediaPlaybackControls.java:118:
sInstance.clearNotification();
On 2015/07/07 19:19:04, whywhat wrote:
> On 2015/07/07 at 17:11:19, Ted C wrote:
> > Hmm...with startForeground is this actually necessary?  I've never used that
> API before and maybe that is the secret sauce that ensures notifications are
> actually cleaned up when the service dies in android.
> > 
> > Have you tested killing the browser process via ADB?  Does the notification
go
> away?
> > 
> > Pre-L if you swipe away a tabbed mode activity from recents does it go away?

> Same in L (if you have only a single document or are in tabbed mode)?
> > 
> > Since the clear call is gated on sInstance being non-null, this will only
work
> if you had previously called show on an notification, which will probably
never
> be the case on startup where this is called.
> > 
> > I'm just thinking that maybe you don't actually need this at all, but you
> should test all possible cases of dismissing/killing chrome to verify that
> hypothesis (although as written I don't see this actually doing anything on
> startup and would need to be changed to not do an sInstance check).
> 
> I seem not to need it in DeferredStartupHandler. All the tests you mentioned
> were fine. However, one case doesn't work as I'd expect - if in tabbed
activity
> mode I close/swipe the tab away in the tab switcher, the notification stays
> until the Undo snackbar disappears.
> 
> Do you have an idea what event should I listen for to catch this case?

The tab isn't actually fully closed until the snackbar disappears, so I wouldn't
really worry about this case.

If you really do care, you can listen to TabObserver#onClosingStateChanged, but
I think it's overkill in this case.

jochen (gone - plz use gerrit)

why not put the methods on the MediaObserver interface?

5 years, 5 months ago (2015-07-08 13:37:26 UTC) #34

whywhat

On 2015/07/08 at 13:37:26, jochen wrote: > why not put the methods on the MediaObserver ...

5 years, 5 months ago (2015-07-08 13:48:04 UTC) #35

jochen (gone - plz use gerrit)

lgtm with comment addressed https://codereview.chromium.org/1159113006/diff/430001/content/public/browser/web_contents.h File content/public/browser/web_contents.h (right): https://codereview.chromium.org/1159113006/diff/430001/content/public/browser/web_contents.h#newcode665 content/public/browser/web_contents.h:665: virtual void OnMediaSessionStateChanged() = 0; ...

5 years, 5 months ago (2015-07-08 15:35:57 UTC) #36

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159113006/450001

5 years, 5 months ago (2015-07-08 16:09:09 UTC) #39

commit-bot: I haz the power

5 years, 5 months ago (2015-07-08 17:08:25 UTC) #41

Message was sent while issue was closed.

Patchset 24 (id:??) landed as
https://crrev.com/41634b16099cde386dc1090423f566bd4624070c
Cr-Commit-Position: refs/heads/master@{#337844}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages