Allow Thread to initialize COM for Windows consumers who need it

Add methods to base::Thread to allow Windows consumers to ask for COM to be intialized on the Thread. 

This makes one functional change: when COM is initialized in STA mode, the Thread is forced to use a TYPE_UI message loop. Most of the modified consumers here didn't previously do that, but after discussion with cpu and siggi, it seems safest, even though it's more heavyweight.

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=163503

[Peter Kasting, 2012-10-03 23:05:07]

cpu: Review
brettw: base/threading/, content/browser/renderer_host/ OWNERS
ananta: chrome_frame/ OWNERS
fischman: media/audio/ OWNERS
jamiewalch: remoting/host/setup/ OWNERS
sky: ui/base/dialogs/ OWNERS

https://codereview.chromium.org/11048029/diff/1/chrome_frame/utils.h
File chrome_frame/utils.h (left):

https://codereview.chromium.org/11048029/diff/1/chrome_frame/utils.h#oldcode399
chrome_frame/utils.h:399: // Thread that enters STA and has a UI message loop.
This seems to be entirely unused.

[jam, 2012-10-03 23:11:35]

I'm not a fan of having non-windows platforms have to use empty but "windowsy"
classes like comthread for the same reason that I didn't like having non-mac
platforms use ScopedNSAutoreleasePool. This is confusing to read.

Can you give some background for the motivation of this change? What are the
advantages of doing this vs creating a ScopedCOMInitializer object in each place
that uses base::Thread now and needs it?

[jam, 2012-10-03 23:13:25]

On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
> I'm not a fan of having non-windows platforms have to use empty but "windowsy"
> classes like comthread for the same reason that I didn't like having non-mac
> platforms use ScopedNSAutoreleasePool. This is confusing to read.
> 
> Can you give some background for the motivation of this change? What are the
> advantages of doing this vs creating a ScopedCOMInitializer object in each
place
> that uses base::Thread now and needs it?

and to be clear, I'm not a fan of deriving from base::Thread to accomplish this.
I've come to regret too many cases where I used inheritance instead of
composition.

[Peter Kasting, 2012-10-03 23:13:53]

On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
> I'm not a fan of having non-windows platforms have to use empty but "windowsy"
> classes like comthread for the same reason that I didn't like having non-mac
> platforms use ScopedNSAutoreleasePool. This is confusing to read.
> 
> Can you give some background for the motivation of this change? What are the
> advantages of doing this vs creating a ScopedCOMInitializer object in each
place
> that uses base::Thread now and needs it?

This is not intended to sound snarky: did you read the rest of the change?  Take
a look at how this saves us 20-40 lines per file in ~a dozen files.  I'm not
sure what argument beyond that I can construct.

[Peter Kasting, 2012-10-03 23:17:52]

On 2012/10/03 23:13:53, Peter Kasting wrote:
> On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
> > I'm not a fan of having non-windows platforms have to use empty but
"windowsy"
> > classes like comthread for the same reason that I didn't like having non-mac
> > platforms use ScopedNSAutoreleasePool. This is confusing to read.
> > 
> > Can you give some background for the motivation of this change? What are the
> > advantages of doing this vs creating a ScopedCOMInitializer object in each
> place
> > that uses base::Thread now and needs it?
> 
> This is not intended to sound snarky: did you read the rest of the change? 
Take
> a look at how this saves us 20-40 lines per file in ~a dozen files.  I'm not
> sure what argument beyond that I can construct.

Also, I did consider whether there was any way to further insulate the callers
in question, say by making this header live in base/win/ and having people only
use it in "#if defined(OS_WIN)" code, but the cost of that is that all these
callers have to litter their headers and .cc files with such ifdefs, to
conditionally typedef their member variables and then conditionally instantiate
them.  This seemed much less readable and even further from the ideal of not
having cross-platform code think too much about a WIndows-specific concept like
COM.

[Peter Kasting, 2012-10-03 23:20:20]

On 2012/10/03 23:13:25, John Abd-El-Malek wrote:
> On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
> > I'm not a fan of having non-windows platforms have to use empty but
"windowsy"
> > classes like comthread for the same reason that I didn't like having non-mac
> > platforms use ScopedNSAutoreleasePool. This is confusing to read.
> > 
> > Can you give some background for the motivation of this change? What are the
> > advantages of doing this vs creating a ScopedCOMInitializer object in each
> place
> > that uses base::Thread now and needs it?
> 
> and to be clear, I'm not a fan of deriving from base::Thread to accomplish
this.
> I've come to regret too many cases where I used inheritance instead of
> composition.

