Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1769703002:
Add viewing of error messages from Keystone upon self-update failure. (Closed)
|
Created:
4 years, 9 months ago by Ryan Myers (chromium) Modified:
4 years, 9 months ago CC:
chromium-reviews, Sorin Jianu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd viewing of error messages from Keystone upon self-update failure.
With newer versions of Keystone's Registration Framework, errors in the
update process (visible on stderr from agent/ksadmin) are supplied to
Chrome in the userInfo dictionary of notifications. Pull this out if
present, and display it in chrome://help underneath the error code.
Example output: https://screenshot.googleplex.com/uH59OwVp5Xi
BUG=512609
Committed: https://crrev.com/fffe8c740df30ab0529e05000a6fce138075023a
Cr-Commit-Position: refs/heads/master@{#383217}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Code review feedback #Patch Set 3 : Code review feedback pt2 #
Total comments: 28
Patch Set 4 : Code review feedback pt3 + error message l10n #
Total comments: 10
Patch Set 5 : Code review feedback pt4 #
Total comments: 5
Patch Set 6 : Display errors on status==FAILED, not !message.empty() #
Total comments: 1
Messages
Total messages: 55 (21 generated)
Description was changed from ========== Add viewing of error messages from Keystone upon self-update failure. With newer versions of Keystone's Registration Framework, errors in the update process (visible on stderr from agent/ksadmin) are supplied to Chrome in the userInfo dictionary of notifications. Pull this out if present, and display it in chrome://help underneath the error code. Example output: https://screenshot.googleplex.com/uH59OwVp5Xi BUG=512609 ========== to ========== Add viewing of error messages from Keystone upon self-update failure. With newer versions of Keystone's Registration Framework, errors in the update process (visible on stderr from agent/ksadmin) are supplied to Chrome in the userInfo dictionary of notifications. Pull this out if present, and display it in chrome://help underneath the error code. Example output: https://screenshot.googleplex.com/uH59OwVp5Xi BUG=512609 ==========
ryanmyers@google.com changed reviewers: + borisv@chromium.org, sorin@chromium.org
PTAL, sorry for not getting this out for review pre-vacation.
Thank you. We would need approval from the OWNERS of the webui/help I assume.
sorin@chromium.org changed reviewers: - sorin@chromium.org
sorin@chromium.org changed reviewers: + sorin@chromium.org
I removed myself from the reviewers list. It is nice for me to know what is being done here, thank you for keeping me in the loop.
Thanks, Ryan. This is really good change! I have just a few comments. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:132: error:(NSString *)error; Fix the spaces around NSString pointer type. I believe that the Chrome style guide is "(NSString*)parameter" https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:571: // Error messages may be included, depending on the version of Keystone. I don't believe that this is correct. It depends on the version of the registration framework, which is typically similar to the version of this code. As such, I would omit the comment altogether. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:572: NSString *errorMessages = Fix the position of the '*': should be "NSString* ". Applies to the whole file. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:576: if ([self isSystemTicketDoomed]) { Even in the happy case there could be warnings, represented in the "errorMessages". Would you like to log them for diagnostic purposes? https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:598: DCHECK(![self asyncOperationPending]); I believe that this should not be a debug check, but just a return if another check is happening. The condition is hit when the user clicks the refresh button on the chrome://help page, as it takes some time for the update to be downloaded. The impact is very interesting when active updates are present: two update checks start, followed by two installs. Both update checks come back as "update available", but only the first "install" succeeds in installing it. The second waits for the first due to Keystone agent lock. The second "install" finds Chrome "up to date", which confuses the registration framework and the user sees error 12. In reality there is no error. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:603: error:@"keystone_glue: Keystone not available."]; This should happen only on debug builds and Chrome clones. Still, maybe we should do something more user-friendly. "Cannot setup automatic updates. KeystoneRegistration is not available." 'Keystone' is an internal name, but KeystoneRegistration is the name of the framework, so maybe it is ok to use it. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:622: // Error messages may be included, depending on the version of Keystone. Ditto for the version of Keystone in the comment https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:633: [self updateStatus:kAutoupdateAvailable version:version error:nil]; Again, maybe we should log the reported errors? It is good to see them in the Console. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:648: error:@"keystone_glue: Keystone not available."]; Ditto https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:663: // Error messages may be included, depending on the version of Keystone. Ditto https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:675: updateSuccessfullyInstalled_ = YES; Ditto for logging https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... File chrome/browser/mac/keystone_registration.mm (right): https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_registration.mm:35: NSString *KSRegistrationUpdateCheckRawErrorMessagesKey = @"RawErrorMessages"; "NSString *" -> "NSString* ". Applies to both lines https://codereview.chromium.org/1769703002/diff/1/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_mac.mm (right): https://codereview.chromium.org/1769703002/diff/1/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_mac.mm:228: keystone_errors_utf8); The code needs to sanitize the contents of keystone_errors_utf8. '<' and '>' are common in the Keystone errors. I am not that familiar with the Chrome code base, but I would assume that there is code for XML/HTML escaping/sanitization. If not, you can use the GTM code, which is already imported by Chrome with -[NSString gtm_stringByEscapingForHTML]
Thanks, Boris! https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:132: error:(NSString *)error; On 2016/03/08 22:32:51, Boris Vidolov wrote: > Fix the spaces around NSString pointer type. I believe that the Chrome style > guide is "(NSString*)parameter" Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:571: // Error messages may be included, depending on the version of Keystone. On 2016/03/08 22:32:51, Boris Vidolov wrote: > I don't believe that this is correct. It depends on the version of the > registration framework, which is typically similar to the version of this code. > As such, I would omit the comment altogether. Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:572: NSString *errorMessages = On 2016/03/08 22:32:51, Boris Vidolov wrote: > Fix the position of the '*': should be "NSString* ". Applies to the whole file. Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:576: if ([self isSystemTicketDoomed]) { On 2016/03/08 22:32:51, Boris Vidolov wrote: > Even in the happy case there could be warnings, represented in the > "errorMessages". Would you like to log them for diagnostic purposes? Done. (Passed them on to updateStatus; version_updater_mac can decide whether or not to render/log them in a happy case. See there, where I log if they're present on status != Failed.) https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:598: DCHECK(![self asyncOperationPending]); On 2016/03/08 22:32:51, Boris Vidolov wrote: > I believe that this should not be a debug check, but just a return if another > check is happening. The condition is hit when the user clicks the refresh button > on the chrome://help page, as it takes some time for the update to be > downloaded. The impact is very interesting when active updates are present: two > update checks start, followed by two installs. Both update checks come back as > "update available", but only the first "install" succeeds in installing it. The > second waits for the first due to Keystone agent lock. The second "install" > finds Chrome "up to date", which confuses the registration framework and the > user sees error 12. In reality there is no error. Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:603: error:@"keystone_glue: Keystone not available."]; On 2016/03/08 22:32:52, Boris Vidolov wrote: > This should happen only on debug builds and Chrome clones. Still, maybe we > should do something more user-friendly. "Cannot setup automatic updates. > KeystoneRegistration is not available." > > 'Keystone' is an internal name, but KeystoneRegistration is the name of the > framework, so maybe it is ok to use it. Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:622: // Error messages may be included, depending on the version of Keystone. On 2016/03/08 22:32:51, Boris Vidolov wrote: > Ditto for the version of Keystone in the comment Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:633: [self updateStatus:kAutoupdateAvailable version:version error:nil]; On 2016/03/08 22:32:52, Boris Vidolov wrote: > Again, maybe we should log the reported errors? It is good to see them in the > Console. Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:648: error:@"keystone_glue: Keystone not available."]; On 2016/03/08 22:32:52, Boris Vidolov wrote: > Ditto Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:663: // Error messages may be included, depending on the version of Keystone. On 2016/03/08 22:32:51, Boris Vidolov wrote: > Ditto Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:675: updateSuccessfullyInstalled_ = YES; On 2016/03/08 22:32:52, Boris Vidolov wrote: > Ditto for logging Done. https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... File chrome/browser/mac/keystone_registration.mm (right): https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_registration.mm:35: NSString *KSRegistrationUpdateCheckRawErrorMessagesKey = @"RawErrorMessages"; On 2016/03/08 22:32:52, Boris Vidolov wrote: > "NSString *" -> "NSString* ". Applies to both lines Done.
ryanmyers@chromium.org changed reviewers: - sorin@chromium.org
Thanks, Ryan! I have a really minor comment (to remove comment). https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:663: // Error messages may be included, depending on the version of Keystone. On 2016/03/22 22:39:22, Ryan Myers (chromium) wrote: > On 2016/03/08 22:32:51, Boris Vidolov wrote: > > Ditto > > Done. Still here...
lgtm
https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/1/chrome/browser/mac/keystone... chrome/browser/mac/keystone_glue.mm:663: // Error messages may be included, depending on the version of Keystone. On 2016/03/23 00:24:13, Boris Vidolov wrote: > On 2016/03/22 22:39:22, Ryan Myers (chromium) wrote: > > On 2016/03/08 22:32:51, Boris Vidolov wrote: > > > Ditto > > > > Done. > > Still here... Done.
The CQ bit was checked by ryanmyers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from borisv@chromium.org Link to the patchset: https://codereview.chromium.org/1769703002/#ps40001 (title: "Code review feedback pt2")
The CQ bit was unchecked by ryanmyers@chromium.org
The CQ bit was checked by ryanmyers@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769703002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769703002/40001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryanmyers@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769703002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769703002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
mark@chromium.org changed reviewers: + mark@chromium.org
https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:62: // Non-localized error message to return when we can't initiate any actions Why is this not localized? This should be localized. We have a bunch of infrastructure for localizing strings. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:549: if (!registration_) { This can happen before you enter the “registering” state. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:583: [userInfo objectForKey:ksr::KSRegistrationUpdateCheckRawErrorMessagesKey]; When you pull things out of a dictionary, use ObjCCast<NSString> to make sure that you’re really getting an NSString. Otherwise, possible type confusion looms. ObjCCast will return nil if the thing isn’t an NSString. You can also use ObjCCastStrict which will CHECK that it’s an NSString or nil, and abort otherwise. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:610: if ([self asyncOperationPending]) { This can come after the registration_ check. Same on line 659. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:612: VLOG(1) << "checkForUpdate ignored; async operation pending"; I wasn’t a fan of the three VLOGs that this file already grew, and now things are getting worse. Do we really need these? This is a form of bloat in my opinion. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:682: NSString* errorMsgs = Don’t abbreviate. Elsewhere, this was named errorMessages. It can have that name here too. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:759: error:(NSString *)error { Chromium style is NSString*, no space in there. Compare to the line above. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:976: [NSString stringWithFormat:@"Pre-flight script failed to launch: %ld", This ought to be localized. Line 987 too. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_mac.mm (right): https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:140: [[dictionary objectForKey:kAutoupdateStatusErrorMessages] UTF8String]; Use base::SysNSStringToUTF8 instead, to get a std::string. That will handle embedded NULs more correctly. It’ll avoid the need to call strlen() below, and since EscapeForHTML() already requires that a std::string be converted, there’s zero savings in doing any work on this const char*. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:227: // TODO: Should we localize the "Error details" string? If so, get the Yes. All of these strings should be localized. Even if the strings we get back from Keystone aren’t always localized, all of our own user-visible strings must be. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:233: base::StringPrintf("<br/><br/>Error details:<br/><pre>%s</pre>", You can build this up with string +ing, probably no good reason to bring StringPrintf into it.
Pushing back on a couple of comments in KeystoneGlue implementation. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:62: // Non-localized error message to return when we can't initiate any actions On 2016/03/23 15:52:03, Mark Mentovai wrote: > Why is this not localized? > > This should be localized. We have a bunch of infrastructure for localizing > strings. If I follow the code correctly, this message should never appear. Why do we need to waste localization resources for this particular message? https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:549: if (!registration_) { On 2016/03/23 15:52:04, Mark Mentovai wrote: > This can happen before you enter the “registering” state. I don't see how this could happen: see +[KeystoneGlue defaultKeystoneGlue] and loadKeystoneRegistration. If I read it correctly, registration_ is always set or defaultKeystoneGlue returns nil, so this method won't be called. All usage of KeystoneGlue is based on the instance returned by defaultKeystoneGlue method.
https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:62: // Non-localized error message to return when we can't initiate any actions Boris Vidolov wrote: > On 2016/03/23 15:52:03, Mark Mentovai wrote: > > Why is this not localized? > > > > This should be localized. We have a bunch of infrastructure for localizing > > strings. > > If I follow the code correctly, this message should never appear. Why do we need > to waste localization resources for this particular message? If it shouldn’t ever happen, then the logic should be a DCHECK(), we shouldn’t be firing off this message (localized or not). https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:549: if (!registration_) { Boris Vidolov wrote: > On 2016/03/23 15:52:04, Mark Mentovai wrote: > > This can happen before you enter the “registering” state. > > I don't see how this could happen: see +[KeystoneGlue defaultKeystoneGlue] and > loadKeystoneRegistration. If I read it correctly, registration_ is always set or > defaultKeystoneGlue returns nil, so this method won't be called. All usage of > KeystoneGlue is based on the instance returned by defaultKeystoneGlue method. So then what’s the point of this check at all? Why don’t we get rid of it?
https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:62: // Non-localized error message to return when we can't initiate any actions On 2016/03/23 17:22:06, Mark Mentovai wrote: > Boris Vidolov wrote: > > On 2016/03/23 15:52:03, Mark Mentovai wrote: > > > Why is this not localized? > > > > > > This should be localized. We have a bunch of infrastructure for localizing > > > strings. > > > > If I follow the code correctly, this message should never appear. Why do we > need > > to waste localization resources for this particular message? > > If it shouldn’t ever happen, then the logic should be a DCHECK(), we shouldn’t > be firing off this message (localized or not). Agree. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:549: if (!registration_) { On 2016/03/23 17:22:06, Mark Mentovai wrote: > Boris Vidolov wrote: > > On 2016/03/23 15:52:04, Mark Mentovai wrote: > > > This can happen before you enter the “registering” state. > > > > I don't see how this could happen: see +[KeystoneGlue defaultKeystoneGlue] and > > loadKeystoneRegistration. If I read it correctly, registration_ is always set > or > > defaultKeystoneGlue returns nil, so this method won't be called. All usage of > > KeystoneGlue is based on the instance returned by defaultKeystoneGlue method. > > So then what’s the point of this check at all? Why don’t we get rid of it? I am fine with DCHECK for "registration_". Then it should be across the whole file. Ryan's call.
Patchset #4 (id:60001) has been deleted
Thanks, Mark. I'm tempted for keystone_glue to put a "using base::mac::ObjCCast;" at the top and shorten some of these lines. Is that acceptable for Chromium style? https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:62: // Non-localized error message to return when we can't initiate any actions On 2016/03/23 17:37:29, Boris Vidolov wrote: > On 2016/03/23 17:22:06, Mark Mentovai wrote: > > Boris Vidolov wrote: > > > On 2016/03/23 15:52:03, Mark Mentovai wrote: > > > > Why is this not localized? > > > > > > > > This should be localized. We have a bunch of infrastructure for localizing > > > > strings. > > > > > > If I follow the code correctly, this message should never appear. Why do we > > need > > > to waste localization resources for this particular message? > > > > If it shouldn’t ever happen, then the logic should be a DCHECK(), we shouldn’t > > be firing off this message (localized or not). > > Agree. OK, removing and going back to DCHECKs. (TBH, I'm skeptical that we even need the DCHECKs, on a second review of the code.) Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:549: if (!registration_) { On 2016/03/23 17:37:29, Boris Vidolov wrote: > On 2016/03/23 17:22:06, Mark Mentovai wrote: > > Boris Vidolov wrote: > > > On 2016/03/23 15:52:04, Mark Mentovai wrote: > > > > This can happen before you enter the “registering” state. > > > > > > I don't see how this could happen: see +[KeystoneGlue defaultKeystoneGlue] > and > > > loadKeystoneRegistration. If I read it correctly, registration_ is always > set > > or > > > defaultKeystoneGlue returns nil, so this method won't be called. All usage > of > > > KeystoneGlue is based on the instance returned by defaultKeystoneGlue > method. > > > > So then what’s the point of this check at all? Why don’t we get rid of it? > I am fine with DCHECK for "registration_". Then it should be across the whole > file. Ryan's call. Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:583: [userInfo objectForKey:ksr::KSRegistrationUpdateCheckRawErrorMessagesKey]; On 2016/03/23 15:52:03, Mark Mentovai wrote: > When you pull things out of a dictionary, use ObjCCast<NSString> to make sure > that you’re really getting an NSString. Otherwise, possible type confusion > looms. ObjCCast will return nil if the thing isn’t an NSString. > > You can also use ObjCCastStrict which will CHECK that it’s an NSString or nil, > and abort otherwise. Done, both here and elsewhere in the file (i.e. in -brandFilePath where we pull the bundle plist.) https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:610: if ([self asyncOperationPending]) { On 2016/03/23 15:52:04, Mark Mentovai wrote: > This can come after the registration_ check. Same on line 659. Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:612: VLOG(1) << "checkForUpdate ignored; async operation pending"; On 2016/03/23 15:52:03, Mark Mentovai wrote: > I wasn’t a fan of the three VLOGs that this file already grew, and now things > are getting worse. Do we really need these? This is a form of bloat in my > opinion. They're nice to have, but not necessary IMO. At best, if we have a long-running async operation in progress, this lets you know why other messages aren't having an effect, without joining the debugger. I don't feel strongly about it, so removed. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:682: NSString* errorMsgs = On 2016/03/23 15:52:03, Mark Mentovai wrote: > Don’t abbreviate. Elsewhere, this was named errorMessages. It can have that name > here too. Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:759: error:(NSString *)error { On 2016/03/23 15:52:03, Mark Mentovai wrote: > Chromium style is NSString*, no space in there. Compare to the line above. Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:976: [NSString stringWithFormat:@"Pre-flight script failed to launch: %ld", On 2016/03/23 15:52:03, Mark Mentovai wrote: > This ought to be localized. Line 987 too. Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_mac.mm (right): https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:140: [[dictionary objectForKey:kAutoupdateStatusErrorMessages] UTF8String]; On 2016/03/23 15:52:04, Mark Mentovai wrote: > Use base::SysNSStringToUTF8 instead, to get a std::string. That will handle > embedded NULs more correctly. It’ll avoid the need to call strlen() below, and > since EscapeForHTML() already requires that a std::string be converted, there’s > zero savings in doing any work on this const char*. Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:227: // TODO: Should we localize the "Error details" string? If so, get the On 2016/03/23 15:52:04, Mark Mentovai wrote: > Yes. All of these strings should be localized. > > Even if the strings we get back from Keystone aren’t always localized, all of > our own user-visible strings must be. Done. https://codereview.chromium.org/1769703002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:233: base::StringPrintf("<br/><br/>Error details:<br/><pre>%s</pre>", On 2016/03/23 15:52:04, Mark Mentovai wrote: > You can build this up with string +ing, probably no good reason to bring > StringPrintf into it. Done.
Thanks, Mark. I'm tempted for keystone_glue to put a "using base::mac::ObjCCast;" at the top and shorten some of these lines. Is that acceptable for Chromium style?
LGTM. Thanks, and good luck on your next project! https://codereview.chromium.org/1769703002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1769703002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:8108: + Automatic update setup failed. (Pre-flight process could not launch with error: <ph name="ERROR_NUMBER">$1<ex>1</ex></ph>) I think that we should simplify a little bit to limit the verbosity of the parenthetical details and make things a little more accessible to ordinary users. How about: Failed to set up automatic updates for all users (preflight launch error %d) Failed to set up automatic updates for all users (preflight execution error %d) https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:376: NSString* appBundleBrandID = base::mac::ObjCCast<NSString>( Thanks for hitting these too. https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:584: if ([status boolValue]) { Verify that KSRegistrationStatusKey will in fact always be an NSNumber. Previously, it could have been (probably incorrectly) an NSString and nobody would have noticed, because NSString responds to -boolValue too. There are three other uses of boolValue in this file that should also be sanity-checked, some using KSRegistrationUpdateCheckErrorKey instead. https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:973: OSSTATUS_LOG(ERROR, status) This LOG and the one on 988 were necessary because we didn’t surface anything useful in the UI. Now that we have good UI indicating the error, we should get rid of these LOGs, or at least downgrade them to DLOGs. https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_mac.mm (right): https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:142: std::string keystone_errors_utf8 = base::SysNSStringToUTF8( Be more generic than keystone_errors since this doesn’t necessarily have to come from Keystone now. The preflight script errors come from Chrome, for example. Also, the _utf8 tag isn’t necessary because there are no equivalent strings in other types floating around in this function any longer. So maybe just error_message or something along those lines is enough.
Thanks! https://codereview.chromium.org/1769703002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1769703002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:8108: + Automatic update setup failed. (Pre-flight process could not launch with error: <ph name="ERROR_NUMBER">$1<ex>1</ex></ph>) On 2016/03/24 18:06:38, Mark Mentovai wrote: > I think that we should simplify a little bit to limit the verbosity of the > parenthetical details and make things a little more accessible to ordinary > users. How about: > > Failed to set up automatic updates for all users (preflight launch error %d) > Failed to set up automatic updates for all users (preflight execution error %d) Done. https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:584: if ([status boolValue]) { On 2016/03/24 18:06:38, Mark Mentovai wrote: > Verify that KSRegistrationStatusKey will in fact always be an NSNumber. > Previously, it could have been (probably incorrectly) an NSString and nobody > would have noticed, because NSString responds to -boolValue too. There are three > other uses of boolValue in this file that should also be sanity-checked, some > using KSRegistrationUpdateCheckErrorKey instead. Good point. I double-checked; both have been an NSNumber since at least 2009. I think it's okay to trust that. https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:973: OSSTATUS_LOG(ERROR, status) On 2016/03/24 18:06:38, Mark Mentovai wrote: > This LOG and the one on 988 were necessary because we didn’t surface anything > useful in the UI. Now that we have good UI indicating the error, we should get > rid of these LOGs, or at least downgrade them to DLOGs. Done. https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_mac.mm (right): https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:142: std::string keystone_errors_utf8 = base::SysNSStringToUTF8( On 2016/03/24 18:06:38, Mark Mentovai wrote: > Be more generic than keystone_errors since this doesn’t necessarily have to come > from Keystone now. The preflight script errors come from Chrome, for example. > Also, the _utf8 tag isn’t necessary because there are no equivalent strings in > other types floating around in this function any longer. So maybe just > error_message or something along those lines is enough. Done. https://codereview.chromium.org/1769703002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_mac.mm:142: std::string keystone_errors_utf8 = base::SysNSStringToUTF8( On 2016/03/24 18:06:38, Mark Mentovai wrote: > Be more generic than keystone_errors since this doesn’t necessarily have to come > from Keystone now. The preflight script errors come from Chrome, for example. > Also, the _utf8 tag isn’t necessary because there are no equivalent strings in > other types floating around in this function any longer. So maybe just > error_message or something along those lines is enough. Done.
LGTM https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... chrome/browser/mac/keystone_glue.mm:588: error:errorMessages]; For non-failure states like this and kAutoupdateRegistered, should we even bother trying to display errorMessages? https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... chrome/browser/mac/keystone_glue.mm:650: error:errorMessages]; This too.
https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... chrome/browser/mac/keystone_glue.mm:588: error:errorMessages]; On 2016/03/24 20:10:47, Mark Mentovai wrote: > For non-failure states like this and kAutoupdateRegistered, should we even > bother trying to display errorMessages? IMHO I think it's worth passing through the keystone glue layer, at least -- it may have a relevant warning message, depending on Keystone's verbosity flags. Deciding whether or not to display it seems like a decision for the HelpUI layer to make. (As of this CL, version_updater_mac will only display errorMessages on a failure state.)
https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... chrome/browser/mac/keystone_glue.mm:588: error:errorMessages]; Ryan Myers (chromium) wrote: > On 2016/03/24 20:10:47, Mark Mentovai wrote: > > For non-failure states like this and kAutoupdateRegistered, should we even > > bother trying to display errorMessages? > > IMHO I think it's worth passing through the keystone glue layer, at least -- it > may have a relevant warning message, depending on Keystone's verbosity flags. > Deciding whether or not to display it seems like a decision for the HelpUI layer > to make. > > (As of this CL, version_updater_mac will only display errorMessages on a failure > state.) Ah, right, you’re relying on !message.empty() before adding more stuff to message. That’s subtle. It’s either worth a comment, or better yet, maybe we should call message something more descriptive, like failure_message.
https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/1769703002/diff/100001/chrome/browser/mac/key... chrome/browser/mac/keystone_glue.mm:588: error:errorMessages]; On 2016/03/24 20:28:48, Mark Mentovai wrote: > Ryan Myers (chromium) wrote: > > On 2016/03/24 20:10:47, Mark Mentovai wrote: > > > For non-failure states like this and kAutoupdateRegistered, should we even > > > bother trying to display errorMessages? > > > > IMHO I think it's worth passing through the keystone glue layer, at least -- > it > > may have a relevant warning message, depending on Keystone's verbosity flags. > > Deciding whether or not to display it seems like a decision for the HelpUI > layer > > to make. > > > > (As of this CL, version_updater_mac will only display errorMessages on a > failure > > state.) > > Ah, right, you’re relying on !message.empty() before adding more stuff to > message. That’s subtle. It’s either worth a comment, or better yet, maybe we > should call message something more descriptive, like failure_message. Yeah, that is a little subtle. Reworked it so it relies on status==FAILED instead of an empty message.
LGTM Thanks again, Ryan!
The CQ bit was checked by ryanmyers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from borisv@chromium.org Link to the patchset: https://codereview.chromium.org/1769703002/#ps120001 (title: "Display errors on status==FAILED, not !message.empty()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769703002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769703002/120001
ryanmyers@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan -- can you scan and approve the change to version_updater_mac.cc in helpui?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm nit is optional. https://codereview.chromium.org/1769703002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_mac.mm (right): https://codereview.chromium.org/1769703002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_mac.mm:240: message += base::UTF8ToUTF16("</pre>"); nit: String cancat is not i18n friendly. We should make IDS_UPGRADE_ERROR_DETAILS a template with placeholders, e.g. in resources.grd <message name="IDS_UPGRADE_ERROR_DETAILS ... Error details:<ph name="BR"><br/></ph> <ph name="BEGIN_PRE"><pre></ph> <ph name="ERROR_DETAILS">%1<ex>A detailed error message.</ex></ph> <ph name="END_PRE"></pre></ph> </message> and here: message += l10n_util::GetStringFUTF16(IDS_UPGRADE_ERROR_DETAILS, net::EscapeForHTML(error_messages)); still not ideal though because details string is still concated to |message|.
On 2016/03/24 22:14:15, xiyuan wrote: > lgtm > > nit is optional. > > https://codereview.chromium.org/1769703002/diff/120001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/help/version_updater_mac.mm (right): > > https://codereview.chromium.org/1769703002/diff/120001/chrome/browser/ui/webu... > chrome/browser/ui/webui/help/version_updater_mac.mm:240: message += > base::UTF8ToUTF16("</pre>"); > nit: String cancat is not i18n friendly. We should make > IDS_UPGRADE_ERROR_DETAILS a template with placeholders, > > e.g. > in resources.grd > <message name="IDS_UPGRADE_ERROR_DETAILS ... > Error details:<ph name="BR"><br/></ph> > <ph name="BEGIN_PRE"><pre></ph> > <ph name="ERROR_DETAILS">%1<ex>A detailed error message.</ex></ph> > <ph name="END_PRE"></pre></ph> > </message> > > and here: > message += l10n_util::GetStringFUTF16(IDS_UPGRADE_ERROR_DETAILS, > net::EscapeForHTML(error_messages)); > > still not ideal though because details string is still concated to |message|. Understood. I'll come back and do a followup CL later that combines |message| and |error_messages| into a single string template. Thanks!
The CQ bit was checked by ryanmyers@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769703002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769703002/120001
Message was sent while issue was closed.
Description was changed from ========== Add viewing of error messages from Keystone upon self-update failure. With newer versions of Keystone's Registration Framework, errors in the update process (visible on stderr from agent/ksadmin) are supplied to Chrome in the userInfo dictionary of notifications. Pull this out if present, and display it in chrome://help underneath the error code. Example output: https://screenshot.googleplex.com/uH59OwVp5Xi BUG=512609 ========== to ========== Add viewing of error messages from Keystone upon self-update failure. With newer versions of Keystone's Registration Framework, errors in the update process (visible on stderr from agent/ksadmin) are supplied to Chrome in the userInfo dictionary of notifications. Pull this out if present, and display it in chrome://help underneath the error code. Example output: https://screenshot.googleplex.com/uH59OwVp5Xi BUG=512609 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add viewing of error messages from Keystone upon self-update failure. With newer versions of Keystone's Registration Framework, errors in the update process (visible on stderr from agent/ksadmin) are supplied to Chrome in the userInfo dictionary of notifications. Pull this out if present, and display it in chrome://help underneath the error code. Example output: https://screenshot.googleplex.com/uH59OwVp5Xi BUG=512609 ========== to ========== Add viewing of error messages from Keystone upon self-update failure. With newer versions of Keystone's Registration Framework, errors in the update process (visible on stderr from agent/ksadmin) are supplied to Chrome in the userInfo dictionary of notifications. Pull this out if present, and display it in chrome://help underneath the error code. Example output: https://screenshot.googleplex.com/uH59OwVp5Xi BUG=512609 Committed: https://crrev.com/fffe8c740df30ab0529e05000a6fce138075023a Cr-Commit-Position: refs/heads/master@{#383217} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fffe8c740df30ab0529e05000a6fce138075023a Cr-Commit-Position: refs/heads/master@{#383217} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
