Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1117263002:
Switch on-demand update checks to the less-old GoogleUpdate3 API. (Closed)
|
Created:
5 years, 7 months ago by grt (UTC plus 2) Modified:
5 years, 7 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch on-demand update checks to the less-old GoogleUpdate3 API.
This brings a number of benefits:
- Download and install progress is now reported to the UX.
- Better reporting of errors from Google Update is now possible.
- Installer failures are now reported via UMA.
- Reporting of updates blocked by GP is now done by way of Google
Update.
BUG=424689, 458564, 461971
Committed: https://crrev.com/1294c7440c5d648f769295ac8ae5b7555e507918
Cr-Commit-Position: refs/heads/master@{#329634}
Patch Set 1 #Patch Set 2 : better ui message when updates are disabled by policy #
Total comments: 76
Patch Set 3 : Introduced delegate interface for reporting status #Patch Set 4 : remaining comments addressed #Patch Set 5 : chromium tweak #
Total comments: 42
Patch Set 6 : latest comments, better parent window handling, proper Google Update API #
Total comments: 13
Patch Set 7 : final nits before rebase and commmit #Patch Set 8 : sync to position 329622 #
Total comments: 8
Messages
Total messages: 43 (22 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
grt@chromium.org changed reviewers: + pkasting@chromium.org
grt@chromium.org changed reviewers: + jwd@chromium.org
grt@chromium.org changed reviewers: + ganesh@chromium.org
Gentlemen, please take a look: pkasting: everything ganesh: IGoogleUpdate3 interaction jwd: histograms.xml Thanks.
histograms.xml LGTM
I've only had time to look at the files with smaller changes (i.e. not the primary .cc/unittest). Will look more tomorrow. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:23: enum GoogleUpdateUpgradeResult { Nit: "Status" would be a better word than "Result" in the name of this. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:28: // An update is available (terminal condition when not installing if newer). Nit: I'm slightly confused by "when not installing if newer" as I don't know whether to parse as "terminal condition when (not (installing if newer))" or "(terminal condition when not installing) if newer" and frankly I don't know what either one of those means anyway. Maybe add just a little more verbosity so there are some nouns/pronouns? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:71: // A callback run once or more while an update check is in progress and once Nit: "once or more" -> "at least once"? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:77: // the error. We have several parameters that only have meaning in different states. What if we supplied a couple of callbacks, a la: OnUpgradeStarted(progress) OnUpgradeError(code, message) OnOtherUpgradeStatus(status) This would be a little more verbose but clearer and harder to misuse. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:86: // applied. |elevation_window| is the window which should own any necessary Nit: "If |install_if_newer| is true, any available update will automatically be applied."? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:95: // A type of callback supplied by tests to provide a custom IGoogleUpdate3 Nit: Perhaps note where a reader can find out more about IGoogleUpdate3? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:179: progress = static_cast<int>((update_progress * 100.0) + 0.5); Technically, things like this should probably use ToFlooredInt() from safe_integer_conversions.h. However, looking at the code here to convert this double to an int, and the code in UpdateCheckDriver::IsIntermediateState() that actually computes the value, I wonder whether making this a double is really the right thing to do. It seems like we could use an int everywhere and save a bit of complexity here, while not really making the code there worse: the "downloading" case basically changes from "divide by 2" to "multiply by 50" and the "installing" case just becomes "50 + install_progress / 2". It doesn't really seem more arbitrary to me to pass an int ranging from 0-100 than a double ranging from 0-1. WDYT? (I'd probably just ditch the min() call too, this code is tightly tied to the other code and it seems OK to rely on that code giving back a valid number. If there is a clamp needed anywhere, I'd suggest it in the "installing" case that fetches the original LONG that's from 0-100 -- if one is even needed there. This would allow ditching the <algorithm> include.) https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:208: // is missing. "if no message is missing"? Is there a double negative there?
https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:164: // Prepares |results| to return the upgrade error indicated by |error_code| What is |results|? Does this mean |result_|? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:168: HRESULT hr, Nit: Some functions use |hr| and some |hresult| -- be consistent https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:174: // could not be obtained. Is it possible to simply return the HRESULT, using SUCCESS where this would be "true" and any other value otherwise? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:218: bool IsIntermediateState( Nit: Somehow conceptually it seems like this should go above the IsErrorState() function, or maybe reverse the order of the three of these? ("is intermediate, final, or error state") https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:225: // Pools Google Update to determine the state of the ongoing check or Nit: Polls https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:229: // previuos notification) and another future poll will be scheduled. Nit: previous https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:242: bool install_if_newer_; Nit: install_update_if_possible_ or install_available_updates_ might be clearer names ("newer" is a bit ambiguous). Same comment applies elsewhere https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:250: // True if operating at system-level. Nit: "...instead of user-level" (I assume) https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:347: system_level_ = !InstallUtil::IsPerUserInstall(chrome_exe); Nit: Maybe we should make the bool member be |per_user_install_| to match this API (seems like a clearer name anyway) https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:349: // CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY is handled here. Nit: This maybe should go inside the conditional https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:363: do { A do { } while(false) construction here just so you can break; to the bottom of it seems like a good indication that this should be a helper function that return;s in these places. (I would probably have the helper return the relevant HRESULT.) https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:367: DLOG(ERROR) << "failed to create GoogleUpdate3 class " << std::hex << hr; Logging is uncommon in Chromium code and the old code didn't do any. Normally we can't collect the logs anyway and they just bloat the binary with the relevant strings. Avoid logging unless you need these for debugging (and plan to remove them soon) or have a plan to collect the log output. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:383: dispatch.Release(); Nit: I tend to think it's clearer to use something like: { base::win::ScopedComPtr<IDispatch> dispatch; ... } ...to indicate that the scope of the object is limited to here, and anything below is unrelated. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:389: } Nit: Maybe blank line below this? Not sure, but it seems like there should at least be some blanks inserted in the rest of this block as everything is just strung together https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:390: // This doesn't work. Perhaps it isn't needed since elevation is done by What does "doesn't work" mean? Also, how is this distinct from when we've already passed this HWND to CoCreateInstanceAsAdmin()? Is that not sufficient? Normally we don't leave commented-out code in unless we're going to re-enable it later, so preferable would be to say something in English like "There's no need to call IAppBundle::puParentHWND() here because ...". https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:440: dispatch.Release(); Why is this call needed? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:478: // that the update failed becuase of a failure in the installer. Nit: because https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:519: } else if (state_value == STATE_INSTALL_COMPLETE) { Nit: No else after return (2 places) https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:587: install_progress >= 0 && install_progress <= 100) { Seems like the effect of this is to clamp all out-of-range values to 0. I wonder if something like ClampToRange() (in cc/base/math_util.h) might be better, or if we need to clamp this at all; is this ever going to return out-of-range values? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:610: Nit: This blank line and the two below seem a bit random https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:653: base::TimeDelta::FromMilliseconds(250L)); Nit: Explicit 'L' suffix not necessary You may want to comment why we have a 250 ms delay (and not 0, or some other value)? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:687: CComObject<MockCurrentState>* mock_state = MakeNextState(STATE_ERROR); I wonder if here and below you should just use "auto" (or auto*) for the temp type. Seems like the type is verbose and we don't actually care about it. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:703: mock_state->ExpectAvailableVersion(new_version); Nit: Just: MakeNextState(STATE_UPDATE_AVAILABLE)->ExpectAvailableVersion(new_version); https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:781: (*current_state)->AddRef(); Where is this released? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:876: base::string16 message(L"disabled by policy"); Nit: I'd just inline the initializer into the PushErrorState() call below (2 places) https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:895: IsEmpty())); // version Nit: I'd omit the "// version" here, and similar descriptors, if you've already commented them in a previous EXPECT in the same test. (Several places below) https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:1031: PushErrorState(0x80040902, error, kInstallerError); // E_INSTALLER_FAILED Nit: Seems like this comment should be on the declaration of kInstallerError?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1117263002/#ps140001 (title: "remaining comments addressed")
Thanks for the great feedback. I was able to trim down VersionUpdaterWin quite a bit as well. Please take another look. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:164: // Prepares |results| to return the upgrade error indicated by |error_code| On 2015/05/07 22:37:29, Peter Kasting wrote: > What is |results|? Does this mean |result_|? Comment revised. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:168: HRESULT hr, On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: Some functions use |hr| and some |hresult| -- be consistent Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:174: // could not be obtained. On 2015/05/07 22:37:29, Peter Kasting wrote: > Is it possible to simply return the HRESULT, using SUCCESS where this would be > "true" and any other value otherwise? Yes, although I find that the logic in PollGoogleUpdate is a bit more clear with these functions returning bools. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:218: bool IsIntermediateState( On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: Somehow conceptually it seems like this should go above the IsErrorState() > function, or maybe reverse the order of the three of these? ("is intermediate, > final, or error state") I have them in this order to match the order in which they're called. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:225: // Pools Google Update to determine the state of the ongoing check or On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: Polls Doh! https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:229: // previuos notification) and another future poll will be scheduled. On 2015/05/07 22:37:29, Peter Kasting wrote: > Nit: previous Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:242: bool install_if_newer_; On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: install_update_if_possible_ or install_available_updates_ might be clearer > names ("newer" is a bit ambiguous). Same comment applies elsewhere Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:250: // True if operating at system-level. On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: "...instead of user-level" (I assume) Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:347: system_level_ = !InstallUtil::IsPerUserInstall(chrome_exe); On 2015/05/07 22:37:29, Peter Kasting wrote: > Nit: Maybe we should make the bool member be |per_user_install_| to match this > API (seems like a clearer name anyway) I agree that it seems backward, but system_level is more prevalent in the codebase (especially in chrome/installer/* where it's most relevant). https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:349: // CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY is handled here. On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: This maybe should go inside the conditional It turns out that this comment was a lie. I've rearranged things a little so that the error handling is consistent throughout this function. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:363: do { On 2015/05/07 22:37:28, Peter Kasting wrote: > A do { } while(false) construction here just so you can break; to the bottom of > it seems like a good indication that this should be a helper function that > return;s in these places. > > (I would probably have the helper return the relevant HRESULT.) Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:367: DLOG(ERROR) << "failed to create GoogleUpdate3 class " << std::hex << hr; On 2015/05/07 22:37:28, Peter Kasting wrote: > Logging is uncommon in Chromium code and the old code didn't do any. Normally > we can't collect the logs anyway and they just bloat the binary with the > relevant strings. Avoid logging unless you need these for debugging (and plan > to remove them soon) or have a plan to collect the log output. All of these are DLOG, so there's no bloat. That said, I agree that there's not much value to having them since anyone doing a debug build to look into a problem here can use a debugger or add their own temporary logging. Removed. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:383: dispatch.Release(); On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: I tend to think it's clearer to use something like: > > { > base::win::ScopedComPtr<IDispatch> dispatch; > ... > } > > ...to indicate that the scope of the object is limited to here, and anything > below is unrelated. Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:389: } On 2015/05/07 22:37:29, Peter Kasting wrote: > Nit: Maybe blank line below this? Not sure, but it seems like there should at > least be some blanks inserted in the rest of this block as everything is just > strung together Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:390: // This doesn't work. Perhaps it isn't needed since elevation is done by On 2015/05/07 22:37:29, Peter Kasting wrote: > What does "doesn't work" mean? Causes Google Update to crash. :-( > Also, how is this distinct from when we've already passed this HWND to > CoCreateInstanceAsAdmin()? Is that not sufficient? I believe so. I chatted with Ganesh and verified that there's unlikely to be a case where Google Update would want to pop any kind of UX. > Normally we don't leave commented-out code in unless we're going to re-enable it > later, so preferable would be to say something in English like "There's no need > to call IAppBundle::puParentHWND() here because ...". I'll just remove the whole thing. I don't think the comment is really needed. There are many other functions on these COM interfaces that aren't being used here. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:440: dispatch.Release(); On 2015/05/07 22:37:28, Peter Kasting wrote: > Why is this call needed? For consistency with the previous uses of IDispatch above. Removed in favor of introducing a scope for the IDispatch as above. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:478: // that the update failed becuase of a failure in the installer. On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: because Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:519: } else if (state_value == STATE_INSTALL_COMPLETE) { On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: No else after return (2 places) Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:587: install_progress >= 0 && install_progress <= 100) { On 2015/05/07 22:37:28, Peter Kasting wrote: > Seems like the effect of this is to clamp all out-of-range values to 0. I > wonder if something like ClampToRange() (in cc/base/math_util.h) might be > better, or if we need to clamp this at all; is this ever going to return > out-of-range values? I can't say if a flaw in Google Update will ever cause it to return a value out of range. It seems like reasonable defensive programming to check for an expected value. This is meant to treat an out-of-range value the same way as a failure to get the value, not to clamp it to the min or max, whichever is appropriate. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:610: On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: This blank line and the two below seem a bit random Removed. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:653: base::TimeDelta::FromMilliseconds(250L)); On 2015/05/07 22:37:28, Peter Kasting wrote: > Nit: Explicit 'L' suffix not necessary > > You may want to comment why we have a 250 ms delay (and not 0, or some other > value)? Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:23: enum GoogleUpdateUpgradeResult { On 2015/05/07 01:12:27, Peter Kasting wrote: > Nit: "Status" would be a better word than "Result" in the name of this. Indeed. With clients receiving this value during the check/upgrade, "Result" no longer makes sense. During the change from a single callback to a delegate, the need for clients to see this value has vanished (as have the first two values), so I've moved it out of the header and kept the name as-is. The remaining values are still logged via UMA. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:28: // An update is available (terminal condition when not installing if newer). On 2015/05/07 01:12:27, Peter Kasting wrote: > Nit: I'm slightly confused by "when not installing if newer" as I don't know > whether to parse as "terminal condition when (not (installing if newer))" or > "(terminal condition when not installing) if newer" and frankly I don't know > what either one of those means anyway. Maybe add just a little more verbosity > so there are some nouns/pronouns? All values represent terminal conditions now, so I've removed the confusing comments. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:71: // A callback run once or more while an update check is in progress and once On 2015/05/07 01:12:27, Peter Kasting wrote: > Nit: "once or more" -> "at least once"? Callback removed. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:77: // the error. On 2015/05/07 01:12:27, Peter Kasting wrote: > We have several parameters that only have meaning in different states. > > What if we supplied a couple of callbacks, a la: > > OnUpgradeStarted(progress) > OnUpgradeError(code, message) > OnOtherUpgradeStatus(status) > > This would be a little more verbose but clearer and harder to misuse. That makes sense. I left it as a single callback initially since this is somewhat similar to the CrOS case, which uses a single method to report everything. I've introduced a delegate with a few methods that seem pretty clear from the consumer's point of view. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:86: // applied. |elevation_window| is the window which should own any necessary On 2015/05/07 01:12:28, Peter Kasting wrote: > Nit: "If |install_if_newer| is true, any available update will automatically be > applied."? Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.h:95: // A type of callback supplied by tests to provide a custom IGoogleUpdate3 On 2015/05/07 01:12:27, Peter Kasting wrote: > Nit: Perhaps note where a reader can find out more about IGoogleUpdate3? I've put a pointer to the idl. Unfortunately, I was unable to find any good documentation published along with Google Update. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:687: CComObject<MockCurrentState>* mock_state = MakeNextState(STATE_ERROR); On 2015/05/07 22:37:30, Peter Kasting wrote: > I wonder if here and below you should just use "auto" (or auto*) for the temp > type. Seems like the type is verbose and we don't actually care about it. Hmm. The fact that it's a mock is important. I have mixed feelings about this. Since spelling out the whole type doesn't force line wrapping, I'm inclined to leave it as-is. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:703: mock_state->ExpectAvailableVersion(new_version); On 2015/05/07 22:37:29, Peter Kasting wrote: > Nit: Just: > > MakeNextState(STATE_UPDATE_AVAILABLE)->ExpectAvailableVersion(new_version); Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:781: (*current_state)->AddRef(); On 2015/05/07 22:37:29, Peter Kasting wrote: > Where is this released? It is transferred to the caller. I've added explanatory comments here and in MakeNextState above. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:876: base::string16 message(L"disabled by policy"); On 2015/05/07 22:37:30, Peter Kasting wrote: > Nit: I'd just inline the initializer into the PushErrorState() call below (2 > places) Done here. The string is used twice in the test below, though. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:895: IsEmpty())); // version On 2015/05/07 22:37:29, Peter Kasting wrote: > Nit: I'd omit the "// version" here, and similar descriptors, if you've already > commented them in a previous EXPECT in the same test. (Several places below) Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:1031: PushErrorState(0x80040902, error, kInstallerError); // E_INSTALLER_FAILED On 2015/05/07 22:37:30, Peter Kasting wrote: > Nit: Seems like this comment should be on the declaration of kInstallerError? Comment rejiggered. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:179: progress = static_cast<int>((update_progress * 100.0) + 0.5); On 2015/05/07 01:12:28, Peter Kasting wrote: > Technically, things like this should probably use ToFlooredInt() from > safe_integer_conversions.h. Awesome, I didn't know that was a thing. > However, looking at the code here to convert this double to an int, and the code > in UpdateCheckDriver::IsIntermediateState() that actually computes the value, I > wonder whether making this a double is really the right thing to do. > > It seems like we could use an int everywhere and save a bit of complexity here, > while not really making the code there worse: the "downloading" case basically > changes from "divide by 2" to "multiply by 50" and the "installing" case just > becomes "50 + install_progress / 2". > > It doesn't really seem more arbitrary to me to pass an int ranging from 0-100 > than a double ranging from 0-1. WDYT? SGTM. I was inspired to use a double by the CrOS case, but I don't think there's any compelling reason to use it here. > (I'd probably just ditch the min() call too, this code is tightly tied to the > other code and it seems OK to rely on that code giving back a valid number. If > there is a clamp needed anywhere, I'd suggest it in the "installing" case that > fetches the original LONG that's from 0-100 -- if one is even needed there. > This would allow ditching the <algorithm> include.) No longer needed. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:208: // is missing. On 2015/05/07 01:12:28, Peter Kasting wrote: > "if no message is missing"? Is there a double negative there? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117263002/140001
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1117263002/#ps180001 (title: "chromium tweak")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117263002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Is the switch to weak pointers necessary? I don't really understand any of this code so I'm more reviewing it for style, and the use of weak pointers jumped out; I'm not sure of the lifetime guarantees of these classes. Maybe one of the classes' class-level comments should describe this? https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:347: system_level_ = !InstallUtil::IsPerUserInstall(chrome_exe); On 2015/05/08 18:51:50, grt wrote: > On 2015/05/07 22:37:29, Peter Kasting wrote: > > Nit: Maybe we should make the bool member be |per_user_install_| to match this > > API (seems like a clearer name anyway) > > I agree that it seems backward, but system_level is more prevalent in the > codebase (especially in chrome/installer/* where it's most relevant). OK; I would still suggest "system_level_install_" then as that seems a bit clearer. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:440: dispatch.Release(); On 2015/05/08 18:51:49, grt wrote: > On 2015/05/07 22:37:28, Peter Kasting wrote: > > Why is this call needed? > > For consistency with the previous uses of IDispatch above. Removed in favor of > introducing a scope for the IDispatch as above. I guess the question is more, either a Release() or a scope makes it seem to the reader that it's important that this be destroyed by this point. In the case above, that's true, because we want to avoid overlapping the lifetime of two temps. In this case, though, it seems like this could just persist to the end of the function. So even an explicit scope seems kinda misleading. I'd just let this be function-scoped. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:587: install_progress >= 0 && install_progress <= 100) { On 2015/05/08 18:51:49, grt wrote: > On 2015/05/07 22:37:28, Peter Kasting wrote: > > Seems like the effect of this is to clamp all out-of-range values to 0. I > > wonder if something like ClampToRange() (in cc/base/math_util.h) might be > > better, or if we need to clamp this at all; is this ever going to return > > out-of-range values? > > I can't say if a flaw in Google Update will ever cause it to return a value out > of range. It seems like reasonable defensive programming to check for an > expected value. This is meant to treat an out-of-range value the same way as a > failure to get the value, not to clamp it to the min or max, whichever is > appropriate. OK, fair enough. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:687: CComObject<MockCurrentState>* mock_state = MakeNextState(STATE_ERROR); On 2015/05/08 18:51:50, grt wrote: > On 2015/05/07 22:37:30, Peter Kasting wrote: > > I wonder if here and below you should just use "auto" (or auto*) for the temp > > type. Seems like the type is verbose and we don't actually care about it. > > Hmm. The fact that it's a mock is important. I have mixed feelings about this. > Since spelling out the whole type doesn't force line wrapping, I'm inclined to > leave it as-is. OK. I figure the variable name |mock_state| makes that clear and the type is unnecessary, but if you don't agree, I'm OK with leaving it. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:39: enum GoogleUpdateUpgradeResult { Nit: I still kinda think "Status" would be a better last word than "Result". I know now that you've deprecated the first two, the rest really are "results", but as a reader I still see those first two and this looks like a status enum to me (which happens to only have "final" statuses as non-deprecated values). https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:59: const int64_t kGoogleUpdatePollIntervalMs = 250; Nit: I would define this in the narrowest possible scope (i.e. the function where it's used) https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:185: // Helper function for starting an update check. Returns a success result if Nit: Maybe "Returns the result of the update check. Sets |error_code| if the result is a failure code of some kind." (I mostly have trouble parsing 'a success result" as good grammar.) https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:192: // the user. As a reader I don't know what it means to "prepare to return" something. Maybe "Sets member state variables appropriately to represent the given upgrade error" or the like? Be specific in referring to particular member variable names where needed. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:284: // The most progress value reported most recently to the caller. Nit: Eliminate first "most" https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:285: int last_progress_; Nit: last_reported_progress_? https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:372: // Return results immediately if the driver is not polling Google Update. Nit: This sounds as if the code will be conditional. Maybe rephrase as "Since we won't be polling Google Update, return immediately." or similar. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:428: { Nit: This scope unnecessary https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.h:66: // 0 and 100 (inclusive), is an estimation as to how far along the process the Nit: "how far along the process" -> "what percent complete"? I know it's sort of inaccurate technically, but it matches the UI we'll show the user and motivates what "0" and "100" mean. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.h:75: // BeginUpdateCheck is called with |install_update_if_possible| == true Nit: Trailing period https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:843: // by Group Policy via E_APP_UPDATE_DISABLED_BY_POLICY (0x80040813). Nit: These comments are clearer than the old comments, but I would be OK with actually having a local const called E_APP_UPDATE_DISABLED_BY_POLICY, that might be even clearer. (Similar comment applies below) https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:26: HWND* returned_window = reinterpret_cast<HWND*>(param); Nit: I'd probably just inline this into the next line. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:35: EnumThreadWindows(GetCurrentThreadId(), &WindowEnumeration, It makes me uncomfortable to have Windows calls doing this. Can't we use normal Browser methods and the like to get the relevant current window for this browser process? https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:47: // VersionUpdater Tiny nit: I'd put a colon after these base class names https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:61: void BeginUpdateCheckOnFileThread(bool install_if_newer); Nit: Probably want to change the name of |install_if_newer| to match what you did in the other file https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:91: BeginUpdateCheckOnFileThread(false /* !install_if_newer */); Nit: I'm not in love with these comments when there's only one arg... I feel like people can look at the function declaration if they need to know what this is. If you thought this was so unclear that it required a comment, then at that point I'd switch from a bool to an enum so as to pass named args (but I don't think that's needed here). https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:105: // restart is needed for it to take effect. Nit: Maybe "Check if we need to restart to pick up a previously-applied update"? The "it" in this sentence has no real antecedent. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:132: // so that the spinner just spins rather than show an error message when there Nit: showing https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:133: // is, in fact, no error. Is there a way to not spin but instead show the "no update is available" UI instead? https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:138: // Google Update provides a nice message for the policy case. Use this Nit: "usually provides"? (If it always provided this, we wouldn't have to handle the case where it didn't.)
Patchset #6 (id:190007) has been deleted
Patchset #6 (id:210001) has been deleted
Patchset #6 (id:230001) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1117263002/#ps250001 (title: "latest comments, better parent window handling, proper Google Update API")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117263002/250001
Thanks for the comments. Regarding your WeakPtr comments/concerns: the old code bound a callback to a single method in VersionUpdaterWin via a WeakPtr so that the VersionUpdaterWin instance could be destroyed while the machinery behind BeginUpdateCheck() was still churning. In that case, the callback was passed around and stored within UpdateCheckDriver. The new code works exactly the same way, but takes a WeakPtr to some object implementing the UpdateCheckDelegate interface. The implementation of that interface (VersionUpdaterWin) can be destroyed safely before the BeginUpdateCheck() machinery completes. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:347: system_level_ = !InstallUtil::IsPerUserInstall(chrome_exe); On 2015/05/09 02:19:01, Peter Kasting wrote: > On 2015/05/08 18:51:50, grt wrote: > > On 2015/05/07 22:37:29, Peter Kasting wrote: > > > Nit: Maybe we should make the bool member be |per_user_install_| to match > this > > > API (seems like a clearer name anyway) > > > > I agree that it seems backward, but system_level is more prevalent in the > > codebase (especially in chrome/installer/* where it's most relevant). > > OK; I would still suggest "system_level_install_" then as that seems a bit > clearer. Done. https://codereview.chromium.org/1117263002/diff/100001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:440: dispatch.Release(); On 2015/05/09 02:19:01, Peter Kasting wrote: > On 2015/05/08 18:51:49, grt wrote: > > On 2015/05/07 22:37:28, Peter Kasting wrote: > > > Why is this call needed? > > > > For consistency with the previous uses of IDispatch above. Removed in favor of > > introducing a scope for the IDispatch as above. > > I guess the question is more, either a Release() or a scope makes it seem to the > reader that it's important that this be destroyed by this point. In the case > above, that's true, because we want to avoid overlapping the lifetime of two > temps. In this case, though, it seems like this could just persist to the end > of the function. So even an explicit scope seems kinda misleading. I'd just > let this be function-scoped. Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:39: enum GoogleUpdateUpgradeResult { On 2015/05/09 02:19:01, Peter Kasting wrote: > Nit: I still kinda think "Status" would be a better last word than "Result". I > know now that you've deprecated the first two, the rest really are "results", > but as a reader I still see those first two and this looks like a status enum to > me (which happens to only have "final" statuses as non-deprecated values). Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:59: const int64_t kGoogleUpdatePollIntervalMs = 250; On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: I would define this in the narrowest possible scope (i.e. the function > where it's used) I believe the convention is to have constants like this up in the unnamed namespace near the top of implementation files so that they're easy to find. Here are some examples: https://code.google.com/p/chromium/codesearch#chromium/src/sql/connection.cc&... https://code.google.com/p/chromium/codesearch#chromium/src/apps/ui/views/app_... https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:185: // Helper function for starting an update check. Returns a success result if On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: Maybe "Returns the result of the update check. Sets |error_code| if the > result is a failure code of some kind." > > (I mostly have trouble parsing 'a success result" as good grammar.) Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:192: // the user. On 2015/05/09 02:19:02, Peter Kasting wrote: > As a reader I don't know what it means to "prepare to return" something. Maybe > "Sets member state variables appropriately to represent the given upgrade error" > or the like? Be specific in referring to particular member variable names where > needed. Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:284: // The most progress value reported most recently to the caller. On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: Eliminate first "most" Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:285: int last_progress_; On 2015/05/09 02:19:01, Peter Kasting wrote: > Nit: last_reported_progress_? Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:372: // Return results immediately if the driver is not polling Google Update. On 2015/05/09 02:19:01, Peter Kasting wrote: > Nit: This sounds as if the code will be conditional. Maybe rephrase as "Since > we won't be polling Google Update, return immediately." or similar. Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:428: { On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: This scope unnecessary Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.h:66: // 0 and 100 (inclusive), is an estimation as to how far along the process the On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: "how far along the process" -> "what percent complete"? > > I know it's sort of inaccurate technically, but it matches the UI we'll show the > user and motivates what "0" and "100" mean. Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.h:75: // BeginUpdateCheck is called with |install_update_if_possible| == true On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: Trailing period Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win_unittest.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win_unittest.cc:843: // by Group Policy via E_APP_UPDATE_DISABLED_BY_POLICY (0x80040813). On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: These comments are clearer than the old comments, but I would be OK with > actually having a local const called E_APP_UPDATE_DISABLED_BY_POLICY, that might > be even clearer. (Similar comment applies below) Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:26: HWND* returned_window = reinterpret_cast<HWND*>(param); On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: I'd probably just inline this into the next line. Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:35: EnumThreadWindows(GetCurrentThreadId(), &WindowEnumeration, On 2015/05/09 02:19:02, Peter Kasting wrote: > It makes me uncomfortable to have Windows calls doing this. Can't we use normal > Browser methods and the like to get the relevant current window for this browser > process? Done. There's a slight change in behavior, but after a long discussion with Siggi, I believe it's the right thing. I've documented the behavior in VersionUpdater::Create. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:47: // VersionUpdater On 2015/05/09 02:19:02, Peter Kasting wrote: > Tiny nit: I'd put a colon after these base class names Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:61: void BeginUpdateCheckOnFileThread(bool install_if_newer); On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: Probably want to change the name of |install_if_newer| to match what you > did in the other file Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:91: BeginUpdateCheckOnFileThread(false /* !install_if_newer */); On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: I'm not in love with these comments when there's only one arg... I feel > like people can look at the function declaration if they need to know what this > is. I've been asked by other reviewers to add these comments repeatedly, so I've gotten in the habit of doing so by default. I don't think it's a great challenge for the reader to look at the function declaration as you suggested, but I also don't think that the comment is noise. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:105: // restart is needed for it to take effect. On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: Maybe "Check if we need to restart to pick up a previously-applied update"? > The "it" in this sentence has no real antecedent. Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:132: // so that the spinner just spins rather than show an error message when there On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: showing Done. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:133: // is, in fact, no error. On 2015/05/09 02:19:02, Peter Kasting wrote: > Is there a way to not spin but instead show the "no update is available" UI > instead? I make a chromium-branded build and found that I was wrong about the behavior. I've updated the comment. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:138: // Google Update provides a nice message for the policy case. Use this On 2015/05/09 02:19:02, Peter Kasting wrote: > Nit: "usually provides"? (If it always provided this, we wouldn't have to > handle the case where it didn't.) Just being defensive here. I don't believe Google Update has a contract anywhere stating that there definitely will be a good message, but there is for up-to-date versions. I've clarified the comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:59: const int64_t kGoogleUpdatePollIntervalMs = 250; On 2015/05/12 20:21:51, grt wrote: > On 2015/05/09 02:19:02, Peter Kasting wrote: > > Nit: I would define this in the narrowest possible scope (i.e. the function > > where it's used) > > I believe the convention is to have constants like this up in the unnamed > namespace near the top of implementation files so that they're easy to find. > Here are some examples: > https://code.google.com/p/chromium/codesearch#chromium/src/sql/connection.cc&... > https://code.google.com/p/chromium/codesearch#chromium/src/apps/ui/views/app_... > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... There are examples both ways in the code. It's not worth fighting over, and I won't demand you do it a particular way, but I think what I suggest is more compliant with the Google style guide's directive to define things in the narrowest scope possible. https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/google/... chrome/browser/google/google_update_win.h:66: // 0 and 100 (inclusive), is an estimation as to what percent of the upgrade Nit: percentage https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/google/... chrome/browser/google/google_update_win.h:108: // For use by tests that wish to provide a custom IGoogleUpdateWeb3 IGoogleUpdate3Web? https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_basic.cc (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_basic.cc:24: content::WebContents* /* web_contents */) { Nit: Do we actually need to comment out the param name? I thought we built with that warning disabled... https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_mac.mm (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_mac.mm:57: content::WebContents* /* web_contents */) { Nit: Same comment https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:96: #if defined(GOOGLE_CHROME_BUILD) Do we need to #if out the contents of these next functions? Seems like on Chromium they'll just never be called. The #ifs are kind of noisy. https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:152: content::BrowserThread::FILE), Perhaps outside the scope of this CL, but with my Code Purple hat on: prefer to use the blocking pool over the FILE thread wherever possible.
Thanks for the eyes. I'll rebase and reupload and then land. https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/180001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:59: const int64_t kGoogleUpdatePollIntervalMs = 250; On 2015/05/12 22:04:33, Peter Kasting wrote: > On 2015/05/12 20:21:51, grt wrote: > > On 2015/05/09 02:19:02, Peter Kasting wrote: > > > Nit: I would define this in the narrowest possible scope (i.e. the function > > > where it's used) > > > > I believe the convention is to have constants like this up in the unnamed > > namespace near the top of implementation files so that they're easy to find. > > Here are some examples: > > > https://code.google.com/p/chromium/codesearch#chromium/src/sql/connection.cc&... > > > https://code.google.com/p/chromium/codesearch#chromium/src/apps/ui/views/app_... > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > There are examples both ways in the code. It's not worth fighting over, and I > won't demand you do it a particular way, but I think what I suggest is more > compliant with the Google style guide's directive to define things in the > narrowest scope possible. Just so you know that this choice isn't arbitrary: my preference is to have constants that are configuration parameters for the module at the top of the implementation so that they're not lost in the depths. https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/google/... File chrome/browser/google/google_update_win.h (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/google/... chrome/browser/google/google_update_win.h:66: // 0 and 100 (inclusive), is an estimation as to what percent of the upgrade On 2015/05/12 22:04:34, Peter Kasting wrote: > Nit: percentage Done. https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/google/... chrome/browser/google/google_update_win.h:108: // For use by tests that wish to provide a custom IGoogleUpdateWeb3 On 2015/05/12 22:04:34, Peter Kasting wrote: > IGoogleUpdate3Web? Done. https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_basic.cc (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_basic.cc:24: content::WebContents* /* web_contents */) { On 2015/05/12 22:04:34, Peter Kasting wrote: > Nit: Do we actually need to comment out the param name? I thought we built with > that warning disabled... The comment in common.gypi that disables the warning has "These warnings are level 4. They will be slowly removed as code is fixed." above it, so I think there's an intent to enable it some day. https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:96: #if defined(GOOGLE_CHROME_BUILD) On 2015/05/12 22:04:34, Peter Kasting wrote: > Do we need to #if out the contents of these next functions? Seems like on > Chromium they'll just never be called. The #ifs are kind of noisy. Agreed. I'm going to land this as-is. I think a better solution (which will require more hacking, so I'll do in a folloup CL) is to have a simplified implementation of the VersionUpdater interface for Win Chromium that only implements RelaunchBrowser. With that change, we can cleanly omit the google_update_win.* files from Win Chromium builds altogether. I imagine this will be a small improvement for bot cycle times. The google_update_win* files could still be build into unit_tests so that we'd spot regressions in Chromium builds. https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:152: content::BrowserThread::FILE), On 2015/05/12 22:04:34, Peter Kasting wrote: > Perhaps outside the scope of this CL, but with my Code Purple hat on: prefer to > use the blocking pool over the FILE thread wherever possible. I tried that early on in this change and found that it can't be done in the blocking pool as it exists today. I'll start a thread on chrome-fast so that the Purplers are aware of the issues and can design accordingly.
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1117263002/#ps280001 (title: "sync to position 329622")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117263002/280001
Message was sent while issue was closed.
Committed patchset #8 (id:280001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1294c7440c5d648f769295ac8ae5b7555e507918 Cr-Commit-Position: refs/heads/master@{#329634}
Message was sent while issue was closed.
lgtm Looks great. Just nits. https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:142: // the ServiceClass. MachineClass https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:511: *hresult = app_bundle_->install(); It is somewhat unexpected to see IsErrorState() calling install(). Perhaps have another function (or) call it directly from PollGoogleUpdate(). https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:532: if (SUCCEEDED(current_state->get_availableVersion(version.Receive()))) Minor code duplication here and in IsIntermediateState(). Would it be possible to call on IsIntermediateState() first, and then IsFinalState(), such that the latter then only has to return "true" for the !install_update_if_possible_ case? https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater.h (right): https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater.h:67: virtual void CheckForUpdate(const StatusCallback& status_callback Perhaps mention here that HelpHandler::RequestUpdate calls this function in response to the chrome://chrome page requesting an update check.
Message was sent while issue was closed.
Thanks for the feedback. https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:142: // the ServiceClass. On 2015/05/13 20:21:40, ganesh wrote: > MachineClass Nice catch. Will take care of in a follow-on CL. https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:511: *hresult = app_bundle_->install(); On 2015/05/13 20:21:41, ganesh wrote: > It is somewhat unexpected to see IsErrorState() calling install(). Perhaps have > another function (or) call it directly from PollGoogleUpdate(). Understood. The reason it isn't called from PollGoogleUpdate is that I wanted to minimize the paths that would end up calling OnUpgradeError. https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/google/... chrome/browser/google/google_update_win.cc:532: if (SUCCEEDED(current_state->get_availableVersion(version.Receive()))) On 2015/05/13 20:21:40, ganesh wrote: > Minor code duplication here and in IsIntermediateState(). Would it be possible > to call on IsIntermediateState() first, and then IsFinalState(), such that the > latter then only has to return "true" for the !install_update_if_possible_ case? It would be possible, but would take some rejiggering of the logic in PollGoogleUpdate, which I think is fairly clear as-is. https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater.h (right): https://codereview.chromium.org/1117263002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater.h:67: virtual void CheckForUpdate(const StatusCallback& status_callback On 2015/05/13 20:21:41, ganesh wrote: > Perhaps mention here that HelpHandler::RequestUpdate calls this function in > response to the chrome://chrome page requesting an update check. I tend to not put that sort of things in code comments since a refactor could change the caller without otherwise needing to touch this file. Codesearch is a wonderful thing!
Message was sent while issue was closed.
https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_basic.cc (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_basic.cc:24: content::WebContents* /* web_contents */) { On 2015/05/13 13:04:05, grt wrote: > On 2015/05/12 22:04:34, Peter Kasting wrote: > > Nit: Do we actually need to comment out the param name? I thought we built > with > > that warning disabled... > > The comment in common.gypi that disables the warning has "These warnings are > level 4. They will be slowly removed as code is fixed." above it, so I think > there's an intent to enable it some day. I'm actually the one on the team working on enabling those warnings, and I have no intent to ever enable this :)
Message was sent while issue was closed.
sorin@chromium.org changed reviewers: + sorin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... File chrome/browser/ui/webui/help/version_updater_win.cc (right): https://codereview.chromium.org/1117263002/diff/250001/chrome/browser/ui/webu... chrome/browser/ui/webui/help/version_updater_win.cc:152: content::BrowserThread::FILE), On 2015/05/13 13:04:05, grt wrote: > On 2015/05/12 22:04:34, Peter Kasting wrote: > > Perhaps outside the scope of this CL, but with my Code Purple hat on: prefer > to > > use the blocking pool over the FILE thread wherever possible. > > I tried that early on in this change and found that it can't be done in the > blocking pool as it exists today. I'll start a thread on chrome-fast so that the > Purplers are aware of the issues and can design accordingly. re: COM on FILE thread, I am interested in the solution to this problem too. Greg, please let me know if you have an answer and when you are changing this code. The component update service uses a similar implementation. We need an single-thread COM apartment to make COM calls to BITS. We use the FILE thread to do this as the FILE thread is initialized for COM already and we thought is impractical to use or create a blocking pool for this reason alone. |
