|
|
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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux native notifications: Support image notifications
BUG=676220
R=peter@chromium.org
Review-Url: https://codereview.chromium.org/2872053002
Cr-Commit-Position: refs/heads/master@{#471993}
Committed: https://chromium.googlesource.com/chromium/src/+/42ebc4fd9b073bbcb4a2845aa5862c8fcab2a8a0
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resize on task runner #
Total comments: 7
Patch Set 3 : address thestig@'s comments #Patch Set 4 : Rebase #
Total comments: 10
Patch Set 5 : Rebase #Patch Set 6 : Add unit test #
Total comments: 8
Patch Set 7 : cleanups #Patch Set 8 : base::Optional<bool> #
Dependent Patchsets: Messages
Total messages: 28 (13 generated)
https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:50: const int kMaxImageHeight = 100; This is tiny! Developers have to provide images with a width of 1440 pixels for modern phones. Screen density makes a huge difference here, so it'd be interesting to learn how this'll work out in practice. https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:229: height)))); Since these images potentially can be huge, we shouldn't do this on the UI thread. Can the task runner thread be used for the resize? That'd avoid the multi-thread warning at the member declaration site below. https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:501: "<img src=\"" + image_file->file_path().value() + "\" alt=\"\"/>"; Is a data: URL an option here? If the toast requires full HTML rendering anyway I wouldn't expect that to be significantly slower. https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:826: } body_images_supported_ = BODY_IMAGES_UNKNOWN; It looks like you're considering base::Optional<bool> semantics here? Could even simplify that and just default to false until line 382 executes.
thomasanderson@google.com changed reviewers: + thestig@chromium.org
+thestig https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:50: const int kMaxImageHeight = 100; On 2017/05/10 12:54:40, Peter Beverloo wrote: > This is tiny! Developers have to provide images with a width of 1440 pixels for > modern phones. Screen density makes a huge difference here, so it'd be > interesting to learn how this'll work out in practice. Yeah, but the only notification server that actually supports inline images (LXQt notification daemon) sort of goes crazy when you give it a larger image. https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:229: height)))); On 2017/05/10 12:54:40, Peter Beverloo wrote: > Since these images potentially can be huge, we shouldn't do this on the UI > thread. Can the task runner thread be used for the resize? That'd avoid the > multi-thread warning at the member declaration site below. Done. https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:501: "<img src=\"" + image_file->file_path().value() + "\" alt=\"\"/>"; On 2017/05/10 12:54:40, Peter Beverloo wrote: > Is a data: URL an option here? If the toast requires full HTML rendering anyway > I wouldn't expect that to be significantly slower. No, the spec says "Images referenced must always be local files." Also: "A full-blown HTML implementation is not required of this spec, and notifications should never take advantage of tags that are not listed above. As notifications are not a substitute for web browsers or complex dialogs, advanced layout is not necessary, and may in fact limit the number of systems that notification services can run on, due to memory usage and screen space. Such examples are PDAs, certain cell phones, and slow PCs or laptops with little memory." https://codereview.chromium.org/2872053002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:826: } body_images_supported_ = BODY_IMAGES_UNKNOWN; On 2017/05/10 12:54:40, Peter Beverloo wrote: > It looks like you're considering base::Optional<bool> semantics here? Could even > simplify that and just default to false until line 382 executes. Done.
https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:66: int ClampInt(int v, int lo, int hi) { return std::max(std::min(value, hi), low); https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:66: int ClampInt(int v, int lo, int hi) { BTW, std::clamp() will be in C++17, so we'll have it on all supported Linux distros in 2025. ;-) This is the same parameter order. I'll probably find some time later to add a parameterized version of clamp() to base, to prepare us for 2025. https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:121: } else { No else after return.
https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:461: void DisplayOnTaskRunner(NotificationCommon::Type notification_type, BTW, this is now ~150 lines long. Time to break off some (more) pieces into helper functions / methods after this CL?
https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:66: int ClampInt(int v, int lo, int hi) { On 2017/05/11 00:54:56, Lei Zhang (OOO) wrote: > return std::max(std::min(value, hi), low); Done. https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:121: } else { On 2017/05/11 00:54:56, Lei Zhang (OOO) wrote: > No else after return. Done. https://codereview.chromium.org/2872053002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:461: void DisplayOnTaskRunner(NotificationCommon::Type notification_type, On 2017/05/11 01:00:00, Lei Zhang (OOO) wrote: > BTW, this is now ~150 lines long. Time to break off some (more) pieces into > helper functions / methods after this CL? Acknowledged.
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.
ping
https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:548: "<img src=\"" + image_file->file_path().value() + "\" alt=\"\"/>"; Let's use an std::stringstream for building |body|. Every + operator can lead to reallocation, which we have a lot of right now. std::stringstream will also allow you to use the much nicer << operator. https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:622: writer.AppendInt32(notification->never_timeout() ? kExpireTimeoutNever As a general comment, some of your CLs are a bit tricky to review because they include other CLs. Both Gerrit and Rietveld can work with dependent patch sets for that - create a local Git branch that tracks another branch. The try-bots can deal with this just fine, while the diff will only show the actual diff, and not that of the CLs it depends on. https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:843: bool body_images_supported_ = true; Technically we should never call Display() before initialization succeeds, right? https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:331: } Should there be a test for big image here?
https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:548: "<img src=\"" + image_file->file_path().value() + "\" alt=\"\"/>"; On 2017/05/15 16:16:00, Peter Beverloo wrote: > Let's use an std::stringstream for building |body|. Every + operator can lead to > reallocation, which we have a lot of right now. std::stringstream will also > allow you to use the much nicer << operator. Done. https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:622: writer.AppendInt32(notification->never_timeout() ? kExpireTimeoutNever On 2017/05/15 16:16:00, Peter Beverloo wrote: > As a general comment, some of your CLs are a bit tricky to review because they > include other CLs. Both Gerrit and Rietveld can work with dependent patch sets > for that - create a local Git branch that tracks another branch. The try-bots > can deal with this just fine, while the diff will only show the actual diff, and > not that of the CLs it depends on. Done. Sorry, the dependent patch set somehow got removed between patch set 3 and patch set 4. https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:843: bool body_images_supported_ = true; On 2017/05/15 16:16:00, Peter Beverloo wrote: > Technically we should never call Display() before initialization succeeds, > right? In the case of unit testing, calls to Display() are made before init finishes https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:331: } On 2017/05/15 16:16:00, Peter Beverloo wrote: > Should there be a test for big image here? Done.
https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:843: bool body_images_supported_ = true; On 2017/05/15 19:18:39, Tom Anderson wrote: > On 2017/05/15 16:16:00, Peter Beverloo wrote: > > Technically we should never call Display() before initialization succeeds, > > right? > > In the case of unit testing, calls to Display() are made before init finishes Is it a lot of work to change the unit tests to not do that? https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:106: std::stringstream stream_; ostringstream to be more specific? https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:106: std::stringstream stream_; #include <sstream>
lgtm w/ suggestion https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:554: body << "<b>" << title << "</b> " << message; One downside of using StringStreamSizable as opposed to a regular std::stringstream is that even literal strings like "<b>" will be converted to a std::string (which allocates memory) prior to being appended. What do you think about simplifying this further by using an std::(o)stringstream directly and using a boolean as opposed to the size on line 547? It's OK to not display the |message| for list-style notifications.
https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:566: body << "<img src=\"" << image_file->file_path().value() BTW, we should escape the inside of the src= attribute, just to be on the safe side.
https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:843: bool body_images_supported_ = true; On 2017/05/15 21:48:40, Lei Zhang wrote: > On 2017/05/15 19:18:39, Tom Anderson wrote: > > On 2017/05/15 16:16:00, Peter Beverloo wrote: > > > Technically we should never call Display() before initialization succeeds, > > > right? > > > > In the case of unit testing, calls to Display() are made before init finishes > > Is it a lot of work to change the unit tests to not do that? Nope, just a one line change in CreatePlatformBridgeLinux(). Done. And I made this variable a base::Optional<bool> so that a DCHECK will trigger if this is ever NOT true. https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:106: std::stringstream stream_; On 2017/05/15 21:48:41, Lei Zhang wrote: > ostringstream to be more specific? Done. https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:106: std::stringstream stream_; On 2017/05/15 21:48:41, Lei Zhang wrote: > #include <sstream> Done. https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:554: body << "<b>" << title << "</b> " << message; On 2017/05/15 22:07:33, Peter Beverloo wrote: > One downside of using StringStreamSizable as opposed to a regular > std::stringstream is that even literal strings like "<b>" will be converted to a > std::string (which allocates memory) prior to being appended. > > What do you think about simplifying this further by using an > std::(o)stringstream directly and using a boolean as opposed to the size on line > 547? It's OK to not display the |message| for list-style notifications. I just realized that tellp() exists :P https://codereview.chromium.org/2872053002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:566: body << "<img src=\"" << image_file->file_path().value() On 2017/05/15 22:13:11, Lei Zhang wrote: > BTW, we should escape the inside of the src= attribute, just to be on the safe > side. Done.
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2872053002/#ps140001 (title: "base::Optional<bool>")
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": 140001, "attempt_start_ts": 1494903061579950, "parent_rev": "a1032ae7289dad09b5c5a27b2189d6654b17a5b5", "commit_rev": "42ebc4fd9b073bbcb4a2845aa5862c8fcab2a8a0"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: Support image notifications BUG=676220 R=peter@chromium.org ========== to ========== Linux native notifications: Support image notifications BUG=676220 R=peter@chromium.org Review-Url: https://codereview.chromium.org/2872053002 Cr-Commit-Position: refs/heads/master@{#471993} Committed: https://chromium.googlesource.com/chromium/src/+/42ebc4fd9b073bbcb4a2845aa586... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/42ebc4fd9b073bbcb4a2845aa586... |