I'm not sure how I'd accomplish this without some kind of inheritance.  We need
something to run during Init() and CleanUp().  Even if I were to use a separate
object to hold the ScopedCOMInitializer, I'd still have to subclass Thread to
override these methods to call into that object.  Unless I'm missing something.

[Jamie, 2012-10-03 23:46:04]

https://codereview.chromium.org/11048029/diff/8008/base/threading/com_thread_...
File base/threading/com_thread_win.cc (right):

https://codereview.chromium.org/11048029/diff/8008/base/threading/com_thread_...
base/threading/com_thread_win.cc:19: if (!use_mta_)
Is this test correct? It's doing the opposite to what the old remoting code did,
and seems to contradict your STA comment below.

[Peter Kasting, 2012-10-03 23:47:11]

https://codereview.chromium.org/11048029/diff/8008/base/threading/com_thread_...
File base/threading/com_thread_win.cc (right):

https://codereview.chromium.org/11048029/diff/8008/base/threading/com_thread_...
base/threading/com_thread_win.cc:19: if (!use_mta_)
On 2012/10/03 23:46:04, Jamie wrote:
> Is this test correct? It's doing the opposite to what the old remoting code
did,
> and seems to contradict your STA comment below.

No, it's wrong.  Sigh.  Thanks :(

[sky, 2012-10-04 00:11:48]

LGTM

[Ami GONE FROM CHROMIUM, 2012-10-04 00:18:11]

peanut-gallery: can you avoid jam@'s concerns by giving Thread() a 
void EnableCOMThingyIfOnWindows(bool use_mta);
method that users can call to effectively decorate Thread's Init/Cleanup
methods?

media/audio LGTM

[Peter Kasting, 2012-10-04 00:34:32]

On 2012/10/04 00:18:11, Ami Fischman wrote:
> peanut-gallery: can you avoid jam@'s concerns by giving Thread() a 
> void EnableCOMThingyIfOnWindows(bool use_mta);
> method that users can call to effectively decorate Thread's Init/Cleanup
> methods?

