Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841193002:
Implement privacy disclosure for an unbound webapk. (Closed)
|
Created:
3 years, 7 months ago by Yaron Modified:
3 years, 6 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, awdf+watch_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, pkotwicz+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, zpeng+watch_chromium.org, srahim+watch_chromium.org, piotrs Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement privacy disclosure for an unbound webapk.
Some WebApks can be run with different browser hosts. This CL adds a
notification that the WebApk is using Chrome as a host and may share
browsing data with the app.
BUG=714738
Review-Url: https://codereview.chromium.org/2841193002
Cr-Commit-Position: refs/heads/master@{#475350}
Committed: https://chromium.googlesource.com/chromium/src/+/88c8153ffb73869b07a17c3d804630e401c730fd
Patch Set 1 #
Total comments: 11
Patch Set 2 : Implement privacy disclosure for an unbound webapk. #Patch Set 3 : Implement privacy disclosure for an unbound webapk. #
Total comments: 7
Patch Set 4 #
Total comments: 11
Patch Set 5 : fixes #Patch Set 6 : fix test #
Total comments: 4
Patch Set 7 : findbug #
Total comments: 12
Patch Set 8 : awdf comments #
Total comments: 6
Patch Set 9 : awdf comments #
Total comments: 6
Patch Set 10 : pkotwicz comments #
Total comments: 2
Patch Set 11 : nit #Messages
Total messages: 50 (22 generated)
yfriedman@chromium.org changed reviewers: + awdf@chromium.org, pkotwicz@chromium.org
awdf: I know there's a lot of trickiness now with notifications so please let me know if I missed something. I'm not entirely sure of the group, channels, etc (I'll also share the mock with you separately) pkotwicz: webapk part. As you'll note the impl is done in chrome - without a translation story I'm not sure how we can do this from the webapk itself. I guess interestingly chrome could also display the notification via the webapk but I think the notification should really originate from the browser host. A bad actor may not offer the disclosure but they may also have different policies. I'm also not sure how a WebApk could actually enforce showing this UI (unless it were using CCT). Happy to discuss/explore further if you're interested
https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java (right): https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:83: public static final String GROUP_WEBAPK = "WebApk"; Nit: Keep the constants in alphabetical order https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:126: WebApkNotificationManager.showDisclosure(mWebappInfo); Shouldn't this code be in onStartWithNative() instead of in onResumeWithNative() ? https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java:26: public class WebApkNotificationManager { You should probably name this class WebApkDisclosureNotificationManager or WebApkDisclosureManager. This class does not manage all of a WebAPK's notifications https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java:58: .setGroup(NotificationConstants.GROUP_WEBAPK); According to the mock in https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F... the "system tray icon" should be hidden. I think that you can hide the system tray icon via setPriority(Notification.PRIORITY_MIN) https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java:61: nm.notify(WEBAPK_OPEN_TAG, WEBAPK_OPEN_ID, builder.buildWithBigTextStyle(actionMessage)); Shouldn't this notification come from the WebAPK and not from Chrome? In particular, shouldn't you use WebApkNotificationClient#notifyNotification() We are in a weird world where there are three types of WebAPKs 1) WebAPKs created by the browser 2) Play WebAPKs 3) Escape hatch APKs (1) and (2) get the special privilege of sharing cookies with the Web browser. (3) does not. Hence we need to show the disclosure UI for (1) and (2) but not (3) I don't think that we need to worry about malicious actors unless we share Chrome's cookie jar in the case of (3)
Thanks removed the icon from status bar and I also moved the intent from the content to an action. Rather than "More Info", I used "Learn more" since it matches other places in chrome and doesn't require add'l translation +piotrs - I know you're working on a more fully fledged version of this but we wanted something short term for WebApks that doesn't depend on the entire resolution of your doc (and all available browser actions) https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java (right): https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:83: public static final String GROUP_WEBAPK = "WebApk"; On 2017/04/27 19:04:25, pkotwicz wrote: > Nit: Keep the constants in alphabetical order Done. https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:126: WebApkNotificationManager.showDisclosure(mWebappInfo); On 2017/04/27 19:04:25, pkotwicz wrote: > Shouldn't this code be in onStartWithNative() instead of in onResumeWithNative() > ? Don't think so. If you move it to background and bring it foreground, you want it to come back https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java:26: public class WebApkNotificationManager { On 2017/04/27 19:04:26, pkotwicz wrote: > You should probably name this class WebApkDisclosureNotificationManager or > WebApkDisclosureManager. This class does not manage all of a WebAPK's > notifications Done. https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java:58: .setGroup(NotificationConstants.GROUP_WEBAPK); On 2017/04/27 19:04:25, pkotwicz wrote: > According to the mock in > https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F... > the "system tray icon" should be hidden. > > I think that you can hide the system tray icon via > setPriority(Notification.PRIORITY_MIN) Done. https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkNotificationManager.java:61: nm.notify(WEBAPK_OPEN_TAG, WEBAPK_OPEN_ID, builder.buildWithBigTextStyle(actionMessage)); On 2017/04/27 19:04:26, pkotwicz wrote: > Shouldn't this notification come from the WebAPK and not from Chrome? In > particular, shouldn't you use WebApkNotificationClient#notifyNotification() > > We are in a weird world where there are three types of WebAPKs > 1) WebAPKs created by the browser > 2) Play WebAPKs > 3) Escape hatch APKs > > (1) and (2) get the special privilege of sharing cookies with the Web browser. > (3) does not. Hence we need to show the disclosure UI for (1) and (2) but not > (3) > > I don't think that we need to worry about malicious actors unless we share > Chrome's cookie jar in the case of (3) Per the mocks (and mirroring InstantApps behaviours) I kept this in chrome. This is going through some larger discussions but we need something as a stop-gap for now. We don't yet know how escape hatch will work so it's hard to account for it and not show the notification :/
Looks like WebApkDisclosureNotificationManager is missing from the CL
Also added back the new file :S https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:126: WebApkNotificationManager.showDisclosure(mWebappInfo); On 2017/04/28 20:31:27, Yaron (limited availability) wrote: > On 2017/04/27 19:04:25, pkotwicz wrote: > > Shouldn't this code be in onStartWithNative() instead of in > onResumeWithNative() > > ? > > Don't think so. If you move it to background and bring it foreground, you want > it to come back My mistake - thanks for clarifying. Moved to onStartWithNative (since some metrics are recorded, native is needed)
lgtm % nit I think it's fine to send this notification to the Browser channel on O. Since it is nondismissable you might want to consider whether it should be configurable (ie use a new channel on O / add a toggle pre-O) for users who find it annoying, though I note it's already min priority which helps. You've added UMA anyway which should tell us if a lot of people turn off notifications for all of Chrome after seeing one of these. (Really sorry I didn't see this sooner - turns out if you toggle rietveld deprecated ui on/off if changes your mailto:+reviewer settings so this didn't hit my 'TO REVIEW' email filter - d'oh. Bring on the gerrit revolution!) https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:40: public static void showDisclosure(WebappInfo mWebappInfo) { nit: s/mWebAppInfo/webAppInfo (& in param documentation above) https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (left): https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2856: <message name="IDS_WEBAPK_UNKNOWN_SOURCES_DIALOG_TITLE" desc="Title of dialog warning user that installation from Unknown sources is required for WebAPK experiment."> are these 3 strings now unused then?
https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (left): https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2856: <message name="IDS_WEBAPK_UNKNOWN_SOURCES_DIALOG_TITLE" desc="Title of dialog warning user that installation from Unknown sources is required for WebAPK experiment."> Yes these strings are now unused. Yaron can you please remove them? I forgot to remove them in https://codereview.chromium.org/2687973005
- The notification looks wrong on Android N See https://drive.google.com/a/google.com/file/d/0B7EmxgaPYQTBcmVfejhtN2tVVU0/vie... In particular the user has to expand the notification to see the disclosure message. - Have you tested this CL when you have two WebAPKs open in multi window mode? https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:101: // "Unbound WebApk" (crbug/714735) so show a notification that it's backed by Chrome. crbug/714735 -> crbug.com/714735
https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:17: import org.chromium.chrome.browser.notifications.ChannelDefinitions; fyi i just hit 'submit' on this cl which moves ChannelDefinitions to a notifications.channels subpackage https://codereview.chromium.org/2863623002/ - you might need to manually fix the import after a rebase once that's in.
Thanks for the heads-up! I've been blocked on getting back to this cause of something else that's come up - I'll update it when I get back to this
yfriedman@chromium.org changed reviewers: + dominickn@chromium.org
+dominickn I still have one issue to resolve but was wondering what you think of this high-level before I finish it up. Namely: 1) Unbound webapks show a hidden notification (this required the changes to chrome's notification classes - Anita: let me know if you want me to change this back to chrome's wrapper of NotificationBuilder vs re-using the push notification's class-hierarchy.) 2) We record whether the notification was dismissed by the user and stop showing it in the future 3) Notifications are presented from the webapk and I've updated the strings per latest spec https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:101: // "Unbound WebApk" (crbug/714735) so show a notification that it's backed by Chrome. On 2017/05/02 18:03:29, pkotwicz wrote: > crbug/714735 -> crbug.com/714735 Done. https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:40: public static void showDisclosure(WebappInfo mWebappInfo) { On 2017/05/02 12:33:35, awdf wrote: > nit: s/mWebAppInfo/webAppInfo (& in param documentation above) Done. https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:168: mNotificationId = WebApkDisclosureNotificationManager.showDisclosure( note, because I moved this out of onStartWithNative, it doesn't re-show if you re-open a webapk, need to come back to this
High level, webapps looks fine, modulo a couple of nits and the fix for the reshow on switch back. https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:28: Nit: extra newline https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:32: public static int showDisclosure(WebappInfo webappInfo, WebappDataStorage storage) { The WebappDataStorage isn't currently used in here? Perhaps it could be passed to getDeleteIntent in place of the ID (WebappDataStorage should only be used on the UI thread so this should be safe)? Otherwise omit? https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. Nit: 2017
https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:150: if (mVisibility != Notification.VISIBILITY_SECRET) { maybe change this to check a new channel id field - see my other comment https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:30: * @return identifier used to display the notification nit: indentation is weird https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:51: NotificationUmaTracker.WEBAPK, ChannelDefinitions.CHANNEL_ID_BROWSER); Here you're recording that the notification is assigned to the Browser channel, but actually the StandardNotificationBuilder (SNB) sets the channel id to CHANNEL_ID_SITES under the hood. You probably want to use the Browser channel in both places, since the browser channel is for silent notifications whereas the sites channel uses default sound/vibrate options.* The fact the SNB is hardcoded to the SITES channel is a bug - the SNB is intended to be usable for any notifications, not tied to web notifications (although it's only used for web notifications right now). To fix this, I would make the channel ID a constructor-parameter of the SNB, and then check the channel id instead of VISIBILITY SECRET when conditionally setting vibrate etc. (Or, you could change this back to using chrome's wrapper of NotificationBuilder and we can fix up SNB to be more generic at a later date, if/when we start using it for all Chrome notifications.. I would slightly prefer the other solution, but ok with this too) *In practice these notifications won't actually be sent to any channel until we create channels for WebApks, once they start targeting O, so the channel properties will be ignored, but let's keep things consistent for simplicity. https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:24: static PendingIntent getDeleteIntent(Context context, String packageName) { I'm not sure it's okay to start a service directly from a notification's PendingIntent, particularly on O with the new background check restrictions. Do you really need a service here anyway? - If you just need to perform a one-off task that can be run on the main thread in under 10s, you can just use a BroadcastReceiver and do the work in onReceive. - If it'll take longer than 10s / needs to run in the background, you should still use a BroadcastReceiver, but schedule a job from onReceive using the new BackgroundScheduler. - This is what we do for web notification click intents, more or less.
https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:32: public static int showDisclosure(WebappInfo webappInfo, WebappDataStorage storage) { On 2017/05/24 01:24:36, dominickn wrote: > The WebappDataStorage isn't currently used in here? Perhaps it could be passed > to getDeleteIntent in place of the ID (WebappDataStorage should only be used on > the UI thread so this should be safe)? > > Otherwise omit? omitted - can't pass it as part of the intent so prefer to stick with a string extra https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:24: static PendingIntent getDeleteIntent(Context context, String packageName) { On 2017/05/24 14:47:42, awdf wrote: > I'm not sure it's okay to start a service directly from a notification's > PendingIntent, particularly on O with the new background check restrictions. > > Do you really need a service here anyway? > > - If you just need to perform a one-off task that can be run on the main thread > in under 10s, you can just use a BroadcastReceiver and do the work in onReceive. > > - If it'll take longer than 10s / needs to run in the background, you should > still use a BroadcastReceiver, but schedule a job from onReceive using the new > BackgroundScheduler. > - This is what we do for web notification click intents, more or less. I was mirroring what was done for the IncognitoNotification{Service,Manager}. A local receiver may also work, will have to look at notification code
On 2017/05/24 14:59:25, Yaron wrote: > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java > (right): > > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:32: > public static int showDisclosure(WebappInfo webappInfo, WebappDataStorage > storage) { > On 2017/05/24 01:24:36, dominickn wrote: > > The WebappDataStorage isn't currently used in here? Perhaps it could be passed > > to getDeleteIntent in place of the ID (WebappDataStorage should only be used > on > > the UI thread so this should be safe)? > > > > Otherwise omit? > > omitted - can't pass it as part of the intent so prefer to stick with a string > extra > > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java > (right): > > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:24: > static PendingIntent getDeleteIntent(Context context, String packageName) { > On 2017/05/24 14:47:42, awdf wrote: > > I'm not sure it's okay to start a service directly from a notification's > > PendingIntent, particularly on O with the new background check restrictions. > > > > Do you really need a service here anyway? > > > > - If you just need to perform a one-off task that can be run on the main > thread > > in under 10s, you can just use a BroadcastReceiver and do the work in > onReceive. > > > > - If it'll take longer than 10s / needs to run in the background, you should > > still use a BroadcastReceiver, but schedule a job from onReceive using the new > > BackgroundScheduler. > > - This is what we do for web notification click intents, more or less. > > I was mirroring what was done for the IncognitoNotification{Service,Manager}. > A local receiver may also work, will have to look at notification code huh, you're right it does. I looked into it some more and sounds like there is some special-casing for pending intents associated with notifications that puts the pending intent's target app on a whitelist against the bg check restrictions, so we should be okay in the incognito case (and perhaps we would have been okay without scheduling a job from the web notification clicks). But I'm struggling to reason about which package would be whitelisted in this webapk example..
On 2017/05/24 15:29:11, awdf wrote: > On 2017/05/24 14:59:25, Yaron wrote: > > > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java > > (right): > > > > > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:32: > > public static int showDisclosure(WebappInfo webappInfo, WebappDataStorage > > storage) { > > On 2017/05/24 01:24:36, dominickn wrote: > > > The WebappDataStorage isn't currently used in here? Perhaps it could be > passed > > > to getDeleteIntent in place of the ID (WebappDataStorage should only be used > > on > > > the UI thread so this should be safe)? > > > > > > Otherwise omit? > > > > omitted - can't pass it as part of the intent so prefer to stick with a string > > extra > > > > > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java > > (right): > > > > > https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:24: > > static PendingIntent getDeleteIntent(Context context, String packageName) { > > On 2017/05/24 14:47:42, awdf wrote: > > > I'm not sure it's okay to start a service directly from a notification's > > > PendingIntent, particularly on O with the new background check restrictions. > > > > > > Do you really need a service here anyway? > > > > > > - If you just need to perform a one-off task that can be run on the main > > thread > > > in under 10s, you can just use a BroadcastReceiver and do the work in > > onReceive. > > > > > > - If it'll take longer than 10s / needs to run in the background, you should > > > still use a BroadcastReceiver, but schedule a job from onReceive using the > new > > > BackgroundScheduler. > > > - This is what we do for web notification click intents, more or less. > > > > I was mirroring what was done for the IncognitoNotification{Service,Manager}. > > A local receiver may also work, will have to look at notification code > > huh, you're right it does. > > I looked into it some more and sounds like there is some special-casing for > pending intents associated with notifications that puts the pending intent's > target app on a whitelist against the bg check restrictions, so we should be > okay in the incognito case (and perhaps we would have been okay without > scheduling a job from the web notification clicks). But I'm struggling to reason > about which package would be whitelisted in this webapk example.. Digging through the source, I think this is fine. It iterates through the targets of all pendingintents on the notification. Even though the notification is dispatched by the webapk, the pendingintents still target chrome so they'd be whitelisted.
The CQ bit was checked by yfriedman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by yfriedman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ok, ready for another pass. I commented on a few changes that I made locally but didn't bother uploading yet. https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:28: On 2017/05/24 01:24:36, dominickn wrote: > Nit: extra newline Done. https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:151: if (!ChannelDefinitions.CHANNEL_ID_BROWSER.equals(mChannelId)) { do you want a comment saying that Browser channel defaults to silent? https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:306: * Sets the priority of the notification (if set to private, overrides \setDefaults| and will fix comment https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java (right): https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:13: * Service that handles the action of clicking on the incognito notification. fixed "incognito reference"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by yfriedman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/2841193002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:151: if (!ChannelDefinitions.CHANNEL_ID_BROWSER.equals(mChannelId)) { On 2017/05/24 18:08:29, Yaron wrote: > do you want a comment saying that Browser channel defaults to silent? yes please. https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:83: // TODO(yfriedman): Pass in the ChannelDefinition yes please! either as part of this review or file a UI>Notifications bug so we don't forget, thanks. https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:128: protected final String mChannelId; nit: move mChannelId up next to the other final fields https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:378: false /* preferCompat */, ChannelDefinitions.CHANNEL_ID_SITES) please change this to use mChannelId now that it is passed in. https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:21: * Manages the notification indicating that a WebApk is backed by chrome code and may share data. nit: A little more detail about the expected lifetime of the notification would be useful here (It's shown when an unbound web apk is displayed in the foreground, until the notification is dismissed or the web apk is no longer in the foreground, right?) https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:47: int id = r.nextInt(); Do we really need to use a new notification id each time? There's only going to be one of these notifications active at a time per web apk, right? If that's the case you could instead distinguish them by passing the package name, or maybe wepappInfo.id() (?), as the platformTag to WebApkNotificationClient.notifyNotification and use a constant notification id, similar to what we do with web notifications - see https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:59: * @param id The identifier for the tag to dismiss. nit: s/tag/notification (although perhaps this param is no longer required if you use my other suggestion)
https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:83: // TODO(yfriedman): Pass in the ChannelDefinition On 2017/05/25 13:56:45, awdf wrote: > yes please! either as part of this review or file a UI>Notifications bug so we > don't forget, thanks. filed crbug/726340 for follow-up https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:128: protected final String mChannelId; On 2017/05/25 13:56:45, awdf wrote: > nit: move mChannelId up next to the other final fields Done. https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:378: false /* preferCompat */, ChannelDefinitions.CHANNEL_ID_SITES) On 2017/05/25 13:56:45, awdf wrote: > please change this to use mChannelId now that it is passed in. Done. https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:21: * Manages the notification indicating that a WebApk is backed by chrome code and may share data. On 2017/05/25 13:56:45, awdf wrote: > nit: A little more detail about the expected lifetime of the notification would > be useful here (It's shown when an unbound web apk is displayed in the > foreground, until the notification is dismissed or the web apk is no longer in > the foreground, right?) Done. https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:47: int id = r.nextInt(); On 2017/05/25 13:56:45, awdf wrote: > Do we really need to use a new notification id each time? There's only > going to be one of these notifications active at a time per web apk, right? If > that's the case you could instead distinguish them by passing the package name, > or maybe wepappInfo.id() (?), as the platformTag to > WebApkNotificationClient.notifyNotification and use a constant notification id, > similar to what we do with web notifications > - see > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > Done. https://codereview.chromium.org/2841193002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:59: * @param id The identifier for the tag to dismiss. On 2017/05/25 13:56:45, awdf wrote: > nit: s/tag/notification > > (although perhaps this param is no longer required if you use my other > suggestion) Done.
lgtm % nits https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:83: // TODO(yfriedman): Pass in the ChannelDefinition nit: TODO(crbug.com/726340) now we have a bug number (thanks for filing!) https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:31: * @return identifier used to display the notification nit: remove @return doc https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:49: webappInfo.webApkPackageName(), PLATFORM_ID); nit: I know I suggested using the package name for the notification tag, but probably worth making the tag dismissal-specific, in case other web apk notifications get introduced in future with the same id. So I'd suggest passing something like DIMISSAL_NOTIFICATION_TAG_PREFIX + webappInfo.webApkPackageName() , where DISMISSAL_NOTIFICATION_TAG_PREFIX = "dismissal_notification_tag_prefix", instead of just webappInfo.webApkPackageName() for the tag.
The CQ bit was checked by yfriedman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:174: maybeShowDisclosure(storage); It is possible for onDeferredStartup() to be called prior to onStartWithNative() if a user opens a WebAPK and switches to recents immediately. This occurs because native is loaded after onStop() is called. This means that it is possible for the notification to show when the WebAPK is not in the foreground. https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java (right): https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:23: static PendingIntent getDeleteIntent(Context context, String packageName) { Nit: Maybe rename |packageName| to |webApkPackageName| to make it clear that |packageName| does not refer to |context|'s package name. https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:38: Nit: Delete new line.
https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java (right): https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:83: // TODO(yfriedman): Pass in the ChannelDefinition On 2017/05/25 15:54:29, awdf wrote: > nit: TODO(crbug.com/726340) now we have a bug number (thanks for filing!) Done. https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java (right): https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:31: * @return identifier used to display the notification On 2017/05/25 15:54:29, awdf wrote: > nit: remove @return doc Done. https://codereview.chromium.org/2841193002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java:49: webappInfo.webApkPackageName(), PLATFORM_ID); On 2017/05/25 15:54:29, awdf wrote: > nit: I know I suggested using the package name for the notification tag, but > probably worth making the tag dismissal-specific, in case other web apk > notifications get introduced in future with the same id. > > So I'd suggest passing something like > DIMISSAL_NOTIFICATION_TAG_PREFIX + webappInfo.webApkPackageName() > , where DISMISSAL_NOTIFICATION_TAG_PREFIX = "dismissal_notification_tag_prefix", > instead of just webappInfo.webApkPackageName() for the tag. We already do use some notifications in the WebApk (e.g. webpush) but those have a different tax. I guess it doesn't hurt to namespace this one though - done. https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:174: maybeShowDisclosure(storage); On 2017/05/25 22:08:42, pkotwicz wrote: > It is possible for onDeferredStartup() to be called prior to onStartWithNative() > if a user opens a WebAPK and switches to recents immediately. This occurs > because native is loaded after onStop() is called. > > This means that it is possible for the notification to show when the WebAPK is > not in the foreground. Done. https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java (right): https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:23: static PendingIntent getDeleteIntent(Context context, String packageName) { On 2017/05/25 22:08:43, pkotwicz wrote: > Nit: Maybe rename |packageName| to |webApkPackageName| to make it clear that > |packageName| does not refer to |context|'s package name. Done. https://codereview.chromium.org/2841193002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationService.java:38: On 2017/05/25 22:08:43, pkotwicz wrote: > Nit: Delete new line. Done.
WebApkActivity.java LGTM
lgtm https://codereview.chromium.org/2841193002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:31: Nit: extra newline
thanks https://codereview.chromium.org/2841193002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2841193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:31: On 2017/05/29 04:34:56, dominickn wrote: > Nit: extra newline Done.
thanks
The CQ bit was checked by yfriedman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from awdf@chromium.org, dominickn@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2841193002/#ps200001 (title: "nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496065106929040,
"parent_rev": "54841760a98142fac982f3ec12064093b8c3a7da", "commit_rev":
"88c8153ffb73869b07a17c3d804630e401c730fd"}
Message was sent while issue was closed.
Description was changed from ========== Implement privacy disclosure for an unbound webapk. Some WebApks can be run with different browser hosts. This CL adds a notification that the WebApk is using Chrome as a host and may share browsing data with the app. BUG=714738 ========== to ========== Implement privacy disclosure for an unbound webapk. Some WebApks can be run with different browser hosts. This CL adds a notification that the WebApk is using Chrome as a host and may share browsing data with the app. BUG=714738 Review-Url: https://codereview.chromium.org/2841193002 Cr-Commit-Position: refs/heads/master@{#475350} Committed: https://chromium.googlesource.com/chromium/src/+/88c8153ffb73869b07a17c3d8046... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/88c8153ffb73869b07a17c3d8046... |
