Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806203003:
Linux native notifications: Support icons (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: Support icons
This CL:
* Adds support for notification icons
* Adds an urgency hint
* Supports updating NotificationCommon::Type
BUG=676220
R=peter@chromium.org,thestig@chromium.org
Review-Url: https://codereview.chromium.org/2806203003
Cr-Commit-Position: refs/heads/master@{#464650}
Committed: https://chromium.googlesource.com/chromium/src/+/d82f2908e3193915cdc573d54f962920714b847d
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address thestig@'s comments #
Total comments: 17
Patch Set 3 : Address thestig and peter's comments #
Total comments: 4
Patch Set 4 : Address thestig's comments #
Total comments: 3
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 34 (9 generated)
peter@ ptal +thestig@ for memory-mapped IO stuff https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:309: g_variant_builder_add(&hints_builder, "{sv}", "image-path", Initially, this used "image-data" so we didn't have to do the temp file thing, but for some reason this was really slow (~0.5s for the notification to show). Writing to the file and using image-path takes just a few ms. I guess we would have needed to do this anyway when image support is added because images cannot be inlined like icons. You always need to use a file.
Lei: Also, what do we do if Chrome crashes and there's stuff in /dev/shm?
https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:309: g_variant_builder_add(&hints_builder, "{sv}", "image-path", On 2017/04/10 23:49:12, Tom Anderson wrote: > Initially, this used "image-data" so we didn't have to do the temp file thing, > but for some reason this was really slow (~0.5s for the notification to show). > Writing to the file and using image-path takes just a few ms. Any idea where that time is going? Maybe it's not worth investigating if we have to support "image-path" anyway? > I guess we would have needed to do this anyway when image support is added > because images cannot be inlined like icons. You always need to use a file. The spec says "Implementations are not required to support images." Which also makes me wonder - do we have to support images? Does it better translate the Web Notifications API if we do? Side note: It's too bad earler versions of DBus did not support file handles. Many things built on top of DBus has to use file paths as a result. Which means we can't do the open file, delete file technique to avoid potentially leaking files. If Chrome crashes while we have pending files to send, then we litter /dev/shm.
WDYT about avoiding ScopedAllowIO? https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:48: // fallthrough super nitty: Intent 2 more because it's the NOTREACHED() that's falling through, rather than the next case. https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: // Writing to a memory-mapped file doesn't require disk IO. /dev/shm is backed by tmpfs. If there is enough memory pressure, tmpfs will use swap, which can potentially hit disk. Yes, the images are small, but let's try to limit ScopedAllowIO usage to where we have to. The bottom line is we want to try to keep (potentially) janky operations off the UI thread. It doesn't look that hard to use PostTaskAndReply to offload this from the UI thread. Do we want to try doing that and then not have to use /dev/shm? The deletion can be a straight up PostTask. https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:99: if (image.IsEmpty() || !GetShmemTempDir(false, &shmem_temp_dir) || Assuming we want to go with PostTask, don't bother even calling this if we already know the image is empty? We'd want to get |image_data| first and then post that over to a file-access capable thread.
On 2017/04/11 01:04:33, Lei Zhang wrote: > https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... > File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): > > https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... > chrome/browser/notifications/notification_platform_bridge_linux.cc:309: > g_variant_builder_add(&hints_builder, "{sv}", "image-path", > On 2017/04/10 23:49:12, Tom Anderson wrote: > > Initially, this used "image-data" so we didn't have to do the temp file thing, > > but for some reason this was really slow (~0.5s for the notification to show). > > > Writing to the file and using image-path takes just a few ms. > > Any idea where that time is going? A trace shows chrome is doing some stuff in the gpu process (I was converting to an SkBitmap to get the raw pixel data). The actual bus transaction takes <1ms. > Maybe it's not worth investigating if we have > to support "image-path" anyway? > image-data and image-path do the same thing. It's inline images via <img> that we should support. > > I guess we would have needed to do this anyway when image support is added > > because images cannot be inlined like icons. You always need to use a file. > > The spec says "Implementations are not required to support images." Which also > makes me wonder - do we have to support images? Does it better translate the Web > Notifications API if we do? > There is a web API for images, so we should probably support them (these are the <img> ones). > > Side note: It's too bad earler versions of DBus did not support file handles. > Many things built on top of DBus has to use file paths as a result. Which means > we can't do the open file, delete file technique to avoid potentially leaking > files. If Chrome crashes while we have pending files to send, then we litter > /dev/shm. On 2017/04/11 01:27:05, Lei Zhang wrote: > WDYT about avoiding ScopedAllowIO? > > https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... > File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): > > https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... > chrome/browser/notifications/notification_platform_bridge_linux.cc:48: // > fallthrough > super nitty: Intent 2 more because it's the NOTREACHED() that's falling through, > rather than the next case. > > https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... > chrome/browser/notifications/notification_platform_bridge_linux.cc:95: // > Writing to a memory-mapped file doesn't require disk IO. > /dev/shm is backed by tmpfs. If there is enough memory pressure, tmpfs will use > swap, which can potentially hit disk. Yes, the images are small, but let's try > to limit ScopedAllowIO usage to where we have to. > Well, if there was high memory pressure, wouldn't mmap()s via malloc() start blocking too as the system pages out to disk? > The bottom line is we want to try to keep (potentially) janky operations off the > UI thread. It doesn't look that hard to use PostTaskAndReply to offload this > from the UI thread. Do we want to try doing that and then not have to use > /dev/shm? The deletion can be a straight up PostTask. > We can do this, if you're sure it will actually solve the problem. For example, if we put stuff in /tmp instead of /dev/shm and chrome crashes, stuff will still be left in /tmp. And /tmp is often mounted as a tmpfs anyway, so it would leak memory in the same way /dev/shm would. Is there some sort of magic that cleans up /tmp but not /dev/shm? > https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... > chrome/browser/notifications/notification_platform_bridge_linux.cc:99: if > (image.IsEmpty() || !GetShmemTempDir(false, &shmem_temp_dir) || > Assuming we want to go with PostTask, don't bother even calling this if we > already know the image is empty? We'd want to get |image_data| first and then > post that over to a file-access capable thread.
On 2017/04/11 18:17:36, Tom Anderson wrote: > There is a web API for images, so we should probably support them (these are the > <img> ones). Ack. > Well, if there was high memory pressure, wouldn't mmap()s via malloc() start > blocking too as the system pages out to disk? Probably. I'm making a crummy argument. What I'm really thinking is we should avoid ScopedAllowIO. > We can do this, if you're sure it will actually solve the problem. For example, > if we put stuff in /tmp instead of /dev/shm and chrome crashes, stuff will still > be left in /tmp. And /tmp is often mounted as a tmpfs anyway, so it would leak > memory in the same way /dev/shm would. /tmp may not always be mounted as tmpfs. I have one computer where it's just part of / for instance. > Is there some sort of magic that cleans up /tmp but not /dev/shm? I know tmpreaper and other programs like it exist and some people have it installed. I don't know if those tools clean /dev/shm (by default) though.
+rsesek for ui/gfx/image thestig: the latest patch set avoids doing IO on the UI thread https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:48: // fallthrough On 2017/04/11 01:27:05, Lei Zhang wrote: > super nitty: Intent 2 more because it's the NOTREACHED() that's falling through, > rather than the next case. Done. https://codereview.chromium.org/2806203003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:99: if (image.IsEmpty() || !GetShmemTempDir(false, &shmem_temp_dir) || On 2017/04/11 01:27:05, Lei Zhang wrote: > Assuming we want to go with PostTask, don't bother even calling this if we > already know the image is empty? We'd want to get |image_data| first and then > post that over to a file-access capable thread. Done. https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:160: DeleteFile(file); So all of the resource files get cleaned up when chrome closes, even if there are still notifications displayed. Not sure if this is a problem
thomasanderson@google.com changed reviewers: + rsesek@chromium.org
+rsesek for ui/gfx/image (for real this time)
https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (image.IsEmpty() || !base::CreateTemporaryFile(&file_path)) One thing that we discussed for the Windows Action Center (I don't know to what extent this applies to Linux) is using a pool of files as opposed to an arbitrary number of temporary files. If Chrome crashes or doesn't perform a clean exit ResetResourceFiles() wouldn't be called, and the files may be left behind. The idea would be to have icon-[0-10].png or similar and iterate through them. This is possible because the icon data is read by the OS when the notification is shown, and is cached internally after. It sounds like we're not sure of Linux' semantics in this area, is there a way to verify? https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:97: auto image_data = image.As1xPNGBytes(); micro nit: I'd spell out the type (scoped_refptr<base::RefCountedMemory>). It fits on a single line and it's not obvious from the call what the type is. https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:155: ResetResourceFiles(); Since the NotificationPlatformBridgeLinux is owned by the BrowserProcessImpl we're already very far in the shutdown process by the time we schedule this task. Will this call do anything at all? https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:181: std::vector<base::FilePath> resource_files; Out of interest, are you aware of base::FileProxy? It's scoped, avoids some of the task complexity and removes the ScopedAllowUI.
https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:99: if (data_len == 0 || base::WriteFile(file_path, image_data->front_as<char>(), Can we fetch the image data on the UI thread, and then pass only the data to WriteImageToTmpFile()? I have no idea if any of the gfx classes are thread safe and the ui/gfx/image/image.cc change looks slightly suspicious.
https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:117: base::Bind( Isn't this just: base::Bind(&base::DeleteFile, file_path, false) ? https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:308: return base::MakeUnique<ResourceFiles>(WriteImageToTmpFile(icon)); If NotifyNow() can take the FilePath as a result and wrap it in a ResourceFiles there, then this becomes: base::Bind(&WriteImageToTmpFile, notification.icon())
not lgtm. See https://codereview.chromium.org/2692343008/
On 2017/04/12 16:43:28, Robert Sesek wrote: > not lgtm. See https://codereview.chromium.org/2692343008/ rsesek: Does the ImageStorage class description need more comments if folks keep trying to make it RefCountedThreadSafe?
On 2017/04/12 18:36:05, Lei Zhang wrote: > On 2017/04/12 16:43:28, Robert Sesek wrote: > > not lgtm. See https://codereview.chromium.org/2692343008/ > > rsesek: Does the ImageStorage class description need more comments if folks keep > trying to make it RefCountedThreadSafe? Yes, that's not a bad idea. I'll add some.
https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:107: void DeleteFile(const base::FilePath& file_path) { BTW, name this DeleteNotificationImage() so it's easier to distinguish vs other functions named DeleteFile.
https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (image.IsEmpty() || !base::CreateTemporaryFile(&file_path)) On 2017/04/12 00:35:16, Peter Beverloo wrote: > One thing that we discussed for the Windows Action Center (I don't know to > what extent this applies to Linux) is using a pool of files as opposed to > an arbitrary number of temporary files. If Chrome crashes or doesn't perform > a clean exit ResetResourceFiles() wouldn't be called, and the files may > be left behind. > > The idea would be to have icon-[0-10].png or similar and iterate through > them. This is possible because the icon data is read by the OS when the > notification is shown, and is cached internally after. It sounds like > we're not sure of Linux' semantics in this area, is there a way to > verify? The spec doesn't make any guarantees about this, but with the hundreds of DEs out there, I'm sure at least one of them wouldn't cache the icon https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:97: auto image_data = image.As1xPNGBytes(); On 2017/04/12 00:35:16, Peter Beverloo wrote: > micro nit: I'd spell out the type (scoped_refptr<base::RefCountedMemory>). It > fits on a single line and it's not obvious from the call what the type is. Done. https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:99: if (data_len == 0 || base::WriteFile(file_path, image_data->front_as<char>(), On 2017/04/12 01:01:41, Lei Zhang wrote: > Can we fetch the image data on the UI thread, and then pass only the data to > WriteImageToTmpFile()? I have no idea if any of the gfx classes are thread safe > and the ui/gfx/image/image.cc change looks slightly suspicious. Done. https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:107: void DeleteFile(const base::FilePath& file_path) { On 2017/04/13 01:12:17, Lei Zhang wrote: > BTW, name this DeleteNotificationImage() so it's easier to distinguish vs other > functions named DeleteFile. Done. https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:117: base::Bind( On 2017/04/12 01:03:54, Lei Zhang wrote: > Isn't this just: > > base::Bind(&base::DeleteFile, file_path, false) ? Done. Turns out base::IgnoreResult() was the missing ingredient https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:155: ResetResourceFiles(); On 2017/04/12 00:35:16, Peter Beverloo wrote: > Since the NotificationPlatformBridgeLinux is owned by the BrowserProcessImpl > we're already very far in the shutdown process by the time we schedule this > task. Will this call do anything at all? Turns out it will not. Added NOTIFICATION_APP_TERMINATING to handle this. https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:181: std::vector<base::FilePath> resource_files; On 2017/04/12 00:35:16, Peter Beverloo wrote: > Out of interest, are you aware of base::FileProxy? It's scoped, avoids some of > the task complexity and removes the ScopedAllowUI. Didn't know about that. The only thing is, it would complicate Notify() because it would require 4 chained callbacks (open icon file, write to icon file, open image file, write to image file). https://codereview.chromium.org/2806203003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:308: return base::MakeUnique<ResourceFiles>(WriteImageToTmpFile(icon)); On 2017/04/12 01:03:54, Lei Zhang wrote: > If NotifyNow() can take the FilePath as a result and wrap it in a ResourceFiles > there, then this becomes: > > base::Bind(&WriteImageToTmpFile, notification.icon()) I want to keep it as a unique_ptr so that the resource files get deleted if NotifyNow() is not called (this happens when NPBL goes away and resets the WeakPtr)
lgtm https://codereview.chromium.org/2806203003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:100: int data_len = data->size(); Can we check the data size first and fail early without even bothering to create a temp file? Or better yet, don't even post a task if we know there's no data to write out. https://codereview.chromium.org/2806203003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:177: // should be cleaned up when the notification is closed. Or on shut down.
(dropping my "not" above - lgtm) Comment added here: https://codereview.chromium.org/2822493002
https://codereview.chromium.org/2806203003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:100: int data_len = data->size(); On 2017/04/13 04:44:35, Lei Zhang wrote: > Can we check the data size first and fail early without even bothering to create > a temp file? Or better yet, don't even post a task if we know there's no data to > write out. Done. https://codereview.chromium.org/2806203003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:177: // should be cleaned up when the notification is closed. On 2017/04/13 04:44:35, Lei Zhang wrote: > Or on shut down. 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...
https://codereview.chromium.org/2806203003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:98: if (data_len == 0) But this can't happen now, right? DCHECK(data_len) and don't bother handling it?
https://codereview.chromium.org/2806203003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:98: if (data_len == 0) On 2017/04/13 20:29:47, Lei Zhang wrote: > But this can't happen now, right? DCHECK(data_len) and don't bother handling it? Not now, but when there's an icon and an image, we'll only be able to directly call NotifyNow() if both are empty. This check handles the case where only one is empty. Now that I think about it, there's way too many subtleties in this async hell. I have a proposal: have a dedicated thread for notifications. File IO and dbus calls can all by synchronous. I would be much more confident in the correctness of the implementation that way. wdyt?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2806203003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2806203003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:98: if (data_len == 0) On 2017/04/13 20:38:40, Tom Anderson wrote: > On 2017/04/13 20:29:47, Lei Zhang wrote: > > But this can't happen now, right? DCHECK(data_len) and don't bother handling > it? > > Not now, but when there's an icon and an image, we'll only be able to directly > call NotifyNow() if both are empty. This check handles the case where only one > is empty. Ah, ok. You have a better mental picture of where this is going, but I haven't looked at all your CLs. > Now that I think about it, there's way too many subtleties in this async hell. > I have a proposal: have a dedicated thread for notifications. File IO and dbus > calls can all by synchronous. I would be much more confident in the correctness > of the implementation that way. wdyt? I thin you can request a TaskRunner with the traits you need to do a lot of this work, but doesn't the Notification class still live on the UI thread. i.e. You still have to work with 2 different threads.
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, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2806203003/#ps60001 (title: "Address thestig's 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": 60001, "attempt_start_ts": 1492133597420550,
"parent_rev": "a77b04a2af76e6eb19db28716651912d2621d040", "commit_rev":
"d82f2908e3193915cdc573d54f962920714b847d"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: Support icons This CL: * Adds support for notification icons * Adds an urgency hint * Supports updating NotificationCommon::Type BUG=676220 R=peter@chromium.org,thestig@chromium.org ========== to ========== Linux native notifications: Support icons This CL: * Adds support for notification icons * Adds an urgency hint * Supports updating NotificationCommon::Type BUG=676220 R=peter@chromium.org,thestig@chromium.org Review-Url: https://codereview.chromium.org/2806203003 Cr-Commit-Position: refs/heads/master@{#464650} Committed: https://chromium.googlesource.com/chromium/src/+/d82f2908e3193915cdc573d54f96... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d82f2908e3193915cdc573d54f96...
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
