Firing event to broker without worker thread.

Firing event to broker without worker thread.

BUG=none
TEST=none

[Sigurður Ásgeirsson, 2010-11-16 18:23:04]

Is it a significant change to keep an RpcClient per EventFunnel instance?

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/bho/events_f...
File ceee/ie/plugin/bho/events_funnel.cc (right):

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/bho/events_f...
ceee/ie/plugin/bho/events_funnel.cc:16: EventsFunnel::EventsFunnel(bool
keep_broker_alive)
IMHO it would it simplify everything to keep an RpcClient per EventsFunnel. We'd
maintain one Rpc connection per tab, that way, which is slightly higher cost,
but we then also have an Rpc server context per tab.
The worker thread was originally single-instanced due to the cost, but that
argument no longer applies, IMHO.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
File ceee/ie/plugin/toolband/toolband_module.cc (right):

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:193: ::CoInitializeEx(0,
COINIT_MULTITHREADED);
I think we can skip the Coinit calls. We're running in a thread that's
guaranteed to be initialized.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:199: broker_rpc_client_.Connect();
test return value and LOG(ERROR) here?

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:201: ::CoUninitialize();
Ooof.
If the ::Coinitialize call fails (e.g. due to the apartment being of the wrong
type), you may or will uninitialize COM for this thread.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:208: CComBSTR
name(event_name.c_str());
This looks like needless conversion and allocation to me. Can we change the RPC
client's interface to receive a const wchar_t*?

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:210: LOG_IF(ERROR,
broker_rpc_client_.FireEvent(name, args)) <<
Fire event returns true on error?
I don't like hiding real functionality inside logging statements like this.
Please do the event firing in a separate statement and store the return value in
a local variable.

[Vitaly Buka corp, 2010-11-18 23:14:21]

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/bho/events_f...
File ceee/ie/plugin/bho/events_funnel.cc (right):

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/bho/events_f...
ceee/ie/plugin/bho/events_funnel.cc:16: EventsFunnel::EventsFunnel(bool
keep_broker_alive)
On 2010/11/16 18:23:04, Ruðrugis wrote:
> IMHO it would it simplify everything to keep an RpcClient per EventsFunnel.
We'd
> maintain one Rpc connection per tab, that way, which is slightly higher cost,
> but we then also have an Rpc server context per tab.
> The worker thread was originally single-instanced due to the cost, but that
> argument no longer applies, IMHO. 

Done.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
File ceee/ie/plugin/toolband/toolband_module.cc (right):

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:193: ::CoInitializeEx(0,
COINIT_MULTITHREADED);
On 2010/11/16 18:23:04, Ruðrugis wrote:
> I think we can skip the Coinit calls. We're running in a thread that's
> guaranteed to be initialized.

Done.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:199: broker_rpc_client_.Connect();
On 2010/11/16 18:23:04, Ruðrugis wrote:
> test return value and LOG(ERROR) here?

Done.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:201: ::CoUninitialize();
On 2010/11/16 18:23:04, Ruðrugis wrote:
> Ooof.
> If the ::Coinitialize call fails (e.g. due to the apartment being of the wrong
> type), you may or will uninitialize COM for this thread.

Done.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:208: CComBSTR
name(event_name.c_str());
On 2010/11/16 18:23:04, Ruðrugis wrote:
> This looks like needless conversion and allocation to me. Can we change the
RPC
> client's interface to receive a const wchar_t*?

Done.

http://codereview.chromium.org/4989002/diff/23001/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:210: LOG_IF(ERROR,
broker_rpc_client_.FireEvent(name, args)) <<
On 2010/11/16 18:23:04, Ruðrugis wrote:
> Fire event returns true on error?
> I don't like hiding real functionality inside logging statements like this.
> Please do the event firing in a separate statement and store the return value
in
> a local variable.

Done.

[Sigurður Ásgeirsson, 2010-11-19 18:29:30]

