Initial support for inspecting extension popups.

Initial support for inspecting extension popups.

The primary change in this CL is a refactor which makes ExtensionPopup a bit more self-contained WRT its clients. It adds the ability to specify an "inspect_with_devtools" flag to its Show() method which will cause the popup to remain open regardless of losing focus and to focus a devtools window on the popup's render view host.

This CL also pulls apart some aspects of the extension_popup_api from ExtensionFunctionDispatcher and ExtensionHost.

Still remaining to be done are:

1) Also the popup to stay open when the host window drags (it current closes)
2) Support for GTK
3) Support for Mac

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

[rafaelw, 2010-03-15 22:03:58]

erik, here ya go.

jeff, please take a look at the extension_popup_api changes. I haven't tested
your use case, but the functional logic should be unchanged apart from the fact
that there is no longer an explicit limit of 1 popup per extension (per our
conversation, the code now relies on focus losing to implicitly manage that).

[Erik does not do reviews, 2010-03-16 00:25:42]

http://codereview.chromium.org/1001002/diff/1/4
File chrome/browser/extensions/extension_action_context_menu_model.cc (right):

http://codereview.chromium.org/1001002/diff/1/4#newcode45
chrome/browser/extensions/extension_action_context_menu_model.cc:45: if
(prefs->GetBoolean(prefs::kExtensionsUIDeveloperMode)) {
it looks like you allow delegate to be NULL below.  maybe just don't add the
menu item in that case?  otherwise, I'd DCHECK below.

http://codereview.chromium.org/1001002/diff/1/5
File chrome/browser/extensions/extension_action_context_menu_model.h (right):

http://codereview.chromium.org/1001002/diff/1/5#newcode32
chrome/browser/extensions/extension_action_context_menu_model.h:32: explicit
ExtensionActionContextMenuModel(Extension* extension,
Can't extension_action be looked up from extension?  Why pass in PrefService? 
(most places seem to just look it up)

http://codereview.chromium.org/1001002/diff/1/6
File chrome/browser/extensions/extension_dom_ui.h (left):

http://codereview.chromium.org/1001002/diff/1/6#oldcode52
chrome/browser/extensions/extension_dom_ui.h:52: virtual RenderViewHost*
GetRenderViewHost();
these next three methods are part of ExtensionPopupHost::Delegate as well, so
they should be removed too.

http://codereview.chromium.org/1001002/diff/1/9
File chrome/browser/extensions/extension_host.h (left):

http://codereview.chromium.org/1001002/diff/1/9#oldcode203
chrome/browser/extensions/extension_host.h:203: virtual Browser* GetBrowser()
const { return GetBrowser(true); }
should these methods be removed?

http://codereview.chromium.org/1001002/diff/1/10
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/1001002/diff/1/10#newcode63
chrome/browser/extensions/extension_popup_api.cc:63: // Because the
close-on-focus-lost behavior is different from page action
wait... how is it different?  is that in the comments somewhere?

http://codereview.chromium.org/1001002/diff/1/10#newcode76
chrome/browser/extensions/extension_popup_api.cc:76:
views::FocusManager::GetWidgetFocusManager()->
should this happen in the closed method?  I was wondering if it was possible for
closed to get called twice somehow - once from some explicit way and once via
focus.

http://codereview.chromium.org/1001002/diff/1/10#newcode99
chrome/browser/extensions/extension_popup_api.cc:99: 
remove newline

http://codereview.chromium.org/1001002/diff/1/10#newcode119
chrome/browser/extensions/extension_popup_api.cc:119: if (host_widget &&
host_widget->ContainsNativeView(focused_now)) {
remove {}

http://codereview.chromium.org/1001002/diff/1/10#newcode257
chrome/browser/extensions/extension_popup_api.cc:257: popup_ =
ExtensionPopup::Show(url, GetBrowser(),
put GetBrowser() on a new line
(btw, this argument list is getting silly.  any way we can figure out how to
slim it down?)

http://codereview.chromium.org/1001002/diff/1/10#newcode266
chrome/browser/extensions/extension_popup_api.cc:266: // popup_host will handle
focus change listening.
nit: newline above

http://codereview.chromium.org/1001002/diff/1/10#newcode267
chrome/browser/extensions/extension_popup_api.cc:267:
popup_->set_close_on_lost_focus(false);
So is this saying that the popup won't close on lost focus or that popup_host_
is going to handle the closing logic for it?  If so, please clarify the comment?

http://codereview.chromium.org/1001002/diff/1/15
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1001002/diff/1/15#newcode139
chrome/browser/views/browser_actions_container.cc:139:
panel_->OnBrowserActionExecuted(this, false); // inspect_with_devtools
nit, two spaces before //  (multiple places)

http://codereview.chromium.org/1001002/diff/1/15#newcode536
chrome/browser/views/browser_actions_container.cc:536: true,   // Activate the
popup window.
nit: two spaces not three

http://codereview.chromium.org/1001002/diff/1/15#newcode1085
chrome/browser/views/browser_actions_container.cc:1085: popup_ = NULL;
DCHECK_EQ(popup_, popup)

http://codereview.chromium.org/1001002/diff/1/20
File chrome/browser/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/1001002/diff/1/20#newcode207
chrome/browser/views/extensions/extension_popup.cc:207: inspect_with_devtools_
||  // The popup is being inspect.
inspected

http://codereview.chromium.org/1001002/diff/1/20#newcode209
chrome/browser/views/extensions/extension_popup.cc:209: lost_focus_to_child)  //
A child of this view got focus.
line this comment up with the others or adjust the others to 2 spaces

http://codereview.chromium.org/1001002/diff/1/20#newcode227
chrome/browser/views/extensions/extension_popup.cc:227: case
NotificationType::EXTENSION_HOST_DID_STOP_LOADING:
indent on the case statements is off

http://codereview.chromium.org/1001002/diff/1/20#newcode250
chrome/browser/views/extensions/extension_popup.cc:250: // If the devtools
window is closing, we post a task to ourselves to
move this comment to the next line and a different comment on this line

http://codereview.chromium.org/1001002/diff/1/22
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/1001002/diff/1/22#newcode1499
chrome/browser/views/location_bar_view.cc:1499: true,   // Activate the popup
window.
2 spaces

http://codereview.chromium.org/1001002/diff/1/22#newcode1548
chrome/browser/views/location_bar_view.cc:1548: context_menu_->Run(extension,
extension->page_action(),
put this on the next line

http://codereview.chromium.org/1001002/diff/1/22#newcode1555
chrome/browser/views/location_bar_view.cc:1555: ExecuteAction(button, false);
comment the bool

http://codereview.chromium.org/1001002/diff/1/22#newcode1636
chrome/browser/views/location_bar_view.cc:1636: true); // inspect_with_devtools
2 spaces

http://codereview.chromium.org/1001002/diff/1/22#newcode1642
chrome/browser/views/location_bar_view.cc:1642: popup_ = NULL;
DHCECK_EQ(popup, popup_)

http://codereview.chromium.org/1001002/diff/1/22#newcode1737
chrome/browser/views/location_bar_view.cc:1737: false);
comment bool

[rafaelw, 2010-03-16 19:17:31]

http://codereview.chromium.org/1001002/diff/1/4
File chrome/browser/extensions/extension_action_context_menu_model.cc (right):

http://codereview.chromium.org/1001002/diff/1/4#newcode45
chrome/browser/extensions/extension_action_context_menu_model.cc:45: if
(prefs->GetBoolean(prefs::kExtensionsUIDeveloperMode)) {
On 2010/03/16 00:25:42, Erik Kay wrote:
> it looks like you allow delegate to be NULL below.  maybe just don't add the
> menu item in that case?  otherwise, I'd DCHECK below.

Done.

http://codereview.chromium.org/1001002/diff/1/5
File chrome/browser/extensions/extension_action_context_menu_model.h (right):

http://codereview.chromium.org/1001002/diff/1/5#newcode32
chrome/browser/extensions/extension_action_context_menu_model.h:32: explicit
ExtensionActionContextMenuModel(Extension* extension,
The problem is that it could be a page action or a browser_action. I know it
can't be now, but it seemed cleaner to be explicit about it (the code contract
doesn't enforce that there's only one).

Not 100% what the question is with PrefService. The menu model needs to inspect
whether the devtools mode is enabled. It seemed like the right place to draw the
line (with the thing that knows the answer). The other two options would have
been Profile* or bool show_inspect_devtools. I don't have a strong preference
here. If you do, just tell me.

On 2010/03/16 00:25:42, Erik Kay wrote:
> Can't extension_action be looked up from extension?  Why pass in PrefService? 
> (most places seem to just look it up)

http://codereview.chromium.org/1001002/diff/1/6
File chrome/browser/extensions/extension_dom_ui.h (left):

http://codereview.chromium.org/1001002/diff/1/6#oldcode52
chrome/browser/extensions/extension_dom_ui.h:52: virtual RenderViewHost*
GetRenderViewHost();
Nope. They are still in use. I removed the interface, but some of the underlying
methods are still needed (and are used elsewhere). I removed everything I could.

On 2010/03/16 00:25:42, Erik Kay wrote:
> these next three methods are part of ExtensionPopupHost::Delegate as well, so
> they should be removed too.

http://codereview.chromium.org/1001002/diff/1/9
File chrome/browser/extensions/extension_host.h (left):

http://codereview.chromium.org/1001002/diff/1/9#oldcode203
chrome/browser/extensions/extension_host.h:203: virtual Browser* GetBrowser()
const { return GetBrowser(true); }
Ditto. Still in use.

On 2010/03/16 00:25:42, Erik Kay wrote:
> should these methods be removed?

http://codereview.chromium.org/1001002/diff/1/10
File chrome/browser/extensions/extension_popup_api.cc (right):

http://codereview.chromium.org/1001002/diff/1/10#newcode63
chrome/browser/extensions/extension_popup_api.cc:63: // Because the
close-on-focus-lost behavior is different from page action
clarified in comments.

On 2010/03/16 00:25:42, Erik Kay wrote:
> wait... how is it different?  is that in the comments somewhere?

http://codereview.chromium.org/1001002/diff/1/10#newcode76
chrome/browser/extensions/extension_popup_api.cc:76:
views::FocusManager::GetWidgetFocusManager()->
No. I'm pretty sure that ExtensionPopupClosed() will only fire once (the caller
destroys itself shortly after).

In any case, leaving this in the dtor guarantees that it'll only ever fire once.

On 2010/03/16 00:25:42, Erik Kay wrote:
> should this happen in the closed method?  I was wondering if it was possible
for
> closed to get called twice somehow - once from some explicit way and once via
> focus.

http://codereview.chromium.org/1001002/diff/1/10#newcode99
chrome/browser/extensions/extension_popup_api.cc:99: 
On 2010/03/16 00:25:42, Erik Kay wrote:
> remove newline

Done.

http://codereview.chromium.org/1001002/diff/1/10#newcode119
chrome/browser/extensions/extension_popup_api.cc:119: if (host_widget &&
host_widget->ContainsNativeView(focused_now)) {
On 2010/03/16 00:25:42, Erik Kay wrote:
> remove {}

Done.

http://codereview.chromium.org/1001002/diff/1/10#newcode257
chrome/browser/extensions/extension_popup_api.cc:257: popup_ =
ExtensionPopup::Show(url, GetBrowser(),
No good way. The only arguments that appear redundant are Browser and Profile,
it looks like Browser is allowed to be NULL.

On 2010/03/16 00:25:42, Erik Kay wrote:
> put GetBrowser() on a new line
> (btw, this argument list is getting silly.  any way we can figure out how to
> slim it down?)

http://codereview.chromium.org/1001002/diff/1/10#newcode266
chrome/browser/extensions/extension_popup_api.cc:266: // popup_host will handle
focus change listening.
On 2010/03/16 00:25:42, Erik Kay wrote:
> nit: newline above

Done.

http://codereview.chromium.org/1001002/diff/1/10#newcode267
chrome/browser/extensions/extension_popup_api.cc:267:
popup_->set_close_on_lost_focus(false);
On 2010/03/16 00:25:42, Erik Kay wrote:
> So is this saying that the popup won't close on lost focus or that popup_host_
> is going to handle the closing logic for it?  If so, please clarify the
comment?

Done.

http://codereview.chromium.org/1001002/diff/1/15
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/1001002/diff/1/15#newcode139
chrome/browser/views/browser_actions_container.cc:139:
panel_->OnBrowserActionExecuted(this, false); // inspect_with_devtools
On 2010/03/16 00:25:42, Erik Kay wrote:
> nit, two spaces before //  (multiple places)

Done.

http://codereview.chromium.org/1001002/diff/1/15#newcode536
chrome/browser/views/browser_actions_container.cc:536: true,   // Activate the
popup window.
On 2010/03/16 00:25:42, Erik Kay wrote:
> nit: two spaces not three

Done.

http://codereview.chromium.org/1001002/diff/1/15#newcode1085
chrome/browser/views/browser_actions_container.cc:1085: popup_ = NULL;
On 2010/03/16 00:25:42, Erik Kay wrote:
> DCHECK_EQ(popup_, popup)

Done.

http://codereview.chromium.org/1001002/diff/1/20
File chrome/browser/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/1001002/diff/1/20#newcode207
chrome/browser/views/extensions/extension_popup.cc:207: inspect_with_devtools_
||  // The popup is being inspect.
On 2010/03/16 00:25:42, Erik Kay wrote:
> inspected

Done.

http://codereview.chromium.org/1001002/diff/1/20#newcode209
chrome/browser/views/extensions/extension_popup.cc:209: lost_focus_to_child)  //
A child of this view got focus.
On 2010/03/16 00:25:42, Erik Kay wrote:
> line this comment up with the others or adjust the others to 2 spaces

Done.

http://codereview.chromium.org/1001002/diff/1/20#newcode227
chrome/browser/views/extensions/extension_popup.cc:227: case
NotificationType::EXTENSION_HOST_DID_STOP_LOADING:
On 2010/03/16 00:25:42, Erik Kay wrote:
> indent on the case statements is off

Done.

http://codereview.chromium.org/1001002/diff/1/20#newcode250
chrome/browser/views/extensions/extension_popup.cc:250: // If the devtools
window is closing, we post a task to ourselves to
On 2010/03/16 00:25:42, Erik Kay wrote:
> move this comment to the next line and a different comment on this line

Done.

http://codereview.chromium.org/1001002/diff/1/22
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/1001002/diff/1/22#newcode1499
chrome/browser/views/location_bar_view.cc:1499: true,   // Activate the popup
window.
On 2010/03/16 00:25:42, Erik Kay wrote:
> 2 spaces

Done.

http://codereview.chromium.org/1001002/diff/1/22#newcode1548
chrome/browser/views/location_bar_view.cc:1548: context_menu_->Run(extension,
extension->page_action(),
On 2010/03/16 00:25:42, Erik Kay wrote:
> put this on the next line

Done.

http://codereview.chromium.org/1001002/diff/1/22#newcode1555
chrome/browser/views/location_bar_view.cc:1555: ExecuteAction(button, false);
On 2010/03/16 00:25:42, Erik Kay wrote:
> comment the bool

Done.

http://codereview.chromium.org/1001002/diff/1/22#newcode1636
chrome/browser/views/location_bar_view.cc:1636: true); // inspect_with_devtools
On 2010/03/16 00:25:42, Erik Kay wrote:
> 2 spaces

Done.

http://codereview.chromium.org/1001002/diff/1/22#newcode1642
chrome/browser/views/location_bar_view.cc:1642: popup_ = NULL;
On 2010/03/16 00:25:42, Erik Kay wrote:
> DHCECK_EQ(popup, popup_)

Done.

http://codereview.chromium.org/1001002/diff/1/22#newcode1737
chrome/browser/views/location_bar_view.cc:1737: false);
On 2010/03/16 00:25:42, Erik Kay wrote:
> comment bool

Done.

[Erik does not do reviews, 2010-03-16 20:25:31]

LGTM

http://codereview.chromium.org/1001002/diff/16001/17019
File chrome/browser/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/1001002/diff/16001/17019#newcode250
chrome/browser/views/extensions/extension_popup.cc:250: // Make sure it's the
devtools window that inspecting our popup.
typo: that's