That was something John also suggested in direct email; among the reasons I'd
elected not to go that way were that it didn't feel right adding a couple data
members and a function to base::Thread to deal with this stuff, when not only
did most users no care about it but really it had nothing to do with Thread
itself; cleaner to isolate things in a helper class that could encapsulate all
this.  Also this would be more verbose on the caller side, and either require
#ifdefs everywhere or else still have the problem of defining Windows-specific
functions that were called by other platforms (which seems to be the core of
John's objection).

I can change the implementation, but personally I think the current route is
superior.

[Jamie, 2012-10-04 01:06:05]

remoting/ lgtm

[Peter Kasting, 2012-10-04 03:29:59]

Brett, when you review this, could you comment on whether you think this
mechanism or the "add hooks in thread.h to have Thread do this stuff itself"
mechanism would be superior (see Ami's comment and my reply above), or if
neither seems significantly better to you?

(Other opinionated parties welcome to reply as well; John is medium-strength in
favor of one method and I'm medium-strength in favor of the other so getting
more feedback would be useful.)

[jam, 2012-10-04 05:49:37]

On Wed, Oct 3, 2012 at 4:17 PM, <pkasting@chromium.org> wrote:

> On 2012/10/03 23:13:53, Peter Kasting wrote:
>
>> On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
>> > I'm not a fan of having non-windows platforms have to use empty but
>>
> "windowsy"
>
>> > classes like comthread for the same reason that I didn't like having
>> non-mac
>> > platforms use ScopedNSAutoreleasePool. This is confusing to read.
>> >
>> > Can you give some background for the motivation of this change? What
>> are the
>> > advantages of doing this vs creating a ScopedCOMInitializer object in
>> each
>> place
>> > that uses base::Thread now and needs it?
>>
>
>  This is not intended to sound snarky: did you read the rest of the change?
>>
> Take
>
>> a look at how this saves us 20-40 lines per file in ~a dozen files.  I'm
>> not
>> sure what argument beyond that I can construct.
>>
>
yep I saw that; I guess I didn't think that outweighted having ComThread
being used by other platforms (the autorelease stuff is still in my memory).


> Also, I did consider whether there was any way to further insulate the
> callers
> in question, say by making this header live in base/win/ and having people
> only
> use it in "#if defined(OS_WIN)" code, but the cost of that is that all
> these
> callers have to litter their headers and .cc files with such ifdefs, to
> conditionally typedef their member variables and then conditionally
> instantiate
> them.  This seemed much less readable and even further from the ideal of
> not
> having cross-platform code think too much about a WIndows-specific concept
> like
> COM.
>

how about a method (behind ifdef) on Thread that tells it to initialize
COM? that way other platforms still use Thread.


>
https://codereview.chromium.**org/11048029/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2012-10-04 06:09:41]

On Wed, Oct 3, 2012 at 4:30 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> On Wed, Oct 3, 2012 at 4:17 PM, <pkasting@chromium.org> wrote:
>
>> On 2012/10/03 23:13:53, Peter Kasting wrote:
>>
>>> On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
>>> > I'm not a fan of having non-windows platforms have to use empty but
>>>
>> "windowsy"
>>
>>> > classes like comthread for the same reason that I didn't like having
>>> non-mac
>>> > platforms use ScopedNSAutoreleasePool. This is confusing to read.
>>> >
>>> > Can you give some background for the motivation of this change? What
>>> are the
>>> > advantages of doing this vs creating a ScopedCOMInitializer object in
>>> each
>>> place
>>> > that uses base::Thread now and needs it?
>>>
>>
>>  This is not intended to sound snarky: did you read the rest of the
>>> change?
>>>
>> Take
>>
>>> a look at how this saves us 20-40 lines per file in ~a dozen files.  I'm
>>> not
>>> sure what argument beyond that I can construct.
>>>
>>
> yep I saw that; I guess I didn't think that outweighted having ComThread
> being used by other platforms (the autorelease stuff is still in my memory).
>

I guess I just don't think this adds a lot of confusion to other platforms.
 The header is very short and the comment in it pretty clear.

Also, before I wrote this change, I consulted with Brett on what design
made the most sense, since I didn't want to go to the effort of doing
something and then get it rejected.  He said this change seemed OK to him.

Also, I did consider whether there was any way to further insulate the
>> callers
>> in question, say by making this header live in base/win/ and having
>> people only
>> use it in "#if defined(OS_WIN)" code, but the cost of that is that all
>> these
>> callers have to litter their headers and .cc files with such ifdefs, to
>> conditionally typedef their member variables and then conditionally
>> instantiate
>> them.  This seemed much less readable and even further from the ideal of
>> not
>> having cross-platform code think too much about a WIndows-specific
>> concept like
>> COM.
>>
>
> how about a method (behind ifdef) on Thread that tells it to initialize
> COM? that way other platforms still use Thread.
>

I considered that, but there's a couple of downsides.  The biggest one, to
me, is that, semantically, this isn't really something Thread itself cares
about.  It feels much more wrong to me to expose platform-specific methods
on Thread to hang random functionality off of, and to add member variables
most consumers don't actually need, than it does to isolate that knowledge
in a wrapper class like ComThread that the relatively fewer consumers who
want this can use.  Another is that this is more verbose, requiring callers
to add #ifdefs.  If we add 4 LOC back to all the callers, we just lost 20%
of the win from doing this.

Incidentally, I originally tried to make ComThread actually be a direct
typedef to base::Thread on non-Windows platforms.  That ended up suffering
from the verbosity problem: callers who wanted MTA then still had to put
platform-specific ifdefs next to their uses of this type, and since the
whole purpose of the type was to eliminate those, it seemed like the wrong
route.

PK

[jam, 2012-10-04 06:10:14]

On Wed, Oct 3, 2012 at 4:50 PM, Peter Kasting <pkasting@chromium.org> wrote:

> On Wed, Oct 3, 2012 at 4:30 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> On Wed, Oct 3, 2012 at 4:17 PM, <pkasting@chromium.org> wrote:
>>
>>> On 2012/10/03 23:13:53, Peter Kasting wrote:
>>>
>>>> On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
>>>> > I'm not a fan of having non-windows platforms have to use empty but
>>>>
>>> "windowsy"
>>>
>>>> > classes like comthread for the same reason that I didn't like having
>>>> non-mac
>>>> > platforms use ScopedNSAutoreleasePool. This is confusing to read.
>>>> >
>>>> > Can you give some background for the motivation of this change? What
>>>> are the
>>>> > advantages of doing this vs creating a ScopedCOMInitializer object in
>>>> each
>>>> place
>>>> > that uses base::Thread now and needs it?
>>>>
>>>
>>>  This is not intended to sound snarky: did you read the rest of the
>>>> change?
>>>>
>>> Take
>>>
>>>> a look at how this saves us 20-40 lines per file in ~a dozen files.
>>>>  I'm not
>>>> sure what argument beyond that I can construct.
>>>>
>>>
>> yep I saw that; I guess I didn't think that outweighted having ComThread
>> being used by other platforms (the autorelease stuff is still in my memory).
>>
>
> I guess I just don't think this adds a lot of confusion to other
> platforms.  The header is very short and the comment in it pretty clear.
>
> I'm not saying that others working on a different platforms wouldn't
understand what's going on. It's just that from my experience with
ScopedNSAutoreleasePool, it doesn't help readability on other platforms
when they're instantiating objects that they don't need.


> Also, before I wrote this change, I consulted with Brett on what design
> made the most sense, since I didn't want to go to the effort of doing
> something and then get it rejected.  He said this change seemed OK to him.
>
> Also, I did consider whether there was any way to further insulate the
>>> callers
>>> in question, say by making this header live in base/win/ and having
>>> people only
>>> use it in "#if defined(OS_WIN)" code, but the cost of that is that all
>>> these
>>> callers have to litter their headers and .cc files with such ifdefs, to
>>> conditionally typedef their member variables and then conditionally
>>> instantiate
>>> them.  This seemed much less readable and even further from the ideal of
>>> not
>>> having cross-platform code think too much about a WIndows-specific
>>> concept like
>>> COM.
>>>
>>
>> how about a method (behind ifdef) on Thread that tells it to initialize
>> COM? that way other platforms still use Thread.
>>
>
> I considered that, but there's a couple of downsides.  The biggest one, to
> me, is that, semantically, this isn't really something Thread itself cares
> about.
>

Sure, the flipside is that ComThread isn't something that other platforms
care about :)


> It feels much more wrong to me to expose platform-specific methods on
> Thread to hang random functionality off of, and to add member variables
> most consumers don't actually need, than it does to isolate that knowledge
> in a wrapper class like ComThread that the relatively fewer consumers who
> want this can use.
>

One of the issues I have with this is that it's not scalable. i.e. if
another platform need something else to be called on their platform what
would they do? Have LinuxFooThread which derives from ComThread?

 Another is that this is more verbose, requiring callers to add #ifdefs.
>  If we add 4 LOC back to all the callers, we just lost 20% of the win from
> doing this.
>

I think measuring LOC may not be the best measure of readability :) But if
you really want to, it looks like it would be less lines overall to add the
4 LOC to each callsite because you could get rid of com_thread* and move
the ~10 lines in there to thread.cc


