Migrate web notifications to ChromeNotificationBuilder

Migrate web notifications to ChromeNotificationBuilder

BUG=678670
Review-Url: https://codereview.chromium.org/2702023002
Cr-Commit-Position: refs/heads/master@{#453774}
Committed: https://chromium.googlesource.com/chromium/src/+/04b8a54dae18ab32b77f2d9a898b1cdce87f72ad

[awdf, 2017-02-18 08:04:11]

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

[awdf, 2017-02-18 08:33:37]

awdf@chromium.org changed reviewers:
+ peter@chromium.org
- beverloo@chromium.org

[awdf, 2017-02-18 08:33:50]

This is based off https://codereview.chromium.org/2699253003

[Peter Beverloo, 2017-02-20 01:10:20]

Add a BUG number here too, please

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:146:
.getChromeNotificationBuilder(false, NotificationConstants.TYPE_ID_GENERAL,
nit: false /* preferCompat */

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:146:
.getChromeNotificationBuilder(false, NotificationConstants.TYPE_ID_GENERAL,
These are not generic notifications. Do you plan to create different types for
Web Notifications later?

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java:181:
return this;
Maybe "assert false;  // unused" or something to clarify that this should have
no callers?

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java:32:
NotificationConstants.TYPE_NAME_GENERAL);
dito re: types

[awdf, 2017-02-22 14:12:17]

Description was changed from

==========
Migrate web notifications to ChromeNotificationBuilder

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

to

==========
Migrate web notifications to ChromeNotificationBuilder

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

[Peter Beverloo, 2017-02-24 16:11:02]

lgtm % comments

[awdf, 2017-02-24 17:36:43]

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

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

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

[awdf, 2017-02-24 17:37:30]

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:146:
.getChromeNotificationBuilder(false, NotificationConstants.TYPE_ID_GENERAL,
On 2017/02/20 at 01:10:20, Peter Beverloo wrote:
> nit: false /* preferCompat */

Done.

> These are not generic notifications. Do you plan to create different types for
Web Notifications later?

I'll see what decision comes out of the privacy discussions and update to Sites
or per-origin then.

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java:181:
return this;
On 2017/02/20 at 01:10:20, Peter Beverloo wrote:
> Maybe "assert false;  // unused" or something to clarify that this should have
no callers?

Done, and in the other places it where it no-ops.

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

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_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:29:08]

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

[nyquist, 2017-02-28 07:29:09]

lgtm % comments

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:143:
// TODO(awdf) We should probably use a Compat builder.
Nit: Should there be a bug filed for this so we don't forget?

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java:57:
assert false; // unused
Nit: Double space before // throughout this file.

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java:30:
((ChromeApplication) mContext)
This seems unsafe. Could you use mContext.getApplicationContext() to ensure it
is in fact the application context here?

[awdf, 2017-02-28 18:02:33]

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

[commit-bot: I haz the power, 2017-02-28 18:02:58]

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

[awdf, 2017-02-28 18:04:12]

Think this addresses all comments, just need to fixup test failures (might
already be fixed by using application context.. we'll see from the dry run).

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:146:
.getChromeNotificationBuilder(false, NotificationConstants.TYPE_ID_GENERAL,
On 2017/02/24 17:37:30, awdf wrote:
> On 2017/02/20 at 01:10:20, Peter Beverloo wrote:
> > nit: false /* preferCompat */
> 
> Done.
> 
> > These are not generic notifications. Do you plan to create different types
for
> Web Notifications later?
> 
> I'll see what decision comes out of the privacy discussions and update to
Sites
> or per-origin then.

Done.

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:143:
// TODO(awdf) We should probably use a Compat builder.
On 2017/02/28 07:29:09, nyquist wrote:
> Nit: Should there be a bug filed for this so we don't forget?

Done.

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java:57:
assert false; // unused
On 2017/02/28 07:29:09, nyquist wrote:
> Nit: Double space before // throughout this file.

Done.

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java
(right):

https://codereview.chromium.org/2702023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java:30:
((ChromeApplication) mContext)
On 2017/02/28 07:29:09, nyquist wrote:
> This seems unsafe. Could you use mContext.getApplicationContext() to ensure it
> is in fact the application context here?

Done. (It should already be as NPB gets application context which is passed
through, but agree this is more robust)

[commit-bot: I haz the power, 2017-02-28 18:05:55]

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

[commit-bot: I haz the power, 2017-02-28 18:05:56]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[awdf, 2017-02-28 19:04:24]

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

[commit-bot: I haz the power, 2017-02-28 19:05:14]

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-28 19:36:34]

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

[commit-bot: I haz the power, 2017-02-28 19:36:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[awdf, 2017-02-28 20:19:53]

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

[commit-bot: I haz the power, 2017-02-28 20:20:34]

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-28 21:35:01]

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

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

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-02-28 22:17:45]

lgtm

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

The CQ bit was checked by awdf@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2702023002/#ps80001
(title: "rebase")

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

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

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

This CL has an open dependency (Issue 2699253003 Patch 200001). 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 00:25:04]

The CQ bit was checked by awdf@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:25:49]

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 00:36:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488327904670880,
"parent_rev": "aa1ab1dc32ed1b5508c7b835085a503ea8d96280", "commit_rev":
"04b8a54dae18ab32b77f2d9a898b1cdce87f72ad"}

[commit-bot: I haz the power, 2017-03-01 00:41:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488327904670880,
"parent_rev": "aa1ab1dc32ed1b5508c7b835085a503ea8d96280", "commit_rev":
"04b8a54dae18ab32b77f2d9a898b1cdce87f72ad"}

[commit-bot: I haz the power, 2017-03-01 00:41:13]

Description was changed from

==========
Migrate web notifications to ChromeNotificationBuilder

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

to

==========
Migrate web notifications to ChromeNotificationBuilder

BUG=678670

Review-Url: https://codereview.chromium.org/2702023002
Cr-Commit-Position: refs/heads/master@{#453774}
Committed:
https://chromium.googlesource.com/chromium/src/+/04b8a54dae18ab32b77f2d9a898b...
==========

[commit-bot: I haz the power, 2017-03-01 00:41:14]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/04b8a54dae18ab32b77f2d9a898b...