[Media>UI] Don't call onServiceStarted when handling a notification action

[Media>UI] Don't call onServiceStarted when handling a notification action

There's an issue where when we receive a PAUSE intent fired from media
notification, we still call MediaNotificationManager.onServiceStarted(),
which will update the notification and call Service.startForeground().
When the PAUSE action is executed, we then update the notification again
and call Service.stopForeground(). These two calls are called in a very
short time window so that a race condition in Android will keep the
notification foreground and unswippable.

In this CL, we distinguish start service intents so that we don't update
the notification when receiving an action.

BUG=701731
R=mlamouri@chromium.org

Review-Url: https://codereview.chromium.org/2750973004
Cr-Commit-Position: refs/heads/master@{#457495}
Committed: https://chromium.googlesource.com/chromium/src/+/cba3aa3a044b578b02a8c237f0bde012260fcf9e

[mlamouri (slow - plz ping), 2017-03-16 12:03:51]

lgtm but it would be great to have a test to make sure we don't regress this :)

https://codereview.chromium.org/2750973004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2750973004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:193:
if (intent.getAction() == null) {
Can you add a comment here explaining why you are doing this?

[Zhiqiang Zhang (Slow), 2017-03-16 15:11:58]

On 2017/03/16 at 12:03:51, mlamouri wrote:
> lgtm but it would be great to have a test to make sure we don't regress this
:)

It is hard to test, as it involves Service :(
I'll do more manual testing before landing this.

[mlamouri (slow - plz ping), 2017-03-16 17:36:24]

On 2017/03/16 at 15:11:58, zqzhang wrote:
> On 2017/03/16 at 12:03:51, mlamouri wrote:
> > lgtm but it would be great to have a test to make sure we don't regress this
:)
> 
> It is hard to test, as it involves Service :(
> I'll do more manual testing before landing this.

sg. let's land the fix then :)

[Zhiqiang Zhang (Slow), 2017-03-16 17:36:39]

https://codereview.chromium.org/2750973004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2750973004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:193:
if (intent.getAction() == null) {
On 2017/03/16 at 12:03:51, mlamouri wrote:
> Can you add a comment here explaining why you are doing this?

Done.

[Zhiqiang Zhang (Slow), 2017-03-16 17:41:06]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-16 17:41:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2750973004/#ps20001
(title: "added comments")

[commit-bot: I haz the power, 2017-03-16 17:42:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 18:20:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489686066819670,
"parent_rev": "e5ec3568878feddb6978cb2a352e1226ccb74bd2", "commit_rev":
"cba3aa3a044b578b02a8c237f0bde012260fcf9e"}

[commit-bot: I haz the power, 2017-03-16 18:20:54]

Description was changed from

==========
[Media>UI] Don't call onServiceStarted when handling a notification action

There's an issue where when we receive a PAUSE intent fired from media
notification, we still call MediaNotificationManager.onServiceStarted(),
which will update the notification and call Service.startForeground().
When the PAUSE action is executed, we then update the notification again
and call Service.stopForeground(). These two calls are called in a very
short time window so that a race condition in Android will keep the
notification foreground and unswippable.

In this CL, we distinguish start service intents so that we don't update
the notification when receiving an action.

BUG=701731
R=mlamouri@chromium.org
==========

to

==========
[Media>UI] Don't call onServiceStarted when handling a notification action

There's an issue where when we receive a PAUSE intent fired from media
notification, we still call MediaNotificationManager.onServiceStarted(),
which will update the notification and call Service.startForeground().
When the PAUSE action is executed, we then update the notification again
and call Service.stopForeground(). These two calls are called in a very
short time window so that a race condition in Android will keep the
notification foreground and unswippable.

In this CL, we distinguish start service intents so that we don't update
the notification when receiving an action.

BUG=701731
R=mlamouri@chromium.org

Review-Url: https://codereview.chromium.org/2750973004
Cr-Commit-Position: refs/heads/master@{#457495}
Committed:
https://chromium.googlesource.com/chromium/src/+/cba3aa3a044b578b02a8c237f0bd...
==========

[commit-bot: I haz the power, 2017-03-16 18:20:55]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/cba3aa3a044b578b02a8c237f0bd...