Handle notification preferences intent from gear icon.

Handle the notification settings intent.

This intent comes from two places. Since Android L there is a settings
cog on two UI surfaces:
(1) On the rear side (after long press) of notifications.
(2) In the "App notifications" screen in Android Settings.

Touching the cog on a flipped notification takes the user to the site
specific preferences screen which displays (among others) the notification
preference for the site origin.

Touching he cog on the App notifications screen takes the user to
Chrome's notifications preferences list for all sites.

BUG=436594, 461885
Committed: https://crrev.com/efff60a90b8eb97bdb9697e8bb59de22745be1a1
Cr-Commit-Position: refs/heads/master@{#318097}

[Michael van Ouwerkerk, 2015-02-20 16:04:57]

mvanouwerkerk@chromium.org changed reviewers:
+ johnme@chromium.org, miguelg@chromium.org, peter@chromium.org

[Michael van Ouwerkerk, 2015-02-20 16:04:57]

Peter, I didn't fully understand what was wrong with this approach, it seems to
be working. Does it need to be plumbed differently?

[Michael van Ouwerkerk, 2015-02-24 16:35:57]

Please take another look, this seems to have correct behavior in manual testing.
I'll take a look at the failing test meanwhile.

[Peter Beverloo, 2015-02-24 17:48:03]

Thanks for the patch, Michael!!

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

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:36:
* Separator used to concatenate multiple pieces of data into a notification tag.
Please clarify in this comment that the separator will only be used to separate
the origin from additional content, e.g. the tag. This is important because
origins can't contain semi-colons, where "a piece of data" can.

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

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:127:
public static void launchNotificationPreferences(Context context, Intent
incomingIntent) {
Please add a docblock comment since this is a public (static) method.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:137:
boolean launchSingleSitePreferences = notificationTag != null;
nit: launchSinceWebsitePreferences to match the fragment name.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:159:
// When coming from the gear on a flipped notification the intent will not be
handled
s/*/When coming from the gear on a flipped notification, we need to ensure that
no existing preference tasks are being re-used in order for it to appear on top.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:183:
String platformTag = origin + NotificationConstants.NOTIFICATION_TAG_SEPARATOR +
tag;
Use TextUtils.isEmpty(tag) please, to make sure that we would correctly handle a
tag==null case.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java
(left):

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java:142:
// The notification settings cog on the flipped side of Notifications and in the
Android
Please be sure to follow up by updating ChromeShell to use the new path as well
- should be as easy as making the same AndroidManifest.xml changes and deferring
in the ChromeShellActivity intent handling functions.

https://codereview.chromium.org/942103003/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/942103003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.cc:421: return;
We don't add the notification to profile_notifications_ when a notification is
being received without Chrome running (the OnNotificationClicked case), so we'd
bail out and would be unable to close the notification here.

[Michael van Ouwerkerk, 2015-02-25 18:12:45]

Thanks for the review Peter. Please take another look.

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

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:36:
* Separator used to concatenate multiple pieces of data into a notification tag.
On 2015/02/24 17:48:02, Peter Beverloo wrote:
> Please clarify in this comment that the separator will only be used to
separate
> the origin from additional content, e.g. the tag. This is important because
> origins can't contain semi-colons, where "a piece of data" can.

Done.

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

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:127:
public static void launchNotificationPreferences(Context context, Intent
incomingIntent) {
On 2015/02/24 17:48:03, Peter Beverloo wrote:
> Please add a docblock comment since this is a public (static) method.

Done.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:137:
boolean launchSingleSitePreferences = notificationTag != null;
On 2015/02/24 17:48:02, Peter Beverloo wrote:
> nit: launchSinceWebsitePreferences to match the fragment name.

Done.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:159:
// When coming from the gear on a flipped notification the intent will not be
handled
On 2015/02/24 17:48:03, Peter Beverloo wrote:
> s/*/When coming from the gear on a flipped notification, we need to ensure
that
> no existing preference tasks are being re-used in order for it to appear on
top.

Done.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:183:
String platformTag = origin + NotificationConstants.NOTIFICATION_TAG_SEPARATOR +
tag;
On 2015/02/24 17:48:03, Peter Beverloo wrote:
> Use TextUtils.isEmpty(tag) please, to make sure that we would correctly handle
a
> tag==null case.

Done.

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java
(left):

https://codereview.chromium.org/942103003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java:142:
// The notification settings cog on the flipped side of Notifications and in the
Android
On 2015/02/24 17:48:03, Peter Beverloo wrote:
> Please be sure to follow up by updating ChromeShell to use the new path as
well
> - should be as easy as making the same AndroidManifest.xml changes and
deferring
> in the ChromeShellActivity intent handling functions.

Done.

https://codereview.chromium.org/942103003/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/942103003/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.cc:421: return;
On 2015/02/24 17:48:03, Peter Beverloo wrote:
> We don't add the notification to profile_notifications_ when a notification is
> being received without Chrome running (the OnNotificationClicked case), so
we'd
> bail out and would be unable to close the notification here.

Done.

[Peter Beverloo, 2015-02-25 18:26:10]

lgtm % comments.

https://codereview.chromium.org/942103003/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/942103003/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:139:
Context applicationContext = context.getApplicationContext();
Did you verify that this is necessary?

https://codereview.chromium.org/942103003/diff/80001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java
(right):

https://codereview.chromium.org/942103003/diff/80001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:125:
// TODO(mvanouwerkerk): Write new preference intent tests for notification
settings.
Please attach a bug to this and mention it in the BUG= line.

https://codereview.chromium.org/942103003/diff/80001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/942103003/diff/80001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:215:
// The notification settings cog on the flipped side of Notifications and in the
Android
I would have expected this to be in onNewIntent(). Does this work in all
situations? (Chrome running / not running vs. Settings page / Notification rear)

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.cc:193:
regenerated_notification_infos_.insert(std::make_pair(id, notification_info));
  regenerated_notification_infos_[id] = RegeneratedNotificationInfo(
      notification_data.tag,
      platform_id,
      origin.spec());

?


Since we need to copy this anyway (unless you'd want to use an IDMap) we may as
well improve readability, and drop <utility>.

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.cc:425:
ScopedJavaLocalRef<jstring> tag =
nit: blank line under 424 and 428.

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.h:24: struct
RegeneratedNotificationInfo {
Why is this public?

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.h:25:
RegeneratedNotificationInfo(base::string16 tag,
const&

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.h:27: std::string
origin)
const&

[Michael van Ouwerkerk, 2015-02-25 19:00:19]

All done, thanks for the review.

https://codereview.chromium.org/942103003/diff/80001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/942103003/diff/80001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:139:
Context applicationContext = context.getApplicationContext();
On 2015/02/25 18:26:09, Peter Beverloo wrote:
> Did you verify that this is necessary?

Yes, that's also what the comment is based on.

https://codereview.chromium.org/942103003/diff/80001/chrome/android/javatests...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java
(right):

https://codereview.chromium.org/942103003/diff/80001/chrome/android/javatests...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:125:
// TODO(mvanouwerkerk): Write new preference intent tests for notification
settings.
On 2015/02/25 18:26:09, Peter Beverloo wrote:
> Please attach a bug to this and mention it in the BUG= line.

Done. Also added an item to the task tracker.

https://codereview.chromium.org/942103003/diff/80001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/942103003/diff/80001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:215:
// The notification settings cog on the flipped side of Notifications and in the
Android
On 2015/02/25 18:26:09, Peter Beverloo wrote:
> I would have expected this to be in onNewIntent(). Does this work in all
> situations? (Chrome running / not running vs. Settings page / Notification
rear)

That's also what I expected, but I could find no way to trigger onNewIntent, it
always went via onCreate.

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.cc:193:
regenerated_notification_infos_.insert(std::make_pair(id, notification_info));
On 2015/02/25 18:26:09, Peter Beverloo wrote:
>   regenerated_notification_infos_[id] = RegeneratedNotificationInfo(
>       notification_data.tag,
>       platform_id,
>       origin.spec());
> 
> ?
> 
> 
> Since we need to copy this anyway (unless you'd want to use an IDMap) we may
as
> well improve readability, and drop <utility>.

Using insert (and a pair) avoids the need for a zero-argument constructor of
RegeneratedNotificationInfo.

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.cc:425:
ScopedJavaLocalRef<jstring> tag =
On 2015/02/25 18:26:09, Peter Beverloo wrote:
> nit: blank line under 424 and 428.

Done.

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.h:24: struct
RegeneratedNotificationInfo {
On 2015/02/25 18:26:09, Peter Beverloo wrote:
> Why is this public?

Done.

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.h:25:
RegeneratedNotificationInfo(base::string16 tag,
On 2015/02/25 18:26:09, Peter Beverloo wrote:
> const&

Done.

https://codereview.chromium.org/942103003/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/notification_ui_manager_android.h:27: std::string
origin)
On 2015/02/25 18:26:10, Peter Beverloo wrote:
> const&

Done.

[Miguel Garcia, 2015-02-25 19:16:40]

lgtm % nits

https://codereview.chromium.org/942103003/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/942103003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:156:
String origin =
DCHECK that origin is there?

https://codereview.chromium.org/942103003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:159:
} else {
can you add // global notification preferences as a comment?

[Michael van Ouwerkerk, 2015-02-25 19:33:23]

Thanks for the quick review.

https://codereview.chromium.org/942103003/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/942103003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:156:
String origin =
On 2015/02/25 19:16:39, Miguel Garcia wrote:
> DCHECK that origin is there?

I've added an assert, but it doesn't seem to do anything on a local debug build.

https://codereview.chromium.org/942103003/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:159:
} else {
On 2015/02/25 19:16:39, Miguel Garcia wrote:
> can you add // global notification preferences as a comment?

Done.

[Michael van Ouwerkerk, 2015-02-25 19:35:52]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2015-02-25 19:35:54]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
miguelg@chromium.org
Link to the patchset: https://codereview.chromium.org/942103003/#ps140001
(title: "Address review comments.")

[commit-bot: I haz the power, 2015-02-25 19:36:34]

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

[commit-bot: I haz the power, 2015-02-25 20:08:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-25 20:09:10]

Patchset 8 (id:??) landed as
https://crrev.com/efff60a90b8eb97bdb9697e8bb59de22745be1a1
Cr-Commit-Position: refs/heads/master@{#318097}