|
|
Created:
3 years, 7 months ago by Tom (Use chromium acct) Modified:
3 years, 7 months ago Reviewers:
Lei Zhang CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux native notifications: use image_path instead of image-path for spec 1.1
You're supposed to use 'image_path' for notification servers that only
support the FDO Notification spec version 1.1, and 'image-path' for
versions >= 1.2. ¯\_(ツ)_/¯
Also, ban servers that have a spec version <= 1.0 because these won't
support notification icons at all.
BUG=676220
R=thestig@chromium.org
Review-Url: https://codereview.chromium.org/2868433004
Cr-Commit-Position: refs/heads/master@{#470237}
Committed: https://chromium.googlesource.com/chromium/src/+/c3b7ae8000d7222d74e2d0942d1d8823ecc0fd52
Patch Set 1 #
Total comments: 5
Patch Set 2 : thestig@'s comments #Patch Set 3 : use base::Version #
Depends on Patchset: Messages
Total messages: 24 (17 generated)
Description was changed from ========== Linux native notifications: use image_path instead of image-path for spec 1.1 You're supposed to use 'image_path' for notification servers that only support the FDO Notification spec version 1.1, and 'image-path' for versions >= 1.2. Also ban servers that have a spec version <= 1.0 because these won't support notification icons. BUG=676220 R=thestig@chromium.org ========== to ========== Linux native notifications: use image_path instead of image-path for spec 1.1 You're supposed to use 'image_path' for notification servers that only support the FDO Notification spec version 1.1, and 'image-path' for versions >= 1.2. ¯\_(ツ)_/¯ Also ban servers that have a spec version <= 1.0 because these won't support notification icons. BUG=676220 R=thestig@chromium.org ==========
Description was changed from ========== Linux native notifications: use image_path instead of image-path for spec 1.1 You're supposed to use 'image_path' for notification servers that only support the FDO Notification spec version 1.1, and 'image-path' for versions >= 1.2. ¯\_(ツ)_/¯ Also ban servers that have a spec version <= 1.0 because these won't support notification icons. BUG=676220 R=thestig@chromium.org ========== to ========== Linux native notifications: use image_path instead of image-path for spec 1.1 You're supposed to use 'image_path' for notification servers that only support the FDO Notification spec version 1.1, and 'image-path' for versions >= 1.2. ¯\_(ツ)_/¯ Also, ban servers that have a spec version <= 1.0 because these won't support notification icons at all. BUG=676220 R=thestig@chromium.org ==========
Patchset #1 (id:1) has been deleted
thestig ptal
lgtm https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:507: image_path_writer.AppendString("image_path"); image_path_writer.AppendString(some_eval ? "image_path" : "image-path"); ? https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:738: int spec_version_major_ = -1; Can we make these local variables to InitOnTaskRunner(), and store an enum (< 1.1, 1.1, 1.2+) here instead?
https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:507: image_path_writer.AppendString("image_path"); On 2017/05/08 21:51:19, Lei Zhang wrote: > image_path_writer.AppendString(some_eval ? "image_path" : "image-path"); ? Done. https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:738: int spec_version_major_ = -1; On 2017/05/08 21:51:19, Lei Zhang wrote: > Can we make these local variables to InitOnTaskRunner(), and store an enum (< > 1.1, 1.1, 1.2+) here instead? Even better I think is a std::pair<int, int> (done in latest patch set). std::pair and std::tuple have lexicographic comparisons, so we can do eg. "spec_version_ < std::make_pair(1, 1)"
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:738: int spec_version_major_ = -1; On 2017/05/08 22:46:07, Tom Anderson wrote: > On 2017/05/08 21:51:19, Lei Zhang wrote: > > Can we make these local variables to InitOnTaskRunner(), and store an enum (< > > 1.1, 1.1, 1.2+) here instead? > > Even better I think is a std::pair<int, int> (done in latest patch set). > std::pair and std::tuple have lexicographic comparisons, so we can do eg. > "spec_version_ < std::make_pair(1, 1)" FWIW, there's also base::Version. I'm not going to stress out too much about this.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/08 22:54:26, Lei Zhang wrote: > https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... > File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): > > https://codereview.chromium.org/2868433004/diff/20001/chrome/browser/notifica... > chrome/browser/notifications/notification_platform_bridge_linux.cc:738: int > spec_version_major_ = -1; > On 2017/05/08 22:46:07, Tom Anderson wrote: > > On 2017/05/08 21:51:19, Lei Zhang wrote: > > > Can we make these local variables to InitOnTaskRunner(), and store an enum > (< > > > 1.1, 1.1, 1.2+) here instead? > > > > Even better I think is a std::pair<int, int> (done in latest patch set). > > std::pair and std::tuple have lexicographic comparisons, so we can do eg. > > "spec_version_ < std::make_pair(1, 1)" > > FWIW, there's also base::Version. I'm not going to stress out too much about > this. Looks like base::Version does parsing for us too. I'm going to use that instead :)
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2868433004/#ps60001 (title: "use base::Version")
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": 1494307490677820, "parent_rev": "140c4b6e2b2db009b9a82a6316497b919c5031fe", "commit_rev": "c3b7ae8000d7222d74e2d0942d1d8823ecc0fd52"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: use image_path instead of image-path for spec 1.1 You're supposed to use 'image_path' for notification servers that only support the FDO Notification spec version 1.1, and 'image-path' for versions >= 1.2. ¯\_(ツ)_/¯ Also, ban servers that have a spec version <= 1.0 because these won't support notification icons at all. BUG=676220 R=thestig@chromium.org ========== to ========== Linux native notifications: use image_path instead of image-path for spec 1.1 You're supposed to use 'image_path' for notification servers that only support the FDO Notification spec version 1.1, and 'image-path' for versions >= 1.2. ¯\_(ツ)_/¯ Also, ban servers that have a spec version <= 1.0 because these won't support notification icons at all. BUG=676220 R=thestig@chromium.org Review-Url: https://codereview.chromium.org/2868433004 Cr-Commit-Position: refs/heads/master@{#470237} Committed: https://chromium.googlesource.com/chromium/src/+/c3b7ae8000d7222d74e2d0942d1d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c3b7ae8000d7222d74e2d0942d1d... |