Remove dependency on content::BrowserThread from GCMDriver

Remove dependency on content::BrowserThread from GCMDriver

This is in preparation for GCM componentization.

BUG=356716
TEST=existing tests
TBR=yoz@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272298

[fgorski, 2014-05-21 21:31:25]

lgtm

[Nicolas Zea, 2014-05-21 22:16:28]

https://codereview.chromium.org/292813007/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_driver.h (right):

https://codereview.chromium.org/292813007/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_driver.h:58: const
scoped_refptr<base::MessageLoopProxy>& ui_thread,
a MessageLoopProxy is a SequencedTaskRunner. You may as well pass everything as
a SequencedTaskRunner

[jianli, 2014-05-21 23:09:34]

https://codereview.chromium.org/292813007/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_driver.h (right):

https://codereview.chromium.org/292813007/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_driver.h:58: const
scoped_refptr<base::MessageLoopProxy>& ui_thread,
On 2014/05/21 22:16:28, Nicolas Zea wrote:
> a MessageLoopProxy is a SequencedTaskRunner. You may as well pass everything
as
> a SequencedTaskRunner

Done.

[Nicolas Zea, 2014-05-21 23:43:29]

https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):

https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread io_thread_;
Could you just have one task runner that you use for everything? I don't think
you need a real thread here

[jianli, 2014-05-22 00:11:28]

https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):

https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread io_thread_;
On 2014/05/21 23:43:29, Nicolas Zea wrote:
> Could you just have one task runner that you use for everything? I don't think
> you need a real thread here

Yes, we can do that but I am afraid that we're not able to catch those DCHECK
failures to validate the correct calling thread.

[Michael van Ouwerkerk, 2014-05-22 10:17:50]

On 2014/05/22 00:11:28, jianli wrote:
>
https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
> File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):
> 
>
https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
> chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread
io_thread_;
> On 2014/05/21 23:43:29, Nicolas Zea wrote:
> > Could you just have one task runner that you use for everything? I don't
think
> > you need a real thread here
> 
> Yes, we can do that but I am afraid that we're not able to catch those DCHECK
> failures to validate the correct calling thread.

I'm curious: why is it bad to depend on content::BrowserThread?

[fgorski, 2014-05-22 16:04:42]

On 2014/05/22 10:17:50, Michael van Ouwerkerk wrote:
> On 2014/05/22 00:11:28, jianli wrote:
> >
>
https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
> > File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
> > chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread
> io_thread_;
> > On 2014/05/21 23:43:29, Nicolas Zea wrote:
> > > Could you just have one task runner that you use for everything? I don't
> think
> > > you need a real thread here
> > 
> > Yes, we can do that but I am afraid that we're not able to catch those
DCHECK
> > failures to validate the correct calling thread.
> 
> I'm curious: why is it bad to depend on content::BrowserThread?

Michael, One of Jian's next patches will move GCMDriver to components, where
content::BrowserThread cannot be referenced.

[Michael van Ouwerkerk, 2014-05-22 16:10:34]

On 2014/05/22 16:04:42, fgorski wrote:
> On 2014/05/22 10:17:50, Michael van Ouwerkerk wrote:
> > On 2014/05/22 00:11:28, jianli wrote:
> > >
> >
>
https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
> > > File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
> > > chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread
> > io_thread_;
> > > On 2014/05/21 23:43:29, Nicolas Zea wrote:
> > > > Could you just have one task runner that you use for everything? I don't
> > think
> > > > you need a real thread here
> > > 
> > > Yes, we can do that but I am afraid that we're not able to catch those
> DCHECK
> > > failures to validate the correct calling thread.
> > 
> > I'm curious: why is it bad to depend on content::BrowserThread?
> 
> Michael, One of Jian's next patches will move GCMDriver to components, where
> content::BrowserThread cannot be referenced.

Ok... but why isn't it allowed? I see there are components that use
BrowserThread.

[jianli, 2014-05-22 16:44:26]

Though we can still choose to have a component layer depend on content
layer, it is not encouraged, esp when you want to support the component for
iOS. Please see layered component design doc for more details.

Since gcm component does not need any content rendering support from
content layer, it makes no sense to use content::BrowserThread since there
is other simple way to use threads.


