experimental.popup support for tab-content-viewed extensions

Refactoring of the chrome.experimental.popup API implementation to allow display of pop-ups for extensions viewed through a tab-contents view.I added a new class, ExtensionPopupHost.  This class implements the necessary environment for managing child popup windows from either an ExtensionHost, or an ExtensionDOMUI.  Note that this class is added as a member to ExtensionHost and ExtensionDOMUI.  

I decided to take this approach to prevent multiple inheritance of the NotificationObserver class:  Both ExtensionPopupHost and ExtensionHost must inherit from this class, and I was uncertain of how the system would behave wrt virtual inheritance.  Please comment on if I should have used the inheritance approach.

I also removed the customHandler tag (in extension_api.json) that I had added in the initial submission.  The arguments in the schema are now those that users of the API see.  The nodocs tags were also removed.

The api experimental.popup.getAnchorWindow() has been renamed to popup.getParentWindow, as per a suggestion from Erik K.

BUG=none
TEST=extension_popup_apitest.cc

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

[jeff.timanus, 2009-11-12 05:26:29]

Hi Guys,

Please review this CL.  It does not address the recently discovered flakiness of
the popup tests, but I will investigate that problem shortly.

Thanks,

Jeff

[Erik does not do reviews, 2009-11-12 22:41:29]

I'll defer to rafael on the api.json changes.

http://codereview.chromium.org/385061/diff/18/20
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/385061/diff/18/20#newcode38
Line 38: extension_popup_host_.reset(new ExtensionPopupHost(this));
This seems wrong to me.  Why does every object get a popup host?  Shouldn't this
be created on an as-needed basis?

