Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814973002:
Linux native notifications: Add support for custom notification buttons (Closed)
|
Created:
3 years, 8 months ago by Tom (Use chromium acct) Modified:
3 years, 8 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux native notifications: Add support for custom notification buttons
BUG=676220
R=peter@chromium.org
Review-Url: https://codereview.chromium.org/2814973002
Cr-Commit-Position: refs/heads/master@{#465091}
Committed: https://chromium.googlesource.com/chromium/src/+/b6b0a4c62db4757a27a61f19921ed70df20a42be
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Fix incorrect range check #
Total comments: 2
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 22 (9 generated)
peter@ ptal Also, my DE (GNOME) only seems to support 3 buttons max. So if I try to create a web notification with 3 buttons, instead I get 2 web buttons and a settings button. Do you think we should remove the settings button? (or remove it if there are already 3 buttons?)
Patchset #1 (id:1) has been deleted
3 buttons is fine, that's how many we support on Android and Mac too. We cap developer input to two buttons: https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul... https://codereview.chromium.org/2814973002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2814973002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:347: AddActionToNotification(&actions_builder, id.c_str(), label.c_str()); Are we concerned about the OS passing us invalid button information? I think we trust input elsewhere. Otherwise it may be simpler to use use a regular loop: for (size_t i = 0; i < notification.buttons().size(); ++i) { const std::string id = base::SizeTToString(i); const std::string label = base::UTF16ToUTF8( notification.buttons()[i].title); AddActionToNotification(&actions_builder, id.c_str(), label.c_str()); } Then we don't need to maintain the |action_start| and |action_end| properties, and just change the check on line 467 to check against some other sensible limit. (You could add NotificationPlatformBridgeLinux::kMaxButtonCount set to 2.)
https://codereview.chromium.org/2814973002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2814973002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:347: AddActionToNotification(&actions_builder, id.c_str(), label.c_str()); On 2017/04/12 00:44:22, Peter Beverloo wrote: > Are we concerned about the OS passing us invalid button information? No, but I wanted to handle this case: 1. Notification is created with buttons "Yes" and "No" 2. User clicks on "Yes", event is being sent 3. Chrome updates notification buttons to say "No" "Yes" before it gets the event 4. Chrome gets the event and thinks the user clicked "No" It would be easier if the protocol were synchronous > I think we > trust input elsewhere. Otherwise it may be simpler to use use a regular loop: > > for (size_t i = 0; i < notification.buttons().size(); ++i) { > const std::string id = base::SizeTToString(i); > const std::string label = base::UTF16ToUTF8( > notification.buttons()[i].title); > > AddActionToNotification(&actions_builder, id.c_str(), label.c_str()); > } > > Then we don't need to maintain the |action_start| and |action_end| properties, > and just change the check on line 467 to check against some other sensible > limit. > > (You could add NotificationPlatformBridgeLinux::kMaxButtonCount set to 2.)
thomasanderson@google.com changed reviewers: + thestig@chromium.org
+thestig
The CQ bit was checked by thomasanderson@google.com 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/2814973002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2814973002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:180: // on this notification. Mention the valid range of action IDs is [action_start, action_end) https://codereview.chromium.org/2814973002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:486: if (id >= n_buttons) On line 360, when it gets the first time, it's using [0, N). On the next call, it's usng [N, M) for the IDs. Later when we reach here, the ID string is being converted back to an ID number. Is |id| going to be in the range [N, M) if the notification updates and then the user clicks? e.g. with 3 buttons initially and 2 in the updated notification, we'll end up with |action_start| = 3 and |action_end| = 5. Valid |id| values would be 3 or 4, right? Whereas |n_buttons| is going to be 2. Wouldn't that result in failing this check?
I take back what I said about GNOME only allowing 3 buttons. You can have at least 20 (didn't test above this). The reason I was only getting 2 was because notification.buttons() only had 2 elements, even though on https://tests.peter.sh/notification-generator/ I selected "3 actions" peter@ do you know why this is happening? https://codereview.chromium.org/2814973002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2814973002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:180: // on this notification. On 2017/04/14 01:19:45, Lei Zhang (OOO Friday) wrote: > Mention the valid range of action IDs is [action_start, action_end) Done. https://codereview.chromium.org/2814973002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:486: if (id >= n_buttons) On 2017/04/14 01:19:45, Lei Zhang (OOO Friday) wrote: > On line 360, when it gets the first time, it's using [0, N). On the next call, > it's usng [N, M) for the IDs. > > Later when we reach here, the ID string is being converted back to an ID number. > Is |id| going to be in the range [N, M) if the notification updates and then the > user clicks? > > e.g. with 3 buttons initially and 2 in the updated notification, we'll end up > with |action_start| = 3 and |action_end| = 5. Valid |id| values would be 3 or 4, > right? Whereas |n_buttons| is going to be 2. Wouldn't that result in failing > this check? Nice catch! This should have been "id - data->action_start"
ping There are a few patch sets that depend on this
lgtm
https://codereview.chromium.org/2814973002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2814973002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:487: size_t id_zero_based = id - data->action_start; Just as a FYI, if |id| is 0, this actually underflows, but (a) unsigned underflow is defined behavior and (b) the check below still works. OTOH, if these values were signed, that's potentially undefined behavior.
https://codereview.chromium.org/2814973002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2814973002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:487: size_t id_zero_based = id - data->action_start; On 2017/04/18 00:05:57, Lei Zhang wrote: > Just as a FYI, if |id| is 0, this actually underflows, but (a) unsigned > underflow is defined behavior and (b) the check below still works. > > OTOH, if these values were signed, that's potentially undefined behavior. Ack. I tried to design this with overflows in mind
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492474312728910,
"parent_rev": "a2b6ae1e02d5b9b526ddf70a63690f80829ca026", "commit_rev":
"b6b0a4c62db4757a27a61f19921ed70df20a42be"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: Add support for custom notification buttons BUG=676220 R=peter@chromium.org ========== to ========== Linux native notifications: Add support for custom notification buttons BUG=676220 R=peter@chromium.org Review-Url: https://codereview.chromium.org/2814973002 Cr-Commit-Position: refs/heads/master@{#465091} Committed: https://chromium.googlesource.com/chromium/src/+/b6b0a4c62db4757a27a61f19921e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b6b0a4c62db4757a27a61f19921e...
Message was sent while issue was closed.
lgtm! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
