DevTools: Extract target discovery and manipulation from DevToolsHttpHandlerImpl

DevTools: Extract target discovery and manipulation from DevToolsHttpHandlerImpl

Target discovery has moved into DevToolsHttpHandlerDelegate. 

The new DevToolsTarget interface is introduced to encapsulate
access to target details (id, title, url etc) and manipulation 
(activate, close, inspect).

It is now possible to have an implementation of DevToolsTarget
tied to a tab id instead of a RenderViewHost instance (which is the
prerequisite for fixing the referenced bug).

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

[Vladislav Kaznacheev, 2013-09-30 07:10:55]

PTAL

[pfeldman, 2013-09-30 12:25:46]

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
File content/public/browser/devtools_http_handler_delegate.h (right):

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
content/public/browser/devtools_http_handler_delegate.h:40: // Returns true if
this delegate supports page thumbnails.
Can we assume this is always the case?

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
content/public/browser/devtools_http_handler_delegate.h:45: virtual std::string
GetPageThumbnailData(const std::string& id) = 0;
Should this refer to targets using const Target& instead?

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
content/public/browser/devtools_http_handler_delegate.h:48: virtual Target*
CreateNewTarget() = 0;
I'd rather use value type for the target.

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
File content/public/browser/devtools_target_descriptor.h (right):

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
content/public/browser/devtools_target_descriptor.h:14: #include
"content/public/browser/worker_service.h"
Forward declaration should be enough.

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
content/public/browser/devtools_target_descriptor.h:24: class CONTENT_EXPORT
DevToolsTargetDescriptor {
Why is this not a struct? It does not really need to have methods.

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
content/public/browser/devtools_target_descriptor.h:32: static
DevToolsTargetDescriptor* FromWorkerInfo(
It is weird that content defers descriptor creation to embedder - is there any
extra information embedder can provide?

https://codereview.chromium.org/24995003/diff/6001/content/public/browser/dev...
content/public/browser/devtools_target_descriptor.h:36: virtual std::string
GetId() const = 0;
Either DevToolsAgentHost or the descriptor needs to have public id, but not
both. In fact, you might get away with no ids at all.

[Vladislav Kaznacheev, 2013-10-01 13:42:35]

PTAL

[pfeldman, 2013-10-01 14:10:24]

lgtm

https://chromiumcodereview.appspot.com/24995003/diff/11001/chrome/browser/dev...
File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/chrome/browser/dev...
chrome/browser/devtools/browser_list_tabcontents_provider.cc:253: this,
indent

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
File content/browser/devtools/devtools_http_handler_impl.cc (left):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:602: Release();  //
Balanced OnJsonRequestUI.
Restore

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:206: GURL
DevToolsHttpHandlerImpl::GetFrontendURL(DevToolsAgentHost* agent_host) {
nuke agent_host param

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:261: DevToolsTarget*
target = GetTarget(target_id);
scoped_refptr<>

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:495: if
(target->Activate())
{}

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:497: else
{}

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:506: if (target->Close())
{}

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:508: else
{}

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:745: if
(!delegate_->GetPageThumbnailData(url).empty())
{}

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
File content/public/browser/devtools_http_handler_delegate.h (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
content/public/browser/devtools_http_handler_delegate.h:23: : public
base::RefCountedThreadSafe<DevToolsHttpHandlerDelegate> {
Could we keep it raw?

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
File content/public/browser/devtools_target.h (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
content/public/browser/devtools_target.h:20: : public
base::RefCountedThreadSafe<DevToolsTarget> {
No need to be thread safe.

[Vladislav Kaznacheev, 2013-10-01 15:00:16]

https://chromiumcodereview.appspot.com/24995003/diff/11001/chrome/browser/dev...
File chrome/browser/devtools/browser_list_tabcontents_provider.cc (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/chrome/browser/dev...
chrome/browser/devtools/browser_list_tabcontents_provider.cc:253: this,
On 2013/10/01 14:10:24, pfeldman wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
File content/browser/devtools/devtools_http_handler_impl.cc (left):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:602: Release();  //
Balanced OnJsonRequestUI.
On 2013/10/01 14:10:24, pfeldman wrote:
> Restore

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:206: GURL
DevToolsHttpHandlerImpl::GetFrontendURL(DevToolsAgentHost* agent_host) {
On 2013/10/01 14:10:24, pfeldman wrote:
> nuke agent_host param

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:261: DevToolsTarget*
target = GetTarget(target_id);
On 2013/10/01 14:10:24, pfeldman wrote:
> scoped_refptr<>

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:495: if
(target->Activate())
On 2013/10/01 14:10:24, pfeldman wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:497: else
On 2013/10/01 14:10:24, pfeldman wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:506: if (target->Close())
On 2013/10/01 14:10:24, pfeldman wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:508: else
On 2013/10/01 14:10:24, pfeldman wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/browser/de...
content/browser/devtools/devtools_http_handler_impl.cc:745: if
(!delegate_->GetPageThumbnailData(url).empty())
On 2013/10/01 14:10:24, pfeldman wrote:
> {}

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
File content/public/browser/devtools_http_handler_delegate.h (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
content/public/browser/devtools_http_handler_delegate.h:23: : public
base::RefCountedThreadSafe<DevToolsHttpHandlerDelegate> {
On 2013/10/01 14:10:24, pfeldman wrote:
> Could we keep it raw?

Done.

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
File content/public/browser/devtools_target.h (right):

https://chromiumcodereview.appspot.com/24995003/diff/11001/content/public/bro...
content/public/browser/devtools_target.h:20: : public
base::RefCountedThreadSafe<DevToolsTarget> {
On 2013/10/01 14:10:24, pfeldman wrote:
> No need to be thread safe.

Done.

[Vladislav Kaznacheev, 2013-10-01 15:15:56]

Hello OWNERS,

jam@, please review:
content/public/browser/devtools_http_handler.h
content/public/browser/devtools_http_handler_delegate.h
content/public/browser/devtools_target.h (new interface)

bulach@, please review:
chrome/browser/android/dev_tools_server.cc

mnaganov@, please review:
android_webview/native/aw_dev_tools_server.cc

Thanks!

  Vlad

[mnaganov (inactive), 2013-10-01 16:15:02]

Android parts LGTM except for the question about #includes

https://chromiumcodereview.appspot.com/24995003/diff/23001/android_webview/na...
File android_webview/native/aw_dev_tools_server.cc (right):

https://chromiumcodereview.appspot.com/24995003/diff/23001/android_webview/na...
android_webview/native/aw_dev_tools_server.cc:18: #include
"content/public/browser/navigation_entry.h"
Do we really need these two includes? I can't see if they are really used.

[bulach, 2013-10-01 16:44:55]

lgtm for chrome/browser/android/dev_tools_server.cc

[jam, 2013-10-01 17:20:50]

https://codereview.chromium.org/24995003/diff/23001/content/public/browser/de...
File content/public/browser/devtools_http_handler_delegate.h (right):

https://codereview.chromium.org/24995003/diff/23001/content/public/browser/de...
content/public/browser/devtools_http_handler_delegate.h:13: #include
"content/public/browser/devtools_target.h"
do you need this or can you just forward declare DevTooolsTarget?

https://codereview.chromium.org/24995003/diff/23001/content/public/browser/de...
content/public/browser/devtools_http_handler_delegate.h:25: typedef
base::Callback<void(const TargetList&)> TargetCallback;
nit: the convention for the content api is to list every typedef right above
where it's used. so line 24 would be above line 42, and line 25 would be above
45

https://codereview.chromium.org/24995003/diff/23001/content/public/browser/de...
File content/public/browser/devtools_target.h (right):

https://codereview.chromium.org/24995003/diff/23001/content/public/browser/de...
content/public/browser/devtools_target.h:14: #include
"content/public/browser/devtools_agent_host.h"
nit: forward declare instead

https://codereview.chromium.org/24995003/diff/23001/content/public/browser/de...
content/public/browser/devtools_target.h:19: class CONTENT_EXPORT DevToolsTarget
: public base::RefCounted<DevToolsTarget> {
1) why is this ref counted?
2) i dont think it needs to be exported since it's just an interface
3) add a comment about what this interface is used for

[Vladislav Kaznacheev, 2013-10-02 10:28:28]

https://chromiumcodereview.appspot.com/24995003/diff/23001/android_webview/na...
File android_webview/native/aw_dev_tools_server.cc (right):

https://chromiumcodereview.appspot.com/24995003/diff/23001/android_webview/na...
android_webview/native/aw_dev_tools_server.cc:18: #include
"content/public/browser/navigation_entry.h"
We do not. That was copypasta.

On 2013/10/01 16:15:02, Mikhail Naganov (Chromium) wrote:
> Do we really need these two includes? I can't see if they are really used.

https://chromiumcodereview.appspot.com/24995003/diff/23001/content/public/bro...
File content/public/browser/devtools_http_handler_delegate.h (right):

https://chromiumcodereview.appspot.com/24995003/diff/23001/content/public/bro...
content/public/browser/devtools_http_handler_delegate.h:13: #include
"content/public/browser/devtools_target.h"
On 2013/10/01 17:20:51, jam wrote:
> do you need this or can you just forward declare DevTooolsTarget?

Used forward declaration.

https://chromiumcodereview.appspot.com/24995003/diff/23001/content/public/bro...
content/public/browser/devtools_http_handler_delegate.h:25: typedef
base::Callback<void(const TargetList&)> TargetCallback;
On 2013/10/01 17:20:51, jam wrote:
> nit: the convention for the content api is to list every typedef right above
> where it's used. so line 24 would be above line 42, and line 25 would be above
> 45

Done.

https://chromiumcodereview.appspot.com/24995003/diff/23001/content/public/bro...
File content/public/browser/devtools_target.h (right):

https://chromiumcodereview.appspot.com/24995003/diff/23001/content/public/bro...
content/public/browser/devtools_target.h:14: #include
"content/public/browser/devtools_agent_host.h"
On 2013/10/01 17:20:51, jam wrote:
> nit: forward declare instead

Done.

https://chromiumcodereview.appspot.com/24995003/diff/23001/content/public/bro...
content/public/browser/devtools_target.h:19: class CONTENT_EXPORT DevToolsTarget
: public base::RefCounted<DevToolsTarget> {
ref counted no longer, CONTENT_EXPORT removed, comment added

On 2013/10/01 17:20:51, jam wrote:
> 1) why is this ref counted?
> 2) i dont think it needs to be exported since it's just an interface
> 3) add a comment about what this interface is used for

Done.

[jam, 2013-10-02 16:58:50]

content/public lgtm

[jam, 2013-10-02 16:59:08]

On 2013/10/02 16:58:50, jam wrote:
> content/public lgtm

and gypi

[commit-bot: I haz the power, 2013-10-02 18:31:38]

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

[commit-bot: I haz the power, 2013-10-02 19:24:17]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-10-03 07:06:19]

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

[commit-bot: I haz the power, 2013-10-03 07:28:27]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-10-03 08:15:24]

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

[commit-bot: I haz the power, 2013-10-03 09:08:43]

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

[commit-bot: I haz the power, 2013-10-03 16:29:08]

Change committed as 226771