Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2316263002:
Notifications in sensitive contexts now display origin + small icon (Closed)
DescriptionNotifications in sensitive contexts now display origin + small icon
BUG=498716
Committed: https://crrev.com/b75661d3d5c5924cffc942945d79d0430b812e1c
Cr-Commit-Position: refs/heads/master@{#418817}
Patch Set 1 : Notifications in sensitive contexts now display origin + small icon #
Total comments: 9
Patch Set 2 : Tweaked string description + use normal Notification builder in tests #Patch Set 3 : Moved public notification creation and icon generation to NotificationBuilderBase #
Total comments: 24
Patch Set 4 : Review comments #
Total comments: 9
Patch Set 5 : Notifications in sensitive contexts now display origin + small icon #Messages
Total messages: 48 (29 generated)
The CQ bit was checked by awdf@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.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
The CQ bit was unchecked by awdf@chromium.org
The CQ bit was checked by awdf@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...
Screenshots available at https://drive.google.com/open?id=0B4fXBlY3PNasVmRuc2FacVFvcTA showing before/after Patch Set 1 on Android L, M and N, with and without a supplied badge icon.
awdf@chromium.org changed reviewers: + peter@chromium.org
what do you think about refactoring as per my comment? screenshots are up to date at https://drive.google.com/open?id=0B4fXBlY3PNasVmRuc2FacVFvcTA btw https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:602: if (badge != null && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { I don't really like all these version checks, would probably be neater to use the fact we already have two types of notification builder (basically depending on N+ vs L/M), and pull all the public notification generation into their build() methods. However, doing that might be less clear than explicitly having a setPublicVersion method on those builders as I've done here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool, this is good :) Thanks! Some opinions follow. As mentioned on chat, you can consider moving the screenshots to your @chromium.org account and share them with the world. https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:586: private Notification createPublicNotification(Bitmap badge, String originForDisplay) { What do you think about moving this to NotificationBuilderBase? It would isolate android.app.Notification usage to that set of classes, and we might be able to move the |mIconGenerator| member there too. https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:602: if (badge != null && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { On 2016/09/09 09:51:01, awdf wrote: > I don't really like all these version checks, would probably be neater to use > the fact we already have two types of notification builder (basically depending > on N+ vs L/M), and pull all the public notification generation into their > build() methods. > > However, doing that might be less clear than explicitly having a > setPublicVersion method on those builders as I've done here. I don't mind the version checks too much, the intention check is well documented and none of the code is hard to follow. Moving this to NotificationBuilderBase (we can't rely on CustomNotificationBuilder being <N as users have the ability to enable/disable custom layouts themselves) might help, since it already has all the necessary information anyway. Having some NotificationBuilderBase.commonBuild() (modulo naming :P) might remove duplication from the {Custom,Standard}NotificationBuilder.build() methods too? https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1651: <message name="IDS_NOTIFICATION_HIDDEN_TEXT" desc="Text shown in place of notification contents when the notification is hidden on a secure lockscreen [CHAR-LIMIT=32]"> nit: "the notification is hidden" -> "the notification's contents have been hidden" https://codereview.chromium.org/2316263002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:77: .setPublicVersion(new CustomNotificationBuilder(context) Mm. This reads a bit odd since we don't use a custom notification builder for the public version in production. Maybe use a regular Notification object here too?
On 2016/09/09 09:45:45, awdf wrote: > Screenshots available at > https://drive.google.com/open?id=0B4fXBlY3PNasVmRuc2FacVFvcTA showing > before/after Patch Set 1 on Android L, M and N, with and without a supplied > badge icon. And at https://drive.google.com/open?id=0B13NYI1M3t93aVJlWi03RDdrTUU viewable by *@chromium.org accounts
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:586: private Notification createPublicNotification(Bitmap badge, String originForDisplay) { On 2016/09/09 12:53:31, Peter Beverloo wrote: > What do you think about moving this to NotificationBuilderBase? It would isolate > android.app.Notification usage to that set of classes, and we might be able to > move the |mIconGenerator| member there too. I agree it makes sense to move this method (& icon generator) to NotificationBuilderBase. But where would you call it from - setPublicVersion() (which would have to take two arguments), or get rid of the setter & call it from the subclass build() methods? https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1651: <message name="IDS_NOTIFICATION_HIDDEN_TEXT" desc="Text shown in place of notification contents when the notification is hidden on a secure lockscreen [CHAR-LIMIT=32]"> On 2016/09/09 12:53:31, Peter Beverloo wrote: > nit: "the notification is hidden" -> "the notification's contents have been > hidden" Done. I actually just copied this from Android's own description for the same string, but happy to go with your version :) https://codereview.chromium.org/2316263002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:77: .setPublicVersion(new CustomNotificationBuilder(context) On 2016/09/09 12:53:31, Peter Beverloo wrote: > Mm. This reads a bit odd since we don't use a custom notification builder for > the public version in production. Maybe use a regular Notification object here > too? Oops that was just leftover from before I switched to using regular Notifications. Good spot. Fixed now.
https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2316263002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:586: private Notification createPublicNotification(Bitmap badge, String originForDisplay) { On 2016/09/09 15:55:26, awdf wrote: > On 2016/09/09 12:53:31, Peter Beverloo wrote: > > What do you think about moving this to NotificationBuilderBase? It would > isolate > > android.app.Notification usage to that set of classes, and we might be able to > > move the |mIconGenerator| member there too. > > I agree it makes sense to move this method (& icon generator) to > NotificationBuilderBase. But where would you call it from - setPublicVersion() > (which would have to take two arguments), or get rid of the setter & call it > from the subclass build() methods? Calling it from the subclass' build() methods makes a lot of sense to me. Instead of mPublicVersion we can have createPublicNotification() that uses the information that's already available.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by peter@chromium.org to run a CQ dry run
This looks great, thank you! I have a few suggestions, and otherwise some nits. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:65: static final int NOTIFICATION_TEXT_SIZE_DP = 28; nit: consider adding docs for these constants (I realize that there weren't any before, but the other two constants in this class do) https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:129: public NotificationBuilderBase setOrigin(@Nullable String origin) { Is this significant? It uses a CharSequence everywhere else. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:265: protected Bitmap getNormalizedLargeIcon() { nit (here and on line 292): please document non-private methods I don't think the style guide mandates this, but consistency within a file takes precedent. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:292: protected Notification createPublicNotification(Context context) { Does this need an @TargetApi annotation because you're using Icon? https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:306: } It might be clearer to turn this around, because we don't know what future Android versions will do. if (N) { use subtext } else { use content title } https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java:23: public Notification build() { dito re: @TargetApi https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java:135: public Bitmap generateIconForUrl(CharSequence url, boolean includePrivateRegistries) { It looks like this method requires a |url| String regardless, so could we just call .toString() at the call-site where we have a CharSequence? https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java:23: loadNativeLibraryNoBrowserProcess(); Would it be worth documenting that not initializing the browser process is safe because GetDomainAndRegistry() (called through the RoundedIconGenerator) is stand-alone? This can be a potential foot-gun if another test gets added that does require initialized parts of the browser process. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java:51: }; There's nothing in your code that calls build(), so I don't think it's necessary to override it? I do love this syntax, though. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:433: private RoundedIconGenerator getRoundedIconGenerator() { Something to consider: could reduce duplication by creating a static NotificationBuilderBase.createRoundedIconGenerator(). https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:29: loadNativeLibraryNoBrowserProcess(); dito: re comment because of the potential foot gun
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.
Patchset #5 (id:160001) has been deleted
Patchset #4 (id:140001) has been deleted
I rebased so there's some irrelevant changes between the latest diffs. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:65: static final int NOTIFICATION_TEXT_SIZE_DP = 28; On 2016/09/12 17:52:02, Peter Beverloo wrote: > nit: consider adding docs for these constants (I realize that there weren't any > before, but the other two constants in this class do) Done. Also renamed to NOTIFICATION_*ICON*_TEXT_SIZE_DP since that's all it's used for. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:129: public NotificationBuilderBase setOrigin(@Nullable String origin) { On 2016/09/12 17:52:02, Peter Beverloo wrote: > Is this significant? It uses a CharSequence everywhere else. oops, no it's not significant - Done. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:265: protected Bitmap getNormalizedLargeIcon() { On 2016/09/12 17:52:02, Peter Beverloo wrote: > nit (here and on line 292): please document non-private methods > > I don't think the style guide mandates this, but consistency within a file takes > precedent. Done. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:292: protected Notification createPublicNotification(Context context) { On 2016/09/12 17:52:02, Peter Beverloo wrote: > Does this need an @TargetApi annotation because you're using Icon? I don't it's necessary because it seems the linter is smart enough to figure out the call is within a version code if-check - at least, Android Studio doesn't give me any inspection warnings and no warnings about it are printed when I build.. (and they are if I remove the if-check). But for consistency with the other @TargetApi annotations in this class I could add it if you prefer? https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:306: } On 2016/09/12 17:52:02, Peter Beverloo wrote: > It might be clearer to turn this around, because we don't know what future > Android versions will do. > > if (N) { use subtext } > else { use content title } Hm not sure how to do that because currently we target M (sdk 23) so the constant Build.VERSION_CODES.N is not available for us to match against! Do you know a way around this? https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java:135: public Bitmap generateIconForUrl(CharSequence url, boolean includePrivateRegistries) { On 2016/09/12 17:52:02, Peter Beverloo wrote: > It looks like this method requires a |url| String regardless, so could we just > call .toString() at the call-site where we have a CharSequence? Yes, but it will need null checks at the two call-sites in case origin is null (true in some tests at least). Done. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1297: Sync is not available for your domain. i think this is just a rebase artefact https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java:23: loadNativeLibraryNoBrowserProcess(); On 2016/09/12 17:52:03, Peter Beverloo wrote: > Would it be worth documenting that not initializing the browser process is safe > because GetDomainAndRegistry() (called through the RoundedIconGenerator) is > stand-alone? This can be a potential foot-gun if another test gets added that > does require initialized parts of the browser process. Done. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java:51: }; On 2016/09/12 17:52:02, Peter Beverloo wrote: > There's nothing in your code that calls build(), so I don't think it's necessary > to override it? > > I do love this syntax, though. NotificationBuilderBase is abstract so in order to instantiate it to call ensureNormalizedIcon I have to create an implementation, which needs to override abstract method build(). I could instantiate one of the existing implementations instead, but this seemed neater. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:433: private RoundedIconGenerator getRoundedIconGenerator() { On 2016/09/12 17:52:03, Peter Beverloo wrote: > Something to consider: could reduce duplication by creating a static > NotificationBuilderBase.createRoundedIconGenerator(). Done. https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:29: loadNativeLibraryNoBrowserProcess(); On 2016/09/12 17:52:03, Peter Beverloo wrote: > dito: re comment because of the potential foot gun Done.
lgtm! Thanks! :) https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:306: } On 2016/09/14 12:30:29, awdf wrote: > On 2016/09/12 17:52:02, Peter Beverloo wrote: > > It might be clearer to turn this around, because we don't know what future > > Android versions will do. > > > > if (N) { use subtext } > > else { use content title } > > Hm not sure how to do that because currently we target M (sdk 23) so the > constant Build.VERSION_CODES.N is not available for us to match against! Do you > know a way around this? Aaah, yes, you're right. Maybe add a TODO because it'd be a nice (super low priority) cleanup once we change our target sdk? https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java (right): https://codereview.chromium.org/2316263002/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java:51: }; On 2016/09/14 12:30:29, awdf wrote: > On 2016/09/12 17:52:02, Peter Beverloo wrote: > > There's nothing in your code that calls build(), so I don't think it's > necessary > > to override it? > > > > I do love this syntax, though. > > NotificationBuilderBase is abstract so in order to instantiate it to call > ensureNormalizedIcon I have to create an implementation, which needs to override > abstract method build(). I could instantiate one of the existing implementations > instead, but this seemed neater. Acknowledged. https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:311: // Use Android's Notification.Builder because we want the default small icon behaviour. nit: the icon behaviour is identical to our notification builder, the primary difference is not having to use custom layouts, which incur a huge cost. Maybe just remove the comment? https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:331: } else if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.M && mOrigin != null) { Did you find a situation where mOrigin is NULL? That's something we'd assert against in C++. Check's fine since we can't assert, though. https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java:138: String text = getIconTextForUrl(url.toString(), includePrivateRegistries); nit: |url| already is a String? https://codereview.chromium.org/2316263002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:208: if (Build.VERSION.SDK_INT > Build.VERSION_CODES.M) { nit: could hoist lines 208-216 in the if() conditional above, since it's still a prerequisite. up to you
The CQ bit was checked by awdf@chromium.org
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/2316263002/#ps200001 (title: "Notifications in sensitive contexts now display origin + small icon")
https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:331: } else if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.M && mOrigin != null) { On 2016/09/14 14:06:54, Peter Beverloo wrote: > Did you find a situation where mOrigin is NULL? That's something we'd assert > against in C++. Check's fine since we can't assert, though. No - but it made tests fail, and this was a quick fix :) I did note that there is no verification within the build() method that any particular fields have been set, so I assumed no fields were really required. If you're saying origin should be required then we could throw an IllegalStateException if origin hasn't been set, or an IllegalArgumentException if the origin passed to NPB.displayNotification is null or empty.. what do you think? https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/RoundedIconGenerator.java:138: String text = getIconTextForUrl(url.toString(), includePrivateRegistries); On 2016/09/14 14:06:54, Peter Beverloo wrote: > nit: |url| already is a String? oops, good spot, thanks. Done. https://codereview.chromium.org/2316263002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:208: if (Build.VERSION.SDK_INT > Build.VERSION_CODES.M) { On 2016/09/14 14:06:54, Peter Beverloo wrote: > nit: could hoist lines 208-216 in the if() conditional above, since it's still a > prerequisite. up to you Yes, but then Android Studio gives me API warnings, because it's not *that* clever about things being inside if-checks, apparently -_- so I think I'll leave it, rather than adding a TargetApi annotation to the whole method or extracting out a new method to apply one to.
The CQ bit was unchecked by awdf@chromium.org
The CQ bit was checked by awdf@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:331: } else if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.M && mOrigin != null) { On 2016/09/14 16:30:05, awdf wrote: > On 2016/09/14 14:06:54, Peter Beverloo wrote: > > Did you find a situation where mOrigin is NULL? That's something we'd assert > > against in C++. Check's fine since we can't assert, though. > > No - but it made tests fail, and this was a quick fix :) > > I did note that there is no verification within the build() method that any > particular fields have been set, so I assumed no fields were really required. If > you're saying origin should be required then we could throw an > IllegalStateException if origin hasn't been set, or an IllegalArgumentException > if the origin passed to NPB.displayNotification is null or empty.. what do you > think? Ok. I think there's a difference between library or API methods, where throwing an IAE would definitely be justified, and a design contract imposed by code that's fully under our control. (Though this will be biased by my C++ driven mind.) You'd want to use an assert in the latter case, but these don't actually do something yet. https://codereview.chromium.org/2316263002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2316263002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:208: if (Build.VERSION.SDK_INT > Build.VERSION_CODES.M) { On 2016/09/14 16:30:05, awdf wrote: > On 2016/09/14 14:06:54, Peter Beverloo wrote: > > nit: could hoist lines 208-216 in the if() conditional above, since it's still > a > > prerequisite. up to you > > Yes, but then Android Studio gives me API warnings, because it's not *that* > clever about things being inside if-checks, apparently -_- so I think I'll leave > it, rather than adding a TargetApi annotation to the whole method or extracting > out a new method to apply one to. We used to have the following code because of John's editor: #if OS(ANDROID) } #else } #endif Let's try to minimize the amount of code necessary to make editors happy. In this case I think there's other tooling that may not be as clever, so findbugs may still be upset.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
awdf@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in android_chrome_strings.grd Thanks!
lgtm
The CQ bit was checked by awdf@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Notifications in sensitive contexts now display origin + small icon BUG=498716 ========== to ========== Notifications in sensitive contexts now display origin + small icon BUG=498716 Committed: https://crrev.com/b75661d3d5c5924cffc942945d79d0430b812e1c Cr-Commit-Position: refs/heads/master@{#418817} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b75661d3d5c5924cffc942945d79d0430b812e1c Cr-Commit-Position: refs/heads/master@{#418817} |
