Enable devtools via context menu in platform apps and popup extensions.

Enable devtools via context menu in platform apps and popup extensions.

This is useful to allow developers of apps and extensions to easily use
the devtools in platform apps and popup extensions.

To enable devtools to work on popup extensions a new notification
NOTIFICATION_DEVTOOLS_WINDOW_OPENING has been added. This lets the popup
become aware it is being debugged, which allows it to stay up when it 
loses focus (e.g. when the devtools are clicked).

BUG=53518, 111017
TEST=Manual testing on Windows that dev tools are usable via the context
menu in these situations, and they are still disabled when they should
be.


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

[not at google - send to devlin, 2012-01-23 05:43:51]

Just a bunch of sanity-checking nits.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:650: }
nit sort of stuff:

Might be nice to save a reference to Extension* here.

Also, my personal preference for these sorts of things is to leave out the
"else" clause and just have "return NULL".

So like

const Extension* extension = host->extension();
if (extension)
  return extension;
return NULL;

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:666: void
RenderViewContextMenu::AppendPlatformAppItems() {
how about passing in the Extension*, save needing to call GetPlatformApp again.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1885: const CommandLine&
command_line = *CommandLine::ForCurrentProcess();
I'd move this down to right before it's used. Also... just store it locally as a
pointer rather than a reference... eh.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1892:
prefs::kWebKitJavascriptEnabled) ||
<- 2 indent

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:199: // in.
1 line

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:203: const Extension*
GetPlatformApp() const;
I reckon we don't need this method at all anymore. It's a bit of a conflation of
two checks (is there an extension, and if so, is it a platform app). From what I
can tell from looking at render_view_context_menu.cc, it's easily inlineable and
I think it would be clearer that way.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:109:
host()->render_view_host()) == details) {
<- 2 indent

[benwells, 2012-01-23 06:42:22]

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:650: }
On 2012/01/23 05:43:51, kalman wrote:
> nit sort of stuff:
> 
> Might be nice to save a reference to Extension* here.
> 
> Also, my personal preference for these sorts of things is to leave out the
> "else" clause and just have "return NULL".
> 
> So like
> 
> const Extension* extension = host->extension();
> if (extension)
>   return extension;
> return NULL;

Done.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:666: void
RenderViewContextMenu::AppendPlatformAppItems() {
On 2012/01/23 05:43:51, kalman wrote:
> how about passing in the Extension*, save needing to call GetPlatformApp
again.

Done.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1885: const CommandLine&
command_line = *CommandLine::ForCurrentProcess();
On 2012/01/23 05:43:51, kalman wrote:
> I'd move this down to right before it's used. Also... just store it locally as
a
> pointer rather than a reference... eh.

Ya much nicer. Done.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1892:
prefs::kWebKitJavascriptEnabled) ||
On 2012/01/23 05:43:51, kalman wrote:
> <- 2 indent

I think it is right as is? There is an extra four space indent for the
GetBoolean call... is that the rule?

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:199: // in.
On 2012/01/23 05:43:51, kalman wrote:
> 1 line

Done.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:203: const Extension*
GetPlatformApp() const;
On 2012/01/23 05:43:51, kalman wrote:
> I reckon we don't need this method at all anymore. It's a bit of a conflation
of
> two checks (is there an extension, and if so, is it a platform app). From what
I
> can tell from looking at render_view_context_menu.cc, it's easily inlineable
and
> I think it would be clearer that way.

