Make automation proxy objects to ref_counted. That allows to process async no...

Make automation proxy objects to ref_counted. That allows to process async notifications directly in channel background thread. Add support for listener-less ChannelProxy.
BUG=none
TEST=none


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

[amit, 2009-05-21 21:54:55]

[+cc jam - please take a look at changes to chrome/common/ipc_channel_proxy.cc
and inside chrome/test/automation/ ]

Just gone through my first pass at it. Overall looks good with a few comments
below. Will continue looking at it further.

http://codereview.chromium.org/113722/diff/1090/1091
File chrome/test/automation/automation_handle_tracker.cc (right):

http://codereview.chromium.org/113722/diff/1090/1091#newcode51
Line 51: AutoLock lock(map_lock_);
If this is the only reason for the lock we should post a task to tnvalidate the
handle.

http://codereview.chromium.org/113722/diff/1090/1097
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/113722/diff/1090/1097#newcode92
Line 92: bool tab_message = IsTabNotifyMessage(message);
I am wondering if it makes sense to read a first int anyway and if it is
corresponds to a tabproxy give it a chance to process it?

http://codereview.chromium.org/113722/diff/1090/1097#newcode113
Line 113: IPC_BEGIN_MESSAGE_MAP(TabProxyNotificationMessageFilter, message)
we can simply write switch(message.type()) {
 case AutomationMsg_NavigationStateChanged::ID:
 case AutomationMsg_UpdateTargetUrl::ID:
...
}

http://codereview.chromium.org/113722/diff/1090/1097#newcode566
Line 566: T* p = static_cast<T*>(tracker_->GetResource(handle));
just doing:
scoped_refptr<T> p = tracker_->GetResource(handle);
does not work, eh?

http://codereview.chromium.org/113722/diff/1090/1096
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/113722/diff/1090/1096#newcode125
Line 125: TabProxy* tab =
static_cast<TabProxy*>(tracker_->GetResource(tab_handle));
How about Tracker::GetOrCreateResource(...). The downside of that is that
Tracker will end up creating proxies, ugg. But maybe there's a way to factor out
this repeatative code.

http://codereview.chromium.org/113722/diff/1090/1092
File chrome/test/automation/tab_proxy.cc (right):

http://codereview.chromium.org/113722/diff/1090/1092#newcode654
Line 654: AutoLock lock(list_lock_);
I don't like this lock at all, can we post add/remove operations to the
background thread?

[stoyan, 2009-05-21 22:18:41]

http://codereview.chromium.org/113722/diff/1090/1091
File chrome/test/automation/automation_handle_tracker.cc (right):

http://codereview.chromium.org/113722/diff/1090/1091#newcode51
Line 51: AutoLock lock(map_lock_);
PostTask to whom? The thread that created AutomationProxy may have not message
loop.
We are going to replace obtaining this lock with obtaining another lock (taken
when posting task or whatever interthread communication is done).

http://codereview.chromium.org/113722/diff/1090/1097
File chrome/test/automation/automation_proxy.cc (right):

http://codereview.chromium.org/113722/diff/1090/1097#newcode92
Line 92: bool tab_message = IsTabNotifyMessage(message);
Could be any type of message with an arbitrary first int. I prefer to keep as it
is.

http://codereview.chromium.org/113722/diff/1090/1097#newcode113
Line 113: IPC_BEGIN_MESSAGE_MAP(TabProxyNotificationMessageFilter, message)
I will keep it as it is for now. :) May change it later in other CL

http://codereview.chromium.org/113722/diff/1090/1097#newcode566
Line 566: T* p = static_cast<T*>(tracker_->GetResource(handle));
No. The scoped_refptr will AddRef() already AddRef-ed pointer. We do not have
scoped_refptr<T>::attach()

And it is important to do T->AddRef() inside tracker's lock so we are sure the
object is alive.

http://codereview.chromium.org/113722/diff/1090/1096
File chrome/test/automation/browser_proxy.cc (right):

http://codereview.chromium.org/113722/diff/1090/1096#newcode125
Line 125: TabProxy* tab =
static_cast<TabProxy*>(tracker_->GetResource(tab_handle));
Right. I really wanted to avoid the tracker's knowledge of proxies. And I have
to pass the sender_ as well.

http://codereview.chromium.org/113722/diff/1090/1092
File chrome/test/automation/tab_proxy.cc (right):

http://codereview.chromium.org/113722/diff/1090/1092#newcode654
Line 654: AutoLock lock(list_lock_);
I don't see the reason why. It's like replacing this lock with messageloop task
queue lock. 
Additionally we have to do TabProxy->AddRef() here and TabProxy->Release() in
background thread after we finish the task in order to ensure TabProxy stays
alive.

We expect not much calls to AddObserver, right? Usually just after creation of
the tab.

[stoyan, 2009-05-26 21:12:38]

ping

[amit, 2009-05-26 21:27:04]

Sorry about delay, I was waiting to see if you got to a resolution about the
SyncChannel issue that jam raised.

I share your views on scoped_refptr ugliness and wish there was a better way to
do things (without fighting the battle with google3).

I think the code looks good with a nit mentioned below.

http://codereview.chromium.org/113722/diff/1090/1092
File chrome/test/automation/tab_proxy.cc (right):

http://codereview.chromium.org/113722/diff/1090/1092#newcode654
Line 654: AutoLock lock(list_lock_);
On 2009/05/21 22:18:41, stoyan wrote:
> I don't see the reason why. It's like replacing this lock with messageloop
task
> queue lock. 
> Additionally we have to do TabProxy->AddRef() here and TabProxy->Release() in
> background thread after we finish the task in order to ensure TabProxy stays
> alive.
> 
> We expect not much calls to AddObserver, right? Usually just after creation of
> the tab.
Well, it's not the AddObserver call I am worried about, it's calling the
'OnMessageReveived' on each observer inside a lock that bothers me.
> 
>

[stoyan, 2009-05-26 21:40:08]

It is expected that observer "knows" that the call is in background channel
thread, so it should proceed quickly.
Holding the lock while calling OnMessageReceived prevents adding/removing
observers from the main thread for the period of the callback.