Extract NotificationBuilderBase for holding the arguments.

Extract NotificationBuilderBase for holding the arguments.

This enables making smarter decisions in StandardNotificationBuilder#build
in future.

BUG=581336
Committed: https://crrev.com/0049a9be71b7cf6c218c0f9ce71068950d776dee
Cr-Commit-Position: refs/heads/master@{#371810}

[Michael van Ouwerkerk, 2016-01-27 11:07:17]

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

[Michael van Ouwerkerk, 2016-01-27 11:07:18]

Peter, could you take a look please?

[Peter Beverloo, 2016-01-27 11:36:03]

# red lines > # green lines \o/

Thanks, this looks like a really good clean up to me. Perhaps we can push it
slightly further still?

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

https://codereview.chromium.org/1639163003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:23:
public abstract class NotificationBuilderBase implements NotificationBuilder {
Do we need to keep the distinction between the interface and the abstract class?
Could we merge them together?

Right now, the diagram would look like:

                                                 CustomNotificationBuilder
NotificationBuilder -> NotificationBuilderBase /
                                               \
                                                 StandardNotificationBuilder

https://codereview.chromium.org/1639163003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:124:
public NotificationBuilder setVibrate(@Nullable long[] pattern) {
This seems incorrect? You're marking it as @Nullable but are using the |pattern|
member on the next line.

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

https://codereview.chromium.org/1639163003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java:25:
NotificationCompat.Builder builder = new NotificationCompat.Builder(mContext);
Something to consider: Perhaps the base could hold an instance of the
NotificationCompat.Builder, and the setters could update those directly as well
(and not store the memory locally where we can avoid it).

A number of these calls (26-42) are duplicated too.

[Michael van Ouwerkerk, 2016-01-27 15:04:25]

Thanks Peter.

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

https://codereview.chromium.org/1639163003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:23:
public abstract class NotificationBuilderBase implements NotificationBuilder {
On 2016/01/27 11:36:03, Peter Beverloo wrote:
> Do we need to keep the distinction between the interface and the abstract
class?
> Could we merge them together?
> 
> Right now, the diagram would look like:
> 
>                                                  CustomNotificationBuilder
> NotificationBuilder -> NotificationBuilderBase /
>                                                \
>                                                  StandardNotificationBuilder

From a purist perspective, I'd like to keep the interface. From a practical and
resource conservation perspective, we should get rid of it. Done.

https://codereview.chromium.org/1639163003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:124:
public NotificationBuilder setVibrate(@Nullable long[] pattern) {
On 2016/01/27 11:36:03, Peter Beverloo wrote:
> This seems incorrect? You're marking it as @Nullable but are using the
|pattern|
> member on the next line.

Done.

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

https://codereview.chromium.org/1639163003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java:25:
NotificationCompat.Builder builder = new NotificationCompat.Builder(mContext);
On 2016/01/27 11:36:03, Peter Beverloo wrote:
> Something to consider: Perhaps the base could hold an instance of the
> NotificationCompat.Builder, and the setters could update those directly as
well
> (and not store the memory locally where we can avoid it).
> 
> A number of these calls (26-42) are duplicated too.

I considered this, but I worry that sprinkling usage of the builder across
multiple files would hurt readability. Perhaps something to reconsider once the
additional logic goes in for handling action icons.

[Peter Beverloo, 2016-01-27 17:09:44]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-01-27 17:09:44]

lgtm

[commit-bot: I haz the power, 2016-01-27 17:12:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1639163003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1639163003/40001

[commit-bot: I haz the power, 2016-01-27 17:19:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-27 17:20:18]

Description was changed from

==========
Extract NotificationBuilderBase for holding the arguments.

This enables making smarter decisions in StandardNotificationBuilder#build
in future.

BUG=581336
==========

to

==========
Extract NotificationBuilderBase for holding the arguments.

This enables making smarter decisions in StandardNotificationBuilder#build
in future.

BUG=581336

Committed: https://crrev.com/0049a9be71b7cf6c218c0f9ce71068950d776dee
Cr-Commit-Position: refs/heads/master@{#371810}
==========

[commit-bot: I haz the power, 2016-01-27 17:20:18]

Patchset 3 (id:??) landed as
https://crrev.com/0049a9be71b7cf6c218c0f9ce71068950d776dee
Cr-Commit-Position: refs/heads/master@{#371810}