Restart of ceee_broker on crash.

Restart of ceee_broker on crash.
Code re-factoring and cleanup.

BUG=none
TEST=none


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

[siggi_google.com, 2010-11-30 13:46:25]

I'm pretty sure allocating a new binding for each new server is superfluous.
The RPC runtime's management of context handles is intended precisely for
this purpose, see
http://msdn.microsoft.com/en-us/library/aa373969(v=VS.85).aspx: "A context
handle becomes invalid if the server is rebooted, or otherwise loses its
connection to the client.".
Maybe what we need to do here is to simply pass the context handle to any
sensitive RPC, in which case it'll throw an exception in the case the server
has restarted.

On Tue, Nov 30, 2010 at 1:20 AM, <vitalybuka@chromium.org> wrote:

> Reviewers: mad1, Siggi,
>
> Description:
> Adde additional process specific RPC endpoint to avoid CEEE execute code in
> wring instance of Broker.
> Added ability to restart broker for some BrokerRpcClient custumers.
> Excracted method BrokerRpcClient::RunRpc with some shared RPC code.
>
> BUG=none
> TEST=none
>
>
> Please review this at http://codereview.chromium.org/5258006/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     ceee/ie/broker/broker_rpc_client.h
>  M     ceee/ie/broker/broker_rpc_client.cc
>  M     ceee/ie/broker/broker_rpc_lib.idl
>  M     ceee/ie/broker/broker_rpc_server.cc
>  M     ceee/ie/broker/broker_rpc_unittest.cc
>  M     ceee/ie/broker/broker_rpc_utils.h
>  M     ceee/ie/broker/broker_rpc_utils.cc
>  M     ceee/ie/common/metrics_util_unittest.cc
>  M     ceee/ie/plugin/bho/browser_helper_object.cc
>  M     ceee/ie/plugin/bho/browser_helper_object_unittest.cc
>  M     ceee/ie/plugin/bho/cookie_accountant.h
>  M     ceee/ie/plugin/bho/executor.cc
>  M     ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
>  M     ceee/ie/plugin/bho/webrequest_notifier.cc
>  M     ceee/ie/testing/mock_broker_and_friends.h
>
>
>

[Sigurður Ásgeirsson, 2010-11-30 13:47:48]