> Incidentally, I originally tried to make ComThread actually be a direct
> typedef to base::Thread on non-Windows platforms.  That ended up suffering
> from the verbosity problem: callers who wanted MTA then still had to put
> platform-specific ifdefs next to their uses of this type, and since the
> whole purpose of the type was to eliminate those, it seemed like the wrong
> route.
>
> PK
>

[jam, 2012-10-04 06:15:09]

On Wed, Oct 3, 2012 at 6:02 PM, Peter Kasting <pkasting@chromium.org> wrote:

> On Wed, Oct 3, 2012 at 5:53 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> On Wed, Oct 3, 2012 at 4:50 PM, Peter Kasting <pkasting@chromium.org>wrote:
>>
>>> On Wed, Oct 3, 2012 at 4:30 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>>>
>>>> On Wed, Oct 3, 2012 at 4:17 PM, <pkasting@chromium.org> wrote:
>>>>
>>>>> On 2012/10/03 23:13:53, Peter Kasting wrote:
>>>>>
>>>>>> On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
>>>>>> > I'm not a fan of having non-windows platforms have to use empty but
>>>>>>
>>>>> "windowsy"
>>>>>
>>>>>> > classes like comthread for the same reason that I didn't like
>>>>>> having non-mac
>>>>>> > platforms use ScopedNSAutoreleasePool. This is confusing to read.
>>>>>> >
>>>>>> > Can you give some background for the motivation of this change?
>>>>>> What are the
>>>>>> > advantages of doing this vs creating a ScopedCOMInitializer object
>>>>>> in each
>>>>>> place
>>>>>> > that uses base::Thread now and needs it?
>>>>>>
>>>>>
>>>>>  This is not intended to sound snarky: did you read the rest of the
>>>>>> change?
>>>>>>
>>>>> Take
>>>>>
>>>>>> a look at how this saves us 20-40 lines per file in ~a dozen files.
>>>>>>  I'm not
>>>>>> sure what argument beyond that I can construct.
>>>>>>
>>>>>
>>>> yep I saw that; I guess I didn't think that outweighted having
>>>> ComThread being used by other platforms (the autorelease stuff is still in
>>>> my memory).
>>>>
>>>
>>> I guess I just don't think this adds a lot of confusion to other
>>> platforms.  The header is very short and the comment in it pretty clear.
>>>
>>> I'm not saying that others working on a different platforms wouldn't
>> understand what's going on. It's just that from my experience with
>> ScopedNSAutoreleasePool, it doesn't help readability on other platforms
>> when they're instantiating objects that they don't need.
>>
>
> But the point here is that all platforms need to instantiate an object.  I
> totally agree that instantiating something pointlessly on other platforms
> would be dumb, but we're not doing that.  There is no extra instantiation
> on non-Win that doesn't need to be there.
>
> Also, before I wrote this change, I consulted with Brett on what design
>>> made the most sense, since I didn't want to go to the effort of doing
>>> something and then get it rejected.  He said this change seemed OK to him.
>>>
>>> Also, I did consider whether there was any way to further insulate the
>>>>> callers
>>>>> in question, say by making this header live in base/win/ and having
>>>>> people only
>>>>> use it in "#if defined(OS_WIN)" code, but the cost of that is that all
>>>>> these
>>>>> callers have to litter their headers and .cc files with such ifdefs, to
>>>>> conditionally typedef their member variables and then conditionally
>>>>> instantiate
>>>>> them.  This seemed much less readable and even further from the ideal
>>>>> of not
>>>>> having cross-platform code think too much about a WIndows-specific
>>>>> concept like
>>>>> COM.
>>>>>
>>>>
>>>> how about a method (behind ifdef) on Thread that tells it to initialize
>>>> COM? that way other platforms still use Thread.
>>>>
>>>
>>> I considered that, but there's a couple of downsides.  The biggest one,
>>> to me, is that, semantically, this isn't really something Thread itself
>>> cares about.
>>>
>>
>> Sure, the flipside is that ComThread isn't something that other platforms
>> care about :)
>>
>
> All the callsites here have to care either way.  But Thread is widely used
> and adding stuff to it seems far more polluting.
>
> It feels much more wrong to me to expose platform-specific methods on
>>> Thread to hang random functionality off of, and to add member variables
>>> most consumers don't actually need, than it does to isolate that knowledge
>>> in a wrapper class like ComThread that the relatively fewer consumers who
>>> want this can use.
>>>
>>
>> One of the issues I have with this is that it's not scalable. i.e. if
>> another platform need something else to be called on their platform what
>> would they do? Have LinuxFooThread which derives from ComThread?
>>
>
> There are cases in this change where people need to do stuff beyond what
> ComThread provides.  In those cases we extend as necessary.  I don't see
> how this change decreases the scalability here at all.  Any cases which
> would need to subclass in order to accomplish something are going to need
> to subclass using either what I'm doing or what you're proposing, unless
> I'm misunderstanding.
>
>  Another is that this is more verbose, requiring callers to add #ifdefs.
>>>  If we add 4 LOC back to all the callers, we just lost 20% of the win from
>>> doing this.
>>>
>>
>> I think measuring LOC may not be the best measure of readability :) But
>> if you really want to, it looks like it would be less lines overall to add
>> the 4 LOC to each callsite because you could get rid of com_thread* and
>> move the ~10 lines in there to thread.cc
>>
>
> The point is not overall lines, it's lines interspersed in the middle of
> other things decreasing readability at n sites.
>
> This isn't a life or death thing.  But I really truly don't find any of
> the alternative proposals to be as clean, readable, or semantically nice.
>  If you're staunchly against the way I've implemented things here even
> after this email, I'd like to get a third-party opinion from Brett or Will
> or someone.
>

