Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
468873002:
Show a desktop notification when the enrollment is completed or failed. (Closed)
|
Created:
6 years, 4 months ago by davidyu Modified:
6 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionShow a desktop notification when the enrollment is completed or failed.
BUG=chromium:353050
TEST=unit_tests
Committed: https://crrev.com/c853a94b32df9fd3bd9308abef21e843eec50618
Cr-Commit-Position: refs/heads/master@{#292355}
Patch Set 1 : #Patch Set 2 : Fixed a bug that NotificationUIManager is created too early. #
Total comments: 107
Patch Set 3 : #
Total comments: 18
Patch Set 4 : #Patch Set 5 : Fix broken build and un-inline virtual functions. #
Messages
Total messages: 21 (0 generated)
Hi Oshima, Can you please review chrome/app/theme/*? The png files have been optimized. The mock of the notification icon (slightly outdated) can be found at https://folio.googleplex.com/_/preview/0B-Vmdj5n-YddWC1CVFdFQlEzeU0#%2F08_Suc.... Thanks!
https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (left): https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5710: <!-- About the device control section in the settings page --> you have removed this comment, which looks to me still valid for the IDS_OPTIONS_DEVICE_CONTROL_SECTION_TITLE. Is it intentional? https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:21: #include "chrome/browser/browser_process.h" do you need this? https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:56: const char* kAttributeOwnerId = "consumer_management.owner_id"; const char[] https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:63: const char* kConsumerManagementOverlay = "/consumer-management-overlay"; ditto https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:381: return; remove this https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:134: const base::Closure& button_click_callback); virtual dtor https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:136: // NotificationDelegate implementation. // NotificationDelegate: is new style. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:144: // message_center::NotificationDelegate implementation. is this dup?
https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5708: <message name="IDS_LOGIN_CONSUMER_MANAGEMENT_ENROLLMENT" desc="A message to show at the signin page when consumer management enrollment is in progress."> Nit: s/signin/sign-in/ https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5744: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_NOTIFICATION_TITLE" desc="The title of the desktop notification shown when the consumer management enrollment is completed"> Nit: s/the consumer/consumer/ https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5747: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_NOTIFICATION_BODY" desc="The body of the desktop notification shown when the consumer management enrollment is completed"> Nit: s/the consumer/consumer/ https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5748: You can now lock this device remotely anytime on Chrome Manager. I presume this string was approved by Chrome UI? "on Chrome Manager" certainly sounds strange to my ears. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5750: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_FAILURE_NOTIFICATION_TITLE" desc="The title of the desktop notification shown when the consumer management enrollment is failed"> Nit 1: s/the consumer/consumer/ Nit 2: s/is failed/fails/ https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5753: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_FAILURE_NOTIFICATION_BODY" desc="The body of the desktop notification shown when the consumer management enrollment is failed"> Nit 1: s/the consumer/consumer/ Nit 2: s/is failed/fails/ https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5754: Enrollment in Google Device Manager was not successful. Here you say "Google Device Manager," in the success message you say "Chrome Manager." That looks very inconsistent. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5756: <message name="IDS_CONSUMER_MANAGEMENT_NOTIFICATION_MODIFY_SETTINGS_BUTTON" desc="The label of the modify settings button on the desktop notification shown when the consumer management enrollment or unenrollment is completed"> Nit: s/the consumer/consumer/ https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5759: <message name="IDS_CONSUMER_MANAGEMENT_NOTIFICATION_TRY_AGAIN_BUTTON" desc="The label of the modify settings button on the desktop notification shown when the consumer management enrollment or unenrollment is failed"> Nit 1: s/the consumer/consumer/ Nit 2: s/is failed/fails/ https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:21: #include "chrome/browser/browser_process.h" Nit: Where is this needed? https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:40: #include "content/public/browser/web_contents.h" Nit: Not used. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:62: // URL This is not a URL. This is a path to something. Please expand the comment to clarify what this is. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:337: base::string16 title, body, button_label; Nit: Declare one variable per line. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:341: title =l10n_util::GetStringUTF16( Nit: s/=/= / https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:352: title =l10n_util::GetStringUTF16( Nit: s/=/= / https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:377: GURL url(chrome::kChromeUISettingsURL); Nit: const. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:381: return; Nit: No need for a return here. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:386: url_string.append(kConsumerManagementOverlay); Nit: Instead of concatenating strings and then turning them into a GURL, can you build a GURL object from the pieces you have, letting GURL take care of URL-compliant concatenation? https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:388: GURL url(url_string); Nit: const. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:394: void ConsumerManagementService::ShowDesktopNotification( Nit: You could fold this into ShowDesktopNotificationAndResetState(). https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:396: const std::string& id, Nit: This will always be kEnrollmentNotificationId. No need for it to be an argument that gets passed in. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:397: const std::string& origin_url, Nit: This will always be kEnrollmentNotificationUrl. No need for it to be an argument that gets passed in. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:402: message_center::RichNotificationData optional_field; Nit: #include "ui/message_center/notification.h" https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:405: scoped_ptr<Notification> notification( Why do you use a scoped_ptr instead of a stack object? https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:407: message_center::NOTIFICATION_TYPE_SIMPLE, Nit: #include "ui/message_center/notification_types.h" https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:413: blink::WebTextDirectionDefault, Nit: #include "third_party/WebKit/public/web/WebTextDirection.h" https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:425: ConsumerManagementService::DesktopNotificationDelegate:: Nit: Declaration and definition order should match. These methods are declared just before OnGetBootAttributeDone(). They should be defined just before it as well. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:426: DesktopNotificationDelegate( Nit: Indent four spaces. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:22: class NotificationUIManager; Nit: Not used. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:32: class WebContents; Nit: This is unnecessary, because WebContents is used in an overridden declaration only. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:129: // |display_callback| is called when the notification is displayed. There is no |display_callback|? https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:130: // |button_click_callback| is called when the button of notification Nit: s/of/in the/ https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:131: // clicked. Nit: s/clicked/is clicked/ https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:137: virtual std::string id() const OVERRIDE { Nit: The style guide forbids inlining virtual functions. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:140: virtual content::WebContents* GetWebContents() const OVERRIDE { Nit: The style guide forbids inlining virtual functions. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:144: // message_center::NotificationDelegate implementation. Nit: That's the same NotificationDelegate as referenced by the comment in line 136. No need for two comments referencing the same base class. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:145: virtual void Display() OVERRIDE {} Nit: The style guide forbids inlining virtual functions. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:147: virtual void Error() OVERRIDE {} Nit: The style guide forbids inlining virtual functions. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:148: virtual void Close(bool by_user) OVERRIDE {} Nit: The style guide forbids inlining virtual functions. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:149: virtual void Click() OVERRIDE {} Nit: The style guide forbids inlining virtual functions. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:217: std::string enrolling_account_id_; Now that there is an |enrolling_profile_|, we can remove the |enrolling_account_id_| because you can always get the account ID from the Profile. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:218: Profile* enrolling_profile_; Now that there is an |enrolling_profile_|, we can remove the |enrolling_token_service_| because you can always get the TokenService from the Profile. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:44: #include "testing/gtest/include/gtest/gtest.h" Nit: No longer used. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:93: g_browser_process->local_state()->GetInteger( Nit: #include "base/prefs/pref_service.h" https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:271: FindById("consumer_management.enroll") != NULL; Nit: No need for the "!= NULL" part.
https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (left): https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5710: <!-- About the device control section in the settings page --> On 2014/08/20 13:24:00, oshima wrote: > you have removed this comment, which looks to me still valid for the > IDS_OPTIONS_DEVICE_CONTROL_SECTION_TITLE. Is it intentional? Yes. I put all consumer management service strings into one single section, so it covers more than just the strings on the settings page. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5708: <message name="IDS_LOGIN_CONSUMER_MANAGEMENT_ENROLLMENT" desc="A message to show at the signin page when consumer management enrollment is in progress."> On 2014/08/20 13:55:48, bartfab wrote: > Nit: s/signin/sign-in/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5744: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_NOTIFICATION_TITLE" desc="The title of the desktop notification shown when the consumer management enrollment is completed"> On 2014/08/20 13:55:48, bartfab wrote: > Nit: s/the consumer/consumer/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5747: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_NOTIFICATION_BODY" desc="The body of the desktop notification shown when the consumer management enrollment is completed"> On 2014/08/20 13:55:48, bartfab wrote: > Nit: s/the consumer/consumer/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5748: You can now lock this device remotely anytime on Chrome Manager. On 2014/08/20 13:55:48, bartfab wrote: > I presume this string was approved by Chrome UI? "on Chrome Manager" certainly > sounds strange to my ears. The string is from a version of the mocks. UI review is still in progress. I will probably change these strings again once the review is done. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5750: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_FAILURE_NOTIFICATION_TITLE" desc="The title of the desktop notification shown when the consumer management enrollment is failed"> On 2014/08/20 13:55:48, bartfab wrote: > Nit 1: s/the consumer/consumer/ > Nit 2: s/is failed/fails/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5753: <message name="IDS_CONSUMER_MANAGEMENT_ENROLLMENT_FAILURE_NOTIFICATION_BODY" desc="The body of the desktop notification shown when the consumer management enrollment is failed"> On 2014/08/20 13:55:48, bartfab wrote: > Nit 1: s/the consumer/consumer/ > Nit 2: s/is failed/fails/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5754: Enrollment in Google Device Manager was not successful. On 2014/08/20 13:55:48, bartfab wrote: > Here you say "Google Device Manager," in the success message you say "Chrome > Manager." That looks very inconsistent. I copied the text from the mock. I'll update the string again when it is finalized. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5756: <message name="IDS_CONSUMER_MANAGEMENT_NOTIFICATION_MODIFY_SETTINGS_BUTTON" desc="The label of the modify settings button on the desktop notification shown when the consumer management enrollment or unenrollment is completed"> On 2014/08/20 13:55:48, bartfab wrote: > Nit: s/the consumer/consumer/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:5759: <message name="IDS_CONSUMER_MANAGEMENT_NOTIFICATION_TRY_AGAIN_BUTTON" desc="The label of the modify settings button on the desktop notification shown when the consumer management enrollment or unenrollment is failed"> On 2014/08/20 13:55:48, bartfab wrote: > Nit 1: s/the consumer/consumer/ > Nit 2: s/is failed/fails/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:21: #include "chrome/browser/browser_process.h" On 2014/08/20 13:55:49, bartfab wrote: > Nit: Where is this needed? Ah, I removed the line that uses this header. Deleted. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:40: #include "content/public/browser/web_contents.h" On 2014/08/20 13:55:49, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:56: const char* kAttributeOwnerId = "consumer_management.owner_id"; On 2014/08/20 13:24:00, oshima wrote: > const char[] Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:62: // URL On 2014/08/20 13:55:50, bartfab wrote: > This is not a URL. This is a path to something. Please expand the comment to > clarify what this is. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:63: const char* kConsumerManagementOverlay = "/consumer-management-overlay"; On 2014/08/20 13:24:01, oshima wrote: > ditto Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:337: base::string16 title, body, button_label; On 2014/08/20 13:55:49, bartfab wrote: > Nit: Declare one variable per line. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:341: title =l10n_util::GetStringUTF16( On 2014/08/20 13:55:50, bartfab wrote: > Nit: s/=/= / Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:352: title =l10n_util::GetStringUTF16( On 2014/08/20 13:55:50, bartfab wrote: > Nit: s/=/= / Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:377: GURL url(chrome::kChromeUISettingsURL); On 2014/08/20 13:55:49, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:381: return; On 2014/08/20 13:55:50, bartfab wrote: > Nit: No need for a return here. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:386: url_string.append(kConsumerManagementOverlay); On 2014/08/20 13:55:51, bartfab wrote: > Nit: Instead of concatenating strings and then turning them into a GURL, can you > build a GURL object from the pieces you have, letting GURL take care of > URL-compliant concatenation? Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:388: GURL url(url_string); On 2014/08/20 13:55:51, bartfab wrote: > Nit: const. Removed. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:394: void ConsumerManagementService::ShowDesktopNotification( On 2014/08/20 13:55:50, bartfab wrote: > Nit: You could fold this into ShowDesktopNotificationAndResetState(). Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:396: const std::string& id, On 2014/08/20 13:55:49, bartfab wrote: > Nit: This will always be kEnrollmentNotificationId. No need for it to be an > argument that gets passed in. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:397: const std::string& origin_url, On 2014/08/20 13:55:49, bartfab wrote: > Nit: This will always be kEnrollmentNotificationUrl. No need for it to be an > argument that gets passed in. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:402: message_center::RichNotificationData optional_field; On 2014/08/20 13:55:49, bartfab wrote: > Nit: #include "ui/message_center/notification.h" Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:405: scoped_ptr<Notification> notification( On 2014/08/20 13:55:50, bartfab wrote: > Why do you use a scoped_ptr instead of a stack object? Converted it back to a simple local variable. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:407: message_center::NOTIFICATION_TYPE_SIMPLE, On 2014/08/20 13:55:51, bartfab wrote: > Nit: #include "ui/message_center/notification_types.h" Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:413: blink::WebTextDirectionDefault, On 2014/08/20 13:55:49, bartfab wrote: > Nit: #include "third_party/WebKit/public/web/WebTextDirection.h" Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:425: ConsumerManagementService::DesktopNotificationDelegate:: On 2014/08/20 13:55:50, bartfab wrote: > Nit: Declaration and definition order should match. These methods are declared > just before OnGetBootAttributeDone(). They should be defined just before it as > well. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:426: DesktopNotificationDelegate( On 2014/08/20 13:55:49, bartfab wrote: > Nit: Indent four spaces. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:22: class NotificationUIManager; On 2014/08/20 13:55:52, bartfab wrote: > Nit: Not used. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:32: class WebContents; On 2014/08/20 13:55:51, bartfab wrote: > Nit: This is unnecessary, because WebContents is used in an overridden > declaration only. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:32: class WebContents; On 2014/08/20 13:55:51, bartfab wrote: > Nit: This is unnecessary, because WebContents is used in an overridden > declaration only. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:129: // |display_callback| is called when the notification is displayed. On 2014/08/20 13:55:52, bartfab wrote: > There is no |display_callback|? Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:130: // |button_click_callback| is called when the button of notification On 2014/08/20 13:55:51, bartfab wrote: > Nit: s/of/in the/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:131: // clicked. On 2014/08/20 13:55:51, bartfab wrote: > Nit: s/clicked/is clicked/ Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:134: const base::Closure& button_click_callback); On 2014/08/20 13:24:01, oshima wrote: > virtual dtor Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:136: // NotificationDelegate implementation. On 2014/08/20 13:24:01, oshima wrote: > // NotificationDelegate: > > is new style. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:137: virtual std::string id() const OVERRIDE { On 2014/08/20 13:55:51, bartfab wrote: > Nit: The style guide forbids inlining virtual functions. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:140: virtual content::WebContents* GetWebContents() const OVERRIDE { On 2014/08/20 13:55:52, bartfab wrote: > Nit: The style guide forbids inlining virtual functions. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:144: // message_center::NotificationDelegate implementation. On 2014/08/20 13:24:01, oshima wrote: > is this dup? Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:145: virtual void Display() OVERRIDE {} On 2014/08/20 13:55:51, bartfab wrote: > Nit: The style guide forbids inlining virtual functions. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:147: virtual void Error() OVERRIDE {} On 2014/08/20 13:55:51, bartfab wrote: > Nit: The style guide forbids inlining virtual functions. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:148: virtual void Close(bool by_user) OVERRIDE {} On 2014/08/20 13:55:52, bartfab wrote: > Nit: The style guide forbids inlining virtual functions. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:149: virtual void Click() OVERRIDE {} On 2014/08/20 13:55:51, bartfab wrote: > Nit: The style guide forbids inlining virtual functions. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:217: std::string enrolling_account_id_; On 2014/08/20 13:55:51, bartfab wrote: > Now that there is an |enrolling_profile_|, we can remove the > |enrolling_account_id_| because you can always get the account ID from the > Profile. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:218: Profile* enrolling_profile_; On 2014/08/20 13:55:52, bartfab wrote: > Now that there is an |enrolling_profile_|, we can remove the > |enrolling_token_service_| because you can always get the TokenService from the > Profile. Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:44: #include "testing/gtest/include/gtest/gtest.h" On 2014/08/20 13:55:52, bartfab wrote: > Nit: No longer used. Shouldn't I include it for EXPECT_* and ASSERT_*? https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:93: g_browser_process->local_state()->GetInteger( On 2014/08/20 13:55:52, bartfab wrote: > Nit: #include "base/prefs/pref_service.h" Done. https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:271: FindById("consumer_management.enroll") != NULL; On 2014/08/20 13:55:52, bartfab wrote: > Nit: No need for the "!= NULL" part. Done.
https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:44: #include "testing/gtest/include/gtest/gtest.h" On 2014/08/21 04:08:26, davidyu wrote: > On 2014/08/20 13:55:52, bartfab wrote: > > Nit: No longer used. > > Shouldn't I include it for EXPECT_* and ASSERT_*? Yes, you are right. Sorry, the include policy gets it wrong sometimes :). https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:64: // The relative path to the settings page URL for opening the consumer How about: The path to the consumer management enrollment/unenrollment confirmation overlay, relative to the settings page URL. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:301: const std::string account_id = GetAccountIdFromProfile(profile); Nit: s/std::string/std::string&/ https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:318: const std::string account_id = GetAccountIdFromProfile(enrolling_profile_); Nit: s/std::string/std::string&/ https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:433: GURL url = base_url.Resolve(kConsumerManagementOverlay); Nit: const. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:24: class ProfileOAuth2TokenService; Nit: No longer used. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:196: const std::string& GetAccountIdFromProfile(Profile* profile) const; Nit 1: "sign-in authenticated account ID" is really hard to parse. How about something like: Returns the account ID signed in to |profile|. Nit 2: This could be a helper method in an anonymous namespace.
c/a/theme lgtm https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:122: class DesktopNotificationDelegate : public NotificationDelegate { Looks like you're not using this in the header. In that case, it's better to move to anonymous namespace in .cc. You can inline virtual methods there (it's prohibited in header but .cc is fine)
https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:122: class DesktopNotificationDelegate : public NotificationDelegate { On 2014/08/21 22:47:07, oshima wrote: > Looks like you're not using this in the header. In that case, it's better to > move to anonymous namespace in .cc. You can inline virtual methods there (it's > prohibited in header but .cc is fine) I have seen inlining of virtual methods done in .cc files (especially in tests, where it is very common) but I cannot see any rule in the style guide that would actually allow it. The style guide says *in bold* that you must never inline virtual methods.
slgtm c/a/theme https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:122: class DesktopNotificationDelegate : public NotificationDelegate { On 2014/08/21 23:35:45, bartfab wrote: > On 2014/08/21 22:47:07, oshima wrote: > > Looks like you're not using this in the header. In that case, it's better to > > move to anonymous namespace in .cc. You can inline virtual methods there (it's > > prohibited in header but .cc is fine) > > I have seen inlining of virtual methods done in .cc files (especially in tests, > where it is very common) but I cannot see any rule in the style guide that would > actually allow it. The style guide says *in bold* that you must never inline > virtual methods. http://www.chromium.org/developers/coding-style/cpp-dos-and-donts It's prohibited in header for technical reasons. In .cc, there are people who don't like and those who likt it. I'm one of those who think this is ok. There are actually a lot of examples in chromium. (I spent enough time on Java, which made me think not inlining in .cc is a kind of dup) That's being said, you're the owner of this dir, so your opinion matters than mine. That's why I lgtm'ed with my comment. I still think this should be in .cc file though.
https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:44: #include "testing/gtest/include/gtest/gtest.h" On 2014/08/21 12:04:25, bartfab wrote: > On 2014/08/21 04:08:26, davidyu wrote: > > On 2014/08/20 13:55:52, bartfab wrote: > > > Nit: No longer used. > > > > Shouldn't I include it for EXPECT_* and ASSERT_*? > > Yes, you are right. Sorry, the include policy gets it wrong sometimes :). Do you have a way to automatically check the header files? https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:64: // The relative path to the settings page URL for opening the consumer On 2014/08/21 12:04:25, bartfab wrote: > How about: > > The path to the consumer management enrollment/unenrollment confirmation > overlay, relative to the settings page URL. Done. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:301: const std::string account_id = GetAccountIdFromProfile(profile); On 2014/08/21 12:04:25, bartfab wrote: > Nit: s/std::string/std::string&/ Done. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:318: const std::string account_id = GetAccountIdFromProfile(enrolling_profile_); On 2014/08/21 12:04:25, bartfab wrote: > Nit: s/std::string/std::string&/ Done. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.cc:433: GURL url = base_url.Resolve(kConsumerManagementOverlay); On 2014/08/21 12:04:25, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:24: class ProfileOAuth2TokenService; On 2014/08/21 12:04:25, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:122: class DesktopNotificationDelegate : public NotificationDelegate { On 2014/08/22 00:44:05, oshima wrote: > On 2014/08/21 23:35:45, bartfab wrote: > > On 2014/08/21 22:47:07, oshima wrote: > > > Looks like you're not using this in the header. In that case, it's better to > > > move to anonymous namespace in .cc. You can inline virtual methods there > (it's > > > prohibited in header but .cc is fine) > > > > I have seen inlining of virtual methods done in .cc files (especially in > tests, > > where it is very common) but I cannot see any rule in the style guide that > would > > actually allow it. The style guide says *in bold* that you must never inline > > virtual methods. > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > It's prohibited in header for technical reasons. In .cc, there are people who > don't > like and those who likt it. I'm one of those who think this is ok. There are > actually > a lot of examples in chromium. (I spent enough time on Java, which made me think > not inlining in .cc is a kind of dup) > > That's being said, you're the owner of this dir, so your opinion matters than > mine. > That's why I lgtm'ed with my comment. I still think this should be in .cc file > though. Moved to .cc. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:196: const std::string& GetAccountIdFromProfile(Profile* profile) const; On 2014/08/21 12:04:25, bartfab wrote: > Nit 1: "sign-in authenticated account ID" is really hard to parse. How about > something like: Returns the account ID signed in to |profile|. > > Nit 2: This could be a helper method in an anonymous namespace. Done.
lgtm https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/468873002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:44: #include "testing/gtest/include/gtest/gtest.h" On 2014/08/22 04:23:10, davidyu wrote: > On 2014/08/21 12:04:25, bartfab wrote: > > On 2014/08/21 04:08:26, davidyu wrote: > > > On 2014/08/20 13:55:52, bartfab wrote: > > > > Nit: No longer used. > > > > > > Shouldn't I include it for EXPECT_* and ASSERT_*? > > > > Yes, you are right. Sorry, the include policy gets it wrong sometimes :). > > Do you have a way to automatically check the header files? Sorry, I mistyped. I meant to say "include police" and that would be me, personally :). I have no automated tools, I just check includes when doing a code review. https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:122: class DesktopNotificationDelegate : public NotificationDelegate { On 2014/08/22 00:44:05, oshima wrote: > On 2014/08/21 23:35:45, bartfab wrote: > > On 2014/08/21 22:47:07, oshima wrote: > > > Looks like you're not using this in the header. In that case, it's better to > > > move to anonymous namespace in .cc. You can inline virtual methods there > (it's > > > prohibited in header but .cc is fine) > > > > I have seen inlining of virtual methods done in .cc files (especially in > tests, > > where it is very common) but I cannot see any rule in the style guide that > would > > actually allow it. The style guide says *in bold* that you must never inline > > virtual methods. > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > It's prohibited in header for technical reasons. In .cc, there are people who > don't > like and those who likt it. I'm one of those who think this is ok. There are > actually > a lot of examples in chromium. (I spent enough time on Java, which made me think > not inlining in .cc is a kind of dup) > > That's being said, you're the owner of this dir, so your opinion matters than > mine. > That's why I lgtm'ed with my comment. I still think this should be in .cc file > though. In addition to the Dos and Don'ts you linked to, there is the actual style guide which explicitly forbids inlining of virtual methods, without making any distinction between header and implementation files: http://www.chromium.org/developers/coding-style#TOC-Inline-functions
https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/468873002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/policy/consumer_management_service.h:122: class DesktopNotificationDelegate : public NotificationDelegate { On 2014/08/25 13:07:30, bartfab wrote: > On 2014/08/22 00:44:05, oshima wrote: > > On 2014/08/21 23:35:45, bartfab wrote: > > > On 2014/08/21 22:47:07, oshima wrote: > > > > Looks like you're not using this in the header. In that case, it's better > to > > > > move to anonymous namespace in .cc. You can inline virtual methods there > > (it's > > > > prohibited in header but .cc is fine) > > > > > > I have seen inlining of virtual methods done in .cc files (especially in > > tests, > > > where it is very common) but I cannot see any rule in the style guide that > > would > > > actually allow it. The style guide says *in bold* that you must never inline > > > virtual methods. > > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > It's prohibited in header for technical reasons. In .cc, there are people who > > don't > > like and those who likt it. I'm one of those who think this is ok. There are > > actually > > a lot of examples in chromium. (I spent enough time on Java, which made me > think > > not inlining in .cc is a kind of dup) > > > > That's being said, you're the owner of this dir, so your opinion matters than > > mine. > > That's why I lgtm'ed with my comment. I still think this should be in .cc file > > though. > > In addition to the Dos and Don'ts you linked to, there is the actual style guide > which explicitly forbids inlining of virtual methods, without making any > distinction between header and implementation files: > > http://www.chromium.org/developers/coding-style#TOC-Inline-functions That line describe the rule for accessor method. The reason why this is not allowed in header is that compiler generates code for each file that include header. Here are examples: https://code.google.com/p/chromium/codesearch#search/&q=virtual.*OVERRIDE.*%7... There is another exception in this rule. You can inline a virtual function in empty body. This is often used in observer classes. Here are examples: https://code.google.com/p/chromium/codesearch#search/&q=virtual.*OVERRIDE.*%7... It's up to OWNERS whether or not to allow it, so you can decide.
The CQ bit was checked by davidyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidyu@chromium.org/468873002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by davidyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidyu@chromium.org/468873002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as 693da91cf29b89309392bbe4e13a415cd448e80f
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c853a94b32df9fd3bd9308abef21e843eec50618 Cr-Commit-Position: refs/heads/master@{#292355} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
