Migrating media notifications to ChromeNotificationBuilder

Migrating media notifications to ChromeNotificationBuilder

BUG=678670
Review-Url: https://codereview.chromium.org/2702963002
Cr-Commit-Position: refs/heads/master@{#453922}
Committed: https://chromium.googlesource.com/chromium/src/+/37674d223abf195e966f385645255cf852bed3f5

[awdf, 2017-02-18 08:09:02]

awdf@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-02-20 11:20:17]

This is changing the media notifications to no longer use NotificationCompat. Do
I understand this correctly?

[awdf, 2017-02-20 11:25:33]

On 2017/02/20 at 11:20:17, mlamouri wrote:
> This is changing the media notifications to no longer use NotificationCompat.
Do I understand this correctly?

This is based on https://codereview.chromium.org/2699253003 so they should still
use compat (passing preferCompat = true)

[awdf, 2017-02-22 14:13:52]

Description was changed from

==========
Migrating media notifications to ChromeNotificationBuilder

BUG=
==========

to

==========
Migrating media notifications to ChromeNotificationBuilder

BUG=678670
==========

[awdf, 2017-02-24 18:08:09]

The CQ bit was checked by awdf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 18:08:30]

Dry run: 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-02-24 19:10:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 19:10:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nyquist, 2017-02-28 07:33:17]

nyquist@chromium.org changed reviewers:
+ nyquist@chromium.org

[nyquist, 2017-02-28 07:33:17]

This looks reasonable to me % comment about context.

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

https://codereview.chromium.org/2702963002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:721:
((ChromeApplication) mContext)
I think you need to do mContext.getApplicationContext() to ensure this is in
fact the application context, and not an activity context.

[mlamouri (slow - plz ping), 2017-02-28 15:23:11]

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

https://codereview.chromium.org/2702963002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:721:
((ChromeApplication) mContext)
On 2017/02/28 at 07:33:17, nyquist wrote:
> I think you need to do mContext.getApplicationContext() to ensure this is in
fact the application context, and not an activity context.

+1. The context you are getting here is probably not the one you are looking
for.

https://codereview.chromium.org/2702963002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:723:
NotificationConstants.CATEGORY_ID_GENERAL,
Should this be media?

[mlamouri (slow - plz ping), 2017-02-28 17:58:36]

lgtm

[awdf, 2017-02-28 18:09:05]

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

https://codereview.chromium.org/2702963002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:721:
((ChromeApplication) mContext)
On 2017/02/28 15:23:11, mlamouri wrote:
> On 2017/02/28 at 07:33:17, nyquist wrote:
> > I think you need to do mContext.getApplicationContext() to ensure this is in
> fact the application context, and not an activity context.
> 
> +1. The context you are getting here is probably not the one you are looking
> for.

Done.

https://codereview.chromium.org/2702963002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:723:
NotificationConstants.CATEGORY_ID_GENERAL,
On 2017/02/28 15:23:11, mlamouri wrote:
> Should this be media?

As discussed, we're going with GENERAL for now.

[nyquist, 2017-02-28 22:18:08]

lgtm

[awdf, 2017-02-28 22:23:13]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-02-28 22:23:13]

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/2702963002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-28 22:23:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 22:23:36]

This CL has an open dependency (Issue 2702023002 Patch 80001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[awdf, 2017-03-01 09:23:24]

The CQ bit was checked by awdf@chromium.org

[commit-bot: I haz the power, 2017-03-01 09:23:44]

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-01 09:50:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 09:50:17]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[awdf, 2017-03-01 12:15:16]

Patchset #6 (id:100001) has been deleted

[awdf, 2017-03-01 12:15:24]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-03-01 12:15:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2702963002/#ps120001
(title: "remove now-unecessary method after rebase")

[commit-bot: I haz the power, 2017-03-01 12:15:34]

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

[awdf, 2017-03-01 12:20:30]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-03-01 12:20:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2702963002/#ps140001
(title: "remove that unecessary method from all the places")

[commit-bot: I haz the power, 2017-03-01 12:20:46]

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-01 12:58:27]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488370830282340,
"parent_rev": "89a48fc8cbf9b7b91694a9667d138bb068edbf5b", "commit_rev":
"37674d223abf195e966f385645255cf852bed3f5"}

[commit-bot: I haz the power, 2017-03-01 12:59:06]

Description was changed from

==========
Migrating media notifications to ChromeNotificationBuilder

BUG=678670
==========

to

==========
Migrating media notifications to ChromeNotificationBuilder

BUG=678670

Review-Url: https://codereview.chromium.org/2702963002
Cr-Commit-Position: refs/heads/master@{#453922}
Committed:
https://chromium.googlesource.com/chromium/src/+/37674d223abf195e966f38564525...
==========

[commit-bot: I haz the power, 2017-03-01 12:59:08]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/37674d223abf195e966f38564525...