Very cool use of the tuple class. I'm going to wait with further review until we
can discuss the alternative of using context handles I suggested in another
email.

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:185: Params params) {
const Params& to reduce copying

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:209: return RunRpc(allow_restarts_,
&BrokerRpcClient_FireEvent,
nit: drop method name down to next line.

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:214: return RunRpc(allow_restarts_,
&BrokerRpcClient_SendUmaHistogramTimes,
ditto.

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.h (right):

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.h:26: // gone after successful connecting.
Client restarts server one time after
nit: indent continuation comment 4 spaces.

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_l...
File ceee/ie/broker/broker_rpc_lib.idl (right):

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_l...
ceee/ie/broker/broker_rpc_lib.idl:18: // Gets endpoint uniq to this instance of
server.
uniq->unique

[Vitaly Buka corp, 2010-11-30 17:13:40]

Actually context handles was initial idea. But I missed the point about
'handle becomes invalid if the server is rebooted'. Now I understand that
was bad choice. I'll fix.

2010/11/30 Sigurður Ásgeirsson <siggi@google.com>

> I'm pretty sure allocating a new binding for each new server is
> superfluous. The RPC runtime's management of context handles is intended
> precisely for this purpose, see
> http://msdn.microsoft.com/en-us/library/aa373969(v=VS.85).aspx: "A context
> handle becomes invalid if the server is rebooted, or otherwise loses its
> connection to the client.".
> Maybe what we need to do here is to simply pass the context handle to any
> sensitive RPC, in which case it'll throw an exception in the case the server
> has restarted.
>
> On Tue, Nov 30, 2010 at 1:20 AM, <vitalybuka@chromium.org> wrote:
>
>> Reviewers: mad1, Siggi,
>>
>> Description:
>> Adde additional process specific RPC endpoint to avoid CEEE execute code
>> in
>> wring instance of Broker.
>> Added ability to restart broker for some BrokerRpcClient custumers.
>> Excracted method BrokerRpcClient::RunRpc with some shared RPC code.
>>
>> BUG=none
>> TEST=none
>>
>>
>> Please review this at http://codereview.chromium.org/5258006/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>>
>> Affected files:
>>  M     ceee/ie/broker/broker_rpc_client.h
>>  M     ceee/ie/broker/broker_rpc_client.cc
>>  M     ceee/ie/broker/broker_rpc_lib.idl
>>  M     ceee/ie/broker/broker_rpc_server.cc
>>  M     ceee/ie/broker/broker_rpc_unittest.cc
>>  M     ceee/ie/broker/broker_rpc_utils.h
>>  M     ceee/ie/broker/broker_rpc_utils.cc
>>  M     ceee/ie/common/metrics_util_unittest.cc
>>  M     ceee/ie/plugin/bho/browser_helper_object.cc
>>  M     ceee/ie/plugin/bho/browser_helper_object_unittest.cc
>>  M     ceee/ie/plugin/bho/cookie_accountant.h
>>  M     ceee/ie/plugin/bho/executor.cc
>>  M     ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
>>  M     ceee/ie/plugin/bho/webrequest_notifier.cc
>>  M     ceee/ie/testing/mock_broker_and_friends.h
>>
>>
>>
>

[Vitaly Buka (NO REVIEWS), 2010-12-01 01:20:12]

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:185: Params params) {
I need something modifiable to change binding handle. See below. Anyway it is
small class.

On 2010/11/30 13:47:48, Ruðrugis wrote:
> const Params& to reduce copying

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:209: return RunRpc(allow_restarts_,
&BrokerRpcClient_FireEvent,
On 2010/11/30 13:47:48, Ruðrugis wrote:
> nit: drop method name down to next line.

Done.

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:214: return RunRpc(allow_restarts_,
&BrokerRpcClient_SendUmaHistogramTimes,
On 2010/11/30 13:47:48, Ruðrugis wrote:
> ditto.

Done.

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.h (right):

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.h:26: // gone after successful connecting.
Client restarts server one time after
On 2010/11/30 13:47:48, Ruðrugis wrote:
> nit: indent continuation comment 4 spaces.

Done.

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_l...
File ceee/ie/broker/broker_rpc_lib.idl (right):

http://codereview.chromium.org/5258006/diff/16001/ceee/ie/broker/broker_rpc_l...
ceee/ie/broker/broker_rpc_lib.idl:18: // Gets endpoint uniq to this instance of
server.
On 2010/11/30 13:47:48, Ruðrugis wrote:
> uniq->unique

Done.

[Sigurður Ásgeirsson, 2010-12-01 14:23:54]

looking pretty sweet!

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:26: HRESULT BindRpc(std::wstring endpoint,
RPC_BINDING_HANDLE* binding_handle) {
const & for endpoint.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:27: DCHECK(binding_handle);
we typically do a NULL check explicitly e.g. 
DCHECK(binding_handle != NULL)

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:160: Params params) {
I would still suggest a const & here. As is, you always copy, even for the
common case.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:174: params.a = binding_handle_;
this is hopefully the less common case, so here you'd be forgiven to create a
second tuple and copy the args.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.h (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.h:59: virtual HRESULT StartServer(IUnknown**
server);
add comment that this is a unittest seam.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:79: LOG(INFO) << "RPC server is started.
Endpoints: " << endpoint;
Endpoints->Endpoint

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:104: LOG_IF(WARNING, RPC_S_OK != status) <<
Nit: I've seen this warning logged when the broker is registered, maybe there
should be a guard against this?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_unittest.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:98: class BrokerRpcClientMock : public
BrokerRpcClient {
Classes that subclass and override implementation for testing are commonly named
TestingXXX, e.g. TestingBrokerRpcClient.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:104: HRESULT StartServer(IUnknown**
server) {
I would suggest mocking this method directly, and adding a  member or two on
this class named 

ExpectStartServer(...)

to set up the various expectations you want. Note you can use testing::Invoke to
invoke server_.Start() where appropriate.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:146: TEST_F(BrokerRpcTest, RestartServer)
{
nice set of tests.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:31: endpoint.resize(std::min<size_t>(52,
endpoint.size()));
back to 52U?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
File ceee/ie/plugin/bho/browser_helper_object.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
ceee/ie/plugin/bho/browser_helper_object.cc:960:
(*iter)->OnNavigateComplete(webbrowser, url_bstr);
nit: indent -1

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
ceee/ie/plugin/bho/browser_helper_object.cc:1383: return S_OK;
nit: indent +2

[MAD, 2010-12-01 16:35:23]

drive by review...

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:29: DCHECK(!protocol.empty());
Why do we need to put this in an std::wstring?
And since kRpcProtocol is an invariant, do we really need to test it every time?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:39: LOG(INFO) << "Connecting to RPC server.
Endpoint: " << endpoint;
We should start using VLOG(n)...

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:41: // Create binding string with given
protocol and end point.
Actually, protocol is not given it's hard coded...

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:50: "Failed to compose binding string.
RPC_STATUS=0x" << com::LogWe(status);
Could also use LogRpcException in an else clause below.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:53: // Create binding from just generated
binding string. Binding handle should
should used -> should be used

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:65: ::RpcBindingFree(binding_handle);
What if :RpcStringBindingCompose() failed? What are we freeing here?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:168: LogRpcException("RPC error in
FireEvent", RpcExceptionCode());
FireEvent -> Proper function name...

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:174: params.a = binding_handle_;
I wish there was a way to validate that params.a is ALWAYS a binding_handle_...
What if we add a new BrokerRpcClient_XXX function that doesn't use the handle as
the first param?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:175: return RunRpc(false, rpc_function,
params);
We should find a way to avoid infinite recursiveness here... What if it's our
call that kills the RPC server?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:28: DCHECK(!protocol.empty());
Again, I don't understand why we put the static const into a local var???

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:67: if (RPC_S_OK == status ||
RPC_S_DUPLICATE_ENDPOINT == status) {
Please add a comment stating why RPC_S_DUPLICATE_ENDPOINT is OK.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
File ceee/ie/plugin/bho/browser_helper_object.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
ceee/ie/plugin/bho/browser_helper_object.cc:960:
(*iter)->OnNavigateComplete(webbrowser, url_bstr);
On 2010/12/01 14:23:54, Ruðrugis wrote:
> nit: indent -1

Actually, -1 for "iter !=" and -2 for "(*iter)->"

[siggi_google.com, 2010-12-01 17:40:54]

Can I ask you to fix the CL description as well. The first sentence should
be a short to-the-point summary of the change, to make svn logs easy to read
later.
Something like:
"Ensure the broker doesn't get outdated event notifications after restart."?

On Tue, Nov 30, 2010 at 1:20 AM, <vitalybuka@chromium.org> wrote:

> Reviewers: mad1, Siggi,
>
> Description:
> Adde additional process specific RPC endpoint to avoid CEEE execute code in
> wring instance of Broker.
> Added ability to restart broker for some BrokerRpcClient custumers.
> Excracted method BrokerRpcClient::RunRpc with some shared RPC code.
>
> BUG=none
> TEST=none
>
>
> Please review this at http://codereview.chromium.org/5258006/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     ceee/ie/broker/broker_rpc_client.h
>  M     ceee/ie/broker/broker_rpc_client.cc
>  M     ceee/ie/broker/broker_rpc_lib.idl
>  M     ceee/ie/broker/broker_rpc_server.cc
>  M     ceee/ie/broker/broker_rpc_unittest.cc
>  M     ceee/ie/broker/broker_rpc_utils.h
>  M     ceee/ie/broker/broker_rpc_utils.cc
>  M     ceee/ie/common/metrics_util_unittest.cc
>  M     ceee/ie/plugin/bho/browser_helper_object.cc
>  M     ceee/ie/plugin/bho/browser_helper_object_unittest.cc
>  M     ceee/ie/plugin/bho/cookie_accountant.h
>  M     ceee/ie/plugin/bho/executor.cc
>  M     ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
>  M     ceee/ie/plugin/bho/webrequest_notifier.cc
>  M     ceee/ie/testing/mock_broker_and_friends.h
>
>
>

[Vitaly Buka corp, 2010-12-01 19:06:04]

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:26: HRESULT BindRpc(std::wstring endpoint,
RPC_BINDING_HANDLE* binding_handle) {
I need modifiable string to avoid const_cast
On 2010/12/01 14:23:54, Ruðrugis wrote:
> const & for endpoint.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:27: DCHECK(binding_handle);
On 2010/12/01 14:23:54, Ruðrugis wrote:
> we typically do a NULL check explicitly e.g. 
> DCHECK(binding_handle != NULL)

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:29: DCHECK(!protocol.empty());
RpcStringBindingCompose need non cost. So I can avoid wstring by using
const_cast

On 2010/12/01 16:35:23, MAD wrote:
> Why do we need to put this in an std::wstring?
> And since kRpcProtocol is an invariant, do we really need to test it every
time?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:39: LOG(INFO) << "Connecting to RPC server.
Endpoint: " << endpoint;
On 2010/12/01 16:35:23, MAD wrote:
> We should start using VLOG(n)...

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:41: // Create binding string with given
protocol and end point.
On 2010/12/01 16:35:23, MAD wrote:
> Actually, protocol is not given it's hard coded...

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:50: "Failed to compose binding string.
RPC_STATUS=0x" << com::LogWe(status);
On 2010/12/01 16:35:23, MAD wrote:
> Could also use LogRpcException in an else clause below.

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:53: // Create binding from just generated
binding string. Binding handle should
On 2010/12/01 16:35:23, MAD wrote:
> should used -> should be used

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:65: ::RpcBindingFree(binding_handle);
On 2010/12/01 16:35:23, MAD wrote:
> What if :RpcStringBindingCompose() failed? What are we freeing here?

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:160: Params params) {
On 2010/12/01 14:23:54, Ruðrugis wrote:
> I would still suggest a const & here. As is, you always copy, even for the
> common case.

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:168: LogRpcException("RPC error in
FireEvent", RpcExceptionCode());
On 2010/12/01 16:35:23, MAD wrote:
> FireEvent -> Proper function name...

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:174: params.a = binding_handle_;
On 2010/12/01 14:23:54, Ruðrugis wrote:
> this is hopefully the less common case, so here you'd be forgiven to create a
> second tuple and copy the args.

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:174: params.a = binding_handle_;
MakeRefTuple allows to remove this assignment.

I don't see much in this sense. 
On 2010/12/01 16:35:23, MAD wrote:
> I wish there was a way to validate that params.a is ALWAYS a
binding_handle_...
> What if we add a new BrokerRpcClient_XXX function that doesn't use the handle
as
> the first param?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:175: return RunRpc(false, rpc_function,
params);
It is impossible because first arg is 'false'.
On 2010/12/01 16:35:23, MAD wrote:
> We should find a way to avoid infinite recursiveness here... What if it's our
> call that kills the RPC server?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.h (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.h:59: virtual HRESULT StartServer(IUnknown**
server);
On 2010/12/01 14:23:54, Ruðrugis wrote:
> add comment that this is a unittest seam.

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:28: DCHECK(!protocol.empty());
To avoid const in ::RpcServerUseProtseqEp(
On 2010/12/01 16:35:23, MAD wrote:
> Again, I don't understand why we put the static const into a local var???

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:67: if (RPC_S_OK == status ||
RPC_S_DUPLICATE_ENDPOINT == status) {
On 2010/12/01 16:35:23, MAD wrote:
> Please add a comment stating why RPC_S_DUPLICATE_ENDPOINT is OK.

Done. See PrepareEndpoint()

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:79: LOG(INFO) << "RPC server is started.
Endpoints: " << endpoint;
On 2010/12/01 14:23:54, Ruðrugis wrote:
> Endpoints->Endpoint

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:104: LOG_IF(WARNING, RPC_S_OK != status) <<
On 2010/12/01 14:23:54, Ruðrugis wrote:
> Nit: I've seen this warning logged when the broker is registered, maybe there
> should be a guard against this?

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_unittest.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:98: class BrokerRpcClientMock : public
BrokerRpcClient {
On 2010/12/01 14:23:54, Ruðrugis wrote:
> Classes that subclass and override implementation for testing are commonly
named
> TestingXXX, e.g. TestingBrokerRpcClient.

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:104: HRESULT StartServer(IUnknown**
server) {
Done without ExpectStartServer. Putting EXPECT_CALL into method is really bad.
EXPECT_CALL provides location where test fails. If we hide it in function
location will be useless.

On 2010/12/01 14:23:54, Ruðrugis wrote:
> I would suggest mocking this method directly, and adding a  member or two on
> this class named 
> 
> ExpectStartServer(...)
> 
> to set up the various expectations you want. Note you can use testing::Invoke
to
> invoke server_.Start() where appropriate.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:146: TEST_F(BrokerRpcTest, RestartServer)
{
On 2010/12/01 14:23:54, Ruðrugis wrote:
> nice set of tests.

Done.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:31: endpoint.resize(std::min<size_t>(52,
endpoint.size()));
it does not matter with <size_t>
However if you like that one better :-).
On 2010/12/01 14:23:54, Ruðrugis wrote:
> back to 52U?

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
File ceee/ie/plugin/bho/browser_helper_object.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
ceee/ie/plugin/bho/browser_helper_object.cc:960:
(*iter)->OnNavigateComplete(webbrowser, url_bstr);
That is strange. I didn't touch this code.
I'll fix for consistency -2. I didn't do -1 because this file has 7 loops in
this way.
On 2010/12/01 16:35:23, MAD wrote:
> On 2010/12/01 14:23:54, Ruðrugis wrote:
> > nit: indent -1
> 
> Actually, -1 for "iter !=" and -2 for "(*iter)->"

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/plugin/bho/browser_...
ceee/ie/plugin/bho/browser_helper_object.cc:1383: return S_OK;
Same here.
On 2010/12/01 14:23:54, Ruðrugis wrote:
> nit: indent +2

[Sigurður Ásgeirsson, 2010-12-01 19:29:38]

lgtm with a last set of nits.

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_unittest.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:104: HRESULT StartServer(IUnknown**
server) {
On 2010/12/01 19:06:04, Vitaly Buka corp wrote:
> Done without ExpectStartServer. Putting EXPECT_CALL into method is really bad.
> EXPECT_CALL provides location where test fails. If we hide it in function
> location will be useless.
> 
It's a tradeoff between maintenance and usability, and as unittest failures
should be rare. It's not that bad if you need to hunt down the call stack to the
expectation that failed, and you can mitigate the problem by breaking up the
tests into multiple test cases if needed.
 We do this liberally in other tests.
> On 2010/12/01 14:23:54, Ruðrugis wrote:
> > I would suggest mocking this method directly, and adding a  member or two on
> > this class named 
> > 
> > ExpectStartServer(...)
> > 
> > to set up the various expectations you want. Note you can use
testing::Invoke
> to
> > invoke server_.Start() where appropriate.
>

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:32: VLOG(2) << "RPC server is starting.
Endpoint: " << endpoint;
nit: for the time being, please use VLOG(1), as Sawbuck can't set log levels in
excess of that. VLOG(1) is one more verbose than INFO, which is === VLOG(0).

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:46: //starting listening.
nit: need one space after //

[MAD, 2010-12-01 19:59:40]

LGTM2 with some nits too...

Thanks!

BYE
MAD

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:174: params.a = binding_handle_;
Ho, cool... Much better :-)

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:175: return RunRpc(false, rpc_function,
params);
On 2010/12/01 19:06:04, Vitaly Buka corp wrote:
> It is impossible because first arg is 'false'.
> On 2010/12/01 16:35:23, MAD wrote:
> > We should find a way to avoid infinite recursiveness here... What if it's
our
> > call that kills the RPC server?
> 
D'ho! I didn't read correctly... Sorry about that... :-)

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:67: if (RPC_S_OK == status ||
RPC_S_DUPLICATE_ENDPOINT == status) {
On 2010/12/01 19:06:04, Vitaly Buka corp wrote:
> On 2010/12/01 16:35:23, MAD wrote:
> > Please add a comment stating why RPC_S_DUPLICATE_ENDPOINT is OK.
> 
> Done. See PrepareEndpoint()

Thanks! :-)

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:30: if (protocol.empty() ||
endpoint.empty())
I think the test for protocol not being empty is not needed...

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:43: return status;
not error -> not an error
may start we start several -> may start several

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:44: }
this is error -> this is an error
instancess -> instances

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:45: 
endpoint.However -> endpoint. However
BrokerRpcServer with fail -> BrokerRpcServer will fail

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:46: }  // namespace
starting listening -> while starting to listen.

[Vitaly Buka corp, 2010-12-01 20:50:29]

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_unittest.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:104: HRESULT StartServer(IUnknown**
server) {
I know that this is tradeoff. But I prefer to introduce new local macro than
hiding code in function. This code is not so complex that we have to give up on
usability.
Do you insist on ExpectStartServer?
If you do I will change and not argue any more.

On 2010/12/01 19:29:38, Ruðrugis wrote:
> On 2010/12/01 19:06:04, Vitaly Buka corp wrote:
> > Done without ExpectStartServer. Putting EXPECT_CALL into method is really
bad.
> > EXPECT_CALL provides location where test fails. If we hide it in function
> > location will be useless.
> > 
> It's a tradeoff between maintenance and usability, and as unittest failures
> should be rare. It's not that bad if you need to hunt down the call stack to
the
> expectation that failed, and you can mitigate the problem by breaking up the
> tests into multiple test cases if needed.
>  We do this liberally in other tests.
> > On 2010/12/01 14:23:54, Ruðrugis wrote:
> > > I would suggest mocking this method directly, and adding a  member or two
on
> > > this class named 
> > > 
> > > ExpectStartServer(...)
> > > 
> > > to set up the various expectations you want. Note you can use
> testing::Invoke
> > to
> > > invoke server_.Start() where appropriate.
> > 
>

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:30: if (protocol.empty() ||
endpoint.empty())
But we will crash on protocol[0] if system out of memory.

On 2010/12/01 19:59:40, MAD wrote:
> I think the test for protocol not being empty is not needed...

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:32: VLOG(2) << "RPC server is starting.
Endpoint: " << endpoint;
On 2010/12/01 19:29:38, Ruðrugis wrote:
> nit: for the time being, please use VLOG(1), as Sawbuck can't set log levels
in
> excess of that. VLOG(1) is one more verbose than INFO, which is === VLOG(0).

Done.

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:43: // This is not error because unittest
may start we start several servers.
On 2010/12/01 19:59:40, MAD wrote:
> not error -> not an error
> may start we start several -> may start several

Done.

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:43: // This is not error because unittest
may start we start several servers.
On 2010/12/01 19:59:40, MAD wrote:
> not error -> not an error
> may start we start several -> may start several

Done.

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:44: // For ceee_broker this is error because
we should not have several instancess
On 2010/12/01 19:59:40, MAD wrote:
> this is error -> this is an error
> instancess -> instances

Done.

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:45: // of broker for the same
endpoint.However BrokerRpcServer with fail anyway
On 2010/12/01 19:59:40, MAD wrote:
> endpoint.However -> endpoint. However
> BrokerRpcServer with fail -> BrokerRpcServer will fail

Done.

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:46: //starting listening.
On 2010/12/01 19:29:38, Ruðrugis wrote:
> nit: need one space after //

Done.

[Sigurður Ásgeirsson, 2010-12-01 20:52:46]

lgtm

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_unittest.cc (right):

http://codereview.chromium.org/5258006/diff/36001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_unittest.cc:104: HRESULT StartServer(IUnknown**
server) {
On 2010/12/01 20:50:30, Vitaly Buka corp wrote:
> I know that this is tradeoff. But I prefer to introduce new local macro than
> hiding code in function. This code is not so complex that we have to give up
on
> usability.
> Do you insist on ExpectStartServer?
> If you do I will change and not argue any more.
> 

No, this is ok as-is.

> On 2010/12/01 19:29:38, Ruðrugis wrote:
> > On 2010/12/01 19:06:04, Vitaly Buka corp wrote:
> > > Done without ExpectStartServer. Putting EXPECT_CALL into method is really
> bad.
> > > EXPECT_CALL provides location where test fails. If we hide it in function
> > > location will be useless.
> > > 
> > It's a tradeoff between maintenance and usability, and as unittest failures
> > should be rare. It's not that bad if you need to hunt down the call stack to
> the
> > expectation that failed, and you can mitigate the problem by breaking up the
> > tests into multiple test cases if needed.
> >  We do this liberally in other tests.
> > > On 2010/12/01 14:23:54, Ruðrugis wrote:
> > > > I would suggest mocking this method directly, and adding a  member or
two
> on
> > > > this class named 
> > > > 
> > > > ExpectStartServer(...)
> > > > 
> > > > to set up the various expectations you want. Note you can use
> > testing::Invoke
> > > to
> > > > invoke server_.Start() where appropriate.
> > > 
> > 
>

[MAD, 2010-12-01 21:09:16]

LGTM again, with just one more comment...

Thanks again!

BYE
MAD

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/5258006/diff/70001/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:30: if (protocol.empty() ||
endpoint.empty())
On 2010/12/01 20:50:30, Vitaly Buka corp wrote:
> But we will crash on protocol[0] if system out of memory.
> 
> On 2010/12/01 19:59:40, MAD wrote:
> > I think the test for protocol not being empty is not needed...
> 

Thus, another reason not to copy this in a std::wstring... :-) I wonder if a
const_cast would be the lesser of the two evils... 

But fine, you can leave it like that, I don't really like it, but I guess it's
OK...