Avoid double-setting of the tool band ID in the CEEE executors manager....

Avoid double-setting of the tool band ID in the CEEE executors manager.

BUG=64854
TEST=ie_unittests.exe


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

[Jói, 2010-11-30 22:19:06]

LGTM

Before you check in, take note of my recent message about how to get ceee stuff
merged to M9.

[motek., 2010-11-30 22:33:09]

Just one suggestion to consider... 

M.

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
File ceee/ie/plugin/toolband/tool_band.cc (right):

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
ceee/ie/plugin/toolband/tool_band.cc:533: EnsureBhoIsAvailable();  // We will
require a working bho to do anything.
So, what about 'if (SUCCEEDED(EnsureBhoIsAvailable)) { do the ID thing }

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
ceee/ie/plugin/toolband/tool_band.cc:747: return S_FALSE;
I am not sure if the return value of 'Ensure' should depend on how many times
the ID has been sent to the recipient. I realize we don't use the return value
of 'Ensure' (and so you may disregard this comment), but since now something
actually depends on it, we should do it?
I do recall this was your comment to my original CL, so this is a bit ironic,
but...

[Cindy Lau, 2010-12-01 00:01:36]

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
File ceee/ie/plugin/toolband/tool_band.cc (right):

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
ceee/ie/plugin/toolband/tool_band.cc:533: EnsureBhoIsAvailable();  // We will
require a working bho to do anything.
On 2010/11/30 22:33:09, motek1 wrote:
> So, what about 'if (SUCCEEDED(EnsureBhoIsAvailable)) { do the ID thing }

The other thing EnsureBhoIsAvailable does is to actually retrieve the handle to
the BHO; if we want to move SendSessionIdToBho outside of it, we'll have to
somehow return the BHO handle from the EnsureBhoIsAvailable call, perhaps by an
output parameter.  I'm not a huge fan of this approach; if the problem is that
EnsureBhoIsAvailable shouldn't return S_FALSE just because SendSessionIdToBho
does, then I will make a change inside EnsureBhoIsAvailable that will make it
return S_OK if SendSessionIdToBho returns S_FALSE.  Does that sound better?

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
ceee/ie/plugin/toolband/tool_band.cc:747: return S_FALSE;
On 2010/11/30 22:33:09, motek1 wrote:
> I am not sure if the return value of 'Ensure' should depend on how many times
> the ID has been sent to the recipient. I realize we don't use the return value
> of 'Ensure' (and so you may disregard this comment), but since now something
> actually depends on it, we should do it?
> I do recall this was your comment to my original CL, so this is a bit ironic,
> but...

See my comment above.

[motek., 2010-12-01 00:13:49]

LGTM

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
File ceee/ie/plugin/toolband/tool_band.cc (right):

http://codereview.chromium.org/5271011/diff/1/ceee/ie/plugin/toolband/tool_ba...
ceee/ie/plugin/toolband/tool_band.cc:533: EnsureBhoIsAvailable();  // We will
require a working bho to do anything.
On 2010/12/01 00:01:36, Cindy Lau wrote:
Yes, my point was only solely about return values (S_FALSE vs. S_OK). What you
suggest makes more sense. Besides, the whole comment was a minor remark to begin
with.