Add "inspect background page" to platform app right-click menu.

Add "inspect background page" to platform app right-click menu.

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

[Marijn Kruisselbrink, 2012-09-14 21:15:17]

http://codereview.chromium.org/10907243/diff/1/chrome/browser/tab_contents/re...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/10907243/diff/1/chrome/browser/tab_contents/re...
chrome/browser/tab_contents/render_view_context_menu.cc:1746: DCHECK(host);
Antony: is this safe, or are there possible situations where a platform app
won't have a running background page?

[asargent_no_longer_on_chrome, 2012-09-14 21:57:41]

nit: can you include a test case where a non-LOAD (ie installed from crx) app is
installed, and assert that the inspect bg page item is *not* in the context menu
for that app?

http://codereview.chromium.org/10907243/diff/1/chrome/browser/tab_contents/re...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/10907243/diff/1/chrome/browser/tab_contents/re...
chrome/browser/tab_contents/render_view_context_menu.cc:1746: DCHECK(host);
On 2012/09/14 21:15:18, Marijn Kruisselbrink wrote:
> Antony: is this safe, or are there possible situations where a platform app
> won't have a running background page?

Let's refactor out the C++ code that the chrome://extensions page dispatches to
since it already handles starting up the background page if needed. That's at:

ExtensionSettingsHandler::HandleInspectMessage in 

chrome/src/chrome/browser/ui/webui/extensions/extension_settings_handler.cc

A good location might be something like:

ExtensionService::inspectBackgroundPage(const std::string& id)

[asargent_no_longer_on_chrome, 2012-09-14 21:58:14]

On 2012/09/14 21:57:41, Antony Sargent wrote:
> nit: can you include a test case where a non-LOAD (ie installed from crx) app
is
> installed, and assert that the inspect bg page item is *not* in the context
menu
> for that app?
> 
>
http://codereview.chromium.org/10907243/diff/1/chrome/browser/tab_contents/re...
> File chrome/browser/tab_contents/render_view_context_menu.cc (right):
> 
>
http://codereview.chromium.org/10907243/diff/1/chrome/browser/tab_contents/re...
> chrome/browser/tab_contents/render_view_context_menu.cc:1746: DCHECK(host);
> On 2012/09/14 21:15:18, Marijn Kruisselbrink wrote:
> > Antony: is this safe, or are there possible situations where a platform app
> > won't have a running background page?
> 
> Let's refactor out the C++ code that the chrome://extensions page dispatches
to
> since it already handles starting up the background page if needed. That's at:
> 
> ExtensionSettingsHandler::HandleInspectMessage in 
> 
> chrome/src/chrome/browser/ui/webui/extensions/extension_settings_handler.cc
> 
> A good location might be something like:
> 
> ExtensionService::inspectBackgroundPage(const std::string& id)

Sorry, I guess the name should start with capital 'I', so InspectBackgroundPage.

[Mihai Parparita -not on Chrome, 2012-09-16 05:05:53]

On Fri, Sep 14, 2012 at 2:57 PM, <asargent@chromium.org> wrote:

> On 2012/09/14 21:15:18, Marijn Kruisselbrink wrote:
>
>> Antony: is this safe, or are there possible situations where a
>>
> platform app
>
>> won't have a running background page?
>>
>
> Let's refactor out the C++ code that the chrome://extensions page
> dispatches to since it already handles starting up the background page
> if needed. That's at:
>
> ExtensionSettingsHandler::**HandleInspectMessage in
>
> chrome/src/chrome/browser/ui/**webui/extensions/extension_**
> settings_handler.cc
>
> A good location might be something like:
>
> ExtensionService::**inspectBackgroundPage(const std::string& id)
>

This is a good refactoring to do anyway, but the background page should
always be running if we're showing a platform app window (app windows can
only be created by a background page, and while windows are visible, the
background page won't be unloaded).

Mihai

[Marijn Kruisselbrink, 2012-09-19 16:53:48]

On 2012/09/14 21:57:41, Antony Sargent wrote:
> nit: can you include a test case where a non-LOAD (ie installed from crx) app
is
> installed, and assert that the inspect bg page item is *not* in the context
menu
> for that app?
Done.

> Let's refactor out the C++ code that the chrome://extensions page dispatches
to
> since it already handles starting up the background page if needed.
Done.

[asargent_no_longer_on_chrome, 2012-09-19 17:06:14]

LGTM

http://codereview.chromium.org/10907243/diff/12001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10907243/diff/12001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:516: +        <message
name="IDS_CONTENT_CONTEXT_INSPECTBACKGROUNDPAGE" desc="The name of the Inspect
Background Page command in the packaged app content area context menu">
"IDS_CONTENT_CONTEXT_INSPECT_BACKGROUND_PAGE" would be slightly easier to read,
so might be a little better, unless that differs from the prevailing naming
conventions.

http://codereview.chromium.org/10907243/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10907243/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:216: // separator and the
developer tools, is all that should be in the menu.
comment nit: actually we're asserting that the developer tools are *not* in the
menu, right?

[Marijn Kruisselbrink, 2012-09-19 17:33:23]

brettw: can you review/approve the chrome/app/chrome_command_ids.h and
chrome/browser/tab_contents/ changes?

http://codereview.chromium.org/10907243/diff/12001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10907243/diff/12001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:516: +        <message
name="IDS_CONTENT_CONTEXT_INSPECTBACKGROUNDPAGE" desc="The name of the Inspect
Background Page command in the packaged app content area context menu">
On 2012/09/19 17:06:14, Antony Sargent wrote:
> "IDS_CONTENT_CONTEXT_INSPECT_BACKGROUND_PAGE" would be slightly easier to
read,
> so might be a little better, unless that differs from the prevailing naming
> conventions. 
I'm not sure there is a prevailing naming convention... there seem to be about
an equal amount of IDS_CONTENT_CONTEXT_* ids with and without underscores
between words in the last part of the name. INSPECTELEMENT doesn't use
underscores.

http://codereview.chromium.org/10907243/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10907243/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:216: // separator and the
developer tools, is all that should be in the menu.
On 2012/09/19 17:06:14, Antony Sargent wrote:
> comment nit: actually we're asserting that the developer tools are *not* in
the
> menu, right?
Ah yes, the danger of copy-pasting code... Fixed.

[brettw, 2012-09-19 20:58:50]

lgtm

[commit-bot: I haz the power, 2012-09-19 21:05:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10907243/16002

[commit-bot: I haz the power, 2012-09-19 23:05:36]

Change committed as 157645