[DevTools] Move target-related methods from DevToolsHttpHandlerDelegate to DevToolsManagerDelegate.

[DevTools] Move target-related methods from DevToolsHttpHandlerDelegate to DevToolsManagerDelegate.

This decouples targets discovery from remote debugging in preparation
of moving discovery to the protocol.

BUG=398049
Committed: https://crrev.com/252e18d2c4116131af20cac7759141ea930dcd28
Cr-Commit-Position: refs/heads/master@{#295953}

[dgozman, 2014-09-11 16:41:24]

dgozman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[dgozman, 2014-09-11 16:41:25]

Could you please take a look?

[pfeldman, 2014-09-11 16:43:24]

pfeldman@chromium.org changed reviewers:
+ vkuzkokov@chromium.org

[pfeldman, 2014-09-11 17:42:35]

https://codereview.chromium.org/560323005/diff/1/android_webview/browser/aw_d...
File android_webview/browser/aw_dev_tools_manager_delegate.cc (right):

https://codereview.chromium.org/560323005/diff/1/android_webview/browser/aw_d...
android_webview/browser/aw_dev_tools_manager_delegate.cc:23: const char
kTargetTypePage[] = "page";
Should we define those in the content/public?

https://codereview.chromium.org/560323005/diff/1/chrome/browser/devtools/chro...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/560323005/diff/1/chrome/browser/devtools/chro...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:159: #if
defined(OS_ANDROID)
Why these checks? Don't we have a separate implementation for Android now?

[dgozman, 2014-09-11 17:48:10]

https://codereview.chromium.org/560323005/diff/1/android_webview/browser/aw_d...
File android_webview/browser/aw_dev_tools_manager_delegate.cc (right):

https://codereview.chromium.org/560323005/diff/1/android_webview/browser/aw_d...
android_webview/browser/aw_dev_tools_manager_delegate.cc:23: const char
kTargetTypePage[] = "page";
On 2014/09/11 17:42:35, pfeldman wrote:
> Should we define those in the content/public?

We discussed this with Vlad when he did recent refactoring, and decided against
it.

https://codereview.chromium.org/560323005/diff/1/chrome/browser/devtools/chro...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/560323005/diff/1/chrome/browser/devtools/chro...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:159: #if
defined(OS_ANDROID)
On 2014/09/11 17:42:35, pfeldman wrote:
> Why these checks? Don't we have a separate implementation for Android now?

We actually need network-related stuff from this file on Android too, so I
inherited from this class on Android.
ifdef is required so this code is not compiled on Android (otherwise it does not
link). I don't feel really good about this. Probably worth extracting network
parts from this and have two separate delegates.

[pfeldman, 2014-09-12 18:24:56]

Lets extract it?

[dgozman, 2014-09-13 04:47:47]

On 2014/09/12 18:24:56, pfeldman wrote:
> Lets extract it?

Posted a patch with extracting: https://codereview.chromium.org/563923006/.

[dgozman, 2014-09-15 12:31:11]

Patchset #2 (id:20001) has been deleted

[dgozman, 2014-09-15 12:32:49]

Rebased on top of 294798, which extracted network from
ChromeDevToolsManagerDelegate. PTAL

[dgozman, 2014-09-15 13:32:44]

Patchset #4 (id:80001) has been deleted

[dgozman, 2014-09-15 14:14:57]

Patchset #4 (id:100001) has been deleted

[vkuzkokov, 2014-09-16 11:46:40]

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:35: #include
"chrome/browser/devtools/chrome_devtools_manager_delegate.h"
chrome_devtools_manager_delegate.h and .cc should be removed from Android
builds.

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:33: #if
!defined(OS_ANDROID)
This #if becomes redundant.

[dgozman, 2014-09-16 13:23:57]

PTAL

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:35: #include
"chrome/browser/devtools/chrome_devtools_manager_delegate.h"
On 2014/09/16 11:46:40, vkuzkokov wrote:
> chrome_devtools_manager_delegate.h and .cc should be removed from Android
> builds.

Done.

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/devtools...
File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/560323005/diff/120001/chrome/browser/devtools...
chrome/browser/devtools/chrome_devtools_manager_delegate.cc:33: #if
!defined(OS_ANDROID)
On 2014/09/16 11:46:40, vkuzkokov wrote:
> This #if becomes redundant.

Done.

[vkuzkokov, 2014-09-16 15:45:26]

On 2014/09/16 13:23:57, dgozman wrote:
> PTAL
> 
>
https://codereview.chromium.org/560323005/diff/120001/chrome/browser/chrome_c...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/560323005/diff/120001/chrome/browser/chrome_c...
> chrome/browser/chrome_content_browser_client.cc:35: #include
> "chrome/browser/devtools/chrome_devtools_manager_delegate.h"
> On 2014/09/16 11:46:40, vkuzkokov wrote:
> > chrome_devtools_manager_delegate.h and .cc should be removed from Android
> > builds.
> 
> Done.
> 
>
https://codereview.chromium.org/560323005/diff/120001/chrome/browser/devtools...
> File chrome/browser/devtools/chrome_devtools_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/560323005/diff/120001/chrome/browser/devtools...
> chrome/browser/devtools/chrome_devtools_manager_delegate.cc:33: #if
> !defined(OS_ANDROID)
> On 2014/09/16 11:46:40, vkuzkokov wrote:
> > This #if becomes redundant.
> 
> Done.

lgtm

[pfeldman, 2014-09-18 11:45:58]

lgtm

https://codereview.chromium.org/560323005/diff/160001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/560323005/diff/160001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:32: static GURL
GetFaviconURLForContents(WebContents* web_contents) {
No need to declare as static since in an anonymous namespace.

https://codereview.chromium.org/560323005/diff/160001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/560323005/diff/160001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2511: return new
DevToolsManagerDelegateAndroid();
nit: ChromeDevToolsManager vs DevToolsManagerDelegateAndroid - the naming
pattern does not sound consistent.

[dgozman, 2014-09-19 10:04:34]

https://codereview.chromium.org/560323005/diff/160001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/560323005/diff/160001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:32: static GURL
GetFaviconURLForContents(WebContents* web_contents) {
On 2014/09/18 11:45:58, pfeldman wrote:
> No need to declare as static since in an anonymous namespace.

Done.

[dgozman, 2014-09-19 10:06:31]

dgozman@chromium.org changed reviewers:
+ bauerb@chromium.org, gunsch@chromium.org, jam@chromium.org,
mnaganov@chromium.org, rockot@chromium.org

[dgozman, 2014-09-19 10:06:32]

Hello, OWNERS. Could you please take a look?

@mnaganov:
  android_webview/
@bauerb:
  chrome/browser/android/
@gunsch:
  chromecast/
@jam:
  content/public/
@rockot:
  extensions/

Thanks,
Dmitry

[mnaganov (inactive), 2014-09-19 10:20:04]

android_webview/ LGTM

[gunsch, 2014-09-19 15:48:32]

https://codereview.chromium.org/560323005/diff/180001/chromecast/shell/browse...
File chromecast/shell/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/560323005/diff/180001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:155: return new
CastDevToolsManagerDelegate();
How does this affect the availability of remote devtools?

We currently manage the lifetime of remote devtools somewhat uniquely,
enabling/disabling in
chromecast/shell/browser/devtools/remote_debugging_server.cc via
content::DevToolsHttpHandler::{Start,Stop}.

This is because, on the product side, we only enable remote debugging if the
current webpage is a registered developer's own application being debugged on a
registered device..

[dgozman, 2014-09-19 16:31:38]

https://codereview.chromium.org/560323005/diff/180001/chromecast/shell/browse...
File chromecast/shell/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/560323005/diff/180001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:155: return new
CastDevToolsManagerDelegate();
On 2014/09/19 15:48:32, gunsch wrote:
> How does this affect the availability of remote devtools?
> 
> We currently manage the lifetime of remote devtools somewhat uniquely,
> enabling/disabling in
> chromecast/shell/browser/devtools/remote_debugging_server.cc via
> content::DevToolsHttpHandler::{Start,Stop}.
> 
> This is because, on the product side, we only enable remote debugging if the
> current webpage is a registered developer's own application being debugged on
a
> registered device..

This has no effect on remote debugging. DevToolsHttpHandler is still responsible
for it.
This change will only have effect in the products with local debugging (like
Chrome or Content shell). For the rest - it's just refactoring.

[gunsch, 2014-09-19 16:34:05]

On 2014/09/19 16:31:38, dgozman wrote:
>
https://codereview.chromium.org/560323005/diff/180001/chromecast/shell/browse...
> File chromecast/shell/browser/cast_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/560323005/diff/180001/chromecast/shell/browse...
> chromecast/shell/browser/cast_content_browser_client.cc:155: return new
> CastDevToolsManagerDelegate();
> On 2014/09/19 15:48:32, gunsch wrote:
> > How does this affect the availability of remote devtools?
> > 
> > We currently manage the lifetime of remote devtools somewhat uniquely,
> > enabling/disabling in
> > chromecast/shell/browser/devtools/remote_debugging_server.cc via
> > content::DevToolsHttpHandler::{Start,Stop}.
> > 
> > This is because, on the product side, we only enable remote debugging if the
> > current webpage is a registered developer's own application being debugged
on
> a
> > registered device..
> 
> This has no effect on remote debugging. DevToolsHttpHandler is still
responsible
> for it.
> This change will only have effect in the products with local debugging (like
> Chrome or Content shell). For the rest - it's just refactoring.

Got it, thanks. chromecast/ LGTM.

[jam, 2014-09-19 18:45:19]

lgtm

[Ken Rockot(use gerrit already), 2014-09-19 22:41:00]

extensions lgtm

[dgozman, 2014-09-22 11:04:38]

dgozman@chromium.org changed reviewers:
+ dtrainor@chromium.org

[dgozman, 2014-09-22 11:04:39]

+dtrainor@ for chrome/browser/android OWNERS review. Could you please take a
look?

[Bernhard Bauer, 2014-09-22 11:05:30]

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:277: return
scoped_ptr<content::DevToolsTarget>();
Nit: empty line after these returns?

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:321:
DevToolsAgentHost::GetOrCreateAll();
This looks like it should fit on the previous line.

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:344: return "";
Use the empty std::string() constructor instead of a "" literal; it saves some
instructions.

[dgozman, 2014-09-22 11:11:21]

PTAL

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
File chrome/browser/android/dev_tools_manager_delegate_android.cc (right):

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:277: return
scoped_ptr<content::DevToolsTarget>();
On 2014/09/22 11:05:30, Bernhard Bauer wrote:
> Nit: empty line after these returns?

Done.

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:321:
DevToolsAgentHost::GetOrCreateAll();
On 2014/09/22 11:05:30, Bernhard Bauer wrote:
> This looks like it should fit on the previous line.

Done.

https://codereview.chromium.org/560323005/diff/180001/chrome/browser/android/...
chrome/browser/android/dev_tools_manager_delegate_android.cc:344: return "";
On 2014/09/22 11:05:30, Bernhard Bauer wrote:
> Use the empty std::string() constructor instead of a "" literal; it saves some
> instructions.

Done.

[Bernhard Bauer, 2014-09-22 11:16:20]

lgtm

[dgozman, 2014-09-22 11:16:40]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-09-22 11:17:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/560323005/200001

[commit-bot: I haz the power, 2014-09-22 11:20:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-22 11:20:30]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dgozman, 2014-09-22 11:46:08]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-09-22 11:47:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/560323005/220001

[commit-bot: I haz the power, 2014-09-22 12:40:16]

Committed patchset #9 (id:220001) as da490001390fd0ce190319b26a95874796ccbf31

[commit-bot: I haz the power, 2014-09-22 12:40:52]

Patchset 9 (id:??) landed as
https://crrev.com/252e18d2c4116131af20cac7759141ea930dcd28
Cr-Commit-Position: refs/heads/master@{#295953}