Support shared workers as debug targets for chrome.debugger API

Support shared workers as debug targets for chrome.debugger API


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

[yurys, 2013-04-03 14:19:57]

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:181:
static_cast<ExtensionDevToolsClientHost*>(*it);
You can change type of the set elements DevToolsClientHost* ->
ExtensionDevToolsClientHost* to avoid this cast.

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.h (right):

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.h:42:
content::DevToolsAgentHost* agent_host_;
Should it be scoped_ref<DevToolsAgentHost> ?

[Vladislav Kaznacheev, 2013-04-03 14:54:36]

PTAL

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:181:
static_cast<ExtensionDevToolsClientHost*>(*it);
On 2013/04/03 14:19:57, Yury Semikhatsky wrote:
> You can change type of the set elements DevToolsClientHost* ->
> ExtensionDevToolsClientHost* to avoid this cast.

Done.

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.h (right):

https://chromiumcodereview.appspot.com/13517002/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.h:42:
content::DevToolsAgentHost* agent_host_;
It is guaranteed to live long enough, but I agree that scoped_refptr is safer.
On 2013/04/03 14:19:57, Yury Semikhatsky wrote:
> Should it be scoped_ref<DevToolsAgentHost> ?

[yurys, 2013-04-04 09:58:09]

lgtm

[Vladislav Kaznacheev, 2013-04-04 10:18:06]

Hi Matt,

Here is the final patch in the series of updates to chrome.debugger ('final'
meaning there would be no more coming from me for some time).

The change to the API is trivial, but the implementation required some
non-trivial refactoring in debugger_api.cc

Thanks,

  Vlad

[Matt Perry, 2013-04-04 19:10:59]

mostly lgtm, 2 small nits

https://chromiumcodereview.appspot.com/13517002/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/13517002/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/debugger/debugger_api.cc:119: std::string
extension_id() { return extension_id_; }
return const ref

https://chromiumcodereview.appspot.com/13517002/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/debugger/debugger_api.cc:267:
UTF16ToUTF8(net::EscapeForHTML(worker.name)));
why does this need to be escaped? it's just used as a string, right?

[Vladislav Kaznacheev, 2013-04-05 09:23:02]

Thanks for the review!

https://chromiumcodereview.appspot.com/13517002/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/13517002/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/debugger/debugger_api.cc:119: std::string
extension_id() { return extension_id_; }
On 2013/04/04 19:11:00, Matt Perry wrote:
> return const ref

Done.

https://chromiumcodereview.appspot.com/13517002/diff/11001/chrome/browser/ext...
chrome/browser/extensions/api/debugger/debugger_api.cc:267:
UTF16ToUTF8(net::EscapeForHTML(worker.name)));
It does not really. This code was originally copied from inspect_ui.cc where the
final destination was a web page. Removed conversion here and for regular
targets as well.
On 2013/04/04 19:11:00, Matt Perry wrote:
> why does this need to be escaped? it's just used as a string, right?

[commit-bot: I haz the power, 2013-04-05 09:24:39]

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

[commit-bot: I haz the power, 2013-04-05 09:24:47]

Presubmit check for 13517002-19001 failed and returned exit status 1.

INFO:root:Found 11 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/test/data/extensions/api_test/debugger/worker.js \
  chrome/test/data/extensions/api_test/debugger/worker_body.js \
  chrome/test/data/extensions/api_test/debugger_extension/worker.js \
  chrome/test/data/extensions/api_test/debugger_extension/worker_body.js

Presubmit checks took 5.3s to calculate.

[commit-bot: I haz the power, 2013-04-05 09:37:45]

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

[commit-bot: I haz the power, 2013-04-05 12:05:16]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-05 13:34:39]

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

[commit-bot: I haz the power, 2013-04-05 15:52:01]

Change committed as 192569