Hide Chrome actions in notifications shown for WebAPKs

Hide Chrome actions in notifications shown for WebAPKs

This CL hides the "Site setting" action when notifications are shown by WebAPKs.
The "additional settings in the app" link will be removed from the notification
settings for WebAPKs in the System UI in Android O. Before Android O, the
settings gear to open app-specific settings shown on the rear of a notification
following a long-press will be gone as well.

BUG=654539
Review-Url: https://codereview.chromium.org/2826223002
Cr-Commit-Position: refs/heads/master@{#467843}
Committed: https://chromium.googlesource.com/chromium/src/+/4df7199f5d2cafd62bf9c9d016caa341230139ca

[Xi Han, 2017-04-19 19:47:19]

Patchset #1 (id:1) has been deleted

[Xi Han, 2017-04-19 21:11:48]

hanxi@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[Xi Han, 2017-04-19 21:11:49]

Hi Peter, could you please take a look? Thanks!

[Peter Beverloo, 2017-04-19 22:04:59]

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

[Peter Beverloo, 2017-04-19 22:04:59]

Last I spoke with Yaron there were still UX approvals for this missing. Is that
on track now?

Changes LG, although you'll also want to disable it for the custom layout
notifications. See //chrome/android/java/res/layout/web_notification_big.xml. We
attach a click handler for the field with ID "origin" in
CustomNotificationBuilder::configureSettingsButton(). I guess that we'd want to
hide the settings icon instead for WebAPKs, and make clicking a no-op.

You can still force-enable them in chrome://flags afaik.

[pkotwicz, 2017-04-20 02:14:25]

You will also need to delete shell_apk/NotificationSettingsLauncherActivity.java

[Xi Han, 2017-04-20 16:27:02]

@Peter B: thanks for reviewing this CL. I turned on the custom notification
flag, and printed the log and saw that the configureSettingsButton() returns
directly when the |mSettingAction| is null
(https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...).
Also, I don't see the site setting icon in the notification after my change in
NotificationPlatformBridge. Did I do what you suggested? Besides, I updated the
bug to ask whether this UX changes is what we want.

@Peter K: thanks for mentioning that, I don't even know that activity exists.

[Peter Beverloo, 2017-04-20 17:05:43]

On 2017/04/20 16:27:02, Xi Han wrote:
> @Peter B: thanks for reviewing this CL. I turned on the custom notification
> flag, and printed the log and saw that the configureSettingsButton() returns
> directly when the |mSettingAction| is null
>
(https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...).
> Also, I don't see the site setting icon in the notification after my change in
> NotificationPlatformBridge. Did I do what you suggested? Besides, I updated
the
> bug to ask whether this UX changes is what we want.

The settings icon is a fully white icon on a transparent background.

Here's what's happening by default:
  1) The settings action has been set, the click handler is attached.
  2) For Material displays (Android K/L) we set a color filter for making it
grey.

Because no settings action is available following your change, what now happens
is:
  1) We bail out in configureSettingsButton().
  2) Image still displays, but in white on presumably a white background. It
looks like weird padding.

We should make sure that we don't display odd padding for affected users. This
can be done by setting the visibility of the @origin_settings_icon element to
View.GONE and updating the padding on the @origin element to be 8dp horizontal.

You'd need something like this in the bail-out case in
configureSettingsButton():

if (mSettingsAction == null) {
    bigView.setViewVisibility(R.id.origin_settings_icon, View.GONE);
    bigView.setViewPadding(R.id.origin, dpToPx(8), 0, dpToPx(8), 0);
    return;
}

[Xi Han, 2017-04-20 17:53:55]

Patchset #3 (id:60001) has been deleted

[Xi Han, 2017-04-20 17:55:18]

Now I see the difference. The transparent icon is gone and the text of the
origin is aligned with the front. Thanks for the explanation. PTAL.

[Peter Beverloo, 2017-04-20 19:08:29]

code lg, but let's get the padding right :)

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

https://codereview.chromium.org/2826223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:238:
bigView.setViewPadding(R.id.origin, padding, 0, padding, 0);
Hmmm could you make a screenshot of what you see? I think I may've been wrong -
there's still too much padding being applied.

https://screenshot.googleplex.com/zEKSvdqChFE

We have two SDK versions we need to verify: 21 (L) and 17 (JB). It might well be
that the needed padding differs for them :/.

https://codereview.chromium.org/2826223002/diff/80001/chrome/android/webapk/s...
File chrome/android/webapk/shell_apk/AndroidManifest.xml (left):

https://codereview.chromium.org/2826223002/diff/80001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/AndroidManifest.xml:37: <category
android:name="android.intent.category.NOTIFICATION_PREFERENCES"/>
Please document in the CL description that this has a secondary effect: the
"additional settings in the app" link will be removed from the notification
settings for WebAPKs in the System UI in Android O. Before Android O, the
settings gear to open app-specific settings shown on the rear of a notification
following a long-press will be gone as well.

[Xi Han, 2017-04-20 19:48:27]

Description was changed from

==========
Hide Chrome actions in notifications shown for WebAPKs

This CL hides the "Site setting" action when notifications are shown by WebAPKs.

BUG=654539
==========

to

==========
Hide Chrome actions in notifications shown for WebAPKs

This CL hides the "Site setting" action when notifications are shown by WebAPKs.
The "additional settings in the app" link will be removed from the notification
settings for WebAPKs in the System UI in Android O. Before Android O, the
settings gear to open app-specific settings shown on the rear of a notification
following a long-press will be gone as well.