On Thu, May 22, 2014 at 9:10 AM, <mvanouwerkerk@chromium.org> wrote:

> On 2014/05/22 16:04:42, fgorski wrote:
>
>> On 2014/05/22 10:17:50, Michael van Ouwerkerk wrote:
>> > On 2014/05/22 00:11:28, jianli wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.org/292813007/diff/20001/
> chrome/browser/services/gcm/gcm_driver_unittest.cc
>
>> > > File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/292813007/diff/20001/
> chrome/browser/services/gcm/gcm_driver_unittest.cc#newcode209
>
>> > > chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread
>> > io_thread_;
>> > > On 2014/05/21 23:43:29, Nicolas Zea wrote:
>> > > > Could you just have one task runner that you use for everything? I
>> don't
>> > think
>> > > > you need a real thread here
>> > >
>> > > Yes, we can do that but I am afraid that we're not able to catch those
>> DCHECK
>> > > failures to validate the correct calling thread.
>> >
>> > I'm curious: why is it bad to depend on content::BrowserThread?
>>
>
>  Michael, One of Jian's next patches will move GCMDriver to components,
>> where
>> content::BrowserThread cannot be referenced.
>>
>
> Ok... but why isn't it allowed? I see there are components that use
> BrowserThread.
>
> https://codereview.chromium.org/292813007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Michael van Ouwerkerk, 2014-05-22 16:45:54]

On 2014/05/22 16:44:26, jianli wrote:
> Though we can still choose to have a component layer depend on content
> layer, it is not encouraged, esp when you want to support the component for
> iOS. Please see layered component design doc for more details.
> 
> Since gcm component does not need any content rendering support from
> content layer, it makes no sense to use content::BrowserThread since there
> is other simple way to use threads.
> 
> 
> On Thu, May 22, 2014 at 9:10 AM, <mailto:mvanouwerkerk@chromium.org> wrote:
> 
> > On 2014/05/22 16:04:42, fgorski wrote:
> >
> >> On 2014/05/22 10:17:50, Michael van Ouwerkerk wrote:
> >> > On 2014/05/22 00:11:28, jianli wrote:
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.org/292813007/diff/20001/
> > chrome/browser/services/gcm/gcm_driver_unittest.cc
> >
> >> > > File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):
> >> > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.org/292813007/diff/20001/
> > chrome/browser/services/gcm/gcm_driver_unittest.cc#newcode209
> >
> >> > > chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread
> >> > io_thread_;
> >> > > On 2014/05/21 23:43:29, Nicolas Zea wrote:
> >> > > > Could you just have one task runner that you use for everything? I
> >> don't
> >> > think
> >> > > > you need a real thread here
> >> > >
> >> > > Yes, we can do that but I am afraid that we're not able to catch those
> >> DCHECK
> >> > > failures to validate the correct calling thread.
> >> >
> >> > I'm curious: why is it bad to depend on content::BrowserThread?
> >>
> >
> >  Michael, One of Jian's next patches will move GCMDriver to components,
> >> where
> >> content::BrowserThread cannot be referenced.
> >>
> >
> > Ok... but why isn't it allowed? I see there are components that use
> > BrowserThread.
> >
> > https://codereview.chromium.org/292813007/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Cool, thanks to you both :-)

[Nicolas Zea, 2014-05-22 17:00:41]

lgtm

https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_driver_unittest.cc (right):

https://codereview.chromium.org/292813007/diff/20001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_driver_unittest.cc:209: base::Thread io_thread_;
On 2014/05/22 00:11:29, jianli wrote:
> On 2014/05/21 23:43:29, Nicolas Zea wrote:
> > Could you just have one task runner that you use for everything? I don't
think
> > you need a real thread here
> 
> Yes, we can do that but I am afraid that we're not able to catch those DCHECK
> failures to validate the correct calling thread.

I'm skeptical about the value of the DCHECK's, and generally in favor of just
keeping the test and code simple. That said, it's your call here.

[jianli, 2014-05-22 17:04:57]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:05:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/292813007/20001

[jianli, 2014-05-22 17:29:32]

The CQ bit was unchecked by jianli@chromium.org

[jianli, 2014-05-22 17:29:38]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:34:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/292813007/20001

[commit-bot: I haz the power, 2014-05-22 20:18:07]

Change committed as 272298