Allow chrome.debugger API to attach to extension background pages.

Allow chrome.debugger API to attach to extension background pages.

Since there is no way to provide a visual notification for the user this will be only possible when --remote-debugging-silent command line switch is present.

BUG=176962
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184645

[Vladislav Kaznacheev, 2013-02-19 15:23:11]

Please review.

[pfeldman, 2013-02-20 10:01:05]

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:426: std::string
DebuggerFunction::GetTargetTypeString() {
You only use these two to report the error. You could consolidate them into one
+ actually make this utility method report the error.

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:440: WebContents*
FindWebContentsForExtension(
static

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:442: for
(RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
Ok, this is the 3rd time it is used for inspector. How about we introduce
RenderViewHosts::AllHostsIterator?

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:478: WebContents*
web_contents = NULL;
This could be extracted as FindWebContentsForTabId

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:575: new
ExtensionDevToolsClientHost(contents_,
Can't you clone the target? Isn't it the Debuggee on its own?

[Vladislav Kaznacheev, 2013-02-20 12:02:26]

PTAL

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:426: std::string
DebuggerFunction::GetTargetTypeString() {
On 2013/02/20 10:01:05, pfeldman wrote:
> You only use these two to report the error. You could consolidate them into
one
> + actually make this utility method report the error.

Done.

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:440: WebContents*
FindWebContentsForExtension(
On 2013/02/20 10:01:05, pfeldman wrote:
> static

Done.

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:442: for
(RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
Filed a separate bug for that:
https://code.google.com/p/chromium/issues/detail?id=177180


On 2013/02/20 10:01:05, pfeldman wrote:
> Ok, this is the 3rd time it is used for inspector. How about we introduce
> RenderViewHosts::AllHostsIterator?

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:478: WebContents*
web_contents = NULL;
As discussed offline it does not really provide any benefit.
On 2013/02/20 10:01:05, pfeldman wrote:
> This could be extracted as FindWebContentsForTabId

https://codereview.chromium.org/12304021/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/debugger/debugger_api.cc:575: new
ExtensionDevToolsClientHost(contents_,
I tried. The copy constructor of this class is private.

On 2013/02/20 10:01:05, pfeldman wrote:
> Can't you clone the target? Isn't it the Debuggee on its own?

[Vladislav Kaznacheev, 2013-02-20 13:36:36]

PTAL. Added a command line switch and a separate test for debugging an
extension.

[pfeldman, 2013-02-20 14:58:39]

https://codereview.chromium.org/12304021/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/12304021/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/debugger/debugger_api.cc:425: debuggee_tab_id_(0),
debuggee_

https://codereview.chromium.org/12304021/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/debugger/debugger_api.cc:574: debuggee_tab_id_,
Pass debuggee_ here and clone it in the constructor.

https://codereview.chromium.org/12304021/diff/8001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/debugger/background.js (right):

https://codereview.chromium.org/12304021/diff/8001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/debugger/background.js:109: function
attachToExtension() {
attachToExtensionWithNoSilentFlag

[Vladislav Kaznacheev, 2013-02-20 15:24:50]

https://codereview.chromium.org/12304021/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/12304021/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/debugger/debugger_api.cc:425: debuggee_tab_id_(0),
On 2013/02/20 14:58:39, pfeldman wrote:
> debuggee_

Done.

https://codereview.chromium.org/12304021/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/debugger/debugger_api.cc:574: debuggee_tab_id_,
On 2013/02/20 14:58:39, pfeldman wrote:
> Pass debuggee_ here and clone it in the constructor.

Done.

https://codereview.chromium.org/12304021/diff/8001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/debugger/background.js (right):

https://codereview.chromium.org/12304021/diff/8001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/debugger/background.js:109: function
attachToExtension() {
On 2013/02/20 14:58:39, pfeldman wrote:
> attachToExtensionWithNoSilentFlag

Done.

[Vladislav Kaznacheev, 2013-02-20 15:37:41]

Hi Matt,

Please review this patch to the chrome.debugger API.

Thanks,

  Vlad

[pfeldman, 2013-02-20 15:43:17]

lgtm

[Matt Perry, 2013-02-20 19:52:01]

What's the use case for this? If we hide it behind a flag, no one is going to
use it.

https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
chrome/browser/extensions/api/debugger/debugger_api.cc:439: static WebContents*
FindWebContentsForExtension(
There can be multiple WebContents for an extension. If you're looking for the
background page, use ExtensionProcessManager::GetBackgroundHostForExtension.

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
File chrome/common/extensions/api/debugger.json (right):

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
chrome/common/extensions/api/debugger.json:15: "extensionId": { "type":
"string", "optional": true, "description": "The id of the extension which you
intend to debug." }
add a "nodoc": true field to this - since you need a command line flag, it seems
like this version has very limited use.

[Vladislav Kaznacheev, 2013-02-21 09:39:09]

https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
chrome/browser/extensions/api/debugger/debugger_api.cc:439: static WebContents*
FindWebContentsForExtension(
Thanks for the pointer! Done.
On 2013/02/20 19:52:01, Matt Perry wrote:
> There can be multiple WebContents for an extension. If you're looking for the
> background page, use ExtensionProcessManager::GetBackgroundHostForExtension.

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
File chrome/common/extensions/api/debugger.json (right):

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
chrome/common/extensions/api/debugger.json:15: "extensionId": { "type":
"string", "optional": true, "description": "The id of the extension which you
intend to debug." }
This API is already a sort of a "specialist" one used by developer tools authors
(I am aware of exactly one external user). I think having this property
documented is not likely to confuse them more than they already are :)

On 2013/02/20 19:52:01, Matt Perry wrote:
> add a "nodoc": true field to this - since you need a command line flag, it
seems
> like this version has very limited use.

[Vladislav Kaznacheev, 2013-02-21 09:40:52]

The use case for this is attaching the debugger to the extension background
page. This can be useful both for external development tools (JetBrains
WebStorm) and for various Chrome automation scenarios.

On 2013/02/20 19:52:01, Matt Perry wrote:
> What's the use case for this? If we hide it behind a flag, no one is going to
> use it.
> 
>
https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
> File chrome/browser/extensions/api/debugger/debugger_api.cc (right):
> 
>
https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
> chrome/browser/extensions/api/debugger/debugger_api.cc:439: static
WebContents*
> FindWebContentsForExtension(
> There can be multiple WebContents for an extension. If you're looking for the
> background page, use ExtensionProcessManager::GetBackgroundHostForExtension.
> 
>
https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
> File chrome/common/extensions/api/debugger.json (right):
> 
>
https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
> chrome/common/extensions/api/debugger.json:15: "extensionId": { "type":
> "string", "optional": true, "description": "The id of the extension which you
> intend to debug." }
> add a "nodoc": true field to this - since you need a command line flag, it
seems
> like this version has very limited use.

[Matt Perry, 2013-02-21 18:35:27]

I can understand the automation use case, but for external tools, a commandline
flag is usually quite prohibitive. Very few users will go to the trouble of
enabling the flag. Then again, I suppose this API is targeted towards more savvy
users who might be more likely to edit commandline flags.

Do you intend for this to be usable on ChromeOS? If so, you should add an
about:flags entry, since it's the only way ChromeOS users can change their
flags.

On 2013/02/21 09:40:52, Vladislav Kaznacheev wrote:
> The use case for this is attaching the debugger to the extension background
> page. This can be useful both for external development tools (JetBrains
> WebStorm) and for various Chrome automation scenarios.
> 
> On 2013/02/20 19:52:01, Matt Perry wrote:
> > What's the use case for this? If we hide it behind a flag, no one is going
to
> > use it.
> > 
> >
>
https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
> > File chrome/browser/extensions/api/debugger/debugger_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/12304021/diff/6007/chrome/browser/extensions/...
> > chrome/browser/extensions/api/debugger/debugger_api.cc:439: static
> WebContents*
> > FindWebContentsForExtension(
> > There can be multiple WebContents for an extension. If you're looking for
the
> > background page, use ExtensionProcessManager::GetBackgroundHostForExtension.
> > 
> >
>
https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
> > File chrome/common/extensions/api/debugger.json (right):
> > 
> >
>
https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
> > chrome/common/extensions/api/debugger.json:15: "extensionId": { "type":
> > "string", "optional": true, "description": "The id of the extension which
you
> > intend to debug." }
> > add a "nodoc": true field to this - since you need a command line flag, it
> seems
> > like this version has very limited use.

[Matt Perry, 2013-02-21 18:35:33]

lgtm

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
File chrome/common/extensions/api/debugger.json (right):

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
chrome/common/extensions/api/debugger.json:15: "extensionId": { "type":
"string", "optional": true, "description": "The id of the extension which you
intend to debug." }
On 2013/02/21 09:39:09, Vladislav Kaznacheev wrote:
> This API is already a sort of a "specialist" one used by developer tools
authors
> (I am aware of exactly one external user). I think having this property
> documented is not likely to confuse them more than they already are :)
> 
> On 2013/02/20 19:52:01, Matt Perry wrote:
> > add a "nodoc": true field to this - since you need a command line flag, it
> seems
> > like this version has very limited use.
> 

OK, could you update the description to note that it won't work without the
flag?

[Vladislav Kaznacheev, 2013-02-26 10:55:38]

Pavel, PTAL

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
File chrome/common/extensions/api/debugger.json (right):

https://codereview.chromium.org/12304021/diff/6007/chrome/common/extensions/a...
chrome/common/extensions/api/debugger.json:15: "extensionId": { "type":
"string", "optional": true, "description": "The id of the extension which you
intend to debug." }
Added a flag to chrome:flags and mentioned it in the "extensionID" property
description.
On 2013/02/21 18:35:33, Matt Perry wrote:
> On 2013/02/21 09:39:09, Vladislav Kaznacheev wrote:
> > This API is already a sort of a "specialist" one used by developer tools
> authors
> > (I am aware of exactly one external user). I think having this property
> > documented is not likely to confuse them more than they already are :)
> > 
> > On 2013/02/20 19:52:01, Matt Perry wrote:
> > > add a "nodoc": true field to this - since you need a command line flag, it
> > seems
> > > like this version has very limited use.
> > 
> 
> OK, could you update the description to note that it won't work without the
> flag?

[pfeldman, 2013-02-26 11:33:12]

lgtm

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/about_flag...
chrome/browser/about_flags.cc:792: "enable-silent-debugging",
enable-silent-chrome.debugger-extension-api-use

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/api/debugger/debugger_api_constants.cc (right):

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/api/debugger/debugger_api_constants.cc:24: "Cannot
attach to an extension, --* command line option required.";
It is now a part of about flags.

https://codereview.chromium.org/12304021/diff/14013/chrome/common/extensions/...
File chrome/common/extensions/api/debugger.json (right):

https://codereview.chromium.org/12304021/diff/14013/chrome/common/extensions/...
chrome/common/extensions/api/debugger.json:14: "tabId": { "type": "integer",
"optional": true, "description": "The id of the tab which you intend to debug."
},
that

[Vladislav Kaznacheev, 2013-02-26 11:43:22]

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/about_flag...
chrome/browser/about_flags.cc:792: "enable-silent-debugging",
Renamed to "silent-debugger-extension-api"

On 2013/02/26 11:33:12, pfeldman wrote:
> enable-silent-chrome.debugger-extension-api-use

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/api/debugger/debugger_api_constants.cc (right):

https://codereview.chromium.org/12304021/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/api/debugger/debugger_api_constants.cc:24: "Cannot
attach to an extension, --* command line option required.";
On 2013/02/26 11:33:12, pfeldman wrote:
> It is now a part of about flags.

Done.

[commit-bot: I haz the power, 2013-02-26 11:46:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12304021/13020

[commit-bot: I haz the power, 2013-02-26 14:28:17]

Change committed as 184645