mole expand/collapse API

mole expand/collapse API, callable from a toolstrip for manual operation

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

[Erik does not do reviews, 2009-07-29 23:08:25]

I need to add a unit test, but I think the code is worth looking at now.

[Aaron Boodman, 2009-07-29 23:36:31]

This looks very exciting!

http://codereview.chromium.org/160276/diff/1068/32
File chrome/browser/extensions/extension_toolstrip_api.h (right):

http://codereview.chromium.org/160276/diff/1068/32#newcode17
Line 17: class ToolstripFunction : public AsyncExtensionFunction {
I don't think these need to be AsyncExtensionFunctions, right?

[Matt Perry, 2009-07-30 00:39:41]

LGTM, handful of nits

http://codereview.chromium.org/160276/diff/1068/29
File chrome/browser/extensions/extension_shelf_model.cc (right):

http://codereview.chromium.org/160276/diff/1068/29#newcode157
Line 157: AddExtensions(
trailing spaces

http://codereview.chromium.org/160276/diff/1068/30
File chrome/browser/extensions/extension_shelf_model.h (right):

http://codereview.chromium.org/160276/diff/1068/30#newcode137
Line 137: class ExtensionShelfModelObserver {
nit: maybe scope this inside the ExtensionShelfModel class?

http://codereview.chromium.org/160276/diff/1068/31
File chrome/browser/extensions/extension_toolstrip_api.cc (right):

http://codereview.chromium.org/160276/diff/1068/31#newcode12
Line 12: const char kExpandFunction[] = "toolstrip.expand";
no indent

http://codereview.chromium.org/160276/diff/1068/31#newcode28
Line 28: ExtensionShelfModel* getModel(ExtensionFunctionDispatcher* dispatcher)
{
C++ style

http://codereview.chromium.org/160276/diff/1068/33
File chrome/browser/views/extensions/extension_shelf.cc (right):

http://codereview.chromium.org/160276/diff/1068/33#newcode347
Line 347: if (info_.mole.is_valid())
indent

http://codereview.chromium.org/160276/diff/1068/35
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/160276/diff/1068/35#newcode605
Line 605: // Make a copy, since the typical behavior is to detach and delete in
this
Another typical pattern is to do:
for (iterator i = begin(); i != end(); ) {
  iterator current = i++;
  current->DestroyMe();
}

All the common STL containers (vector, list, set, map) support removal without
invalidating other iterators.

http://codereview.chromium.org/160276/diff/1068/42
File chrome/test/data/extensions/samples/mappy/mappy_toolstrip.html (right):

http://codereview.chromium.org/160276/diff/1068/42#newcode62
Line 62: console.log("The content script said: " + data.message + " with values:
" + data.values);
nit: line length

http://codereview.chromium.org/160276/diff/1068/42#newcode66
Line 66: //window.open("http://maps.google.com/maps?f=q&source=s_q&q=" +
address);
ditto

[Erik does not do reviews, 2009-07-30 21:50:46]

http://codereview.chromium.org/160276/diff/1068/29
File chrome/browser/extensions/extension_shelf_model.cc (right):

http://codereview.chromium.org/160276/diff/1068/29#newcode157
Line 157: AddExtensions(
On 2009/07/30 00:39:41, Matt Perry wrote:
> trailing spaces

I think this is just a Rietveld UI thing (there are no spaces in my client).  It
looks like spaces since the left hand side was > 80 cols (that's what I was
fixing).

http://codereview.chromium.org/160276/diff/1068/31
File chrome/browser/extensions/extension_toolstrip_api.cc (right):

http://codereview.chromium.org/160276/diff/1068/31#newcode28
Line 28: ExtensionShelfModel* getModel(ExtensionFunctionDispatcher* dispatcher)
{
On 2009/07/30 00:39:41, Matt Perry wrote:
> C++ style

I had forgotten to just delete this (it's unused)

http://codereview.chromium.org/160276/diff/1068/32
File chrome/browser/extensions/extension_toolstrip_api.h (right):

http://codereview.chromium.org/160276/diff/1068/32#newcode17
Line 17: class ToolstripFunction : public AsyncExtensionFunction {
On 2009/07/29 23:36:31, Aaron Boodman wrote:
> I don't think these need to be AsyncExtensionFunctions, right?

right.

http://codereview.chromium.org/160276/diff/1068/35
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/160276/diff/1068/35#newcode605
Line 605: // Make a copy, since the typical behavior is to detach and delete in
this
On 2009/07/30 00:39:41, Matt Perry wrote:
> Another typical pattern is to do:
> for (iterator i = begin(); i != end(); ) {
>   iterator current = i++;
>   current->DestroyMe();
> }
> 
> All the common STL containers (vector, list, set, map) support removal without
> invalidating other iterators.

Thanks.  I hadn't seen this one.  I guess I just always had ++i ingrained into
me.

[Matt Perry, 2009-07-30 22:05:04]

lgtm

[Erik does not do reviews, 2009-07-30 23:54:30]

Paweł, could you please review the gtk changes I've added to this CL?

[Paweł Hajdan Jr., 2009-07-31 00:03:21]

Gtk changes LGTM.

http://codereview.chromium.org/160276/diff/1100/1106
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/160276/diff/1100/1106#newcode127
Line 127: virtual ExtensionHost* GetHost() { return this; }
I would prefer a long name: GetExtensionHost.

[Erik does not do reviews, 2009-07-31 00:33:02]

http://codereview.chromium.org/160276/diff/1100/1106
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/160276/diff/1100/1106#newcode127
Line 127: virtual ExtensionHost* GetHost() { return this; }
On 2009/07/31 00:03:21, Paweł Hajdan Jr. wrote:
> I would prefer a long name: GetExtensionHost.

good point.  fixed.