add mole info to toolstrip definition and enable moles

add mole info to toolstrip definition and enable moles

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

[Matt Perry, 2009-07-24 23:11:11]

LG overall.  Can you add a sample extension with moles to the test/data dir (or
modify one of the existing ones)?

We need some tests for this too:
- loader tests to make sure we read/parse the mole info correctly.
- a browsertest to make sure clicking on the bubble loads the mole url, and
back.

The browsertest can wait for a future CL, but I'd like to see the loader test in
this one.

http://codereview.chromium.org/159202/diff/2001/17
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/159202/diff/2001/17#newcode196
Line 196: new ExtensionFunctionDispatcher(render_view_host_, this, url_));
I think we need to use params.url here, or this won't work for
renderer-initiated navigations.  We probably also want to update url_.

http://codereview.chromium.org/159202/diff/2001/20
File chrome/browser/extensions/extension_shelf_model.h (right):

http://codereview.chromium.org/159202/diff/2001/20#newcode86
Line 86: int IndexOfToolstrip(ExtensionHost* toolstrip);
IndexOfExtension?

http://codereview.chromium.org/159202/diff/2001/24
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/159202/diff/2001/24#newcode343
Line 343: extension_size.set_height(25);  // TODO(erikkay) hard-coded?
any way we can get this information from the height of the shelf?

http://codereview.chromium.org/159202/diff/2001/28
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/159202/diff/2001/28#newcode755
Line 755: if (list_value->GetString(i, &toolstrip_path)) {
is this temporary for back compat, or do you plan on leaving this here
permanently?  if temporary, add a TODO.

[Erik does not do reviews, 2009-07-26 00:34:45]

I did add a mole to the loading tests, but didn't add the verification of the
new data to the test (sorry).  I'll upload a new CL that does this this weekend.

http://codereview.chromium.org/159202/diff/2001/20
File chrome/browser/extensions/extension_shelf_model.h (right):

http://codereview.chromium.org/159202/diff/2001/20#newcode86
Line 86: int IndexOfToolstrip(ExtensionHost* toolstrip);
On 2009/07/24 23:11:11, Matt Perry wrote:
> IndexOfExtension?

No.  An extension can have multiple toolstrips.

http://codereview.chromium.org/159202/diff/2001/24
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/159202/diff/2001/24#newcode343
Line 343: extension_size.set_height(25);  // TODO(erikkay) hard-coded?
On 2009/07/24 23:11:11, Matt Perry wrote:
> any way we can get this information from the height of the shelf?

yeah, I had a constant that I should have been using here anyway.

http://codereview.chromium.org/159202/diff/2001/28
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/159202/diff/2001/28#newcode755
Line 755: if (list_value->GetString(i, &toolstrip_path)) {
On 2009/07/24 23:11:11, Matt Perry wrote:
> is this temporary for back compat, or do you plan on leaving this here
> permanently?  if temporary, add a TODO.

It's unclear.  I added a comment with a TODO to look at this later.

[Matt Perry, 2009-07-27 17:41:50]

http://codereview.chromium.org/159202/diff/2001/20
File chrome/browser/extensions/extension_shelf_model.h (right):

http://codereview.chromium.org/159202/diff/2001/20#newcode86
Line 86: int IndexOfToolstrip(ExtensionHost* toolstrip);
On 2009/07/26 00:34:46, Erik Kay wrote:
> On 2009/07/24 23:11:11, Matt Perry wrote:
> > IndexOfExtension?
> 
> No.  An extension can have multiple toolstrips.

Then what good is this method?

Actually, after a quick scan through the .cc files, it seems you only ever call
this as IndexOfToolstrip(toolstrip->host).  You probably want it to take a
Toolstrip* instead.

[Erik does not do reviews, 2009-07-27 18:35:53]

On Mon, Jul 27, 2009 at 10:41 AM, <mpcomplete@chromium.org> wrote:

>
> http://codereview.chromium.org/159202/diff/2001/20
> File chrome/browser/extensions/extension_shelf_model.h (right):
>
> http://codereview.chromium.org/159202/diff/2001/20#newcode86
> Line 86: int IndexOfToolstrip(ExtensionHost* toolstrip);
> On 2009/07/26 00:34:46, Erik Kay wrote:
>
>> On 2009/07/24 23:11:11, Matt Perry wrote:
>> > IndexOfExtension?
>>
>
>  No.  An extension can have multiple toolstrips.
>>
>
> Then what good is this method?
>
> Actually, after a quick scan through the .cc files, it seems you only
> ever call this as IndexOfToolstrip(toolstrip->host).  You probably want
> it to take a Toolstrip* instead.


The use for this comes when we're outside of the shelf and just have an
ExtensionHost.

I'm working on a toolstrip API where the toolstrip can expand as a mole or
collapse back into the shelf.  In order to do this, we need to be able to
get from ExtensionHost to Toolstrip.

Perhaps it will be better to change IndexOfToolstrip to take a Toolstrip* as
you suggest and to add a ToolstripForHost for the other use case.  I'll look
at that as part of my next change.



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