Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876603004:
Linux native notifications: Add attribution (Closed)
|
Created:
3 years, 7 months ago by Tom (Use chromium acct) Modified:
3 years, 7 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, Jun Mukai Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux native notifications: Add attribution
This CL adds the origin url to the notification body (if one exists),
similar to how Chrome notifications do currently.
This is one of the requirements specified in [1] (Google only).
[1] https://docs.google.com/a/google.com/document/d/1cqXPpIyCf1RaEYx9XCPjZbO6U5oDeyslajznnG9wd_U/edit?usp=sharing
BUG=676220
R=peter@chromium.org,thestig@chromium.org
TBR=mukai@chromium.org
Review-Url: https://codereview.chromium.org/2876603004
Cr-Commit-Position: refs/heads/master@{#472160}
Committed: https://chromium.googlesource.com/chromium/src/+/f139811a9c8a079969a903da9b718daa04e2ced9
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 1
Patch Set 3 : Rebase #
Total comments: 8
Patch Set 4 : Address peter and yoshiki's comments #
Total comments: 8
Patch Set 5 : Address comments #
Depends on Patchset: Messages
Total messages: 23 (11 generated)
Patchset #1 (id:1) has been deleted
reviewers ptal +mukai for a trivial change in message_center
mukai@google.com changed reviewers: + mukai@google.com, yoshiki@chromium.org - mukai@chromium.org
+yoshiki for ui/message_center. I put my thought but defer to yoshiki for that. https://codereview.chromium.org/2876603004/diff/40001/ui/message_center/notif... File ui/message_center/notification.h (right): https://codereview.chromium.org/2876603004/diff/40001/ui/message_center/notif... ui/message_center/notification.h:132: void set_origin_url(const GURL& origin_url) { origin_url_ = origin_url; } I prefer not to have setter like this for tests. Consider using the constructor.
pinging peter and yoshiki
LGTM with nit. https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:137: gfx::Image ResizeImageToFdoMaxSize(const gfx::Image& image) { nit: Could you add a DCHECK to ensure it's not on the UI thread? Someone in future may try to call this on the UI thread wrongly. DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI))
A few questions, code LG otherwise! I'm fine with the MC addition too. It's unfortunate that it's only used for testing purposes, but it is consistent with the other properties in there. https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:555: if (!notification->origin_url().is_empty()) { How certain are we that the attribution is going to be displayed when at the bottom? Can a notification center decide to maximize the length of a toast, and cut it off? (We display the origin *before* the content on Mac OS X.) https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:555: if (!notification->origin_url().is_empty()) { If the notification has a context message, we should display that instead of the origin. That's freeflow text however. https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:561: url_formatter::SchemeDisplay::OMIT_HTTP_AND_HTTPS)); What would wrapped contents look like? For example, super long domain names.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:137: gfx::Image ResizeImageToFdoMaxSize(const gfx::Image& image) { On 2017/05/15 02:35:15, yoshiki wrote: > nit: Could you add a DCHECK to ensure it's not on the UI thread? Someone in > future may try to call this on the UI thread wrongly. > > DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) Done. https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:555: if (!notification->origin_url().is_empty()) { On 2017/05/15 16:19:41, Peter Beverloo wrote: > If the notification has a context message, we should display that instead of the > origin. That's freeflow text however. Done. https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:555: if (!notification->origin_url().is_empty()) { On 2017/05/15 16:19:41, Peter Beverloo wrote: > How certain are we that the attribution is going to be displayed when at the > bottom? Can a notification center decide to maximize the length of a toast, and > cut it off? (We display the origin *before* the content on Mac OS X.) Ok, I moved the context to the beginning of the body. I've added some pictures on the bug for this. https://codereview.chromium.org/2876603004/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:561: url_formatter::SchemeDisplay::OMIT_HTTP_AND_HTTPS)); On 2017/05/15 16:19:41, Peter Beverloo wrote: > What would wrapped contents look like? For example, super long domain names. See the pictures on the bug to see how it looks on GNOME. In general, there's no way to know how long a line can be, so we cannot wrap it ourselves. Giving the whole thing to the notification server is the correct thing to do, I believe.
https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:551: std::string spec = notification->origin_url().spec(); Do we know if the origin URL is guaranteed to be valid at this point? https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:416: .SetMessage(base::UTF8ToUTF16("Body text")) ACIIToUTF16() when you know it's ASCII. https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:417: .SetOriginUrl(GURL("https://google.com/")) I'm not sure what guarantees the notification subsystem provides, but if we can test with something more exotic as well, that would be good to have. So we can see if the newly added UseOriginAsContextMessage() handler code escapes the exotic origin properly.
lgtm from my end https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:551: std::string spec = notification->origin_url().spec(); On 2017/05/15 22:19:59, Lei Zhang wrote: > Do we know if the origin URL is guaranteed to be valid at this point? Yes. UseOriginAsContextMessage() verifies this. https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:561: EscapeBodyText(&context); Is it possible for |body_markup| to be FALSE but kCapabilityBodyHyperlinks to be TRUE, like we check on line 552? In that case <a> would still work if we don't escape.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:561: EscapeBodyText(&context); On 2017/05/15 22:43:26, Peter Beverloo wrote: > Is it possible for |body_markup| to be FALSE but kCapabilityBodyHyperlinks to be > TRUE, no. body-hyperlinks => body-markup >like we check on line 552? In that case <a> would still work if we don't > escape. https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:416: .SetMessage(base::UTF8ToUTF16("Body text")) On 2017/05/15 22:19:59, Lei Zhang wrote: > ACIIToUTF16() when you know it's ASCII. Done. https://codereview.chromium.org/2876603004/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:417: .SetOriginUrl(GURL("https://google.com/")) On 2017/05/15 22:19:59, Lei Zhang wrote: > I'm not sure what guarantees the notification subsystem provides, but if we can > test with something more exotic as well, that would be good to have. So we can > see if the newly added UseOriginAsContextMessage() handler code escapes the > exotic origin properly. Done. (maybe)
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.
lgtm
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/2876603004/#ps120001 (title: "Address comments")
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": 120001, "attempt_start_ts": 1494957150596840,
"parent_rev": "d760627f0528f7ac3969c4109ba33abb2265b5c3", "commit_rev":
"f139811a9c8a079969a903da9b718daa04e2ced9"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: Add attribution This CL adds the origin url to the notification body (if one exists), similar to how Chrome notifications do currently. This is one of the requirements specified in [1] (Google only). [1] https://docs.google.com/a/google.com/document/d/1cqXPpIyCf1RaEYx9XCPjZbO6U5oD... BUG=676220 R=peter@chromium.org,thestig@chromium.org TBR=mukai@chromium.org ========== to ========== Linux native notifications: Add attribution This CL adds the origin url to the notification body (if one exists), similar to how Chrome notifications do currently. This is one of the requirements specified in [1] (Google only). [1] https://docs.google.com/a/google.com/document/d/1cqXPpIyCf1RaEYx9XCPjZbO6U5oD... BUG=676220 R=peter@chromium.org,thestig@chromium.org TBR=mukai@chromium.org Review-Url: https://codereview.chromium.org/2876603004 Cr-Commit-Position: refs/heads/master@{#472160} Committed: https://chromium.googlesource.com/chromium/src/+/f139811a9c8a079969a903da9b71... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f139811a9c8a079969a903da9b71... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
