Move on-demand update checks from the FILE thread to the blocking pool.

Move on-demand update checks from the FILE thread to the blocking pool.

BUG=2462
Committed: https://crrev.com/12cdde78e334c9380bb0b07a85a1198d0094cfe0
Cr-Commit-Position: refs/heads/master@{#409468}

[grt (UTC plus 2), 2016-08-02 14:05:34]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 14:05:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2016-08-02 14:06:41]

Description was changed from

==========
Move on-demand update checks from the FILE thread to the blocking pool.

BUG=2462
==========

to

==========
Move on-demand update checks from the FILE thread to the blocking pool.

BUG=2462
==========

[grt (UTC plus 2), 2016-08-02 14:06:41]

grt@chromium.org changed reviewers:
+ pkasting@chromium.org

[grt (UTC plus 2), 2016-08-02 14:06:56]

PTAL

[commit-bot: I haz the power, 2016-08-02 14:34:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 14:34:38]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-08-03 01:24:34]

I don't feel like I did a very good job reviewing the memory management pieces
of this.  I did try to check for double-frees and leaks, but I only understood
some of the changes enough to do so.

LGTM

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
File chrome/browser/google/google_update_win.cc (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:63: GlobalInterfaceTableClassFactory*
g_git_factory = nullptr;
Nit: Prefer to avoid unusual abbreviations; |g_interface_table_factory| might be
better.  This is especially true since "git" is a word with a different meaning
to most developers.

This also applies to things like GitCookie below.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:224: ~GitCookie() { Revoke(); }
Nit: You may do either, but personally I find it slightly more readable when the
class declaration and method definitions are separate from each other, as in
e.g. the UpdateCheckDriver class below.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:238: return hresult;
This function and the next two should not return values if you're going to
DCHECK that the return code is always S_OK.  DCHECK means that no other outcome
can ever happen, even in an exceptional case.  If that's true, return void.  If
not, don't DCHECK, and ensure callers handle all possible return values, even if
unusual.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:495: 
Nit: Unnecessary blank line

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:609: 
Nit: Seems like this blank line should be above the previous statement rather
than below, as that's the error code for if the next statements fail.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
File chrome/browser/google/google_update_win.h (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.h:94: // invoked on the caller's
sequence to provide feedback on the operation, with
Nit: sequence -> task runner?  (I think of "sequenced" as an adjective
describing the task runner behavior rather than "sequence" being a noun you can
invoke things on.)

This also applies in other places where you've used similar language.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.h:102: // A type of callback supplied by
tests to provide a custom
Nit: You're welcome to omit "A type of callback" on these next two comments if
you want, since it's kinda obvious.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
File chrome/browser/google/google_update_win_unittest.cc (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win_unittest.cc:116: };
Nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win_unittest.cc:669: ASSERT_EQ(0U,
fake_git_->size());
Nit: You could also elect to provide .empty() in place of .size(), which would
make this assert slightly shorter, at the cost of omitting the precise count
from the logs in cases where the assert fails.  (I'd probably
ASSERT_TRUE(fake_git_->empty()) if it were an STL class.)

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win_unittest.cc:680: HRESULT
GetFakeGit(base::win::ScopedComPtr<IGlobalInterfaceTable>* git) {
Seems like this should return void.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/version_updater_win.cc:122:
base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN),
The comments on CONTINUE_ON_SHUTDOWN warn that you could get called back
anytime, even in the midst of final app teardown, so anything global could be
garbage.  Make sure this doesn't cause you any problems...

[grt (UTC plus 2), 2016-08-03 05:36:31]

Thanks for the thorough review.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
File chrome/browser/google/google_update_win.cc (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:63: GlobalInterfaceTableClassFactory*
g_git_factory = nullptr;
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: Prefer to avoid unusual abbreviations; |g_interface_table_factory| might
be
> better.  This is especially true since "git" is a word with a different
meaning
> to most developers.
> 
> This also applies to things like GitCookie below.

Done.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:224: ~GitCookie() { Revoke(); }
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: You may do either, but personally I find it slightly more readable when
the
> class declaration and method definitions are separate from each other, as in
> e.g. the UpdateCheckDriver class below.

Done.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:238: return hresult;
On 2016/08/03 01:24:34, Peter Kasting wrote:
> This function and the next two should not return values if you're going to
> DCHECK that the return code is always S_OK.  DCHECK means that no other
outcome
> can ever happen, even in an exceptional case.  If that's true, return void. 
If
> not, don't DCHECK, and ensure callers handle all possible return values, even
if
> unusual.

Done.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:495: 
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: Unnecessary blank line

Done.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.cc:609: 
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: Seems like this blank line should be above the previous statement rather
> than below, as that's the error code for if the next statements fail.

I grouped it that way because I saw it as reverting the assignment on line 608,
but I don't see a significant difference either way. Going with your preference.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
File chrome/browser/google/google_update_win.h (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.h:94: // invoked on the caller's
sequence to provide feedback on the operation, with
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: sequence -> task runner?  (I think of "sequenced" as an adjective
> describing the task runner behavior rather than "sequence" being a noun you
can
> invoke things on.)
> 
> This also applies in other places where you've used similar language.

Done.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win.h:102: // A type of callback supplied by
tests to provide a custom
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: You're welcome to omit "A type of callback" on these next two comments if
> you want, since it's kinda obvious.

Acknowledged.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
File chrome/browser/google/google_update_win_unittest.cc (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win_unittest.cc:116: };
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win_unittest.cc:669: ASSERT_EQ(0U,
fake_git_->size());
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Nit: You could also elect to provide .empty() in place of .size(), which would
> make this assert slightly shorter, at the cost of omitting the precise count
> from the logs in cases where the assert fails.  (I'd probably
> ASSERT_TRUE(fake_git_->empty()) if it were an STL class.)

Done.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/google/g...
chrome/browser/google/google_update_win_unittest.cc:680: HRESULT
GetFakeGit(base::win::ScopedComPtr<IGlobalInterfaceTable>* git) {
On 2016/08/03 01:24:34, Peter Kasting wrote:
> Seems like this should return void.

This is bound into a GlobalInterfaceTableClassFactory callback, which returns
HRESULT.

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

https://codereview.chromium.org/2207523002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/version_updater_win.cc:122:
base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN),
On 2016/08/03 01:24:34, Peter Kasting wrote:
> The comments on CONTINUE_ON_SHUTDOWN warn that you could get called back
> anytime, even in the midst of final app teardown, so anything global could be
> garbage.  Make sure this doesn't cause you any problems...

I did consider this, and believe it to be safe. Nothing done in the
UpdateCheckDriver touches any global state as far as I know (e.g.,
g_browser_process). Notifications back through the UpdateCheckDelegate all take
place on the caller's SequencedTaskRunner. PostTask to that thread from the
blocking pool will return false if it's gone, and will work normally if not
(it's no big deal if those tasks are dropped on the floor). Let me know if
you're aware of other considerations. Thanks.

[grt (UTC plus 2), 2016-08-03 05:36:36]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2016-08-03 05:36:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2207523002/#ps40001
(title: "pk review")

[commit-bot: I haz the power, 2016-08-03 05:36:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 06:18:16]

Description was changed from

==========
Move on-demand update checks from the FILE thread to the blocking pool.

BUG=2462
==========

to

==========
Move on-demand update checks from the FILE thread to the blocking pool.

BUG=2462
==========

[commit-bot: I haz the power, 2016-08-03 06:18:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-03 06:20:02]

Description was changed from

==========
Move on-demand update checks from the FILE thread to the blocking pool.

BUG=2462
==========

to

==========
Move on-demand update checks from the FILE thread to the blocking pool.

BUG=2462

Committed: https://crrev.com/12cdde78e334c9380bb0b07a85a1198d0094cfe0
Cr-Commit-Position: refs/heads/master@{#409468}
==========

[commit-bot: I haz the power, 2016-08-03 06:20:03]

Patchset 3 (id:??) landed as
https://crrev.com/12cdde78e334c9380bb0b07a85a1198d0094cfe0
Cr-Commit-Position: refs/heads/master@{#409468}

[grt (UTC plus 2), 2016-09-07 11:33:56]

I am reverting this change in https://codereview.chromium.org/2318073002/. It
turns out that it doesn't work quite so well after all; see other issue.