Clean up Automation and Chrome Frame IPC code.

Clean up Automation and Chrome Frame IPC code.-only use routed messages when needed-use routing IDs to avoid manually unpacking messages-remove data structures from IPC namespace (that should only be used for IPC code, and param traits)Note that I temporarily commented out part of a test in external_tab_test.cc because I couldn't figure out how to get the updated gmock macros to compile.

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

[jam, 2010-12-23 02:51:14]

hey guys, only one of you needs to review this, but it would probably be good if
everyone had a quick glance at it so that these patterns don't come up again :) 
Thanks!

[tommi (sloooow) - chröme, 2010-12-23 03:28:27]

excellent change!  From my perspective I see no issues besides getting the tests
fixed.  Stoyan - can we ask you to take a look at the gmock issues please?

[ananta, 2010-12-23 04:57:50]

Looks great overall. Thanks for doing this cleanup. 

One thing I noticed. The chrome_frame_automation_provider.cc/.h files were
deleted. The ChromeFrameAutomationProvider object basically validates automation
IPCs coming from chrome frame.i.e. only allows a subset of the automation IPC's
to come through. Should these files be deleted?

[jam, 2010-12-23 05:31:25]

On Wed, Dec 22, 2010 at 8:57 PM, <ananta@chromium.org> wrote:

> Looks great overall. Thanks for doing this cleanup.
>
> One thing I noticed. The chrome_frame_automation_provider.cc/.h files were
> deleted. The ChromeFrameAutomationProvider object basically validates
> automation
> IPCs coming from chrome frame.i.e. only allows a subset of the automation
> IPC's
> to come through. Should these files be deleted?


oops, I didn't realize that's why they were there, thanks.  Reverted that
part.

It does seems that the security angle here isn't that helpful, given that it
allows IPCs which can install extensions.


>
>
>
> http://codereview.chromium.org/5998006/
>

[stoyan, 2010-12-23 13:10:03]

The GMock problem is because DoAll() requires at least two actions and Return(0)
is not good for function returning void. You can remove DoAll and Return(0) and
it should compile fine. Yet the test will fail because we have to fire/call the
TabLoaded. 

I would suggest to keep ChromeProxyDelegate intact and to move the code of
ExternalTabProxy::OnMessageReceived to DispatchTabMessageToDelegate (in
cfproxy_support.cc) - I guess IPC_XXX macros can forward to arbitrary object
(not just 'this').

The idea is that ExternalTabProxy does not know about IPC messages:
1) easy testing
2) we can plug ChromeProxy that does not do IPC to chrome but uses browser.lib
in-proc.

[jam, 2010-12-23 17:29:44]

On Thu, Dec 23, 2010 at 5:10 AM, <stoyan@chromium.org> wrote:

> The GMock problem is because DoAll() requires at least two actions and
> Return(0)
> is not good for function returning void. You can remove DoAll and Return(0)
> and
> it should compile fine. Yet the test will fail because we have to fire/call
> the
> TabLoaded.
>
> I would suggest to keep ChromeProxyDelegate intact and to move the code of
> ExternalTabProxy::OnMessageReceived to DispatchTabMessageToDelegate (in
> cfproxy_support.cc) - I guess IPC_XXX macros can forward to arbitrary
> object
> (not just 'this').
>
> The idea is that ExternalTabProxy does not know about IPC messages:
> 1) easy testing
> 2) we can plug ChromeProxy that does not do IPC to chrome but uses
> browser.lib
> in-proc.


In general, we're moving away from the old way of having messages be
dispatched in one object, and that object calling others.  It has turned out
to be not maintainable as messages keep growing.  It leads to a lot of busy
work as each IPC leads to two or more functions (i.e. original dispatcher,
code that actually wants it, and maybe an interface in-between).

I feel there's not much to gain by having it in a separate function.
 Testing that the messages can be dispatched is not really that helpful,
again IMO it leads to busy work of writing tests that are really just
testing the IPC system, which already has its own tests.  As for in-proc, I
think once that's needed we can revisit.  For in-proc Chrome, we still use
IPCs, but we just send them across threads or to even the same thread
instead of function calls.  It makes the two modes a lot similar which is
nice for a variety of reasons.


>
>
>
>
> http://codereview.chromium.org/5998006/
>

[jam, 2010-12-23 18:17:07]

On Thu, Dec 23, 2010 at 9:29 AM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Thu, Dec 23, 2010 at 5:10 AM, <stoyan@chromium.org> wrote:
>
>> The GMock problem is because DoAll() requires at least two actions and
>> Return(0)
>> is not good for function returning void. You can remove DoAll and
>> Return(0) and
>> it should compile fine. Yet the test will fail because we have to
>> fire/call the
>> TabLoaded.
>>
>> I would suggest to keep ChromeProxyDelegate intact and to move the code of
>> ExternalTabProxy::OnMessageReceived to DispatchTabMessageToDelegate (in
>> cfproxy_support.cc) - I guess IPC_XXX macros can forward to arbitrary
>> object
>> (not just 'this').
>>
>> The idea is that ExternalTabProxy does not know about IPC messages:
>> 1) easy testing
>> 2) we can plug ChromeProxy that does not do IPC to chrome but uses
>> browser.lib
>> in-proc.
>
>
> In general, we're moving away from the old way of having messages be
> dispatched in one object, and that object calling others.  It has turned out
> to be not maintainable as messages keep growing.  It leads to a lot of busy
> work as each IPC leads to two or more functions (i.e. original dispatcher,
> code that actually wants it, and maybe an interface in-between).
>
> I feel there's not much to gain by having it in a separate function.
>  Testing that the messages can be dispatched is not really that helpful,
> again IMO it leads to busy work of writing tests that are really just
> testing the IPC system, which already has its own tests.  As for in-proc, I
> think once that's needed we can revisit.  For in-proc Chrome, we still use
> IPCs, but we just send them across threads or to even the same thread
> instead of function calls.  It makes the two modes a lot similar which is
> nice for a variety of reasons.
>

Just to clear up any confusion: above I was referring to the testability of
the dispatch function.  For the testability of the class, that can be done
by calling its OnMessageReceived with IPC messages that are created in a
test.  We use this pattern in a bunch of tests.

>
>
>>
>>
>>
>>
>> http://codereview.chromium.org/5998006/
>>
>
>

[stoyan, 2010-12-23 21:43:43]

On 2010/12/23 18:17:07, John Abd-El-Malek wrote:
> On Thu, Dec 23, 2010 at 9:29 AM, John Abd-El-Malek <mailto:jam@chromium.org>
wrote:
> Just to clear up any confusion: above I was referring to the testability of
> the dispatch function.  For the testability of the class, that can be done
> by calling its OnMessageReceived with IPC messages that are created in a
> test.  We use this pattern in a bunch of tests.

I still find calling method easier from testability point of view than to create
a message.
Anyway I'm not going to debate, go ahead and submit.