Add a preference for disabling vibration in notifications.

Add a preference for disabling vibration in notifications.

Settings > Site settings > Notifications > Vibrate

Screenshot: http://imgur.com/Dt2XwUI

BUG=543534
Committed: https://crrev.com/9f0f38a755a92f7208c5bd3831da295e023d505f
Cr-Commit-Position: refs/heads/master@{#398542}

[Michael van Ouwerkerk, 2016-06-06 16:02:21]

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

[commit-bot: I haz the power, 2016-06-06 16:02:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2039953002/1

[Michael van Ouwerkerk, 2016-06-06 16:02:43]

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

[Michael van Ouwerkerk, 2016-06-06 16:02:43]

Peter, could you take a look please?

[commit-bot: I haz the power, 2016-06-06 17:20:50]

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

[commit-bot: I haz the power, 2016-06-06 17:20:51]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-06-07 12:46:01]

lgtm

Could you include a screenshot?

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

https://codereview.chromium.org/2039953002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:562:
// pattern.
Can we make this significantly more concise? What about:

        // Block the fallback vibration pattern the Android Framework will apply
when the device is
        // in vibrate mode, there is no custom pattern and the vibration default
has been disabled.

https://codereview.chromium.org/2039953002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2039953002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:153:
registry->RegisterBooleanPref(prefs::kNotificationsVibrateEnabled, true);
Should this be part of the NotificationPlatformBridgeAndroid?

[Michael van Ouwerkerk, 2016-06-07 16:26:33]

Description was changed from

==========
Add a preference for disabling vibration in notifications.

Settings > Site settings > Notifications > Vibrate

BUG=543534
==========

to

==========
Add a preference for disabling vibration in notifications.

Settings > Site settings > Notifications > Vibrate

Screenshot: http://imgur.com/Dt2XwUI

BUG=543534
==========

[Michael van Ouwerkerk, 2016-06-07 16:34:05]

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

[Michael van Ouwerkerk, 2016-06-07 16:34:21]

Thanks Peter.

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

https://codereview.chromium.org/2039953002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:562:
// pattern.
On 2016/06/07 12:46:01, Peter Beverloo - OOO June 8-16 wrote:
> Can we make this significantly more concise? What about:
> 
>         // Block the fallback vibration pattern the Android Framework will
apply
> when the device is
>         // in vibrate mode, there is no custom pattern and the vibration
default
> has been disabled.

Sure, I did something shorter like that.

https://codereview.chromium.org/2039953002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2039953002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:153:
registry->RegisterBooleanPref(prefs::kNotificationsVibrateEnabled, true);
On 2016/06/07 12:46:01, Peter Beverloo - OOO June 8-16 wrote:
> Should this be part of the NotificationPlatformBridgeAndroid?

Done.

[commit-bot: I haz the power, 2016-06-07 16:34:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2039953002/20001

[Michael van Ouwerkerk, 2016-06-07 16:38:32]

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

[Michael van Ouwerkerk, 2016-06-07 16:38:33]

Dan, could you please review this one as OWNER of the android bits?

[Michael van Ouwerkerk, 2016-06-07 16:39:19]

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

[Michael van Ouwerkerk, 2016-06-07 16:39:19]

Bernhard, could you please review this one as OWNER of browser_prefs.cc?

[gone, 2016-06-07 17:32:54]

lgtm, leaving nits to your judgement.

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

https://codereview.chromium.org/2039953002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:562:
vibrationPattern = new int[0];
Might it be clearer to add a

private static final int[] EMPTY_VIBRATION_PATTERN = new int[0];

https://codereview.chromium.org/2039953002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
(right):

https://codereview.chromium.org/2039953002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:121:
public void testNotificationVibratePreferenceDisabledDefault() throws Exception
{
This and the below test differ only by one line; does it make sense to combine
them and pass in the expected vibration string?

[commit-bot: I haz the power, 2016-06-07 17:55:32]

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

[commit-bot: I haz the power, 2016-06-07 17:55:33]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-06-07 19:03:05]

Prefs LGTM

[Michael van Ouwerkerk, 2016-06-08 13:28:13]

Thanks guys!

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

https://codereview.chromium.org/2039953002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:562:
vibrationPattern = new int[0];
On 2016/06/07 17:32:54, dfalcantara wrote:
> Might it be clearer to add a
> 
> private static final int[] EMPTY_VIBRATION_PATTERN = new int[0];

Done.

https://codereview.chromium.org/2039953002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
(right):

https://codereview.chromium.org/2039953002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:121:
public void testNotificationVibratePreferenceDisabledDefault() throws Exception
{
On 2016/06/07 17:32:54, dfalcantara wrote:
> This and the below test differ only by one line; does it make sense to combine
> them and pass in the expected vibration string?

Done.

[Michael van Ouwerkerk, 2016-06-08 13:28:17]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-06-08 13:28:18]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
bauerb@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2039953002/#ps40001
(title: "Rebase. Address Dan's comments.")

[commit-bot: I haz the power, 2016-06-08 13:28:42]

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

[commit-bot: I haz the power, 2016-06-08 14:11:56]

Description was changed from

==========
Add a preference for disabling vibration in notifications.

Settings > Site settings > Notifications > Vibrate

Screenshot: http://imgur.com/Dt2XwUI

BUG=543534
==========

to

==========
Add a preference for disabling vibration in notifications.

Settings > Site settings > Notifications > Vibrate

Screenshot: http://imgur.com/Dt2XwUI

BUG=543534
==========

[commit-bot: I haz the power, 2016-06-08 14:11:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-08 14:13:11]

Description was changed from

==========
Add a preference for disabling vibration in notifications.

Settings > Site settings > Notifications > Vibrate

Screenshot: http://imgur.com/Dt2XwUI

BUG=543534
==========

to

==========
Add a preference for disabling vibration in notifications.

Settings > Site settings > Notifications > Vibrate

Screenshot: http://imgur.com/Dt2XwUI

BUG=543534

Committed: https://crrev.com/9f0f38a755a92f7208c5bd3831da295e023d505f
Cr-Commit-Position: refs/heads/master@{#398542}
==========

[commit-bot: I haz the power, 2016-06-08 14:13:12]

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