sg, I'll wait for others as well. I'm not very strongly against this, just
medium...

>
> Which, speaking of... I'd have liked to have this discussion on the public
> CL where other people can chime in in the first place.
>
argh, looks like rietveld's reply by email isn't working again. i'll debug
that. in the meantime, as a hack I've removed the ellipse characters from
the subject which should make this get through.

>
> PK
>

[Peter Kasting, 2012-10-04 06:18:23]

On Wed, Oct 3, 2012 at 5:53 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> On Wed, Oct 3, 2012 at 4:50 PM, Peter Kasting <pkasting@chromium.org>wrote:
>
>> On Wed, Oct 3, 2012 at 4:30 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>>
>>> On Wed, Oct 3, 2012 at 4:17 PM, <pkasting@chromium.org> wrote:
>>>
>>>> On 2012/10/03 23:13:53, Peter Kasting wrote:
>>>>
>>>>> On 2012/10/03 23:11:35, John Abd-El-Malek wrote:
>>>>> > I'm not a fan of having non-windows platforms have to use empty but
>>>>>
>>>> "windowsy"
>>>>
>>>>> > classes like comthread for the same reason that I didn't like having
>>>>> non-mac
>>>>> > platforms use ScopedNSAutoreleasePool. This is confusing to read.
>>>>> >
>>>>> > Can you give some background for the motivation of this change? What
>>>>> are the
>>>>> > advantages of doing this vs creating a ScopedCOMInitializer object
>>>>> in each
>>>>> place
>>>>> > that uses base::Thread now and needs it?
>>>>>
>>>>
>>>>  This is not intended to sound snarky: did you read the rest of the
>>>>> change?
>>>>>
>>>> Take
>>>>
>>>>> a look at how this saves us 20-40 lines per file in ~a dozen files.
>>>>>  I'm not
>>>>> sure what argument beyond that I can construct.
>>>>>
>>>>
>>> yep I saw that; I guess I didn't think that outweighted having ComThread
>>> being used by other platforms (the autorelease stuff is still in my
memory).
>>>
>>
>> I guess I just don't think this adds a lot of confusion to other
>> platforms.  The header is very short and the comment in it pretty clear.
>>
>> I'm not saying that others working on a different platforms wouldn't
> understand what's going on. It's just that from my experience with
> ScopedNSAutoreleasePool, it doesn't help readability on other platforms
> when they're instantiating objects that they don't need.
>