BUG=654539
==========

[Xi Han, 2017-04-20 20:14:20]

PTAL, thanks!

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

https://codereview.chromium.org/2826223002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:238:
bigView.setViewPadding(R.id.origin, padding, 0, padding, 0);
On 2017/04/20 19:08:29, Peter Beverloo wrote:
> Hmmm could you make a screenshot of what you see? I think I may've been wrong
-
> there's still too much padding being applied.
> 
> https://screenshot.googleplex.com/zEKSvdqChFE
> 
> We have two SDK versions we need to verify: 21 (L) and 17 (JB). It might well
be
> that the needed padding differs for them :/.

Change the padding to 0, 0, 8, 0. Only leave the padding on the right side of
the button.

https://codereview.chromium.org/2826223002/diff/80001/chrome/android/webapk/s...
File chrome/android/webapk/shell_apk/AndroidManifest.xml (left):

https://codereview.chromium.org/2826223002/diff/80001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/AndroidManifest.xml:37: <category
android:name="android.intent.category.NOTIFICATION_PREFERENCES"/>
On 2017/04/20 19:08:29, Peter Beverloo wrote:
> Please document in the CL description that this has a secondary effect: the
> "additional settings in the app" link will be removed from the notification
> settings for WebAPKs in the System UI in Android O. Before Android O, the
> settings gear to open app-specific settings shown on the rear of a
notification
> following a long-press will be gone as well.

Done.

[Xi Han, 2017-04-20 20:16:52]

Please hold off the review. I just found on JB, it might needs extra padding on
the left side of the origin button to make it align with title properly.

[Xi Han, 2017-04-21 13:53:52]

I have tested on JB and KK, both of them need left padding since the icon of the
notification is on the left side. Besides, Owen has replied to the bug, and it
seems everyone is ok with the removing. I guess we can go ahead to finish this
CL:) PTAL, thanks!

[Xi Han, 2017-04-21 13:53:58]

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

[commit-bot: I haz the power, 2017-04-21 13:54:13]

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

[Peter Beverloo, 2017-04-21 14:34:28]

lgtm

[commit-bot: I haz the power, 2017-04-21 15:43:32]

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

[commit-bot: I haz the power, 2017-04-21 15:43:32]

Dry run: This issue passed the CQ dry run.

[Xi Han, 2017-04-21 17:04:37]

Hi Peter K: could you please review the webapk part? Thanks!

[pkotwicz, 2017-04-25 18:56:40]

LGTM

[Xi Han, 2017-04-25 19:05:32]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2017-04-25 19:06:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xi Han, 2017-04-25 19:13:13]

The CQ bit was unchecked by hanxi@chromium.org

[Xi Han, 2017-04-25 19:30:25]

Hi Peter, I updated the shell apk version. PTAL, thanks!

[pkotwicz, 2017-04-26 19:49:48]

Still LGTM on my part. Xi I am unsure which Peter you wanted to look at the code
again

[Xi Han, 2017-04-26 20:06:13]

On 2017/04/26 19:49:48, pkotwicz wrote:
> Still LGTM on my part. Xi I am unsure which Peter you wanted to look at the
code
> again

Thanks! Just you, Peter K:)

[Xi Han, 2017-04-26 20:06:25]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2017-04-26 20:06:26]

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/2826223002/#ps140001
(title: "Update shell_apk_version.gni.")

[commit-bot: I haz the power, 2017-04-26 20:07:02]

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-04-27 00:26:00]

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

[commit-bot: I haz the power, 2017-04-27 00:26:01]

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

[Xi Han, 2017-04-27 16:36:41]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2017-04-27 16:37:20]

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-04-27 16:56:45]

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

[commit-bot: I haz the power, 2017-04-27 16:56:46]

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

[Xi Han, 2017-04-27 19:38:51]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2017-04-27 19:38:52]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2826223002/#ps160001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-27 19:39:23]

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-04-27 19:55:47]

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

[commit-bot: I haz the power, 2017-04-27 19:55:48]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xi Han, 2017-04-27 22:00:31]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2017-04-27 22:01:24]

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-04-28 02:15:29]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493330431871830,
"parent_rev": "5dad32e367d077bf8b84f1700a6cd00e74a20cc1", "commit_rev":
"4df7199f5d2cafd62bf9c9d016caa341230139ca"}

[commit-bot: I haz the power, 2017-04-28 02:15:37]

Description was changed from

==========
Hide Chrome actions in notifications shown for WebAPKs

This CL hides the "Site setting" action when notifications are shown by WebAPKs.
The "additional settings in the app" link will be removed from the notification
settings for WebAPKs in the System UI in Android O. Before Android O, the
settings gear to open app-specific settings shown on the rear of a notification
following a long-press will be gone as well.

BUG=654539
==========

to

==========
Hide Chrome actions in notifications shown for WebAPKs

This CL hides the "Site setting" action when notifications are shown by WebAPKs.
The "additional settings in the app" link will be removed from the notification
settings for WebAPKs in the System UI in Android O. Before Android O, the
settings gear to open app-specific settings shown on the rear of a notification
following a long-press will be gone as well.

BUG=654539

Review-Url: https://codereview.chromium.org/2826223002
Cr-Commit-Position: refs/heads/master@{#467843}
Committed:
https://chromium.googlesource.com/chromium/src/+/4df7199f5d2cafd62bf9c9d016ca...
==========

[commit-bot: I haz the power, 2017-04-28 02:15:38]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/4df7199f5d2cafd62bf9c9d016ca...