http://codereview.chromium.org/385061/diff/18/20#newcode224
Line 224: if (tab_contents()) {
at a minimum, remove {}.  In these kinds of cases however, chrome-style seems
often to use ternary operators.
return tab_contents() ? tab_contents()->render_view_host() : NULL;

http://codereview.chromium.org/385061/diff/18/23
File chrome/browser/extensions/extension_dom_ui.h (right):

http://codereview.chromium.org/385061/diff/18/23#newcode81
Line 81: scoped_ptr<ExtensionPopupHost> extension_popup_host_;
add comment

http://codereview.chromium.org/385061/diff/18/24
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/385061/diff/18/24#newcode130
Line 130: if (ViewType::EXTENSION_POPUP != host_type) {
remove {}

http://codereview.chromium.org/385061/diff/18/24#newcode131
Line 131: extension_popup_host_.reset(new ExtensionPopupHost(this));
same comment as before.  It seems odd for this to always be instantiated.

http://codereview.chromium.org/385061/diff/18/27
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/385061/diff/18/27#newcode220
Line 220: scoped_ptr<ExtensionPopupHost> extension_popup_host_;
Since this will exist in all ExtensionPopupHost::Delegate instances, why not add
the data member and accessor there?

http://codereview.chromium.org/385061/diff/18/26
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/385061/diff/18/26#newcode15
Line 15: #if defined(TOOLKIT_VIEWS)
this doesn't have a VIEWS dependency

http://codereview.chromium.org/385061/diff/18/26#newcode23
Line 23: #include "chrome/browser/tab_contents/tab_contents.h"
neither does this

http://codereview.chromium.org/385061/diff/18/26#newcode144
Line 144: bool PopupShowFunction::ComputeHostScreenPosition(gfx::Point* origin)
{
rename this to ConvertHostPointToScreen

http://codereview.chromium.org/385061/diff/18/26#newcode159
Line 159: #endif  // defined(TOOLKIT_VIEWS)
#else  // TODO(port)
  NOTIMPLEMENTED();

http://codereview.chromium.org/385061/diff/18/32
File chrome/browser/extensions/extension_popup_host.cc (right):

http://codereview.chromium.org/385061/diff/18/32#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
2009

http://codereview.chromium.org/385061/diff/18/32#newcode6
Line 6: 
remove newline

http://codereview.chromium.org/385061/diff/18/32#newcode10
Line 10: #endif  // defined(TOOLKIT_VIEWS)
remove comment

http://codereview.chromium.org/385061/diff/18/32#newcode13
Line 13: #include "chrome/browser/browser.h"
most of these aren't views specific and should be outside of the #ifdef

http://codereview.chromium.org/385061/diff/18/32#newcode26
Line 26: #endif  // defined(TOOLKIT_VIEWS)
remove comment

http://codereview.chromium.org/385061/diff/18/32#newcode28
Line 28: {
move to previous line

http://codereview.chromium.org/385061/diff/18/32#newcode45
Line 45: void ExtensionPopupHost::BubbleBrowserWindowClosing(BrowserBubble*
bubble) {
add newline above

http://codereview.chromium.org/385061/diff/18/31
File chrome/browser/extensions/extension_popup_host.h (right):

http://codereview.chromium.org/385061/diff/18/31#newcode11
Line 11: #endif  // defined(TOOLKIT_VIEWS)
remove comment (not needed when so close together)

http://codereview.chromium.org/385061/diff/18/31#newcode17
Line 17: #endif  // defined(TOOLKIT_VIEWS)
remove comment

http://codereview.chromium.org/385061/diff/18/31#newcode32
Line 32: class PopupDelegate {
you can just name this Delegate since it's a nested class

http://codereview.chromium.org/385061/diff/18/31#newcode70
Line 70: void DismissPopup();
move this before the interface implementations

http://codereview.chromium.org/385061/diff/18/29
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/385061/diff/18/29#newcode445
Line 445: var popUpInfo = {
popupInfo

http://codereview.chromium.org/385061/diff/18/29#newcode460
Line 460: return GetPopupAnchorView();
this should be renamed as well

[jeff.timanus, 2009-11-13 04:03:21]

Thanks for your review, Erik.  I addressed all of the changes you suggested.

Also, I've discovered some interesting C++ trivia - see below.

Jeff

http://codereview.chromium.org/385061/diff/18/20
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/385061/diff/18/20#newcode38
Line 38: extension_popup_host_.reset(new ExtensionPopupHost(this));
On 2009/11/12 22:41:29, Erik Kay wrote:
> This seems wrong to me.  Why does every object get a popup host?  Shouldn't
this
> be created on an as-needed basis?

Hmmm.  Good point.  I had first implemented the host through inheritance, so my
thinking was to follow through and construct it during the constructor.  I think
you're right.  We could wait and only create the pop-up host upon first request.

Done.

http://codereview.chromium.org/385061/diff/18/20#newcode224
Line 224: if (tab_contents()) {
On 2009/11/12 22:41:29, Erik Kay wrote:
> at a minimum, remove {}.  In these kinds of cases however, chrome-style seems
> often to use ternary operators.
> return tab_contents() ? tab_contents()->render_view_host() : NULL;

Done.

http://codereview.chromium.org/385061/diff/18/24
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/385061/diff/18/24#newcode131
Line 131: extension_popup_host_.reset(new ExtensionPopupHost(this));
On 2009/11/12 22:41:29, Erik Kay wrote:
> same comment as before.  It seems odd for this to always be instantiated.

Done.

http://codereview.chromium.org/385061/diff/18/27
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/385061/diff/18/27#newcode220
Line 220: scoped_ptr<ExtensionPopupHost> extension_popup_host_;
On 2009/11/12 22:41:29, Erik Kay wrote:
> Since this will exist in all ExtensionPopupHost::Delegate instances, why not
add
> the data member and accessor there?
> 

Good idea.  Done.

http://codereview.chromium.org/385061/diff/18/26
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/385061/diff/18/26#newcode15
Line 15: #if defined(TOOLKIT_VIEWS)
On 2009/11/12 22:41:29, Erik Kay wrote:
> this doesn't have a VIEWS dependency

Done.

http://codereview.chromium.org/385061/diff/18/26#newcode23
Line 23: #include "chrome/browser/tab_contents/tab_contents.h"
On 2009/11/12 22:41:29, Erik Kay wrote:
> neither does this

Done.

http://codereview.chromium.org/385061/diff/18/26#newcode144
Line 144: bool PopupShowFunction::ComputeHostScreenPosition(gfx::Point* origin)
{
On 2009/11/12 22:41:29, Erik Kay wrote:
> rename this to ConvertHostPointToScreen

Done.

http://codereview.chromium.org/385061/diff/18/26#newcode159
Line 159: #endif  // defined(TOOLKIT_VIEWS)
On 2009/11/12 22:41:29, Erik Kay wrote:
> #else  // TODO(port)
>   NOTIMPLEMENTED();

Done.

http://codereview.chromium.org/385061/diff/18/32
File chrome/browser/extensions/extension_popup_host.cc (right):

http://codereview.chromium.org/385061/diff/18/32#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/11/12 22:41:29, Erik Kay wrote:
> 2009

Done.

http://codereview.chromium.org/385061/diff/18/32#newcode6
Line 6: 
On 2009/11/12 22:41:29, Erik Kay wrote:
> remove newline

Done.

http://codereview.chromium.org/385061/diff/18/32#newcode10
Line 10: #endif  // defined(TOOLKIT_VIEWS)
On 2009/11/12 22:41:29, Erik Kay wrote:
> remove comment

Done.

http://codereview.chromium.org/385061/diff/18/32#newcode13
Line 13: #include "chrome/browser/browser.h"
On 2009/11/12 22:41:29, Erik Kay wrote:
> most of these aren't views specific and should be outside of the #ifdef

Done.

http://codereview.chromium.org/385061/diff/18/32#newcode26
Line 26: #endif  // defined(TOOLKIT_VIEWS)
On 2009/11/12 22:41:29, Erik Kay wrote:
> remove comment

Done.

http://codereview.chromium.org/385061/diff/18/32#newcode28
Line 28: {
On 2009/11/12 22:41:29, Erik Kay wrote:
> move to previous line

Done.

http://codereview.chromium.org/385061/diff/18/32#newcode45
Line 45: void ExtensionPopupHost::BubbleBrowserWindowClosing(BrowserBubble*
bubble) {
On 2009/11/12 22:41:29, Erik Kay wrote:
> add newline above

Done.

http://codereview.chromium.org/385061/diff/18/31
File chrome/browser/extensions/extension_popup_host.h (right):

http://codereview.chromium.org/385061/diff/18/31#newcode11
Line 11: #endif  // defined(TOOLKIT_VIEWS)
On 2009/11/12 22:41:29, Erik Kay wrote:
> remove comment (not needed when so close together)

Done.

http://codereview.chromium.org/385061/diff/18/31#newcode17
Line 17: #endif  // defined(TOOLKIT_VIEWS)
On 2009/11/12 22:41:29, Erik Kay wrote:
> remove comment

Done.

http://codereview.chromium.org/385061/diff/18/31#newcode32
Line 32: class PopupDelegate {
On 2009/11/12 22:41:29, Erik Kay wrote:
> you can just name this Delegate since it's a nested class

Done, errr, wait . . . not done!  = (
I had originally named this class Delegate, but had to rename it due to
mysterious name-clashes with BrowserBubble::Delegate.  With your suggestion to
revert the name back to Delegate, I took a moment to investigate the behaviour
and found some interesting C++ trivia.  The following will not compile and
produces the given error:

class A {
 public:
  class D {
   public:
    virtual void func() = 0;
  }; 
};

class B : public A::D {
 public:
  class D {
    void func1();
  };

  void func() {}
};

void B::D::func1() {}

error: C2039: 'func1' : is not a member of 'A::D'

The rule seems to be as follows:  If you inherit from a class with a given name,
then you cannot declare a nested class with that same name unless all of that
class's methods are inline.

http://codereview.chromium.org/385061/diff/18/31#newcode70
Line 70: void DismissPopup();
On 2009/11/12 22:41:29, Erik Kay wrote:
> move this before the interface implementations

Done.

http://codereview.chromium.org/385061/diff/18/29
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/385061/diff/18/29#newcode445
Line 445: var popUpInfo = {
On 2009/11/12 22:41:29, Erik Kay wrote:
> popupInfo

Done.

http://codereview.chromium.org/385061/diff/18/29#newcode460
Line 460: return GetPopupAnchorView();
On 2009/11/12 22:41:29, Erik Kay wrote:
> this should be renamed as well

Done.

[Erik does not do reviews, 2009-11-13 18:38:20]

It looks like you didn't upload these changes yet.

Erik


On Thu, Nov 12, 2009 at 8:03 PM, <jeff.timanus@gmail.com> wrote:

> Thanks for your review, Erik.  I addressed all of the changes you
> suggested.
>
> Also, I've discovered some interesting C++ trivia - see below.
>
> Jeff
>
>
>
> http://codereview.chromium.org/385061/diff/18/20
> File chrome/browser/extensions/extension_dom_ui.cc (right):
>
> http://codereview.chromium.org/385061/diff/18/20#newcode38
> Line 38: extension_popup_host_.reset(new ExtensionPopupHost(this));
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> This seems wrong to me.  Why does every object get a popup host?
>>
> Shouldn't this
>
>> be created on an as-needed basis?
>>
>
> Hmmm.  Good point.  I had first implemented the host through
> inheritance, so my thinking was to follow through and construct it
> during the constructor.  I think you're right.  We could wait and only
> create the pop-up host upon first request.
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/20#newcode224
> Line 224: if (tab_contents()) {
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> at a minimum, remove {}.  In these kinds of cases however,
>>
> chrome-style seems
>
>> often to use ternary operators.
>> return tab_contents() ? tab_contents()->render_view_host() : NULL;
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/24
> File chrome/browser/extensions/extension_host.cc (right):
>
> http://codereview.chromium.org/385061/diff/18/24#newcode131
> Line 131: extension_popup_host_.reset(new ExtensionPopupHost(this));
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> same comment as before.  It seems odd for this to always be
>>
> instantiated.
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/27
> File chrome/browser/extensions/extension_host.h (right):
>
> http://codereview.chromium.org/385061/diff/18/27#newcode220
> Line 220: scoped_ptr<ExtensionPopupHost> extension_popup_host_;
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> Since this will exist in all ExtensionPopupHost::Delegate instances,
>>
> why not add
>
>> the data member and accessor there?
>>
>
>
> Good idea.  Done.
>
>
> http://codereview.chromium.org/385061/diff/18/26
> File chrome/browser/extensions/extension_popup_api.cc (right):
>
> http://codereview.chromium.org/385061/diff/18/26#newcode15
> Line 15: #if defined(TOOLKIT_VIEWS)
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> this doesn't have a VIEWS dependency
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/26#newcode23
> Line 23: #include "chrome/browser/tab_contents/tab_contents.h"
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> neither does this
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/26#newcode144
> Line 144: bool PopupShowFunction::ComputeHostScreenPosition(gfx::Point*
> origin) {
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> rename this to ConvertHostPointToScreen
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/26#newcode159
> Line 159: #endif  // defined(TOOLKIT_VIEWS)
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> #else  // TODO(port)
>>   NOTIMPLEMENTED();
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/32
> File chrome/browser/extensions/extension_popup_host.cc (right):
>
> http://codereview.chromium.org/385061/diff/18/32#newcode1
> Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights
> reserved.
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> 2009
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/32#newcode6
> Line 6:
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> remove newline
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/32#newcode10
> Line 10: #endif  // defined(TOOLKIT_VIEWS)
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> remove comment
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/32#newcode13
> Line 13: #include "chrome/browser/browser.h"
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> most of these aren't views specific and should be outside of the
>>
> #ifdef
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/32#newcode26
> Line 26: #endif  // defined(TOOLKIT_VIEWS)
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> remove comment
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/32#newcode28
> Line 28: {
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> move to previous line
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/32#newcode45
> Line 45: void
> ExtensionPopupHost::BubbleBrowserWindowClosing(BrowserBubble* bubble) {
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> add newline above
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/31
> File chrome/browser/extensions/extension_popup_host.h (right):
>
> http://codereview.chromium.org/385061/diff/18/31#newcode11
> Line 11: #endif  // defined(TOOLKIT_VIEWS)
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> remove comment (not needed when so close together)
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/31#newcode17
> Line 17: #endif  // defined(TOOLKIT_VIEWS)
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> remove comment
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/31#newcode32
> Line 32: class PopupDelegate {
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> you can just name this Delegate since it's a nested class
>>
>
> Done, errr, wait . . . not done!  = (
> I had originally named this class Delegate, but had to rename it due to
> mysterious name-clashes with BrowserBubble::Delegate.  With your
> suggestion to revert the name back to Delegate, I took a moment to
> investigate the behaviour and found some interesting C++ trivia.  The
> following will not compile and produces the given error:
>
> class A {
>  public:
>  class D {
>   public:
>    virtual void func() = 0;
>  };
> };
>
> class B : public A::D {
>  public:
>  class D {
>    void func1();
>  };
>
>  void func() {}
> };
>
> void B::D::func1() {}
>
> error: C2039: 'func1' : is not a member of 'A::D'
>
> The rule seems to be as follows:  If you inherit from a class with a
> given name, then you cannot declare a nested class with that same name
> unless all of that class's methods are inline.
>
>
> http://codereview.chromium.org/385061/diff/18/31#newcode70
> Line 70: void DismissPopup();
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> move this before the interface implementations
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/29
> File chrome/renderer/resources/extension_process_bindings.js (right):
>
> http://codereview.chromium.org/385061/diff/18/29#newcode445
> Line 445: var popUpInfo = {
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> popupInfo
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061/diff/18/29#newcode460
> Line 460: return GetPopupAnchorView();
> On 2009/11/12 22:41:29, Erik Kay wrote:
>
>> this should be renamed as well
>>
>
> Done.
>
>
> http://codereview.chromium.org/385061
>

[jeff.timanus, 2009-11-13 18:52:53]

On 2009/11/13 18:38:20, Erik Kay wrote:
> It looks like you didn't upload these changes yet.
Changes uploaded.  
> 
> Erik
> 
> 
> On Thu, Nov 12, 2009 at 8:03 PM, <mailto:jeff.timanus@gmail.com> wrote:
> 
> > Thanks for your review, Erik.  I addressed all of the changes you
> > suggested.
> >
> > Also, I've discovered some interesting C++ trivia - see below.
> >
> > Jeff
> >
> >
> >
> > http://codereview.chromium.org/385061/diff/18/20
> > File chrome/browser/extensions/extension_dom_ui.cc (right):
> >
> > http://codereview.chromium.org/385061/diff/18/20#newcode38
> > Line 38: extension_popup_host_.reset(new ExtensionPopupHost(this));
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> This seems wrong to me.  Why does every object get a popup host?
> >>
> > Shouldn't this
> >
> >> be created on an as-needed basis?
> >>
> >
> > Hmmm.  Good point.  I had first implemented the host through
> > inheritance, so my thinking was to follow through and construct it
> > during the constructor.  I think you're right.  We could wait and only
> > create the pop-up host upon first request.
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/20#newcode224
> > Line 224: if (tab_contents()) {
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> at a minimum, remove {}.  In these kinds of cases however,
> >>
> > chrome-style seems
> >
> >> often to use ternary operators.
> >> return tab_contents() ? tab_contents()->render_view_host() : NULL;
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/24
> > File chrome/browser/extensions/extension_host.cc (right):
> >
> > http://codereview.chromium.org/385061/diff/18/24#newcode131
> > Line 131: extension_popup_host_.reset(new ExtensionPopupHost(this));
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> same comment as before.  It seems odd for this to always be
> >>
> > instantiated.
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/27
> > File chrome/browser/extensions/extension_host.h (right):
> >
> > http://codereview.chromium.org/385061/diff/18/27#newcode220
> > Line 220: scoped_ptr<ExtensionPopupHost> extension_popup_host_;
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> Since this will exist in all ExtensionPopupHost::Delegate instances,
> >>
> > why not add
> >
> >> the data member and accessor there?
> >>
> >
> >
> > Good idea.  Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/26
> > File chrome/browser/extensions/extension_popup_api.cc (right):
> >
> > http://codereview.chromium.org/385061/diff/18/26#newcode15
> > Line 15: #if defined(TOOLKIT_VIEWS)
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> this doesn't have a VIEWS dependency
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/26#newcode23
> > Line 23: #include "chrome/browser/tab_contents/tab_contents.h"
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> neither does this
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/26#newcode144
> > Line 144: bool PopupShowFunction::ComputeHostScreenPosition(gfx::Point*
> > origin) {
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> rename this to ConvertHostPointToScreen
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/26#newcode159
> > Line 159: #endif  // defined(TOOLKIT_VIEWS)
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> #else  // TODO(port)
> >>   NOTIMPLEMENTED();
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/32
> > File chrome/browser/extensions/extension_popup_host.cc (right):
> >
> > http://codereview.chromium.org/385061/diff/18/32#newcode1
> > Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights
> > reserved.
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> 2009
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/32#newcode6
> > Line 6:
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> remove newline
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/32#newcode10
> > Line 10: #endif  // defined(TOOLKIT_VIEWS)
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> remove comment
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/32#newcode13
> > Line 13: #include "chrome/browser/browser.h"
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> most of these aren't views specific and should be outside of the
> >>
> > #ifdef
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/32#newcode26
> > Line 26: #endif  // defined(TOOLKIT_VIEWS)
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> remove comment
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/32#newcode28
> > Line 28: {
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> move to previous line
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/32#newcode45
> > Line 45: void
> > ExtensionPopupHost::BubbleBrowserWindowClosing(BrowserBubble* bubble) {
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> add newline above
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/31
> > File chrome/browser/extensions/extension_popup_host.h (right):
> >
> > http://codereview.chromium.org/385061/diff/18/31#newcode11
> > Line 11: #endif  // defined(TOOLKIT_VIEWS)
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> remove comment (not needed when so close together)
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/31#newcode17
> > Line 17: #endif  // defined(TOOLKIT_VIEWS)
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> remove comment
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/31#newcode32
> > Line 32: class PopupDelegate {
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> you can just name this Delegate since it's a nested class
> >>
> >
> > Done, errr, wait . . . not done!  = (
> > I had originally named this class Delegate, but had to rename it due to
> > mysterious name-clashes with BrowserBubble::Delegate.  With your
> > suggestion to revert the name back to Delegate, I took a moment to
> > investigate the behaviour and found some interesting C++ trivia.  The
> > following will not compile and produces the given error:
> >
> > class A {
> >  public:
> >  class D {
> >   public:
> >    virtual void func() = 0;
> >  };
> > };
> >
> > class B : public A::D {
> >  public:
> >  class D {
> >    void func1();
> >  };
> >
> >  void func() {}
> > };
> >
> > void B::D::func1() {}
> >
> > error: C2039: 'func1' : is not a member of 'A::D'
> >
> > The rule seems to be as follows:  If you inherit from a class with a
> > given name, then you cannot declare a nested class with that same name
> > unless all of that class's methods are inline.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/31#newcode70
> > Line 70: void DismissPopup();
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> move this before the interface implementations
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/29
> > File chrome/renderer/resources/extension_process_bindings.js (right):
> >
> > http://codereview.chromium.org/385061/diff/18/29#newcode445
> > Line 445: var popUpInfo = {
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> popupInfo
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061/diff/18/29#newcode460
> > Line 460: return GetPopupAnchorView();
> > On 2009/11/12 22:41:29, Erik Kay wrote:
> >
> >> this should be renamed as well
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/385061
> >
>

[rafaelw, 2009-11-13 19:03:25]

only reviewed extension_process_bindings.js and extension_api.json

http://codereview.chromium.org/385061/diff/5018/5028
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/385061/diff/5018/5028#newcode1656
Line 1656: "type": "any",
Did you intentionally remove the isInstanceOf: HTMLElement here? If so, why?

http://codereview.chromium.org/385061/diff/5018/5029
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/385061/diff/5018/5029#newcode420
Line 420: // Schema used internally to send re-packaged arguments to
don't define the entire schema here. see below

http://codereview.chromium.org/385061/diff/5018/5029#newcode450
Line 450: return sendRequest(this.name,
define local:

// Second argument is a transform from HTMLElement to Rect.
var internalSchema = [
  this.definition.parameters[0],
  {
    type: "object",
    name: "domAnchor",
    properties: {
      top: { type: "integer", minimum: 0 },
      left: { type: "integer", minimum: 0 },
      width: { type: "integer", minimum: 0 },
      height: { type: "integer", minimum: 0 }
    }
  },
  this.definition.parameters[1]
];

and then call sendRequestion(,,internalSchema).

[jeff.timanus, 2009-11-13 19:51:31]

Comments addressed.

http://codereview.chromium.org/385061/diff/5018/5028
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/385061/diff/5018/5028#newcode1656
Line 1656: "type": "any",
On 2009/11/13 19:03:26, rafaelw wrote:
> Did you intentionally remove the isInstanceOf: HTMLElement here? If so, why?

Got too aggressive resolving merge conflicts here.  Re-added the instanceOf tag.

http://codereview.chromium.org/385061/diff/5018/5029
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/385061/diff/5018/5029#newcode420
Line 420: // Schema used internally to send re-packaged arguments to
On 2009/11/13 19:03:26, rafaelw wrote:
> don't define the entire schema here. see below

Done.

http://codereview.chromium.org/385061/diff/5018/5029#newcode450
Line 450: return sendRequest(this.name,
On 2009/11/13 19:03:26, rafaelw wrote:
> define local:
> 
> // Second argument is a transform from HTMLElement to Rect.
> var internalSchema = [
>   this.definition.parameters[0],
>   {
>     type: "object",
>     name: "domAnchor",
>     properties: {
>       top: { type: "integer", minimum: 0 },
>       left: { type: "integer", minimum: 0 },
>       width: { type: "integer", minimum: 0 },
>       height: { type: "integer", minimum: 0 }
>     }
>   },
>   this.definition.parameters[1]
> ];
> 
> and then call sendRequestion(,,internalSchema).
> 
> 

Done.

[rafaelw, 2009-11-13 19:54:07]

lgtm on extension_process_bindings.js and extension_api.json

[Erik does not do reviews, 2009-11-13 21:38:57]

LGTM - just nits

http://codereview.chromium.org/385061/diff/7002/7016
File chrome/browser/extensions/extension_popup_host.cc (right):

http://codereview.chromium.org/385061/diff/7002/7016#newcode15
Line 15: #endif  // defined(TOOLKIT_VIEWS)
remove comment

http://codereview.chromium.org/385061/diff/7002/7016#newcode24
Line 24: }
remove {}

http://codereview.chromium.org/385061/diff/7002/7013
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/385061/diff/7002/7013#newcode322
Line 322: if (this.handleRequest)
add {} here (multiline block plus JS style)

http://codereview.chromium.org/385061/diff/7002/7013#newcode326
Line 326: this.definition.parameters);
indent

[twiz, 2009-11-13 21:55:07]

http://codereview.chromium.org/385061/diff/7002/7016
File chrome/browser/extensions/extension_popup_host.cc (right):

http://codereview.chromium.org/385061/diff/7002/7016#newcode15
Line 15: #endif  // defined(TOOLKIT_VIEWS)
On 2009/11/13 21:38:57, Erik Kay wrote:
> remove comment

Done.

http://codereview.chromium.org/385061/diff/7002/7016#newcode24
Line 24: }
On 2009/11/13 21:38:57, Erik Kay wrote:
> remove {}

Done.

http://codereview.chromium.org/385061/diff/7002/7013
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/385061/diff/7002/7013#newcode322
Line 322: if (this.handleRequest)
On 2009/11/13 21:38:57, Erik Kay wrote:
> add {} here (multiline block plus JS style)

Done.

http://codereview.chromium.org/385061/diff/7002/7013#newcode326
Line 326: this.definition.parameters);
On 2009/11/13 21:38:57, Erik Kay wrote:
> indent

Done.