Const-ify RefCountedThreadSafe::AddRef and Release.

Const-ify RefCountedThreadSafe::AddRef and Release.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63457

[willchan no longer on Chromium, 2010-10-20 18:12:48]

lgtm

[Matt Perry, 2010-10-20 18:53:09]

It failed to compile. I had to add a const_cast to the pointer since the
Destruct traits is not constified.

Alternatively I can constify Destruct as well, but I'm not sure if it's worth
it. Most users simply do a delete, but I end up having to cast away const in
MessageLoopProxyImpl::OnDestruct.

[willchan no longer on Chromium, 2010-10-20 20:50:12]

On Wed, Oct 20, 2010 at 11:53 AM, <mpcomplete@chromium.org> wrote:

> It failed to compile. I had to add a const_cast to the pointer since the
> Destruct traits is not constified.
>
> Alternatively I can constify Destruct as well, but I'm not sure if it's
> worth
> it. Most users simply do a delete, but I end up having to cast away const
> in
> MessageLoopProxyImpl::OnDestruct.


I believe that Destruct should be const.  Looks like the
MessageLoopProxyImpl::message_loop_lock_ member variable should be mutable.
 Why do we even post its deletion to the thread?  Oh, it's because it uses
the DestructionObserver.  That's silly.  base::Thread::ThreadMain should
just tell the MessageLoopProxy that it's shutting down so it can NULL out
target_message_loop_.  DestructionObserver is often a bad idea.


>
>
> http://codereview.chromium.org/3869003/show
>

[Matt Perry, 2010-10-20 21:51:09]

On 2010/10/20 20:50:12, willchan wrote:
> On Wed, Oct 20, 2010 at 11:53 AM, <mailto:mpcomplete@chromium.org> wrote:
> 
> > It failed to compile. I had to add a const_cast to the pointer since the
> > Destruct traits is not constified.
> >
> > Alternatively I can constify Destruct as well, but I'm not sure if it's
> > worth
> > it. Most users simply do a delete, but I end up having to cast away const
> > in
> > MessageLoopProxyImpl::OnDestruct.
> 
> 
> I believe that Destruct should be const.

OK.

>  Looks like the
> MessageLoopProxyImpl::message_loop_lock_ member variable should be mutable.
>  Why do we even post its deletion to the thread?  Oh, it's because it uses
> the DestructionObserver.  That's silly.  base::Thread::ThreadMain should
> just tell the MessageLoopProxy that it's shutting down so it can NULL out
> target_message_loop_.  DestructionObserver is often a bad idea.

Hmm.. maybe. I'm unfamiliar with this code, so I took the easy out and just
const-casted for the MessageLoopProxyImpl case. (message_loop_lock_ and
target_message_loop_ both need to be mutable otherwise).

[willchan no longer on Chromium, 2010-10-20 22:01:34]

On Wed, Oct 20, 2010 at 2:51 PM, <mpcomplete@chromium.org> wrote:

> On 2010/10/20 20:50:12, willchan wrote:
>
>  On Wed, Oct 20, 2010 at 11:53 AM, <mailto:mpcomplete@chromium.org> wrote:
>>
>
>  > It failed to compile. I had to add a const_cast to the pointer since the
>> > Destruct traits is not constified.
>> >
>> > Alternatively I can constify Destruct as well, but I'm not sure if it's
>> > worth
>> > it. Most users simply do a delete, but I end up having to cast away
>> const
>> > in
>> > MessageLoopProxyImpl::OnDestruct.
>>
>
>
>  I believe that Destruct should be const.
>>
>
> OK.
>
>
>   Looks like the
>> MessageLoopProxyImpl::message_loop_lock_ member variable should be
>> mutable.
>>  Why do we even post its deletion to the thread?  Oh, it's because it uses
>> the DestructionObserver.  That's silly.  base::Thread::ThreadMain should
>> just tell the MessageLoopProxy that it's shutting down so it can NULL out
>> target_message_loop_.  DestructionObserver is often a bad idea.
>>
>
> Hmm.. maybe. I'm unfamiliar with this code, so I took the easy out and just
> const-casted for the MessageLoopProxyImpl case. (message_loop_lock_ and
> target_message_loop_ both need to be mutable otherwise).