Done.

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/9235002/diff/2001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_popup.cc:109:
host()->render_view_host()) == details) {
On 2012/01/23 05:43:51, kalman wrote:
> <- 2 indent

OK, I think I've fixed it and the similar code above.

[Aaron Boodman, 2012-01-23 06:49:05]

What is the effect of this for browser action popups? Can you right-click on the
bubble and select inspect? And then it actually works?

Because I remember we tried really hard to make this work back in prehistory and
gave up -- resulting in the current hack (right-click on the browser action and
select 'inspect' to see it in action -- it's terrible).

[benwells, 2012-01-23 06:54:53]

On 2012/01/23 06:49:05, Aaron Boodman wrote:
> What is the effect of this for browser action popups? Can you right-click on
the
> bubble and select inspect? And then it actually works?
> 
> Because I remember we tried really hard to make this work back in prehistory
and
> gave up -- resulting in the current hack (right-click on the browser action
and
> select 'inspect' to see it in action -- it's terrible).

Yes, it now works - unless I'm not thinking of some special cases or something.
I've tested it with a little popup calculator extension.

It needed some slight trickiness to prevent the popup disappearing when the
devtools appeared.

Let me know if you know of anything special to test / look out for.

[not at google - send to devlin, 2012-01-23 23:05:52]

cool, lgtm (from a sanity perspective, I don't know the high-level context of
this code particularly well, besides what I've looked at as part of
understanding the patch)

[benwells, 2012-01-23 23:11:11]

+mihai for apps review
+ben as browser/ui owner
+avi as browser/tab_contents owner

[Mihai Parparita -not on Chrome, 2012-01-23 23:17:29]

LGTM

http://codereview.chromium.org/9235002/diff/5001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1873: // for a popup
extension or a platform app) as they have javascript
Nit: Capitalize JavaScript.

[Ben Goodger (Google), 2012-01-23 23:46:54]

LGTM for browser/ui

[benwells, 2012-01-25 00:21:07]

-avi as reviewer who is on leave
+brettw as owner of chrome/browser/tab_contents

[benwells, 2012-01-25 08:11:52]

I just realised today that this only has the necessary changes in the views
implementation of the ExtensionPopup :(

I'll look into adding the mac and gtk changes on Friday (tomorrow is a holiday
here).

[benwells, 2012-01-27 07:15:57]

For now I've kept the Inspect Element command disabled on GTK and mac. I'd like
to address GTK and mac in separate patches, if that's OK.

http://codereview.chromium.org/9235002/diff/5001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1873: // for a popup
extension or a platform app) as they have javascript
On 2012/01/23 23:17:29, Mihai Parparita wrote:
> Nit: Capitalize JavaScript.

Done.

[benwells, 2012-02-03 05:39:40]

-brettw who is on leave
+avi who is not on leave any more
+msw who made changes in popup extension context menus recently.

msw: this should remove the same items your change removed, but lets me add the
'Inspect element' item in the menu all the time. I hope I kept the logic about
what menu items are shown the same.

[msw, 2012-02-03 07:57:24]

Awesome, this seems like a better solution than what I had before.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:650: const Extension*
extension = host->extension();
Why not just return host->extension(); here? The NULL check appears pointless.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:676: if
(!spelling_menu_observer_.get()) {
nit: Drop the braces for a single-line if block.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:679: if
(spelling_menu_observer_.get()) {
Is this check necessary? The above conditional ensures this is non-NULL.
If anything, maybe use a DCHECK instead?

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:685: if
(params_.is_editable)
There's an if (params_.is_editable) block just above, please combine them.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1912: // Don't enable
the web inspector if JavaScript is disabled. We don't
Why skip these checks for extensions? I prefer code simplicity over 'perf' here.
Then, in the non-views block below, just return false if (GetExtension()).

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1928: #if
defined(TOOLKIT_GTK) || defined(OS_MAC)
Instead, check #if !defined(TOOLKIT_VIEWS)

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1933: // TODO(benwells):
Add support for these builds and remote this #if.
s/remote/remove

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:92: details) {
nit: Braces here are optional, not necessary.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:100: // Set
inspect_with_devtools_ so the popup will be kept open while
Does this work? I think you'll need to call set_close_on_deactivate(false);

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:104: registrar_.Add(this,
content::NOTIFICATION_DEVTOOLS_WINDOW_CLOSING,
optional: consider replacing this statement and the duplicate in
ExtensionPopup::ShowBubble with an unconditional call in the ctor.

[benwells, 2012-02-06 03:40:46]

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:650: const Extension*
extension = host->extension();
On 2012/02/03 07:57:24, msw wrote:
> Why not just return host->extension(); here? The NULL check appears pointless.

Done.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:676: if
(!spelling_menu_observer_.get()) {
On 2012/02/03 07:57:24, msw wrote:
> nit: Drop the braces for a single-line if block.

Needs braces after change for next comment.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:679: if
(spelling_menu_observer_.get()) {
On 2012/02/03 07:57:24, msw wrote:
> Is this check necessary? The above conditional ensures this is non-NULL.
> If anything, maybe use a DCHECK instead?

I put in an else.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:685: if
(params_.is_editable)
On 2012/02/03 07:57:24, msw wrote:
> There's an if (params_.is_editable) block just above, please combine them.

Done.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1912: // Don't enable
the web inspector if JavaScript is disabled. We don't
On 2012/02/03 07:57:24, msw wrote:
> Why skip these checks for extensions? I prefer code simplicity over 'perf'
here.
> Then, in the non-views block below, just return false if (GetExtension()).

The code below doesn't work for popup extensions or platform apps as the
contents_wrapper returned by GetCurrentWrapperForContents is NULL.

In the non-Views case the desired behaviour is for platform apps to have the
command enabled but extension popups not to. I've made this a bit clearer by
using !is_platform_app instead of !is_app. These end up being the same as
GetExtension only returns an extension for platform apps and  popup extensions
but that isn't obvious.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1928: #if
defined(TOOLKIT_GTK) || defined(OS_MAC)
On 2012/02/03 07:57:24, msw wrote:
> Instead, check #if !defined(TOOLKIT_VIEWS)

Done.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1933: // TODO(benwells):
Add support for these builds and remote this #if.
On 2012/02/03 07:57:24, msw wrote:
> s/remote/remove

Done.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:92: details) {
On 2012/02/03 07:57:24, msw wrote:
> nit: Braces here are optional, not necessary.

OK. I think with the if conditional spread over two lines the braces improve
clarity. Unless you object I'll leave them in.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:100: // Set
inspect_with_devtools_ so the popup will be kept open while
On 2012/02/03 07:57:24, msw wrote:
> Does this work? I think you'll need to call set_close_on_deactivate(false);

Thanks, it will need that to work on chromeos and aura.

Done.

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:104: registrar_.Add(this,
content::NOTIFICATION_DEVTOOLS_WINDOW_CLOSING,
On 2012/02/03 07:57:24, msw wrote:
> optional: consider replacing this statement and the duplicate in
> ExtensionPopup::ShowBubble with an unconditional call in the ctor.

Done.

[benwells, 2012-02-06 04:31:26]

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/29002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:679: if
(spelling_menu_observer_.get()) {
On 2012/02/06 03:40:46, benwells wrote:
> On 2012/02/03 07:57:24, msw wrote:
> > Is this check necessary? The above conditional ensures this is non-NULL.
> > If anything, maybe use a DCHECK instead?
> 
> I put in an else.

Arg, just reread the original code. Will fix as suggested.

[msw, 2012-02-07 00:34:57]

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_browsertest.cc:168: // The context_menu
app has one context menu item. This is all that should
nit: remove "This is all that should be in the menu, " it doesn't make sense
when followed by other things that should be in the menu...

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:671: // Add a menu item
that shows suggestions.
This block is copied from RenderViewContextMenu::InitMenu().
Move it into AppendEditableItems or its own function.
Update the original block accordingly.

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:672: if
(!spelling_menu_observer_.get()) {
repeat nit: Drop the braces for a single-line if block.
I don't understand your earlier response:
"Needs braces after change for next comment."

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:676:
observers_.AddObserver(spelling_menu_observer_.get());
Can a RenderViewContextMenu be InitMenu'ed twice?

If not, spelling_menu_observer_ will always be NULL above (and in the duplicate
block). So there's no point in checking, it should just be set here.

If so, we're leaving stale/deleted entries of spelling_menu_observer_ in the
observers_ list :(

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:93: // Make sure its the
devtools window that inspecting our popup.
nit: Please change "its" to "it's" or "it is", thanks.

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:178: if
(inspect_with_devtools_) {
nit: you can remove these braces, if you'd like.

[benwells, 2012-02-08 03:49:16]

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_browsertest.cc:168: // The context_menu
app has one context menu item. This is all that should
On 2012/02/07 00:34:57, msw wrote:
> nit: remove "This is all that should be in the menu, " it doesn't make sense
> when followed by other things that should be in the menu...

Done.

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:671: // Add a menu item
that shows suggestions.
On 2012/02/07 00:34:57, msw wrote:
> This block is copied from RenderViewContextMenu::InitMenu().
> Move it into AppendEditableItems or its own function.
> Update the original block accordingly.

OK I've refactored it somewhat. There is still code which is common to this
function and InitMenu, but I'm hesitant to factorize further.

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:672: if
(!spelling_menu_observer_.get()) {
On 2012/02/07 00:34:57, msw wrote:
> repeat nit: Drop the braces for a single-line if block.
> I don't understand your earlier response:
> "Needs braces after change for next comment."

Yeah sorry that comment didn't make sense after the last patch.

Braces are now gone in the final resting place of this code.

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:676:
observers_.AddObserver(spelling_menu_observer_.get());
On 2012/02/07 00:34:57, msw wrote:
> Can a RenderViewContextMenu be InitMenu'ed twice?
> 
> If not, spelling_menu_observer_ will always be NULL above (and in the
duplicate
> block). So there's no point in checking, it should just be set here.
> 
> If so, we're leaving stale/deleted entries of spelling_menu_observer_ in the
> observers_ list :(

I've removed the DCHECK. This doesn't change the logic of the original code
which I'm assuming is correct.

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/9235002/diff/29004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_popup.cc:93: // Make sure its the
devtools window that inspecting our popup.
On 2012/02/07 00:34:57, msw wrote:
> nit: Please change "its" to "it's" or "it is", thanks.

Done.

[msw, 2012-02-08 04:04:42]

LGTM

[Avi (use Gerrit), 2012-02-08 04:26:45]

lgtm

http://codereview.chromium.org/9235002/diff/38001/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/9235002/diff/38001/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_browsertest.cc:169: // seperator and the
developer tools, is all that should be in the menu.
separator

[commit-bot: I haz the power, 2012-02-08 08:34:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9235002/40003

[commit-bot: I haz the power, 2012-02-08 09:38:31]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 120955, job name
was 9235002-40003 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-02-08 22:37:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9235002/40003

[commit-bot: I haz the power, 2012-02-09 01:55:36]

Change committed as 121145