Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794103002:
Add initial support for native Linux desktop notifications (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, yoshiki Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd initial support for native Linux desktop notifications
This CL adds a stub implementation of NotificationPlatformBridgeLinux,
which is responsible for communicating notification changes to the desktop
environment via D-Bus. Once this class is fully implemented, it is
intended to be used by default when the host supports notifications.
BUG=676220
R=thestig@chromium.org,yoshiki@chromium.org
Review-Url: https://codereview.chromium.org/2794103002
Cr-Commit-Position: refs/heads/master@{#461990}
Committed: https://chromium.googlesource.com/chromium/src/+/421f86bb22686a086062ffde46fb746039dd2490
Patch Set 1 #Patch Set 2 : Refactor, move impl to chrome/browser/notifications #
Total comments: 6
Patch Set 3 : Rebase, address peter@'s comments #
Total comments: 23
Patch Set 4 : Address peter and thestig's comments #Patch Set 5 : Remove #endif comment #
Total comments: 2
Patch Set 6 : Move enable_native_notifications to chrome/common/features.gni #
Total comments: 2
Patch Set 7 : Remove unused import #Dependent Patchsets: Messages
Total messages: 50 (35 generated)
Description was changed from ========== [ DO NOT LAND ] Native linux desktop notifications WIP ========== to ========== Add initial support for native Linux desktop notifications This CL adds a stub implmentation of DbusNotificationManager, which is responsible for communicating notification changes to the desktop environment via dbus. Once this class is fully implemented, it is inteded to be used by default when the system supports notifications. This CL also adds a command line switch and chrome flag for enabling this feature. thomasanderson@ and thestig@ are added as OWNERS of the dbus files so that I don't have to bug the notification OWNERS with a bunch of Linux-specific CLs. BUG=676220 R=thestig@chromium.org,yoshiki@chromium.org ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Description was changed from ========== Add initial support for native Linux desktop notifications This CL adds a stub implmentation of DbusNotificationManager, which is responsible for communicating notification changes to the desktop environment via dbus. Once this class is fully implemented, it is inteded to be used by default when the system supports notifications. This CL also adds a command line switch and chrome flag for enabling this feature. thomasanderson@ and thestig@ are added as OWNERS of the dbus files so that I don't have to bug the notification OWNERS with a bunch of Linux-specific CLs. BUG=676220 R=thestig@chromium.org,yoshiki@chromium.org ========== to ========== Add initial support for native Linux desktop notifications This CL adds a stub implmentation of DbusNotificationManager, which is responsible for communicating notification changes to the desktop environment via dbus. Once this class is fully implemented, it is inteded to be used by default when the system supports notifications. This CL also adds a command line switch and chrome flag for enabling this feature. thomasanderson@ and thestig@ are added as OWNERS of the dbus files so that I don't have to bug the notification OWNERS with a bunch of Linux-specific CLs. BUG=676220 R=thestig@chromium.org,yoshiki@chromium.org ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thomasanderson@google.com changed reviewers: + thestig@chromium.org, yoshiki@chromium.org
yoshiki@ ptal thestig@ fyi: Are you OK with reviewing the subsequent changes to make this work?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
peter@chromium.org changed reviewers: + peter@chromium.org - yoshiki@chromium.org
Cool - this is super exciting to see! Please use me as the notifications reviewer for this work :). We're planning to launch support for native Mac notifications in M59, so it's probably worth syncing up about your plans here. https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2489: {"use-system-notifications", flag_descriptions::kUseSystemNotificationsName, Can you reuse "enable-native-notifications" (already included in this file) and just enable it for Linux too? https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/dbus_notification_manager.cc (right): https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... chrome/browser/notifications/dbus_notification_manager.cc:44: // TODO(thomasanderson): Add a complete implemenatation. What's the best documentation I can read on the available features for these notifications? https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... chrome/browser/notifications/message_center_notification_manager.cc:75: DbusNotificationManager::CreateDbusNotificationManager(message_center)); Instead of putting this in MessageCenterNotificationManager, which contains logic that won't be relevant anymore, you should create a NotificationPlatformBridgeLinux and make sure that we create a NativeNotificationDisplayService instance with it in the NotificationDisplayServiceFactory. This pattern already exists for our usage of Mac native notifications.
Patchset #3 (id:120001) has been deleted
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...
On 2017/04/03 23:39:29, Peter Beverloo wrote: > Cool - this is super exciting to see! > > Please use me as the notifications reviewer for this work :). We're planning to > launch support for native Mac notifications in M59, so it's probably worth > syncing up about your plans here. > Reverted the OWNERS change https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2489: {"use-system-notifications", flag_descriptions::kUseSystemNotificationsName, On 2017/04/03 23:39:29, Peter Beverloo wrote: > Can you reuse "enable-native-notifications" (already included in this file) and > just enable it for Linux too? Done. https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/dbus_notification_manager.cc (right): https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... chrome/browser/notifications/dbus_notification_manager.cc:44: // TODO(thomasanderson): Add a complete implemenatation. On 2017/04/03 23:39:29, Peter Beverloo wrote: > What's the best documentation I can read on the available features for these > notifications? The Freedesktop notifications spec: https://developer.gnome.org/notification-spec https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/2794103002/diff/100001/chrome/browser/notific... chrome/browser/notifications/message_center_notification_manager.cc:75: DbusNotificationManager::CreateDbusNotificationManager(message_center)); On 2017/04/03 23:39:29, Peter Beverloo wrote: > Instead of putting this in MessageCenterNotificationManager, which contains > logic that won't be relevant anymore, you should create a > NotificationPlatformBridgeLinux and make sure that we create a > NativeNotificationDisplayService instance with it in the > NotificationDisplayServiceFactory. This pattern already exists for our usage of > Mac native notifications. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add initial support for native Linux desktop notifications This CL adds a stub implmentation of DbusNotificationManager, which is responsible for communicating notification changes to the desktop environment via dbus. Once this class is fully implemented, it is inteded to be used by default when the system supports notifications. This CL also adds a command line switch and chrome flag for enabling this feature. thomasanderson@ and thestig@ are added as OWNERS of the dbus files so that I don't have to bug the notification OWNERS with a bunch of Linux-specific CLs. BUG=676220 R=thestig@chromium.org,yoshiki@chromium.org ========== to ========== Add initial support for native Linux desktop notifications This CL adds a stub implementation of NotificationPlatformBridgeLinux, which is responsible for communicating notification changes to the desktop environment via D-Bus. Once this class is fully implemented, it is intended to be used by default when the host supports notifications. BUG=676220 R=thestig@chromium.org,yoshiki@chromium.org ==========
Tom: happy to review until it goes completely over my head and I start referring you to other reviewers. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_display_service_factory.cc (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_display_service_factory.cc:18: (defined(OS_LINUX) && !defined(OS_CHROMEOS)) To make this even longer, do we need to gate this on defined(USE_GIO) as well, in case someone building Chromium decides to set use_gio = false? https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:15: } // anonymous namespace s/anonymous // https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:26: if (notification_proxy) braces, or if (!notification_proxy) return nullptr; return new NotificationPlatformBridgeLinux(...); https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:36: (void)message_center_; // Silence the unused warning. Is this because we plan to use |message_center_| in the NOTIMPLEMENTED() methods? https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. It's 2017. :) https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:21: // Overridden from NotificationPlatformBridge. Just // NotificationPlatformBridge: will suffice. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:37: GDBusProxy* notification_proxy_; GDBusProxy* const, or maybe use ScopedGObject? https://codereview.chromium.org/2794103002/diff/140001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/common/chrome_f... chrome/common/chrome_features.h:121: #endif // defined(OS_MACOSX) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) BTW, my style opinion is: for one line bodies that are not nested, don't bother with the matching end comment. See line 117 above.
This is really cool! lgtm http://imgur.com/a/ykhAk I'm also OK with per-file OWNERS. I'll read all the docs and will try to keep up, but I have no illusions of matching your level of understanding. In case you're not aware, the following site will probably be very useful in testing the implementation: https://tests.peter.sh/notification-generator/ https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_display_service_factory.cc (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_display_service_factory.cc:18: (defined(OS_LINUX) && !defined(OS_CHROMEOS)) On 2017/04/04 07:47:21, Lei Zhang wrote: > To make this even longer, do we need to gate this on defined(USE_GIO) as well, > in case someone building Chromium decides to set use_gio = false? It probably makes sense to use a BUILDFLAG for this so that we can define the logic once in GN. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:29: return nullptr; If my understanding is correct, this means that NotificationPlatformBridge::Create() will return a nullptr when the platform does not provide the freedesktop notification service, right? If so, why would this class need a pointer to the message center? Ideally the implementation of it, once created, would just assume that functionality is available w/o additional bail-out checks. (We experimented with falling back to the message_center when unsupported notification features were requested, e.g. progress bars, but that created for a terrible user experience.) https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:69: NOTIMPLEMENTED(); nit: the |callback| has an argument to indicate whether it's supported. callback.Run(base::MakeUnique<std::set<std::string>>(), false /* supports_synchronization */); https://codereview.chromium.org/2794103002/diff/140001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/common/chrome_f... chrome/common/chrome_features.h:121: #endif // defined(OS_MACOSX) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) On 2017/04/04 07:47:22, Lei Zhang wrote: > BTW, my style opinion is: for one line bodies that are not nested, don't bother > with the matching end comment. See line 117 above. +1
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/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_display_service_factory.cc (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_display_service_factory.cc:18: (defined(OS_LINUX) && !defined(OS_CHROMEOS)) On 2017/04/04 13:34:46, Peter Beverloo wrote: > On 2017/04/04 07:47:21, Lei Zhang wrote: > > To make this even longer, do we need to gate this on defined(USE_GIO) as well, > > in case someone building Chromium decides to set use_gio = false? > > It probably makes sense to use a BUILDFLAG for this so that we can define the > logic once in GN. Added the BUILDFLAG and made the condition depend on use_gio as well. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:15: } // anonymous namespace On 2017/04/04 07:47:21, Lei Zhang wrote: > s/anonymous // Done. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:26: if (notification_proxy) On 2017/04/04 07:47:21, Lei Zhang wrote: > braces, or > > if (!notification_proxy) > return nullptr; > return new NotificationPlatformBridgeLinux(...); Done. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:29: return nullptr; On 2017/04/04 13:34:46, Peter Beverloo wrote: > If my understanding is correct, this means that > NotificationPlatformBridge::Create() will return a nullptr when the platform > does not provide the freedesktop notification service, right? > Yes > If so, why would this class need a pointer to the message center? Ideally the > implementation of it, once created, would just assume that functionality is > available w/o additional bail-out checks. > Removed message_center_. I thought I needed it to call MessageCenter::ClickOnNotification et al. How does NotificationPlatformBridge handle clicks? > (We experimented with falling back to the message_center when unsupported > notification features were requested, e.g. progress bars, but that created for a > terrible user experience.) https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:36: (void)message_center_; // Silence the unused warning. On 2017/04/04 07:47:21, Lei Zhang wrote: > Is this because we plan to use |message_center_| in the NOTIMPLEMENTED() > methods? Yes, but I think my understanding of MessageCenter was wrong, so I removed it https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:69: NOTIMPLEMENTED(); On 2017/04/04 13:34:46, Peter Beverloo wrote: > nit: the |callback| has an argument to indicate whether it's supported. > > callback.Run(base::MakeUnique<std::set<std::string>>(), false /* > supports_synchronization */); Done. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/04/04 07:47:21, Lei Zhang wrote: > It's 2017. :) Done. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:21: // Overridden from NotificationPlatformBridge. On 2017/04/04 07:47:21, Lei Zhang wrote: > Just // NotificationPlatformBridge: will suffice. Done. https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:37: GDBusProxy* notification_proxy_; On 2017/04/04 07:47:21, Lei Zhang wrote: > GDBusProxy* const, or maybe use ScopedGObject? Done. https://codereview.chromium.org/2794103002/diff/140001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/common/chrome_f... chrome/common/chrome_features.h:121: #endif // defined(OS_MACOSX) || (defined(OS_LINUX) && !defined(OS_CHROMEOS)) On 2017/04/04 13:34:46, Peter Beverloo wrote: > On 2017/04/04 07:47:22, Lei Zhang wrote: > > BTW, my style opinion is: for one line bodies that are not nested, don't > bother > > with the matching end comment. See line 117 above. > > +1 Done.
https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2794103002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:29: return nullptr; On 2017/04/04 21:47:10, Tom Anderson wrote: > On 2017/04/04 13:34:46, Peter Beverloo wrote: > > If so, why would this class need a pointer to the message center? Ideally the > > implementation of it, once created, would just assume that functionality is > > available w/o additional bail-out checks. > > > > Removed message_center_. I thought I needed it to call > MessageCenter::ClickOnNotification et al. How does NotificationPlatformBridge > handle clicks? It's still a bit cumbersome, but following Chromium's rule-of-three (code duplication) I'll pick this up soonish and generalize the entire flow between the three platforms that now support native notifications. 1) Load the profile based on |profile_id| and |is_incognito|. https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio... 2) Call NativeNotificationDisplayService::ProcessNotificationOperation() https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...
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 peter@chromium.org Link to the patchset: https://codereview.chromium.org/2794103002/#ps180001 (title: "Remove #endif comment")
The CQ bit was unchecked by thomasanderson@google.com
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2794103002/diff/180001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2794103002/diff/180001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:5: import("//build/buildflag_header.gni") Maybe use chrome/common/features.gni instead, then you can use BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) in chrome/common/chrome_features.* as well?
https://codereview.chromium.org/2794103002/diff/180001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2794103002/diff/180001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:5: import("//build/buildflag_header.gni") On 2017/04/05 03:55:39, Lei Zhang wrote: > Maybe use chrome/common/features.gni instead, then you can use > BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) in chrome/common/chrome_features.* as > well? 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 https://codereview.chromium.org/2794103002/diff/190001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2794103002/diff/190001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:5: import("//build/buildflag_header.gni") No longer needed.
https://codereview.chromium.org/2794103002/diff/190001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2794103002/diff/190001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:5: import("//build/buildflag_header.gni") On 2017/04/05 04:57:21, Lei Zhang wrote: > No longer needed. Done.
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, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2794103002/#ps210001 (title: "Remove unused import")
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": 210001, "attempt_start_ts": 1491368459958130,
"parent_rev": "14a3ede2a87ed2e81396338aaa61e747e4103100", "commit_rev":
"421f86bb22686a086062ffde46fb746039dd2490"}
Message was sent while issue was closed.
Description was changed from ========== Add initial support for native Linux desktop notifications This CL adds a stub implementation of NotificationPlatformBridgeLinux, which is responsible for communicating notification changes to the desktop environment via D-Bus. Once this class is fully implemented, it is intended to be used by default when the host supports notifications. BUG=676220 R=thestig@chromium.org,yoshiki@chromium.org ========== to ========== Add initial support for native Linux desktop notifications This CL adds a stub implementation of NotificationPlatformBridgeLinux, which is responsible for communicating notification changes to the desktop environment via D-Bus. Once this class is fully implemented, it is intended to be used by default when the host supports notifications. BUG=676220 R=thestig@chromium.org,yoshiki@chromium.org Review-Url: https://codereview.chromium.org/2794103002 Cr-Commit-Position: refs/heads/master@{#461990} Committed: https://chromium.googlesource.com/chromium/src/+/421f86bb22686a086062ffde46fb... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:210001) as https://chromium.googlesource.com/chromium/src/+/421f86bb22686a086062ffde46fb... |