But the point here is that all platforms need to instantiate an object.  I
totally agree that instantiating something pointlessly on other platforms
would be dumb, but we're not doing that.  There is no extra instantiation
on non-Win that doesn't need to be there.

Also, before I wrote this change, I consulted with Brett on what design
>> made the most sense, since I didn't want to go to the effort of doing
>> something and then get it rejected.  He said this change seemed OK to him.
>>
>> Also, I did consider whether there was any way to further insulate the
>>>> callers
>>>> in question, say by making this header live in base/win/ and having
>>>> people only
>>>> use it in "#if defined(OS_WIN)" code, but the cost of that is that all
>>>> these
>>>> callers have to litter their headers and .cc files with such ifdefs, to
>>>> conditionally typedef their member variables and then conditionally
>>>> instantiate
>>>> them.  This seemed much less readable and even further from the ideal
>>>> of not
>>>> having cross-platform code think too much about a WIndows-specific
>>>> concept like
>>>> COM.
>>>>
>>>
>>> how about a method (behind ifdef) on Thread that tells it to initialize
>>> COM? that way other platforms still use Thread.
>>>
>>
>> I considered that, but there's a couple of downsides.  The biggest one,
>> to me, is that, semantically, this isn't really something Thread itself
>> cares about.
>>
>
> Sure, the flipside is that ComThread isn't something that other platforms
> care about :)
>

All the callsites here have to care either way.  But Thread is widely used
and adding stuff to it seems far more polluting.

It feels much more wrong to me to expose platform-specific methods on
>> Thread to hang random functionality off of, and to add member variables
>> most consumers don't actually need, than it does to isolate that knowledge
>> in a wrapper class like ComThread that the relatively fewer consumers who
>> want this can use.
>>
>
> One of the issues I have with this is that it's not scalable. i.e. if
> another platform need something else to be called on their platform what
> would they do? Have LinuxFooThread which derives from ComThread?
>

There are cases in this change where people need to do stuff beyond what
ComThread provides.  In those cases we extend as necessary.  I don't see
how this change decreases the scalability here at all.  Any cases which
would need to subclass in order to accomplish something are going to need
to subclass using either what I'm doing or what you're proposing, unless
I'm misunderstanding.

 Another is that this is more verbose, requiring callers to add #ifdefs.
>>  If we add 4 LOC back to all the callers, we just lost 20% of the win from
>> doing this.
>>
>
> I think measuring LOC may not be the best measure of readability :) But if
> you really want to, it looks like it would be less lines overall to add the
> 4 LOC to each callsite because you could get rid of com_thread* and move
> the ~10 lines in there to thread.cc
>

The point is not overall lines, it's lines interspersed in the middle of
other things decreasing readability at n sites.

This isn't a life or death thing.  But I really truly don't find any of the
alternative proposals to be as clean, readable, or semantically nice.  If
you're staunchly against the way I've implemented things here even after
this email, I'd like to get a third-party opinion from Brett or Will or
someone.

Which, speaking of... I'd have liked to have this discussion on the public
CL where other people can chime in in the first place.

PK

[alexeypa (please no reviews), 2012-10-04 15:23:49]

