Abstracting over Notification.Builder + NotificationCompat.Builder

Abstracting over Notification.Builder + NotificationCompat.Builder

Migrated the following notifications to use the new
ChromeNotificationBuilder (all except web + media):
- Incognito
- Sync
- Download
- Content suggestions
- MediaCapture


BUG=678670
Review-Url: https://codereview.chromium.org/2699253003
Cr-Commit-Position: refs/heads/master@{#453764}
Committed: https://chromium.googlesource.com/chromium/src/+/74957899a3f38a0ba3d012231ef66252adc28b52

[awdf, 2017-02-18 07:56:36]

Description was changed from

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

- Migrated incognito and sync notifications to the new buildert cl
upload

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

to

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

    Migrated the following notifications to use the new
    ChromeNotificationBuilder (all except web + media):
    - Incognito
    - Sync
    - Download
    - Content suggestions
    - MediaCapture



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

[awdf, 2017-02-18 07:56:36]

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

[Peter Beverloo, 2017-02-20 01:06:44]

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

[Peter Beverloo, 2017-02-20 01:06:45]

Please attach a BUG number to this CL.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:434: 
docs++

Maybe add a TODO at the same time that we can delete this in Chrome 59?

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:576:
.getChromeNotificationBuilder(true, NotificationConstants.TYPE_ID_GENERAL,
nit: true -> true /* preferCompat */ (elsewhere too)

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java:12:
* Created by awdf on 2/16/17.
nit: delete - elsewhere too.

Here, too, a TODO saying that we can delete it in an upcoming release can be
useful.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilder.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilder.java:19:
protected final Notification.Builder mBuilder;
nit: why protected?

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:86:
"Incognito"; // TODO Define this in GRD not here
We should fulfil these TODOs in this CL.

[awdf, 2017-02-20 11:21:17]

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

[commit-bot: I haz the power, 2017-02-20 11:21:39]

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-20 13:09:38]

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

[commit-bot: I haz the power, 2017-02-20 13:09:39]

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...)

[nyquist, 2017-02-22 10:43:59]

mostly agree with peter here.

