Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2337963003:
Plumb through notification action types and placeholders on Android (Closed)
DescriptionPlumb through the notification action types and placeholder text
- Notification actions with type="text" now trigger an 'inline reply'
input textbox to be displayed on Android N+ (and trigger voice input
on Android Wear K+) when experimental web platform features are enabled.
- However, this change does not do anything with text entered in the
textbox.
BUG=599859
Committed: https://crrev.com/e022c1faebded93d1f771b74efd2f8c0914d8eed
Cr-Commit-Position: refs/heads/master@{#421510}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Plumb through the notification action types and placeholder text for inline replies #
Total comments: 23
Patch Set 3 : Review fixes, improved comments and tests, rebased. #
Total comments: 13
Patch Set 4 : Add explicit out-of-line copy constructor for ButtonInfo #Patch Set 5 : Responding to review comments #
Total comments: 20
Patch Set 6 : Final review nits #
Total comments: 8
Patch Set 7 : Little experiment to try on buildbots - NOT FOR REVIEW OR COMMIT #Patch Set 8 : Remove unused import + put =default in header file #Patch Set 9 : Put =default back in .cc file #
Total comments: 2
Messages
Total messages: 57 (34 generated)
Description was changed from ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android. - However, this change does not do anything with text entered in the textbox. BUG= ========== to ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android (when experimental web platform features are enabled). - However, this change does not do anything with text entered in the textbox. BUG= ==========
awdf@chromium.org changed reviewers: + awdf@chromium.org, peter@chromium.org
here you go - hacky but works! https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (left): https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:214: * Verifies that the ONLY_ALERT_ONCE flag is not set when renotify is true. oops, copy-pasted this comment by accident https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:164: public void testSetActionWithRemoteInput() { is there any way to make a test run only on certain versions of android, other than having a big 'if' within the test? https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_android.cc:235: action_types_vector.push_back(base::ASCIIToUTF16("text")); I'm aware I probably want a vector of booleans or an enum type here instead of strings, but this was the quickest way I could get it working! (ToJavaArrayOfBooleans didn't seem to exist). https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:33: enum class NotificationActionType { BUTTON, TEXT }; wasn't sure where to put this
https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:37: public RemoteInput remoteInput; Both the NotificationBuilderBase and the Action sub-class store the data, then apply this when actually building the notification. Should it perhaps be the responsibility of the NotificationBuilder to translate the type/placeholder into a RemoteInput object too, as opposed to the NotificationPlatformBridge doing so? https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:204: @Nullable RemoteInput remoteInput) { This gives me the feeling that we can't decide on an API contract and instead settled on "give me whatever. or nothing." Not necessarily for this patch, but we should really decide that e.g. |intent| must be passed. https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:310: builder.addAction(actionBuilder.build()); The four lines of duplication are unfortunate. Could we instead differentiate how we instantiate |actionBuilder|, and then have lines 307-310 in a common code path? https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:486: * @param actionIcons Icons of actions to display alongside the notification. nit: please add documentation https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:164: public void testSetActionWithRemoteInput() { On 2016/09/13 14:14:55, awdf wrote: > is there any way to make a test run only on certain versions of android, other > than having a big 'if' within the test? @MinAndroidSdkLevel https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_android.cc:235: action_types_vector.push_back(base::ASCIIToUTF16("text")); On 2016/09/13 14:14:55, awdf wrote: > I'm aware I probably want a vector of booleans or an enum type here instead of > strings, but this was the quickest way I could get it working! > (ToJavaArrayOfBooleans didn't seem to exist). I think you're looking at too small of a problem— the fact that we have all these action_* arrays which are expected (and assumed) to be of the right size is an issue already. What would be fantastic is if we could take a step back, and instead be able to instantiate some Action object from C++, so that displayNotification in Java gets an Action[]. Would you be interested in taking a look at that? Certainly doesn't have to be this CL. https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:33: enum class NotificationActionType { BUTTON, TEXT }; On 2016/09/13 14:14:55, awdf wrote: > wasn't sure where to put this This is fine. Probably should call it ButtonType to be consistent with the other definitions in this file. https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:38: message_center::NotificationActionType type; nit: no need to prefix with "message_center::", that's the namespace you're in. https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:39: base::NullableString16 placeholder; I think you should see compile errors about this class now needing an out-of-line move operator. At that point you should apply the rule of three: http://en.cppreference.com/w/cpp/language/rule_of_three However, does |placeholder| need to be a NullableString? How is NULL semantically different from an empty string for the action input's behaviour? If it's not, this could just be a base::string16.
Patchset #2 (id:20001) has been deleted
Responding to review comments. Next I'm going to look into retrieving the text entered in the notification. It doesn't really make sense for these changes to go live without that - how do I go about making a custom flag to hide them behind? (Or would you just wait until the rest of it is ready?) https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:37: public RemoteInput remoteInput; On 2016/09/14 16:27:12, Peter Beverloo wrote: > Both the NotificationBuilderBase and the Action sub-class store the data, then > apply this when actually building the notification. > > Should it perhaps be the responsibility of the NotificationBuilder to translate > the type/placeholder into a RemoteInput object too, as opposed to the > NotificationPlatformBridge doing so? Done. https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:204: @Nullable RemoteInput remoteInput) { On 2016/09/14 16:27:12, Peter Beverloo wrote: > This gives me the feeling that we can't decide on an API contract and instead > settled on "give me whatever. or nothing." Not necessarily for this patch, but > we should really decide that e.g. |intent| must be passed. Acknowledged. https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:310: builder.addAction(actionBuilder.build()); On 2016/09/14 16:27:12, Peter Beverloo wrote: > The four lines of duplication are unfortunate. Could we instead differentiate > how we instantiate |actionBuilder|, and then have lines 307-310 in a common code > path? Done. https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:486: * @param actionIcons Icons of actions to display alongside the notification. On 2016/09/14 16:27:12, Peter Beverloo wrote: > nit: please add documentation Done. https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:164: public void testSetActionWithRemoteInput() { On 2016/09/14 16:27:12, Peter Beverloo wrote: > On 2016/09/13 14:14:55, awdf wrote: > > is there any way to make a test run only on certain versions of android, other > > than having a big 'if' within the test? > > @MinAndroidSdkLevel Done. https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_android.cc:235: action_types_vector.push_back(base::ASCIIToUTF16("text")); On 2016/09/14 16:27:12, Peter Beverloo wrote: > On 2016/09/13 14:14:55, awdf wrote: > > I'm aware I probably want a vector of booleans or an enum type here instead of > > strings, but this was the quickest way I could get it working! > > (ToJavaArrayOfBooleans didn't seem to exist). > > I think you're looking at too small of a problem— the fact that we have all > these action_* arrays which are expected (and assumed) to be of the right size > is an issue already. > > What would be fantastic is if we could take a step back, and instead be able to > instantiate some Action object from C++, so that displayNotification in Java > gets an Action[]. > > Would you be interested in taking a look at that? Certainly doesn't have to be > this CL. Sure, it would definitely be nice to cut down the giant list of parameters in NPB.displayNotification :) Have added a TODO in NotificationPlatformBridge and will do this in a future CL. https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:33: enum class NotificationActionType { BUTTON, TEXT }; On 2016/09/14 16:27:12, Peter Beverloo wrote: > On 2016/09/13 14:14:55, awdf wrote: > > wasn't sure where to put this > > This is fine. Probably should call it ButtonType to be consistent with the other > definitions in this file. Done. https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:39: base::NullableString16 placeholder; On 2016/09/14 16:27:12, Peter Beverloo wrote: > I think you should see compile errors about this class now needing an > out-of-line move operator. At that point you should apply the rule of three: > > http://en.cppreference.com/w/cpp/language/rule_of_three > > However, does |placeholder| need to be a NullableString? How is NULL > semantically different from an empty string for the action input's behaviour? If > it's not, this could just be a base::string16. I didn't see any compile errors about this, can you explain why you think I should? I made it a NullableString because that's what it is in PlatformNotificationData before it's converted into this ButtonInfo (https://cs.chromium.org/chromium/src/content/public/common/platform_notificat...) - I could convert to a normal string (mapping NULL -> "") there, or change it in PlatformNotificationData too, so convert at the point where that's constructed from a WebNotificationAction - it uses a WebString there - might the WebString be NULL if not specified in the javascript?
Responding to review comments. Next I'm going to look into retrieving the text entered in the notification. It doesn't really make sense for these changes to go live without that - how do I go about making a custom flag to hide them behind? (Or would you just wait until the rest of it is ready?)
https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:39: base::NullableString16 placeholder; On 2016/09/22 14:17:12, awdf wrote: > On 2016/09/14 16:27:12, Peter Beverloo wrote: > > I think you should see compile errors about this class now needing an > > out-of-line move operator. At that point you should apply the rule of three: > > > > http://en.cppreference.com/w/cpp/language/rule_of_three > > > > However, does |placeholder| need to be a NullableString? How is NULL > > semantically different from an empty string for the action input's behaviour? > If > > it's not, this could just be a base::string16. > > I didn't see any compile errors about this, can you explain why you think I > should? I made it a NullableString because that's what it is in > PlatformNotificationData before it's converted into this ButtonInfo > (https://cs.chromium.org/chromium/src/content/public/common/platform_notificat...) > - I could convert to a normal string (mapping NULL -> "") there, or change it in > PlatformNotificationData too, so convert at the point where that's constructed > from a WebNotificationAction - it uses a WebString there - might the WebString > be NULL if not specified in the javascript? I saw compile errors when building this. Maybe run the trybots? Keeping this a base::NullableString16 SGTM based on our offline chat. https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:228: * @param placeholder - placeholder text for the text box nit: why document only |placeholder|? Either document everything, or nothing. https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:237: @Nullable PendingIntent intent, Action.Type actionType, String placeholder) { @Nullable String placeholder (You pass NULL on line 220.) https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:405: new RemoteInput.Builder("key_text_reply") nit: make this a constant in NotificationConstants? (The project generally doesn't like random strings with semantic meaning like this.) https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:467: * // TODO awdf: Combine the following four parameters into a single array of Action objects. nits: (1) No need to comment a comment in a comment, (2) Move this to either the top of the @param list, or below the @see. https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:238: @MinAndroidSdkLevel(20) // RemoteInputs were only added in API 20. Can we use Build.VERSION_CODES.KITKAT_WATCH instead of "20"? https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:202: assertEquals("Placeholder", notification.actions[0].getRemoteInputs()[0].getLabel()); nit: should we assert that getRemoveInputs() has entries, or do you think the NPE is clear enough? https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:15: #include "base/strings/string_number_conversions.h" nit: merge conflict? I removed this in a previous CL, we don't need it anymore :). https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:245: if (!button.placeholder.is_null()) { nit: why not unify the for loops into a single one? https://codereview.chromium.org/2337963003/diff/40001/ui/message_center/notif... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/40001/ui/message_center/notif... ui/message_center/notification.h:38: ButtonType type; Please define the default value, since this is a scalar.
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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:228: * @param placeholder - placeholder text for the text box On 2016/09/22 18:07:55, Peter Beverloo wrote: > nit: why document only |placeholder|? Either document everything, or nothing. Done. https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:237: @Nullable PendingIntent intent, Action.Type actionType, String placeholder) { On 2016/09/22 18:07:55, Peter Beverloo wrote: > @Nullable String placeholder > > (You pass NULL on line 220.) Done. https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:405: new RemoteInput.Builder("key_text_reply") On 2016/09/22 18:07:55, Peter Beverloo wrote: > nit: make this a constant in NotificationConstants? (The project generally > doesn't like random strings with semantic meaning like this.) Done. I'm sorry for all these obvious rookie mistakes. I should have gone through this properly and fixed it up first, or started from scratch, instead of asking you to review the initial hacky patch (broke the rule of always throwing away prototype code!) Never mind though. I have now gone through it all so please continue reviewing as normal. What's your thoughts on pushing this (when ready) before the rest of the inline reply work (to send on the replies) is done? I know it's good to commit little and often instead of having a long-running branch, but this feature doesn't really make sense without the rest, so, hide it behind a new flag? Or maybe these changes are small and isolated enough that it's ok to keep them on my own branch while working on the rest..? https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:467: * // TODO awdf: Combine the following four parameters into a single array of Action objects. On 2016/09/22 18:07:55, Peter Beverloo wrote: > nits: > (1) No need to comment a comment in a comment, > (2) Move this to either the top of the @param list, or below the @see. Done. (oops - guess I'm just so used to typing "// TODO" ^^ ) https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:238: @MinAndroidSdkLevel(20) // RemoteInputs were only added in API 20. On 2016/09/22 18:07:55, Peter Beverloo wrote: > Can we use Build.VERSION_CODES.KITKAT_WATCH instead of "20"? Done. https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:202: assertEquals("Placeholder", notification.actions[0].getRemoteInputs()[0].getLabel()); On 2016/09/22 18:07:55, Peter Beverloo wrote: > nit: should we assert that getRemoveInputs() has entries, or do you think the > NPE is clear enough? Slightly nicer to assert it explicitly I think. And that there's the expected number of actions. Done. https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:15: #include "base/strings/string_number_conversions.h" On 2016/09/22 18:07:56, Peter Beverloo wrote: > nit: merge conflict? I removed this in a previous CL, we don't need it anymore > :). Done. https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:245: if (!button.placeholder.is_null()) { On 2016/09/22 18:07:56, Peter Beverloo wrote: > nit: why not unify the for loops into a single one? Done. https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:248: action_placeholders_vector.push_back(base::ASCIIToUTF16("")); here I should shouldn't be replacing NULL with an empty string, as discussed, we should instead use a default placeholder if the developer didn't specify one. But where does it make most sense to replace NULL with a default placeholder? I guess we want the same string on all platforms, so we should replace it before this point, maybe at chrome/browser/notifications/platform_notification_service_impl.cc ? (And then it can be a regular string instead of a nullable one from there). https://codereview.chromium.org/2337963003/diff/40001/ui/message_center/notif... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/40001/ui/message_center/notif... ui/message_center/notification.h:38: ButtonType type; On 2016/09/22 18:07:56, Peter Beverloo wrote: > Please define the default value, since this is a scalar. Done. https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:529: if (actionTypes[actionIndex].equals("text")) { So we agreed I will refactor all the action parameters into a single array of some Action type in a future commit, does that mean it's okay to continue sending through the action types as strings for now?
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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:405: new RemoteInput.Builder("key_text_reply") On 2016/09/23 15:24:56, awdf wrote: > On 2016/09/22 18:07:55, Peter Beverloo wrote: > > nit: make this a constant in NotificationConstants? (The project generally > > doesn't like random strings with semantic meaning like this.) > > Done. > > I'm sorry for all these obvious rookie mistakes. I should have gone through this > properly and fixed it up first, or started from scratch, instead of asking you > to review the initial hacky patch (broke the rule of always throwing away > prototype code!) Don't be! Really, Chromium is a super complicated project with lots of preferences, far from all of which are written down. > Never mind though. I have now gone through it all so please continue reviewing > as normal. > > What's your thoughts on pushing this (when ready) before the rest of the inline > reply work (to send on the replies) is done? I know it's good to commit little > and often instead of having a long-running branch, but this feature doesn't > really make sense without the rest, so, hide it behind a new flag? Or maybe > these changes are small and isolated enough that it's ok to keep them on my own > branch while working on the rest..? Absolutely. I'm a big fan of smaller patches - make them as small and granular as you'd like, with the nit that it's a good idea to set the bar at "something that's testable", no matter how bare bone. https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:248: action_placeholders_vector.push_back(base::ASCIIToUTF16("")); On 2016/09/23 15:24:56, awdf wrote: > here I should shouldn't be replacing NULL with an empty string, as discussed, we > should instead use a default placeholder if the developer didn't specify one. > But where does it make most sense to replace NULL with a default placeholder? I > guess we want the same string on all platforms, so we should replace it before > this point, maybe at > chrome/browser/notifications/platform_notification_service_impl.cc ? (And then > it can be a regular string instead of a nullable one from there). I agree - adding it in PlatformNotificationServiceImpl sounds like the best place to me, and being sure that it's not NULL beyond that point is a good advantage. https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:529: if (actionTypes[actionIndex].equals("text")) { On 2016/09/23 15:24:56, awdf wrote: > So we agreed I will refactor all the action parameters into a single array of > some Action type in a future commit, does that mean it's okay to continue > sending through the action types as strings for now? Yes, since we have a concrete plan to clean it up. (I guess we'd ideally have constants instead?) https://codereview.chromium.org/2337963003/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:305: assertEquals(1, notification.actions[0].getRemoteInputs().length); https://developer.android.com/reference/android/app/Notification.Action.html#... tells me that this'd return NULL instead of an empty array https://codereview.chromium.org/2337963003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:230: : base::ASCIIToUTF16("button")); So occasionally "git cl format" does things that are not very nice :-P. What about extracting this to a local variable? ========================= base::string16 type; switch (button.type) { case message_center::ButtonType::BUTTON: type = base::ASCIIToUTF16("button"); break; case message_center::ButtonType::TEXT: type = base::ASCIIToUTF16("text"); break; } action_types_vector.push_back(std::move(type)); ========================= While this has more lines of code, it (a) is more readable, and (b) will yield a compile error when somebody adds another value to message_center::ButtonType that's not being handled here. https://codereview.chromium.org/2337963003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:233: : base::ASCIIToUTF16("")); As discussed, this should become non-nullable. (Which also impacts the rest of the code.) https://codereview.chromium.org/2337963003/diff/80001/ui/message_center/notif... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/80001/ui/message_center/notif... ui/message_center/notification.h:35: struct MESSAGE_CENTER_EXPORT ButtonInfo { See my earlier comment about the rule of three, since this does break on the bots: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
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.
https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_android.cc:235: action_types_vector.push_back(base::ASCIIToUTF16("text")); On 2016/09/22 14:17:11, awdf wrote: > On 2016/09/14 16:27:12, Peter Beverloo wrote: > > On 2016/09/13 14:14:55, awdf wrote: > > > I'm aware I probably want a vector of booleans or an enum type here instead > of > > > strings, but this was the quickest way I could get it working! > > > (ToJavaArrayOfBooleans didn't seem to exist). > > > > I think you're looking at too small of a problem— the fact that we have all > > these action_* arrays which are expected (and assumed) to be of the right size > > is an issue already. > > > > What would be fantastic is if we could take a step back, and instead be able > to > > instantiate some Action object from C++, so that displayNotification in Java > > gets an Action[]. > > > > Would you be interested in taking a look at that? Certainly doesn't have to be > > this CL. > > Sure, it would definitely be nice to cut down the giant list of parameters in > NPB.displayNotification :) > > Have added a TODO in NotificationPlatformBridge and will do this in a future CL. Is it reasonable for me to create a Chromium issue and assign it to myself so I don't forget to do this? (I like using issue trackers to remind me what I have outstanding, but I can use some other tool if this refactor is too small / only-relevant-to-me to merit its own issue) https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:38: message_center::NotificationActionType type; On 2016/09/14 16:27:12, Peter Beverloo wrote: > nit: no need to prefix with "message_center::", that's the namespace you're in. Done. https://codereview.chromium.org/2337963003/diff/1/ui/message_center/notificat... ui/message_center/notification.h:39: base::NullableString16 placeholder; On 2016/09/22 18:07:55, Peter Beverloo wrote: > On 2016/09/22 14:17:12, awdf wrote: > > On 2016/09/14 16:27:12, Peter Beverloo wrote: > > > I think you should see compile errors about this class now needing an > > > out-of-line move operator. At that point you should apply the rule of three: > > > > > > http://en.cppreference.com/w/cpp/language/rule_of_three > > > > > > However, does |placeholder| need to be a NullableString? How is NULL > > > semantically different from an empty string for the action input's > behaviour? > > If > > > it's not, this could just be a base::string16. > > > > I didn't see any compile errors about this, can you explain why you think I > > should? I made it a NullableString because that's what it is in > > PlatformNotificationData before it's converted into this ButtonInfo > > > (https://cs.chromium.org/chromium/src/content/public/common/platform_notificat...) > > - I could convert to a normal string (mapping NULL -> "") there, or change it > in > > PlatformNotificationData too, so convert at the point where that's constructed > > from a WebNotificationAction - it uses a WebString there - might the WebString > > be NULL if not specified in the javascript? > > I saw compile errors when building this. Maybe run the trybots? > > Keeping this a base::NullableString16 SGTM based on our offline chat. Done. https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:405: new RemoteInput.Builder("key_text_reply") On 2016/09/23 16:01:22, Peter Beverloo wrote: > On 2016/09/23 15:24:56, awdf wrote: > > On 2016/09/22 18:07:55, Peter Beverloo wrote: > > > nit: make this a constant in NotificationConstants? (The project generally > > > doesn't like random strings with semantic meaning like this.) > > > > Done. > > > > I'm sorry for all these obvious rookie mistakes. I should have gone through > this > > properly and fixed it up first, or started from scratch, instead of asking you > > to review the initial hacky patch (broke the rule of always throwing away > > prototype code!) > > Don't be! Really, Chromium is a super complicated project with lots of > preferences, far from all of which are written down. > > > Never mind though. I have now gone through it all so please continue reviewing > > as normal. > > > > What's your thoughts on pushing this (when ready) before the rest of the > inline > > reply work (to send on the replies) is done? I know it's good to commit little > > and often instead of having a long-running branch, but this feature doesn't > > really make sense without the rest, so, hide it behind a new flag? Or maybe > > these changes are small and isolated enough that it's ok to keep them on my > own > > branch while working on the rest..? > > Absolutely. I'm a big fan of smaller patches - make them as small and granular > as you'd like, with the nit that it's a good idea to set the bar at "something > that's testable", no matter how bare bone. Acknowledged. https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:248: action_placeholders_vector.push_back(base::ASCIIToUTF16("")); On 2016/09/23 16:01:22, Peter Beverloo wrote: > On 2016/09/23 15:24:56, awdf wrote: > > here I should shouldn't be replacing NULL with an empty string, as discussed, > we > > should instead use a default placeholder if the developer didn't specify one. > > But where does it make most sense to replace NULL with a default placeholder? > I > > guess we want the same string on all platforms, so we should replace it before > > this point, maybe at > > chrome/browser/notifications/platform_notification_service_impl.cc ? (And then > > it can be a regular string instead of a nullable one from there). > > I agree - adding it in PlatformNotificationServiceImpl sounds like the best > place to me, and being sure that it's not NULL beyond that point is a good > advantage. Done. https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:529: if (actionTypes[actionIndex].equals("text")) { On 2016/09/23 16:01:22, Peter Beverloo wrote: > On 2016/09/23 15:24:56, awdf wrote: > > So we agreed I will refactor all the action parameters into a single array of > > some Action type in a future commit, does that mean it's okay to continue > > sending through the action types as strings for now? > > Yes, since we have a concrete plan to clean it up. (I guess we'd ideally have > constants instead?) Ideally I want an enum type that can be passed through, but I'm guessing that's not possible across the JNI (?). So yeah, some shared Action type that has named int constants that can be accessed from both native & java I guess. https://codereview.chromium.org/2337963003/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:305: assertEquals(1, notification.actions[0].getRemoteInputs().length); On 2016/09/23 16:01:22, Peter Beverloo wrote: > https://developer.android.com/reference/android/app/Notification.Action.html#... > > tells me that this'd return NULL instead of an empty array Good point. Added an assertNotNull before this line, and in the other tests. https://codereview.chromium.org/2337963003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:230: : base::ASCIIToUTF16("button")); On 2016/09/23 16:01:22, Peter Beverloo wrote: > So occasionally "git cl format" does things that are not very nice :-P. > > What about extracting this to a local variable? > > ========================= > base::string16 type; > switch (button.type) { > case message_center::ButtonType::BUTTON: > type = base::ASCIIToUTF16("button"); > break; > case message_center::ButtonType::TEXT: > type = base::ASCIIToUTF16("text"); > break; > } > > action_types_vector.push_back(std::move(type)); > ========================= > > While this has more lines of code, it (a) is more readable, and (b) will yield a > compile error when somebody adds another value to message_center::ButtonType > that's not being handled here. Done. https://codereview.chromium.org/2337963003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_android.cc:233: : base::ASCIIToUTF16("")); On 2016/09/23 16:01:22, Peter Beverloo wrote: > As discussed, this should become non-nullable. (Which also impacts the rest of > the code.) Done. https://codereview.chromium.org/2337963003/diff/80001/ui/message_center/notif... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/80001/ui/message_center/notif... ui/message_center/notification.h:35: struct MESSAGE_CENTER_EXPORT ButtonInfo { On 2016/09/23 16:01:22, Peter Beverloo wrote: > See my earlier comment about the rule of three, since this does break on the > bots: > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Done. https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:65: context).setSmallIcon(R.drawable.ic_chrome) git cl format is responsible for this ugly linebreaking... not sure why it suddenly decided to change this
lgtm! https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_android.cc:235: action_types_vector.push_back(base::ASCIIToUTF16("text")); On 2016/09/26 15:33:28, awdf wrote: > On 2016/09/22 14:17:11, awdf wrote: > > On 2016/09/14 16:27:12, Peter Beverloo wrote: > > > On 2016/09/13 14:14:55, awdf wrote: > > > > I'm aware I probably want a vector of booleans or an enum type here > instead > > of > > > > strings, but this was the quickest way I could get it working! > > > > (ToJavaArrayOfBooleans didn't seem to exist). > > > > > > I think you're looking at too small of a problem— the fact that we have all > > > these action_* arrays which are expected (and assumed) to be of the right > size > > > is an issue already. > > > > > > What would be fantastic is if we could take a step back, and instead be able > > to > > > instantiate some Action object from C++, so that displayNotification in Java > > > gets an Action[]. > > > > > > Would you be interested in taking a look at that? Certainly doesn't have to > be > > > this CL. > > > > Sure, it would definitely be nice to cut down the giant list of parameters in > > NPB.displayNotification :) > > > > Have added a TODO in NotificationPlatformBridge and will do this in a future > CL. > > Is it reasonable for me to create a Chromium issue and assign it to myself so I > don't forget to do this? (I like using issue trackers to remind me what I have > outstanding, but I can use some other tool if this refactor is too small / > only-relevant-to-me to merit its own issue) Absolutely :) Bugs are free. https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:529: if (actionTypes[actionIndex].equals("text")) { On 2016/09/26 15:33:28, awdf wrote: > On 2016/09/23 16:01:22, Peter Beverloo wrote: > > On 2016/09/23 15:24:56, awdf wrote: > > > So we agreed I will refactor all the action parameters into a single array > of > > > some Action type in a future commit, does that mean it's okay to continue > > > sending through the action types as strings for now? > > > > Yes, since we have a concrete plan to clean it up. (I guess we'd ideally have > > constants instead?) > > Ideally I want an enum type that can be passed through, but I'm guessing that's > not possible across the JNI (?). So yeah, some shared Action type that has named > int constants that can be accessed from both native & java I guess. Yeah, that's possible. We have a thing called GENERATED_JAVA_ENUM_PACKAGE but it doesn't actually seem to be documented. There's plenty of examples though, let's figure it out ourselves? https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:247: @Nullable String placeholder) { nit: drop @Nullable from |placeholder| https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:422: .setLabel(action.placeholder != null ? action.placeholder : "") nit: drop the NULL check - addTextAction() is the only path through which we can get here, where we know it not to be NULL. You could add some future-proofing decoration too: assert !TextUtils.isEmpty(action.placeholder); https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:444: * TODO awdf: Combine the four 'action*' parameters into a single array of Action objects. micro nit: we generally format our TODOs as: TODO(awdf): Hello, world! TODO(crbug.com/123456): Hello, world! https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:65: context).setSmallIcon(R.drawable.ic_chrome) On 2016/09/26 15:33:29, awdf wrote: > git cl format is responsible for this ugly linebreaking... not sure why it > suddenly decided to change this I don't think we force "git cl format" over our Java code, so you could disagree with it and undo it manually? Dito on line 180. https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.cc:222: std::vector<base::string16> action_titles_vector; nit: might be worth duplicating the TODO about merging stuff into an Action struct? https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:449: content::PlatformNotificationAction action = notification_data.actions[i]; nit: const& (right now we're making a copy, which is not necessary) https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... ui/message_center/notification.h:42: ButtonInfo(const ButtonInfo& other); Out of interest, are these still necessary when you change the copy to be a const reference instead? It'd be great if they're not!
Description was changed from ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android (when experimental web platform features are enabled). - However, this change does not do anything with text entered in the textbox. BUG= ========== to ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android (when experimental web platform features are enabled). - However, this change does not do anything with text entered in the textbox. BUG=599859 ==========
Description was changed from ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android (when experimental web platform features are enabled). - However, this change does not do anything with text entered in the textbox. BUG=599859 ========== to ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android N+ (and trigger voice input on Android Wear K+) when experimental web platform features are enabled. - However, this change does not do anything with text entered in the textbox. BUG=599859 ==========
Description was changed from ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android N+ (and trigger voice input on Android Wear K+) when experimental web platform features are enabled. - However, this change does not do anything with text entered in the textbox. BUG=599859 ========== to ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android N+ (and trigger voice input on Android Wear K+) when experimental web platform features are enabled. - However, this change does not do anything with text entered in the textbox. BUG=599859 ==========
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:529: if (actionTypes[actionIndex].equals("text")) { On 2016/09/26 16:04:34, Peter Beverloo wrote: > On 2016/09/26 15:33:28, awdf wrote: > > On 2016/09/23 16:01:22, Peter Beverloo wrote: > > > On 2016/09/23 15:24:56, awdf wrote: > > > > So we agreed I will refactor all the action parameters into a single array > > of > > > > some Action type in a future commit, does that mean it's okay to continue > > > > sending through the action types as strings for now? > > > > > > Yes, since we have a concrete plan to clean it up. (I guess we'd ideally > have > > > constants instead?) > > > > Ideally I want an enum type that can be passed through, but I'm guessing > that's > > not possible across the JNI (?). So yeah, some shared Action type that has > named > > int constants that can be accessed from both native & java I guess. > > Yeah, that's possible. We have a thing called GENERATED_JAVA_ENUM_PACKAGE but it > doesn't actually seem to be documented. There's plenty of examples though, let's > figure it out ourselves? Acknowledged. https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:247: @Nullable String placeholder) { On 2016/09/26 16:04:34, Peter Beverloo wrote: > nit: drop @Nullable from |placeholder| Done. https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:422: .setLabel(action.placeholder != null ? action.placeholder : "") On 2016/09/26 16:04:34, Peter Beverloo wrote: > nit: drop the NULL check - addTextAction() is the only path through which we can > get here, where we know it not to be NULL. > > You could add some future-proofing decoration too: > > assert !TextUtils.isEmpty(action.placeholder); I thought we agreed that we'd allow the developer to set an empty string as the placeholder if they want to? Have added an 'assert action.placeholder != null;' though, and removed the other nullcheck https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:444: * TODO awdf: Combine the four 'action*' parameters into a single array of Action objects. On 2016/09/26 16:04:34, Peter Beverloo wrote: > micro nit: we generally format our TODOs as: > > TODO(awdf): Hello, world! > TODO(crbug.com/123456): Hello, world! Done. https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:65: context).setSmallIcon(R.drawable.ic_chrome) On 2016/09/26 16:04:34, Peter Beverloo wrote: > On 2016/09/26 15:33:29, awdf wrote: > > git cl format is responsible for this ugly linebreaking... not sure why it > > suddenly decided to change this > > I don't think we force "git cl format" over our Java code, so you could disagree > with it and undo it manually? Dito on line 180. Done. Manually overriding the auto format tool that's supposed to help me is annoying though. How do I report this as a bug in the autoformatter? https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.cc:222: std::vector<base::string16> action_titles_vector; On 2016/09/26 16:04:34, Peter Beverloo wrote: > nit: might be worth duplicating the TODO about merging stuff into an Action > struct? Done. https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:449: content::PlatformNotificationAction action = notification_data.actions[i]; On 2016/09/26 16:04:34, Peter Beverloo wrote: > nit: const& (right now we're making a copy, which is not necessary) Done. (Gah C++.. expect a lot more of these kinds of errors from me! I'm trying to look up all my mistakes and learn from them, and vaguely read cpp guides & tips, and sign up for grow courses, but it'll take a while. Meanwhile I really appreciate you taking the time to point out all my n00b mistakes :D thanks! ) https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... ui/message_center/notification.h:42: ButtonInfo(const ButtonInfo& other); On 2016/09/26 16:04:34, Peter Beverloo wrote: > Out of interest, are these still necessary when you change the copy to be a > const reference instead? It'd be great if they're not! Which copy are you talking about, the copy of the notification action on line 449 of https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... ? Or are you saying I should change the copy constructor signature here? I thought these methods were required now that there are more than 3 fields in ButtonInfo so that it needs an out-of-line move operator? https://codereview.chromium.org/2337963003/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2337963003/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:12148: + Send message Do I need to add someone from https://cs.chromium.org/chromium/src/chrome/app/OWNERS to approve this string?
still lgtm Nothing actionable in the comments. Please check whether we can remove the added constructors and operators from ButtonInfo, after which you'll need an OWNER for //ui/message_center/ for the notification.h changes. https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:422: .setLabel(action.placeholder != null ? action.placeholder : "") On 2016/09/27 14:45:27, awdf wrote: > On 2016/09/26 16:04:34, Peter Beverloo wrote: > > nit: drop the NULL check - addTextAction() is the only path through which we > can > > get here, where we know it not to be NULL. > > > > You could add some future-proofing decoration too: > > > > assert !TextUtils.isEmpty(action.placeholder); > > I thought we agreed that we'd allow the developer to set an empty string as the > placeholder if they want to? > > Have added an 'assert action.placeholder != null;' though, and removed the other > nullcheck Sorry, you're right! We only have to assert that the placeholder is not NULL. https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:65: context).setSmallIcon(R.drawable.ic_chrome) On 2016/09/27 14:45:27, awdf wrote: > On 2016/09/26 16:04:34, Peter Beverloo wrote: > > On 2016/09/26 15:33:29, awdf wrote: > > > git cl format is responsible for this ugly linebreaking... not sure why it > > > suddenly decided to change this > > > > I don't think we force "git cl format" over our Java code, so you could > disagree > > with it and undo it manually? Dito on line 180. > > Done. > > Manually overriding the auto format tool that's supposed to help me is annoying > though. How do I report this as a bug in the autoformatter? https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md ""If clang-format is broken, or produces badly formatted code, please file a bug. Assign it to thakis@chromium.org or nick@chromium.org who will route it upstream."" (Has a convenient link when opening the page in your browser.) In general, the /docs/ folder has all sorts of useful stuff in it. https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:449: content::PlatformNotificationAction action = notification_data.actions[i]; On 2016/09/27 14:45:27, awdf wrote: > On 2016/09/26 16:04:34, Peter Beverloo wrote: > > nit: const& (right now we're making a copy, which is not necessary) > > Done. > > (Gah C++.. expect a lot more of these kinds of errors from me! I'm trying to > look up all my mistakes and learn from them, and vaguely read cpp guides & tips, > and sign up for grow courses, but it'll take a while. Meanwhile I really > appreciate you taking the time to point out all my n00b mistakes :D thanks! ) No problem at all! There's so many parts of the language that don't make any sort of sense to my either. Yet. https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... ui/message_center/notification.h:42: ButtonInfo(const ButtonInfo& other); On 2016/09/27 14:45:27, awdf wrote: > On 2016/09/26 16:04:34, Peter Beverloo wrote: > > Out of interest, are these still necessary when you change the copy to be a > > const reference instead? It'd be great if they're not! > > Which copy are you talking about, the copy of the notification action on line > 449 of > https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... > ? Or are you saying I should change the copy constructor signature here? > > I thought these methods were required now that there are more than 3 fields in > ButtonInfo so that it needs an out-of-line move operator? The one in platform_notification_service_impl.cc indeed. The rule-of-three (and rule-of-five) are not about number of members, but rather about which constructors and operators you want to define when the compiler's asking one of them to be present. In this case, it may have been the copy that required the copy constructor to have to be defined at all. If that's the case, we may be able to undo these modifications, which'd be nice. On the other hand, it might be that the clang plugin just has a threshold of three members, after which it starts giving that warning. That'd be confusing in this instance. Best to try! https://codereview.chromium.org/2337963003/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2337963003/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:12148: + Send message On 2016/09/27 14:45:27, awdf wrote: > Do I need to add someone from > https://cs.chromium.org/chromium/src/chrome/app/OWNERS to approve this string? No, because of the following line in the OWNERS file: per-file generated_resources.grd=* That means that, beyond the owners for every file (lines 5 and 6), "*" is an owner of generated_resources.grd. Wildcards basically mean *any* committer of the Chromium Project.
https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... ui/message_center/notification.h:42: ButtonInfo(const ButtonInfo& other); On 2016/09/27 15:28:32, Peter Beverloo wrote: > On 2016/09/27 14:45:27, awdf wrote: > > On 2016/09/26 16:04:34, Peter Beverloo wrote: > > > Out of interest, are these still necessary when you change the copy to be a > > > const reference instead? It'd be great if they're not! > > > > Which copy are you talking about, the copy of the notification action on line > > 449 of > > > https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... > > ? Or are you saying I should change the copy constructor signature here? > > > > I thought these methods were required now that there are more than 3 fields in > > ButtonInfo so that it needs an out-of-line move operator? > > The one in platform_notification_service_impl.cc indeed. > > The rule-of-three (and rule-of-five) are not about number of members, but rather > about which constructors and operators you want to define when the compiler's > asking one of them to be present. > > In this case, it may have been the copy that required the copy constructor to > have to be defined at all. If that's the case, we may be able to undo these > modifications, which'd be nice. > > On the other hand, it might be that the clang plugin just has a threshold of > three members, after which it starts giving that warning. That'd be confusing in > this instance. Best to try! Oh but I got the clang error before I started caching the notification action in a variable there, (prior to patch 5) so it must be the threshold-of-3-members explanation, not a result of copying it there.
On 2016/09/27 15:36:27, awdf wrote: > https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... > File ui/message_center/notification.h (right): > > https://codereview.chromium.org/2337963003/diff/120001/ui/message_center/noti... > ui/message_center/notification.h:42: ButtonInfo(const ButtonInfo& other); > On 2016/09/27 15:28:32, Peter Beverloo wrote: > > On 2016/09/27 14:45:27, awdf wrote: > > > On 2016/09/26 16:04:34, Peter Beverloo wrote: > > > > Out of interest, are these still necessary when you change the copy to be > a > > > > const reference instead? It'd be great if they're not! > > > > > > Which copy are you talking about, the copy of the notification action on > line > > > 449 of > > > > > > https://codereview.chromium.org/2337963003/diff/120001/chrome/browser/notific... > > > ? Or are you saying I should change the copy constructor signature here? > > > > > > I thought these methods were required now that there are more than 3 fields > in > > > ButtonInfo so that it needs an out-of-line move operator? > > > > The one in platform_notification_service_impl.cc indeed. > > > > The rule-of-three (and rule-of-five) are not about number of members, but > rather > > about which constructors and operators you want to define when the compiler's > > asking one of them to be present. > > > > In this case, it may have been the copy that required the copy constructor to > > have to be defined at all. If that's the case, we may be able to undo these > > modifications, which'd be nice. > > > > On the other hand, it might be that the clang plugin just has a threshold of > > three members, after which it starts giving that warning. That'd be confusing > in > > this instance. Best to try! > > Oh but I got the clang error before I started caching the notification action in > a variable there, (prior to patch 5) so it must be the threshold-of-3-members > explanation, not a result of copying it there. Seems like it would still be needed for the copy at https://cs.chromium.org/chromium/src/ui/message_center/views/notification_vie...
awdf@chromium.org changed reviewers: + dewittj@chromium.org
dewittj@chromium.org: Please review changes in ui/message_center/ Thanks.
lgtm https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... ui/message_center/notification.h:13: #include "base/strings/nullable_string16.h" unused? https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... ui/message_center/notification.h:44: ButtonInfo& operator=(const ButtonInfo& other); any reason not to put =default here instead of .cc file?
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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2337963003/#ps220001 (title: "Remove unused import + put =default in header file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... ui/message_center/notification.h:13: #include "base/strings/nullable_string16.h" On 2016/09/28 02:41:58, dewittj wrote: > unused? Done, thanks. Is there an automated tool I can use to clean up unused imports as I edit? https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... ui/message_center/notification.h:44: ButtonInfo& operator=(const ButtonInfo& other); On 2016/09/28 02:41:58, dewittj wrote: > any reason not to put =default here instead of .cc file? Nope, other than I didn't know you could (and I was copying from the example at https://www.chromium.org/developers/coding-style/chromium-style-checker-errors for how to fix the "Complex class/struct needs an explicit out-of-line copy constructor" error). So in general is it preferred style to always put =default implementations in the header file then?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... File ui/message_center/notification.h (right): https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... ui/message_center/notification.h:13: #include "base/strings/nullable_string16.h" On 2016/09/28 11:42:09, awdf wrote: > On 2016/09/28 02:41:58, dewittj wrote: > > unused? > > Done, thanks. > > Is there an automated tool I can use to clean up unused imports as I edit? Unfortunately there isn't. (This is a hard problem because one of the includes after here might "use" NullableString16 as well.) https://codereview.chromium.org/2337963003/diff/180001/ui/message_center/noti... ui/message_center/notification.h:44: ButtonInfo& operator=(const ButtonInfo& other); On 2016/09/28 11:42:09, awdf wrote: > On 2016/09/28 02:41:58, dewittj wrote: > > any reason not to put =default here instead of .cc file? > > Nope, other than I didn't know you could (and I was copying from the example at > https://www.chromium.org/developers/coding-style/chromium-style-checker-errors > for how to fix the "Complex class/struct needs an explicit out-of-line copy > constructor" error). > > So in general is it preferred style to always put =default implementations in > the header file then? Well, by moving =default to the header file you're inlining them again, so I would expect this to yield the Clang error in the CL you just CQed. For this particular class, the compiler-generated constructor has to roughly do four things: ``` ButtonInfo::ButtonInfo(const base::string16& other_title) { title.assign(other_title); // will malloc() new (&icon) gfx::Image(); type = ButtonType::BUTTON; new (&placeholder) base::string16(); } ``` That's three calls and an assign— more if either of the called constructors will be inlined themselves. By inlining the constructor, this code will be copied to each function that creates a new ButtonInfo object. This can very quickly get out of bounds, and lead to a significant bump in binary size. By outlining the constructor, i.e. defining =default in the .cc file, each call site will instead have one call to the constructor, which in turn executes the code that would otherwise be inlined. This is an abstraction that favors size over speed, but often at negligible levels. To give a concrete example, look at the diff in the first two files of the following CL, that moves a function's definition from the header file (thus might be inlined) to the definition file. Super trivial, but saved 50KB of binary size. https://codereview.chromium.org/331593006/
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, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2337963003/#ps240001 (title: "Put =default back in .cc file")
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.
Description was changed from ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android N+ (and trigger voice input on Android Wear K+) when experimental web platform features are enabled. - However, this change does not do anything with text entered in the textbox. BUG=599859 ========== to ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android N+ (and trigger voice input on Android Wear K+) when experimental web platform features are enabled. - However, this change does not do anything with text entered in the textbox. BUG=599859 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android N+ (and trigger voice input on Android Wear K+) when experimental web platform features are enabled. - However, this change does not do anything with text entered in the textbox. BUG=599859 ========== to ========== Plumb through the notification action types and placeholder text - Notification actions with type="text" now trigger an 'inline reply' input textbox to be displayed on Android N+ (and trigger voice input on Android Wear K+) when experimental web platform features are enabled. - However, this change does not do anything with text entered in the textbox. BUG=599859 Committed: https://crrev.com/e022c1faebded93d1f771b74efd2f8c0914d8eed Cr-Commit-Position: refs/heads/master@{#421510} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e022c1faebded93d1f771b74efd2f8c0914d8eed Cr-Commit-Position: refs/heads/master@{#421510}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2337963003/diff/240001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/2337963003/diff/240001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:449: content::PlatformNotificationAction const& action = Isn't this more commonly written as "const Foo&" ? https://codereview.chromium.org/2337963003/diff/240001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:460: switch (action.type) { Something is uninitialized there. Filed https://crbug.com/651165 and looking... |