On 2012/10/04 03:29:59, Peter Kasting wrote:
> Brett, when you review this, could you comment on whether you think this
> mechanism or the "add hooks in thread.h to have Thread do this stuff itself"
> mechanism would be superior (see Ami's comment and my reply above), or if
> neither seems significantly better to you?
> 
> (Other opinionated parties welcome to reply as well; John is medium-strength
in
> favor of one method and I'm medium-strength in favor of the other so getting
> more feedback would be useful.)

My 2c: it feels like the problem could be solved it base::Thread provided a way
to set and keep alive until thread is about to exit an object (or a collection
of objects). Much like TLS  does but with RAII semantic. Then we could use a
generic base::Thread and assign Windows-specific scoped COM initializer to it.
It will take a few lines though: #ifdef, posting a task to create the object on
the thread and so on.

Now this does not solve the message loop type issue. The cross platform code
creating the thread will need to know that it should have a UI loop only because
of Windows. But maybe it is a not a big deal.

[Wez, 2012-10-04 17:34:59]

On 2012/10/04 15:23:49, alexeypa wrote:
> On 2012/10/04 03:29:59, Peter Kasting wrote:
> > Brett, when you review this, could you comment on whether you think this
> > mechanism or the "add hooks in thread.h to have Thread do this stuff itself"
> > mechanism would be superior (see Ami's comment and my reply above), or if
> > neither seems significantly better to you?
> > 
> > (Other opinionated parties welcome to reply as well; John is medium-strength
> in
> > favor of one method and I'm medium-strength in favor of the other so getting
> > more feedback would be useful.)
> 
> My 2c: it feels like the problem could be solved it base::Thread provided a
way
> to set and keep alive until thread is about to exit an object (or a collection
> of objects). Much like TLS  does but with RAII semantic. Then we could use a
> generic base::Thread and assign Windows-specific scoped COM initializer to it.
> It will take a few lines though: #ifdef, posting a task to create the object
on
> the thread and so on.
> 
> Now this does not solve the message loop type issue. The cross platform code
> creating the thread will need to know that it should have a UI loop only
because
> of Windows. But maybe it is a not a big deal.

How about adding MessageLoop::TYPE_COM, which would be a UI message loop w/
initialization on Windows, and a default message loop on other platforms?

[Peter Kasting, 2012-10-04 18:08:31]

On 2012/10/04 15:23:49, alexeypa wrote:
> My 2c: it feels like the problem could be solved it base::Thread provided a
way
> to set and keep alive until thread is about to exit an object (or a collection
> of objects). Much like TLS  does but with RAII semantic. Then we could use a
> generic base::Thread and assign Windows-specific scoped COM initializer to it.
> It will take a few lines though: #ifdef, posting a task to create the object
on
> the thread and so on.
> 
> Now this does not solve the message loop type issue. The cross platform code
> creating the thread will need to know that it should have a UI loop only
because
> of Windows. But maybe it is a not a big deal.

Just making the lifetimes work correctly would be tricky -- how do you create
your objects at the right time to begin with?  (In this case, the initializer
needs to be created during Init(), not when the Thread object is created, so
it's not easy for callers to just pass in an object to stuff onto the thread.)

That aside, this feels overengineered to me.  A generic object store that the
Thread auto-manages the lifetime of is a complex system for the simple problem
we need solved here.  The fact that despite its complexity it still doesn't
solve the message loop issue also seems like a minus.

[cpu_(ooo_6.6-7.5), 2012-10-04 18:09:02]

http://codereview.chromium.org/11048029/diff/7015/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/11048029/diff/7015/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:140: }
I rather not derive from base::Thread, I rather have

#if defined(OS_WIN)
  base::Thread("Chrome_InProcRendererThread", COM_STA),
#else
  base::Thread("Chrome_InProcRendererThread"),
#endif

or some such, for the people that need STA or MTA, if you can get by not
initializing COM in this thread then then the ctor with 1 argument works and you
don't need ifdefs or ComThreads in cross platform code.

[Peter Kasting, 2012-10-04 18:13:11]

On 2012/10/04 17:34:59, Wez wrote:
> How about adding MessageLoop::TYPE_COM, which would be a UI message loop w/
> initialization on Windows, and a default message loop on other platforms?

This has the precise problems John has articulated with my existing solution: it
invents a new cross-platform concept which is really only meaningful on one
platform.

It's also somewhat less nice to use because the behavior has to be specified at
the point where you Start() the Thread in question, as opposed to where you
create the Thread object.  That may be more difficult for some callers and seems
somewhat more error-prone.

[jam, 2012-10-04 18:16:52]

On 2012/10/04 18:13:11, Peter Kasting wrote:
> On 2012/10/04 17:34:59, Wez wrote:
> > How about adding MessageLoop::TYPE_COM, which would be a UI message loop w/
> > initialization on Windows, and a default message loop on other platforms?
> 
> This has the precise problems John has articulated with my existing solution:
it
> invents a new cross-platform concept which is really only meaningful on one
> platform.
> 
> It's also somewhat less nice to use because the behavior has to be specified
at
> the point where you Start() the Thread in question, as opposed to where you
> create the Thread object.  That may be more difficult for some callers and
seems
> somewhat more error-prone.

one other issue with TYPE_COM would be that places that check that the current
message loop type is TYPE_UI would need to know about this COM thread type as
well

[Peter Kasting, 2012-10-04 18:18:09]

On 2012/10/04 18:09:02, cpu wrote:
>
http://codereview.chromium.org/11048029/diff/7015/content/browser/renderer_ho...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
http://codereview.chromium.org/11048029/diff/7015/content/browser/renderer_ho...
> content/browser/renderer_host/render_process_host_impl.cc:140: }
> I rather not derive from base::Thread, I rather have
> 
> #if defined(OS_WIN)
>   base::Thread("Chrome_InProcRendererThread", COM_STA),
> #else
>   base::Thread("Chrome_InProcRendererThread"),
> #endif
> 
> or some such, for the people that need STA or MTA, if you can get by not
> initializing COM in this thread then then the ctor with 1 argument works and
you
> don't need ifdefs or ComThreads in cross platform code.

I didn't understand that last clause.  It's already (vacuously) true that if you
don't need COM, you don't need any ifdefs or ComThreads or the like.

This proposal seems substantially the same as the idea of adding a "add com
support" function on Thread, with the trivial difference that it does away with
the need for an extra call but replaces it with the need for an extra #else
clause at construction.

[cpu_(ooo_6.6-7.5), 2012-10-05 01:12:26]

maybe trivial but the point is that the COM-ness is (or should be) a one shot
deal, upon construction of the thread ye shall decide if you want COM and if so
what apartment model it should be.

[brettw, 2012-10-05 04:18:19]

I don't actually have a strong opinion but I can note that we have been moving
toward having less code that compiles everywhere and does nothing on other
platforms. Maybe this is just because John is doing the most refactoring lately
and doesn't like this pattern. But for consistency, it seems like it would be
better to prefer another pattern if all else is equal.

[Peter Kasting, 2012-10-16 01:18:09]

Rewrote to add an OS_WIN method on Thread for specifying that COM should be
initialized.  Reviewers, please look again.

[ananta, 2012-10-16 01:26:43]

LGTM with one nit.

http://codereview.chromium.org/11048029/diff/25001/chrome_frame/urlmon_url_re...
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/11048029/diff/25001/chrome_frame/urlmon_url_re...
chrome_frame/urlmon_url_request.cc:1406: #if defined(OS_WIN)
We don't need the OS_WIN check here? as CF is windows specific

[Peter Kasting, 2012-10-16 01:42:09]

http://codereview.chromium.org/11048029/diff/25001/chrome_frame/urlmon_url_re...
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/11048029/diff/25001/chrome_frame/urlmon_url_re...
chrome_frame/urlmon_url_request.cc:1406: #if defined(OS_WIN)
On 2012/10/16 01:26:43, ananta wrote:
> We don't need the OS_WIN check here? as CF is windows specific

Good point.

[grt (UTC plus 2), 2012-10-16 01:58:12]

http://codereview.chromium.org/11048029/diff/26014/remoting/host/setup/daemon...
File remoting/host/setup/daemon_controller_win.cc (right):

http://codereview.chromium.org/11048029/diff/26014/remoting/host/setup/daemon...
remoting/host/setup/daemon_controller_win.cc:161: #if defined(OS_WIN)
this is a _win.cc file, so the #if isn't needed, no?

http://codereview.chromium.org/11048029/diff/26014/ui/base/dialogs/base_shell...
File ui/base/dialogs/base_shell_dialog_win.cc (right):

http://codereview.chromium.org/11048029/diff/26014/ui/base/dialogs/base_shell...
ui/base/dialogs/base_shell_dialog_win.cc:67: #if defined(OS_WIN)
_win.cc here, too

[Ami GONE FROM CHROMIUM, 2012-10-16 02:52:03]

still LGTM for media/

http://codereview.chromium.org/11048029/diff/26014/base/threading/thread.cc
File base/threading/thread.cc (right):

http://codereview.chromium.org/11048029/diff/26014/base/threading/thread.cc#n...
base/threading/thread.cc:197: com_initializer_.reset();
observation: this variable is only used in this function, so it could be a local
instead of class-member (and stack-allocated instead of heap, if it's not big).

This would let you drop the fwd-decl in the .h, too.

[alexeypa (please no reviews), 2012-10-16 15:26:56]

remoting/* - lgtm

[sky, 2012-10-16 16:46:58]

SLGTM

[cpu_(ooo_6.6-7.5), 2012-10-16 17:32:45]

lgtm

[Peter Kasting, 2012-10-17 17:35:50]

brettw: ping

[brettw, 2012-10-18 19:43:11]

base lgtm