I don't see why target_message_loop_ needs to be mutable.  Where is it being
modified in OnDestruct()?  I'd prefer avoiding the const_cast if possible.


>
>
> http://codereview.chromium.org/3869003/show
>

[Matt Perry, 2010-10-20 22:23:31]

On 2010/10/20 22:01:34, willchan wrote:
> > Hmm.. maybe. I'm unfamiliar with this code, so I took the easy out and just
> > const-casted for the MessageLoopProxyImpl case. (message_loop_lock_ and
> > target_message_loop_ both need to be mutable otherwise).
> 
> 
> I don't see why target_message_loop_ needs to be mutable.  Where is it being
> modified in OnDestruct()?  I'd prefer avoiding the const_cast if possible.

You're right. I should have tested that before replying. I assumed the call to
DeleteSoon() would fail since DeleteSoon is not marked const. But I guess it's
the target_message_loop_ pointer value that is const here, not the pointed-to
object.

Anyway, fixed.

[Matt Perry, 2010-10-20 22:41:47]

I've updated the CL again to fix some compile issues. const is spreading like
the plague. It's worse than the GPL.

[willchan no longer on Chromium, 2010-10-21 20:37:39]

LGTM

http://codereview.chromium.org/3869003/diff/23001/24010
File chrome/browser/browser_thread_unittest.cc (right):

http://codereview.chromium.org/3869003/diff/23001/24010#newcode80
chrome/browser/browser_thread_unittest.cc:80: mutable MessageLoop loop_;
Ew, this is broken.  But I don't know that I have the heart to ask you to fix
it, especially since it's so benign.  I think it's fine, but you should document
in a comment that this should be fixed.

http://codereview.chromium.org/3869003/diff/23001/24015
File chrome/common/net/url_request_context_getter.cc (right):

http://codereview.chromium.org/3869003/diff/23001/24015#newcode19
chrome/common/net/url_request_context_getter.cc:19:
const_cast<URLRequestContextGetter*>(this)->GetIOMessageLoopProxy();
I think that GetIOMessageLoopProxy() can be a const member function on
URLRequestContextGetter.  I don't conceptually view
URLRequestContextGetter/URLRequestContext as "owning" the
MessageLoop/MessageLoopProxy.  So this const_cast<> shouldn't be necessary.

[Matt Perry, 2010-10-21 21:31:21]

http://codereview.chromium.org/3869003/diff/23001/24010
File chrome/browser/browser_thread_unittest.cc (right):

http://codereview.chromium.org/3869003/diff/23001/24010#newcode80
chrome/browser/browser_thread_unittest.cc:80: mutable MessageLoop loop_;
On 2010/10/21 20:37:39, willchan wrote:
> Ew, this is broken.  But I don't know that I have the heart to ask you to fix
> it, especially since it's so benign.  I think it's fine, but you should
document
> in a comment that this should be fixed.

Is it? MessageLoop::PostTask is non-const, but it actually does modify its
members (incoming_queue_). Are you proposing that those members should be
mutable?

http://codereview.chromium.org/3869003/diff/23001/24015
File chrome/common/net/url_request_context_getter.cc (right):

http://codereview.chromium.org/3869003/diff/23001/24015#newcode19
chrome/common/net/url_request_context_getter.cc:19:
const_cast<URLRequestContextGetter*>(this)->GetIOMessageLoopProxy();
On 2010/10/21 20:37:39, willchan wrote:
> I think that GetIOMessageLoopProxy() can be a const member function on
> URLRequestContextGetter.  I don't conceptually view
> URLRequestContextGetter/URLRequestContext as "owning" the
> MessageLoop/MessageLoopProxy.  So this const_cast<> shouldn't be necessary.

Done.

[willchan no longer on Chromium, 2010-10-21 21:45:25]

http://codereview.chromium.org/3869003/diff/23001/24010
File chrome/browser/browser_thread_unittest.cc (right):

