Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2440483002:
Combine action parameters sent to Android displayNotification (Closed)
DescriptionCombine action parameters sent to Android displayNotification
- Combine the 4 notification action arrays sent through JNI
into a single array of ActionInfo java objects
- Pass action type as a java-exported enum/int
BUG=650302
Committed: https://crrev.com/8e0c4af8157b210323a4622ff9f614216b58f6af
Cr-Commit-Position: refs/heads/master@{#426535}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Remove commented out code and rename action_infos > actions #Patch Set 3 : Comments #
Total comments: 1
Patch Set 4 : Put NotificationActionType enum in notification_platform_bridge_android.cc #
Messages
Total messages: 26 (13 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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Pass action type as a java-exported enum/int Combine the notification action arrays sent through JNI into one BUG=650302 ========== to ========== Combine action parameters sent to Android displayNotification - Combine the 4 notification action arrays sent through JNI into a single array of ActionInfo java objects - Pass action type as a java-exported enum/int BUG=650302 ==========
awdf@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_types_android.h (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_types_android.h:10: enum NotificationActionType { I tried making it an enum class but that doesn't allow implicit conversion to int values. Also, I filed https://codereview.chromium.org/2440483002/ for the bug where it doesn't work when the enum is all on one line. I'm happy to take this on if you like?
https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:257: // TODO(crbug.com/650302): Combine these action_* vectors into a single vector Argh, commented out code - thought I removed this. Will fix.
Description was changed from ========== Combine action parameters sent to Android displayNotification - Combine the 4 notification action arrays sent through JNI into a single array of ActionInfo java objects - Pass action type as a java-exported enum/int BUG=650302 ========== to ========== Combine action parameters sent to Android displayNotification - Combine the 4 notification action arrays sent through JNI into a single array of ActionInfo java objects - Pass action type as a java-exported enum/int BUG=650302 ==========
https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java:13: * Helper class for passing notification action information over the JNI. Heh, funny -- this reads wrong to me, but actually is correct when pronouncing the JNI initialism. https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java:15: @JNINamespace("chrome") nit: drop https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:50: ScopedJavaLocalRef<jobject> j_bitmap = NULL; nit: no need to assign NULL - that's the default constructor already. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:51: if (!skbitmap.drawsNothing()) { micro nit: no {} for one line statements https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:64: } Not all compilers like this. Nothing stops a static_cast<> from setting |button.type| to 42. What do we return in that case? Instead, it's common for us to output an error and choose a default value. NOTREACHED(); return NotificationActionType::BUTTON; It *should* never be reached. Where the switch() gives you compile time guarantees, the NOTREACHED() does the same for runtime guarantees. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:72: jobjectArray array = env->NewObjectArray(buttons.size(), clazz.obj(), nit: s/array/actions/? https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:77: ScopedJavaLocalRef<jstring> title = nit: consider having a const reference to the button as the first line in the loop, allows you to remove all the "[i]"s. const auto& button = buttons[i]; https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_types_android.h (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_types_android.h:10: enum NotificationActionType { On 2016/10/20 15:18:07, awdf wrote: > I tried making it an enum class but that doesn't allow implicit conversion to > int values. > > Also, I filed https://codereview.chromium.org/2440483002/ for the bug where it > doesn't work when the enum is all on one line. I'm happy to take this on if you > like? SGTM. I think you mean https://crbug.com/657847. I wouldn't worry about fixing it, let's focus on growing your familiarity with our own code first? If it's just this enum, you could also decide to just define it in notification_platform_bridge_android.cc. Up to you.
https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java:13: * Helper class for passing notification action information over the JNI. On 2016/10/20 15:36:20, Peter Beverloo wrote: > Heh, funny -- this reads wrong to me, but actually is correct when pronouncing > the JNI initialism. yeah? what would you say? https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java:15: @JNINamespace("chrome") On 2016/10/20 15:36:20, Peter Beverloo wrote: > nit: drop Done. https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java:15: @JNINamespace("chrome") On 2016/10/20 15:36:20, Peter Beverloo wrote: > nit: drop Done. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:50: ScopedJavaLocalRef<jobject> j_bitmap = NULL; On 2016/10/20 15:36:20, Peter Beverloo wrote: > nit: no need to assign NULL - that's the default constructor already. Done. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:51: if (!skbitmap.drawsNothing()) { On 2016/10/20 15:36:20, Peter Beverloo wrote: > micro nit: no {} for one line statements Done. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:64: } On 2016/10/20 15:36:20, Peter Beverloo wrote: > Not all compilers like this. Nothing stops a static_cast<> from setting > |button.type| to 42. What do we return in that case? > > Instead, it's common for us to output an error and choose a default value. > > NOTREACHED(); > return NotificationActionType::BUTTON; > > It *should* never be reached. Where the switch() gives you compile time > guarantees, the NOTREACHED() does the same for runtime guarantees. Done. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:72: jobjectArray array = env->NewObjectArray(buttons.size(), clazz.obj(), On 2016/10/20 15:36:20, Peter Beverloo wrote: > nit: s/array/actions/? Done. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:77: ScopedJavaLocalRef<jstring> title = On 2016/10/20 15:36:20, Peter Beverloo wrote: > nit: consider having a const reference to the button as the first line in the > loop, allows you to remove all the "[i]"s. > > const auto& button = buttons[i]; Done. https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_types_android.h (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_types_android.h:10: enum NotificationActionType { On 2016/10/20 15:36:20, Peter Beverloo wrote: > On 2016/10/20 15:18:07, awdf wrote: > > I tried making it an enum class but that doesn't allow implicit conversion to > > int values. > > > > Also, I filed https://codereview.chromium.org/2440483002/ for the bug where it > > doesn't work when the enum is all on one line. I'm happy to take this on if > you > > like? > > SGTM. > > I think you mean https://crbug.com/657847. I wouldn't worry about fixing it, > let's focus on growing your familiarity with our own code first? Ah yes, that's the one. I've given it the label 'Build-Tools', so maybe someone will notice it & pick it up, let me know if there's a more specific label that applies. > > If it's just this enum, you could also decide to just define it in > notification_platform_bridge_android.cc. Up to you. Hm, I was thinking we could put more types here if we reduce the parameter list further, but actually that will just mean making more Java classes like ActionInfo.java, since there's no more enums, so this new file is probably unnecessary.
lgtm % removing the default case https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java:13: * Helper class for passing notification action information over the JNI. On 2016/10/20 16:05:39, awdf wrote: > On 2016/10/20 15:36:20, Peter Beverloo wrote: > > Heh, funny -- this reads wrong to me, but actually is correct when pronouncing > > the JNI initialism. > > yeah? what would you say? I think this is good :). [[Insert The Social Network quote about dropping "the"]] https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_types_android.h (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_types_android.h:10: enum NotificationActionType { On 2016/10/20 16:05:39, awdf wrote: > On 2016/10/20 15:36:20, Peter Beverloo wrote: > > On 2016/10/20 15:18:07, awdf wrote: > > > I tried making it an enum class but that doesn't allow implicit conversion > to > > > int values. > > > > > > Also, I filed https://codereview.chromium.org/2440483002/ for the bug where > it > > > doesn't work when the enum is all on one line. I'm happy to take this on if > > you > > > like? > > > > SGTM. > > > > I think you mean https://crbug.com/657847. I wouldn't worry about fixing it, > > let's focus on growing your familiarity with our own code first? > > Ah yes, that's the one. I've given it the label 'Build-Tools', so maybe someone > will notice it & pick it up, let me know if there's a more specific label that > applies. That label sounds fine to me. https://codereview.chromium.org/2440483002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2440483002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:63: default: Nah, sorry - don't use "default". Basically, if you have a switch() statement without a default clause the compiler will complain if you don't handle all the cases. That's fantastic for verification - this would be easy to miss if somebody ever adds a new ButtonType. Just hoist the two lines to the end of the function's control flow.
https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_types_android.h (right): https://codereview.chromium.org/2440483002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_types_android.h:10: enum NotificationActionType { > Hm, I was thinking we could put more types here if we reduce the parameter list > further, but actually that will just mean making more Java classes like > ActionInfo.java, since there's no more enums, so this new file is probably > unnecessary. Done (put the enum in the other file and removed this new file). Also added a comment to prevent future 'git cl format' issues.
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/2440483002/#ps80001 (title: "Put NotificationActionType enum in notification_platform_bridge_android.cc")
The CQ bit was unchecked by awdf@chromium.org
awdf@chromium.org changed reviewers: + miguelg@chromium.org
miguelg@ - please review changes in: chrome/android/BUILD.gn
lgtm for chrome/android/BUILD.gn
On 2016/10/20 16:55:17, awdf wrote: > miguelg@ - please review changes in: > chrome/android/BUILD.gn Or don't, actually, sorry, I misunderstood what Peter was saying about TBR
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...
On 2016/10/20 16:57:19, awdf wrote: > On 2016/10/20 16:55:17, awdf wrote: > > miguelg@ - please review changes in: > > chrome/android/BUILD.gn > > Or don't, actually, sorry, I misunderstood what Peter was saying about TBR Ah never mind, thanks for the quick lgtm!
Message was sent while issue was closed.
Description was changed from ========== Combine action parameters sent to Android displayNotification - Combine the 4 notification action arrays sent through JNI into a single array of ActionInfo java objects - Pass action type as a java-exported enum/int BUG=650302 ========== to ========== Combine action parameters sent to Android displayNotification - Combine the 4 notification action arrays sent through JNI into a single array of ActionInfo java objects - Pass action type as a java-exported enum/int BUG=650302 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Combine action parameters sent to Android displayNotification - Combine the 4 notification action arrays sent through JNI into a single array of ActionInfo java objects - Pass action type as a java-exported enum/int BUG=650302 ========== to ========== Combine action parameters sent to Android displayNotification - Combine the 4 notification action arrays sent through JNI into a single array of ActionInfo java objects - Pass action type as a java-exported enum/int BUG=650302 Committed: https://crrev.com/8e0c4af8157b210323a4622ff9f614216b58f6af Cr-Commit-Position: refs/heads/master@{#426535} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8e0c4af8157b210323a4622ff9f614216b58f6af Cr-Commit-Position: refs/heads/master@{#426535} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
