Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2069263002:
Fix retry when GU reports APP_USING_EXTERNAL_UPDATER. (Closed)
|
Created:
4 years, 6 months ago by grt (UTC plus 2) Modified:
4 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix retry when GU reports APP_USING_EXTERNAL_UPDATER.
The previous attempt at this was looking for the error at the wrong spot, hence had no impact.
BUG=504516
Committed: https://crrev.com/0dad6398ec11bd2ac82562e30c8f25669b3f4708
Cr-Commit-Position: refs/heads/master@{#400420}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 20 (10 generated)
The CQ bit was checked by grt@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/2069263002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix retry when GU reports APP_USING_EXTERNAL_UPDATER. BUG=504516 ========== to ========== Fix retry when GU reports APP_USING_EXTERNAL_UPDATER. The previous attempt at this was looking for the error at the wrong spot, hence had no impact. BUG=504516 ==========
grt@chromium.org changed reviewers: + pkasting@chromium.org
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2069263002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It looks like this is mostly a code move. I don't understand things well enough to verify the logic of the moved code and conditionals, but the style and the memory management look OK. LGTM https://codereview.chromium.org/2069263002/diff/20001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/2069263002/diff/20001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:467: DCHECK(FAILED(hresult)); This doesn't seem very meaningful, since we already did "if (SUCCEEDED(hresult)) return" not far above. Unless hresult has some kind of weird state I'm not aware of that's not SUCCEEDED or FAILED.
Thanks. https://codereview.chromium.org/2069263002/diff/20001/chrome/browser/google/g... File chrome/browser/google/google_update_win.cc (right): https://codereview.chromium.org/2069263002/diff/20001/chrome/browser/google/g... chrome/browser/google/google_update_win.cc:467: DCHECK(FAILED(hresult)); On 2016/06/16 21:23:00, Peter Kasting wrote: > This doesn't seem very meaningful, since we already did "if (SUCCEEDED(hresult)) > return" not far above. > > Unless hresult has some kind of weird state I'm not aware of that's not > SUCCEEDED or FAILED. I put it here as a defense against future code changes to make it completely clear that this is the failure case.
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2069263002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix retry when GU reports APP_USING_EXTERNAL_UPDATER. The previous attempt at this was looking for the error at the wrong spot, hence had no impact. BUG=504516 ========== to ========== Fix retry when GU reports APP_USING_EXTERNAL_UPDATER. The previous attempt at this was looking for the error at the wrong spot, hence had no impact. BUG=504516 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix retry when GU reports APP_USING_EXTERNAL_UPDATER. The previous attempt at this was looking for the error at the wrong spot, hence had no impact. BUG=504516 ========== to ========== Fix retry when GU reports APP_USING_EXTERNAL_UPDATER. The previous attempt at this was looking for the error at the wrong spot, hence had no impact. BUG=504516 Committed: https://crrev.com/0dad6398ec11bd2ac82562e30c8f25669b3f4708 Cr-Commit-Position: refs/heads/master@{#400420} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0dad6398ec11bd2ac82562e30c8f25669b3f4708 Cr-Commit-Position: refs/heads/master@{#400420} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