http://codereview.chromium.org/3869003/diff/23001/24010#newcode80
chrome/browser/browser_thread_unittest.cc:80: mutable MessageLoop loop_;
On 2010/10/21 21:31:21, Matt Perry wrote:
> On 2010/10/21 20:37:39, willchan wrote:
> > Ew, this is broken.  But I don't know that I have the heart to ask you to
fix
> > it, especially since it's so benign.  I think it's fine, but you should
> document
> > in a comment that this should be fixed.
> 
> Is it? MessageLoop::PostTask is non-const, but it actually does modify its
> members (incoming_queue_). Are you proposing that those members should be
> mutable?

I'm not suggesting the brokenness is in MessageLoop::PostTask().  I'm saying
it's here, in BrowserThreadTest.  Release() shouldn't be tried to call a
non-const function on a member variable.

Actually, looking at this now, shouldn't we be able to just call
MessageLoop::current()->PostTask() instead of loop_.PostTask()?  Then loop_
doesn't need to be mutable.

[Matt Perry, 2010-10-21 21:55:58]

On 2010/10/21 21:45:25, willchan wrote:
> http://codereview.chromium.org/3869003/diff/23001/24010
> File chrome/browser/browser_thread_unittest.cc (right):
> 
> http://codereview.chromium.org/3869003/diff/23001/24010#newcode80
> chrome/browser/browser_thread_unittest.cc:80: mutable MessageLoop loop_;
> On 2010/10/21 21:31:21, Matt Perry wrote:
> > On 2010/10/21 20:37:39, willchan wrote:
> > > Ew, this is broken.  But I don't know that I have the heart to ask you to
> fix
> > > it, especially since it's so benign.  I think it's fine, but you should
> > document
> > > in a comment that this should be fixed.
> > 
> > Is it? MessageLoop::PostTask is non-const, but it actually does modify its
> > members (incoming_queue_). Are you proposing that those members should be
> > mutable?
> 
> I'm not suggesting the brokenness is in MessageLoop::PostTask().  I'm saying
> it's here, in BrowserThreadTest.  Release() shouldn't be tried to call a
> non-const function on a member variable.
> 
> Actually, looking at this now, shouldn't we be able to just call
> MessageLoop::current()->PostTask() instead of loop_.PostTask()?  Then loop_
> doesn't need to be mutable.

I see. We might be able to use current(), but that just seems like cheating to
me. It's accessing the same variable, just in a roundabout way. I don't really
like any of the options though... they all seem broken in some way. 'mutable'
seems to at least be the clearest about what's going on.

[willchan no longer on Chromium, 2010-10-21 22:02:46]

On 2010/10/21 21:55:58, Matt Perry wrote:
> On 2010/10/21 21:45:25, willchan wrote:
> > http://codereview.chromium.org/3869003/diff/23001/24010
> > File chrome/browser/browser_thread_unittest.cc (right):
> > 
> > http://codereview.chromium.org/3869003/diff/23001/24010#newcode80
> > chrome/browser/browser_thread_unittest.cc:80: mutable MessageLoop loop_;
> > On 2010/10/21 21:31:21, Matt Perry wrote:
> > > On 2010/10/21 20:37:39, willchan wrote:
> > > > Ew, this is broken.  But I don't know that I have the heart to ask you
to
> > fix
> > > > it, especially since it's so benign.  I think it's fine, but you should
> > > document
> > > > in a comment that this should be fixed.
> > > 
> > > Is it? MessageLoop::PostTask is non-const, but it actually does modify its
> > > members (incoming_queue_). Are you proposing that those members should be
> > > mutable?
> > 
> > I'm not suggesting the brokenness is in MessageLoop::PostTask().  I'm saying
> > it's here, in BrowserThreadTest.  Release() shouldn't be tried to call a
> > non-const function on a member variable.
> > 
> > Actually, looking at this now, shouldn't we be able to just call
> > MessageLoop::current()->PostTask() instead of loop_.PostTask()?  Then loop_
> > doesn't need to be mutable.
> 
> I see. We might be able to use current(), but that just seems like cheating to
> me. It's accessing the same variable, just in a roundabout way. I don't really
> like any of the options though... they all seem broken in some way. 'mutable'
> seems to at least be the clearest about what's going on.

Well, the other way to fix it is to simply post the QuitTask the test body
before calling Run().  Since it'll run after the Release() function is invoked,
it should Quit() appropriately.

This is a nit, so don't feel like you have to hold yourself up on my comments
here, but I think mutable is suboptimal in this case.