Componentize component_updater: Break content/ dependency for rest of component_updater tests.

Componentize component_updater: Break content/ dependency for rest of component_updater tests.

BUG=371463
Committed: https://crrev.com/097d3a4a3fa6c627978022ebc9081d51f9ee3c90
Cr-Commit-Position: refs/heads/master@{#292202}

[tommycli, 2014-08-27 00:41:27]

tommycli@chromium.org changed reviewers:
+ sorin@chromium.org

[tommycli, 2014-08-27 00:41:48]

tommycli@chromium.org changed reviewers:
+ blundell@chromium.org

[tommycli, 2014-08-27 00:41:48]

sorin: This breaks tho content/ dependency for the rest of the component_updater
tests.

[blundell, 2014-08-27 12:44:56]

LGTM with question

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
File components/component_updater/test/update_checker_unittest.cc (right):

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
components/component_updater/test/update_checker_unittest.cc:108:
RunThreadsUntilIdle();
Why didn't this need to be done before?

[tommycli, 2014-08-27 15:36:30]

blundell: replied below.

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
File components/component_updater/test/update_checker_unittest.cc (right):

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
components/component_updater/test/update_checker_unittest.cc:108:
RunThreadsUntilIdle();
On 2014/08/27 12:44:56, blundell wrote:
> Why didn't this need to be done before?

In the destructor of content::TestBrowserThreadBundle it runs every message loop
until idle.

The real issue, IMO, is that PostInterceptor (and TestUrlRequestInterceptor has
the same problem) requires the IO message loops to be run to clean itself up
correctly.

I think it should either be reworked to not require this, or run the loop
itself, or hang the constructor until the IO thread cleanup task is run.

The way it is currently, you have to do this, or you'll get a failure on the
next test that uses the interceptor.

[tommycli, 2014-08-27 15:37:03]

On 2014/08/27 15:36:30, tommycli wrote:
> blundell: replied below.
> 
>
https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
> File components/component_updater/test/update_checker_unittest.cc (right):
> 
>
https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
> components/component_updater/test/update_checker_unittest.cc:108:
> RunThreadsUntilIdle();
> On 2014/08/27 12:44:56, blundell wrote:
> > Why didn't this need to be done before?
> 
> In the destructor of content::TestBrowserThreadBundle it runs every message
loop
> until idle.
> 
> The real issue, IMO, is that PostInterceptor (and TestUrlRequestInterceptor
has
> the same problem) requires the IO message loops to be run to clean itself up
> correctly.
> 
> I think it should either be reworked to not require this, or run the loop
> itself, or hang the constructor until the IO thread cleanup task is run.
> 
> The way it is currently, you have to do this, or you'll get a failure on the
> next test that uses the interceptor.

correction: hang the destructor (not the constructor)

[blundell, 2014-08-27 16:56:08]

LGTM

Please turn on the component_updater tests on iOS in components_tests.gyp in
this CL (it's in the OS==ios section).

[Sorin Jianu, 2014-08-27 17:36:02]

lgtm

Thank you  Tommy!

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
File components/component_updater/test/url_request_post_interceptor.h (right):

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
components/component_updater/test/url_request_post_interceptor.h:97: explicit
URLRequestPostInterceptor(
In general, explicit is used for one param ctor in C++03.

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
components/component_updater/test/url_request_post_interceptor.h:143:
InterceptorFactory(
explicit

[tommycli, 2014-08-27 17:59:22]

blundell: I can't turn on these tests for iOS until
https://codereview.chromium.org/508473002/ goes in, since that has a content/
dependency that needs breaking too.

sorin: thanks

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
File components/component_updater/test/url_request_post_interceptor.h (right):

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
components/component_updater/test/url_request_post_interceptor.h:97: explicit
URLRequestPostInterceptor(
On 2014/08/27 17:36:02, Sorin Jianu wrote:
> In general, explicit is used for one param ctor in C++03.

Done.

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
components/component_updater/test/url_request_post_interceptor.h:143:
InterceptorFactory(
On 2014/08/27 17:36:02, Sorin Jianu wrote:
> explicit

Done.

[tommycli, 2014-08-27 18:01:41]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-08-27 18:03:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/514473002/60001

[blundell, 2014-08-27 18:27:18]

On 2014/08/27 17:59:22, tommycli wrote:
> blundell: I can't turn on these tests for iOS until
> https://codereview.chromium.org/508473002/ goes in, since that has a content/
> dependency that needs breaking too.
> 

sgtm

> sorin: thanks
> 
>
https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
> File components/component_updater/test/url_request_post_interceptor.h (right):
> 
>
https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
> components/component_updater/test/url_request_post_interceptor.h:97: explicit
> URLRequestPostInterceptor(
> On 2014/08/27 17:36:02, Sorin Jianu wrote:
> > In general, explicit is used for one param ctor in C++03.
> 
> Done.
> 
>
https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
> components/component_updater/test/url_request_post_interceptor.h:143:
> InterceptorFactory(
> On 2014/08/27 17:36:02, Sorin Jianu wrote:
> > explicit
> 
> Done.

[commit-bot: I haz the power, 2014-08-27 19:06:12]

Committed patchset #4 (id:60001) as 2c6cbe946b1e69c5f7663af9d13587fe76dd8e7c

[Ryan Sleevi, 2014-09-03 00:12:16]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2014-09-03 00:12:17]

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
File components/component_updater/test/update_checker_unittest.cc (right):

https://codereview.chromium.org/514473002/diff/40001/components/component_upd...
components/component_updater/test/update_checker_unittest.cc:108:
RunThreadsUntilIdle();
On 2014/08/27 15:36:30, tommycli wrote:
> On 2014/08/27 12:44:56, blundell wrote:
> > Why didn't this need to be done before?
> 
> In the destructor of content::TestBrowserThreadBundle it runs every message
loop
> until idle.
> 
> The real issue, IMO, is that PostInterceptor (and TestUrlRequestInterceptor
has
> the same problem) requires the IO message loops to be run to clean itself up
> correctly.
> 
> I think it should either be reworked to not require this, or run the loop
> itself, or hang the constructor until the IO thread cleanup task is run.
> 
> The way it is currently, you have to do this, or you'll get a failure on the
> next test that uses the interceptor.

This is more of a question for Paul. content:TBTB is "Doing the right thing" and
matching the guarantees of the //content API.

I don't see the issue you described as a "real issue", but perhaps I'm missing
something.

[commit-bot: I haz the power, 2014-09-10 02:53:20]

Patchset 4 (id:??) landed as
https://crrev.com/097d3a4a3fa6c627978022ebc9081d51f9ee3c90
Cr-Commit-Position: refs/heads/master@{#292202}