Also; the indent for the CL description seems all weird.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:435:
public ChromeNotificationBuilder getChromeNotificationBuilder(
Also, could we change this to create? We seem to be creating the builder for
each call.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java:14:
public interface ChromeNotificationBuilder {
Could you also add a high-level comment about this wrapper class?

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

Description was changed from

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

    Migrated the following notifications to use the new
    ChromeNotificationBuilder (all except web + media):
    - Incognito
    - Sync
    - Download
    - Content suggestions
    - MediaCapture



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

to

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

    Migrated the following notifications to use the new
    ChromeNotificationBuilder (all except web + media):
    - Incognito
    - Sync
    - Download
    - Content suggestions
    - MediaCapture



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

[awdf, 2017-02-23 21:52:35]

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

[commit-bot: I haz the power, 2017-02-23 21:52:56]

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-23 23:01:45]

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

[commit-bot: I haz the power, 2017-02-23 23:01:46]

Dry run: This issue passed the CQ dry run.

[awdf, 2017-02-24 00:14:44]

Description was changed from

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

    Migrated the following notifications to use the new
    ChromeNotificationBuilder (all except web + media):
    - Incognito
    - Sync
    - Download
    - Content suggestions
    - MediaCapture



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

to

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

Migrated the following notifications to use the new
ChromeNotificationBuilder (all except web + media):
- Incognito
- Sync
- Download
- Content suggestions
- MediaCapture


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

[awdf, 2017-02-24 00:38:47]

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:434: 
On 2017/02/20 at 01:06:45, Peter Beverloo wrote:
> docs++
> 
> Maybe add a TODO at the same time that we can delete this in Chrome 59?

Done.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:435:
public ChromeNotificationBuilder getChromeNotificationBuilder(
On 2017/02/22 at 10:43:59, nyquist wrote:
> Also, could we change this to create? We seem to be creating the builder for
each call.

Done.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:576:
.getChromeNotificationBuilder(true, NotificationConstants.TYPE_ID_GENERAL,
On 2017/02/20 at 01:06:45, Peter Beverloo wrote:
> nit: true -> true /* preferCompat */ (elsewhere too)

Done.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java:12:
* Created by awdf on 2/16/17.
On 2017/02/20 at 01:06:45, Peter Beverloo wrote:
> nit: delete - elsewhere too.
> 
> Here, too, a TODO saying that we can delete it in an upcoming release can be
useful.

Done.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java:14:
public interface ChromeNotificationBuilder {
On 2017/02/22 at 10:43:59, nyquist wrote:
> Could you also add a high-level comment about this wrapper class?

Done.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilder.java:14:
public interface ChromeNotificationBuilder {
On 2017/02/22 at 10:43:59, nyquist wrote:
> Could you also add a high-level comment about this wrapper class?

Done.

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
(right):

https://codereview.chromium.org/2699253003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:86:
"Incognito"; // TODO Define this in GRD not here
On 2017/02/20 at 01:06:45, Peter Beverloo wrote:
> We should fulfil these TODOs in this CL.

Done. (Added 'General' string to grd file, removed incognito category as
discussed.)

[Peter Beverloo, 2017-02-24 00:43:01]

lgtm

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:576:
.createChromeNotificationBuilder(true /* preferCompat */,
micro nit: is this indentation correct?

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java
(right):

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java:15:
* Created by awdf on 2/16/17.
nit: delete (does your editor do this)?

[awdf, 2017-02-24 00:56:34]

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
(right):

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:576:
.createChromeNotificationBuilder(true /* preferCompat */,
On 2017/02/24 at 00:43:01, Peter Beverloo wrote:
> micro nit: is this indentation correct?

git cl format didn't change it, so I'm gonna say Yes.

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java
(right):

https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java:15:
* Created by awdf on 2/16/17.
On 2017/02/24 at 00:43:01, Peter Beverloo wrote:
> nit: delete (does your editor do this)?

darn, thought I got them all. And yes, but I will go and change that right now!

[Peter Beverloo, 2017-02-24 00:57:12]

On 2017/02/24 00:56:34, awdf wrote:
>
https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java
> (right):
> 
>
https://codereview.chromium.org/2699253003/diff/100001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java:15:
> * Created by awdf on 2/16/17.
> On 2017/02/24 at 00:43:01, Peter Beverloo wrote:
> > nit: delete (does your editor do this)?
> 
> darn, thought I got them all. And yes, but I will go and change that right
now!

lol, tomorrow's fine too :-P

[nyquist, 2017-02-28 07:15:53]

lgtm

[awdf, 2017-02-28 13:01:20]

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

[commit-bot: I haz the power, 2017-02-28 13:01:51]

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 14:26:49]

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

[commit-bot: I haz the power, 2017-02-28 14:26:50]

Dry run: This issue passed the CQ dry run.

[awdf, 2017-02-28 19:02:57]

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:03:49]

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:31:29]

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

[commit-bot: I haz the power, 2017-02-28 19:31:30]

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 19:33: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:33:57]

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 22:20:15]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-02-28 22:20:16]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2699253003/#ps200001
(title: "remove erroneously added methods from rebase")

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

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 23:22:55]

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

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

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-02-28 23:39:37]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-02-28 23:40:33]

On 2017/02/28 23:22:56, commit-bot: I haz the power wrote:
> 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...)

failed without patch too so trying again

[commit-bot: I haz the power, 2017-02-28 23:40:55]

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:17:28]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488325177525710,
"parent_rev": "3719f5627a5768df2ff8eba6be15dc29c17a2e69", "commit_rev":
"74957899a3f38a0ba3d012231ef66252adc28b52"}

[commit-bot: I haz the power, 2017-03-01 00:18:28]

Description was changed from

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

Migrated the following notifications to use the new
ChromeNotificationBuilder (all except web + media):
- Incognito
- Sync
- Download
- Content suggestions
- MediaCapture


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

to

==========
Abstracting over Notification.Builder + NotificationCompat.Builder

Migrated the following notifications to use the new
ChromeNotificationBuilder (all except web + media):
- Incognito
- Sync
- Download
- Content suggestions
- MediaCapture


BUG=678670

Review-Url: https://codereview.chromium.org/2699253003
Cr-Commit-Position: refs/heads/master@{#453764}
Committed:
https://chromium.googlesource.com/chromium/src/+/74957899a3f38a0ba3d012231ef6...
==========

[commit-bot: I haz the power, 2017-03-01 00:18:29]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/74957899a3f38a0ba3d012231ef6...