Mondo cool. I have but a couple of nits. 
Feel free to address the Task subclass removal with a TODO if you're fed up with
this change.

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.cc
File ceee/ie/broker/broker.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.cc#ne...
ceee/ie/broker/broker.cc:31:
ChromePostman::GetInstance()->FireEvent(CW2A(event_name), CW2A(event_args));
A NULL BSTR is synonymous with the empty string, so we have to pass through
com::ToString.
Also please use the chrome base string conversions, probably WideToUTF8 here.

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.gyp
File ceee/ie/broker/broker.gyp (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.gyp#n...
ceee/ie/broker/broker.gyp:23: 'DefaultCharType': '0',
I think this is worth a short comment? Why is this needed?

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/chrome_postm...
File ceee/ie/broker/chrome_postman.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/chrome_postm...
ceee/ie/broker/chrome_postman.cc:125: message_loop->PostTask(FROM_HERE,
while you're in here, can I ask you to remove the task subclass and change this
to use NewRunnableMethod? 
Also fine to add a TODO and address in a separate CL if you're getting fed up
with this one :)

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/events_f...
File ceee/ie/plugin/bho/events_funnel.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/events_f...
ceee/ie/plugin/bho/events_funnel.cc:47: return hr;
Do you want to do broker_rpc_client_.reset() on failure cases here? Maybe keep a
local scoped_ptr and hand over ownership to the member ptr only on success?

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/events_f...
File ceee/ie/plugin/bho/events_funnel_unittest.cc (left):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/events_f...
ceee/ie/plugin/bho/events_funnel_unittest.cc:21:
MOCK_STATIC_INIT2(ceee_module_util::FireEventToBroker,
Excellent! :D

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/web_prog...
File ceee/ie/plugin/bho/web_progress_notifier.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/web_prog...
ceee/ie/plugin/bho/web_progress_notifier.cc:126: if
(webnavigation_events_funnel()) {
retrieve pointer to a local and DCHECK? If this is expected to be able to fail
at runtime, then no DCHECK but do an error log?

[Vitaly Buka corp, 2010-11-19 20:30:38]

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.cc
File ceee/ie/broker/broker.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.cc#ne...
ceee/ie/broker/broker.cc:31:
ChromePostman::GetInstance()->FireEvent(CW2A(event_name), CW2A(event_args));
On 2010/11/19 18:29:30, Ruðrugis wrote:
> A NULL BSTR is synonymous with the empty string, so we have to pass through
> com::ToString.
> Also please use the chrome base string conversions, probably WideToUTF8 here. 


Done.

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.gyp
File ceee/ie/broker/broker.gyp (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/broker.gyp#n...
ceee/ie/broker/broker.gyp:23: 'DefaultCharType': '0',
On 2010/11/19 18:29:30, Ruðrugis wrote:
> I think this is worth a short comment? Why is this needed?

Done.

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/chrome_postm...
File ceee/ie/broker/chrome_postman.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/broker/chrome_postm...
ceee/ie/broker/chrome_postman.cc:125: message_loop->PostTask(FROM_HERE,
On 2010/11/19 18:29:30, Ruðrugis wrote:
> while you're in here, can I ask you to remove the task subclass and change
this
> to use NewRunnableMethod? 
> Also fine to add a TODO and address in a separate CL if you're getting fed up
> with this one :)

Done.

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/events_f...
File ceee/ie/plugin/bho/events_funnel.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/events_f...
ceee/ie/plugin/bho/events_funnel.cc:47: return hr;
No. It is by design. It prevents from new attempts to init broker. It will kill
IE if we are broken
On 2010/11/19 18:29:30, Ruðrugis wrote:
> Do you want to do broker_rpc_client_.reset() on failure cases here? Maybe keep
a
> local scoped_ptr and hand over ownership to the member ptr only on success?

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/web_prog...
File ceee/ie/plugin/bho/web_progress_notifier.cc (right):

http://codereview.chromium.org/4989002/diff/34002/ceee/ie/plugin/bho/web_prog...
ceee/ie/plugin/bho/web_progress_notifier.cc:126: if
(webnavigation_events_funnel()) {
On 2010/11/19 18:29:30, Ruðrugis wrote:
> retrieve pointer to a local and DCHECK? If this is expected to be able to fail
> at runtime, then no DCHECK but do an error log?

Done.

[Sigurður Ásgeirsson, 2010-11-19 20:53:20]

lgtm with one last nit.

http://codereview.chromium.org/4989002/diff/77002/ceee/ie/plugin/bho/web_prog...
File ceee/ie/plugin/bho/web_progress_notifier.cc (right):

http://codereview.chromium.org/4989002/diff/77002/ceee/ie/plugin/bho/web_prog...
ceee/ie/plugin/bho/web_progress_notifier.cc:126: WebNavigationEventsFunnel*
funel = webnavigation_events_funnel();
nit: funel -> funnel

http://codereview.chromium.org/4989002/diff/77002/ceee/ie/plugin/bho/web_prog...
ceee/ie/plugin/bho/web_progress_notifier.cc:200: WebNavigationEventsFunnel*
funel = webnavigation_events_funnel();
ditto.

http://codereview.chromium.org/4989002/diff/77002/ceee/ie/plugin/bho/web_prog...
ceee/ie/plugin/bho/web_progress_notifier.cc:274: WebNavigationEventsFunnel*
funel = webnavigation_events_funnel();
et al.

[Vitaly Buka corp, 2010-11-19 21:21:27]

http://codereview.chromium.org/4989002/diff/77002/ceee/ie/plugin/bho/web_prog...
File ceee/ie/plugin/bho/web_progress_notifier.cc (right):

http://codereview.chromium.org/4989002/diff/77002/ceee/ie/plugin/bho/web_prog...
ceee/ie/plugin/bho/web_progress_notifier.cc:200: WebNavigationEventsFunnel*
funel = webnavigation_events_funnel();
On 2010/11/19 20:53:20, Ruðrugis wrote:
> ditto.

Done.

[Sigurður Ásgeirsson, 2010-11-20 15:57:21]

I patched this to submit it, but I'm consistently getting a unittest failure in
debug and release:

[----------] 4 tests from WindowEventsFunnelTest
[ RUN      ] WindowEventsFunnelTest.SendEvent
[1120/105051:ERROR:.\chrome_postman.cc(132)] Trying to post a message before the
 API worker thread iscompletely initialized and ready.
.\broker\window_events_funnel_unittest.cc(75): error: Actual function call count
 doesn't match EXPECT_CALL(postman, FireEvent( StrEq(CComBSTR(kEventName).m_str)
, StrEq(CComBSTR(event_args_str.c_str()).m_str)))...
         Expected: to be called once
           Actual: never called - unsatisfied and active

[Vitaly Buka corp, 2010-11-22 06:29:26]

Sorry, I forgot to upload ceee/ie/broker/window_events_funnel_unittest.cc
On 2010/11/20 15:57:21, Ruðrugis wrote:
> I patched this to submit it, but I'm consistently getting a unittest failure
in
> debug and release:
> 
> [----------] 4 tests from WindowEventsFunnelTest
> [ RUN      ] WindowEventsFunnelTest.SendEvent
> [1120/105051:ERROR:.\chrome_postman.cc(132)] Trying to post a message before
the
>  API worker thread iscompletely initialized and ready.
> .\broker\window_events_funnel_unittest.cc(75): error: Actual function call
count
>  doesn't match EXPECT_CALL(postman, FireEvent(
StrEq(CComBSTR(kEventName).m_str)
> , StrEq(CComBSTR(event_args_str.c_str()).m_str)))...
>          Expected: to be called once
>            Actual: never called - unsatisfied and active

[Sigurður Ásgeirsson, 2010-11-22 13:53:09]

lgtm, will attempt to commit

[Sigurður Ásgeirsson, 2010-11-22 15:36:08]

Submitted as http://src.chromium.org/viewvc/chrome?view=rev&revision=66946