Simplify devtools code on android and enable devtools for android content_shell.

Simplify devtools code on android and enable devtools for android content_shell.

This CL removes the unnecessary RemoteDebuggingcontroller java class and associated
c++ code. Also devtools_server.* is moved from content/ to chrome/ so that the
code to access profiles, version etc. can be handled here to make DevToolsServer
a self contained class in chrome/ layer handling devtools for android. The code
making use of this class is not yet ready to be upstreamed due to dependencies
and will land in future.

Also added shell_devtools_delegate_android.cc to enable devtools for content_shell
on android. This class and chrome/browser/android/devtools_server.cc need a
common place to put the IsUserAllowedToConnect method that ensures only adb is
allowed to access devtools on android (via the abstract unix socket, for security
purposes) hence that method is placed in DevToolsHttpHandler in content/

BUG=136682, 136318
TEST=manual. Run android content shell, execute "adb forward tcp:9222 localabstract:content_shell_devtools_remote" and open "localhost:9222" on the host machine to use devtools.

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

[Satish, 2012-08-02 13:46:17]

jam@ for the content api changes
pfeldman@ for the overall devtools flow

[mnaganov (inactive), 2012-08-02 16:16:45]

Looks good

https://chromiumcodereview.appspot.com/10832112/diff/1/content/public/browser...
File content/public/browser/devtools_http_handler.h (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/public/browser...
content/public/browser/devtools_http_handler.h:13: #include <pwd.h>
<pwd.h> should go before <string>:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

[pfeldman, 2012-08-02 17:05:50]

DevTools part looks mostly Ok.

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
File chrome/browser/android/devtools_server.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
chrome/browser/android/devtools_server.cc:29:
"http://chrome-devtools-frontend.appspot.com/static/%s/devtools.html";
I think entire URL should be specified by the embedder.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.cc:123: bool
DevToolsHttpHandler::IsUserAllowedToConnect(uid_t uid, gid_t gid) {
I don't think this code belongs here. It looks like a random Android-specific
function to me.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/public/browser...
File content/public/browser/devtools_http_handler.h (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/public/browser...
content/public/browser/devtools_http_handler.h:12: #if defined(OS_ANDROID)
I don't think it should contain Android-specifics.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_br...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_br...
content/shell/shell_browser_main_parts.cc:84: devtools_delegate_ = new
ShellDevToolsDelegate(
So you want to always have remote debugging-enabled shell for Android?

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
File content/shell/shell_devtools_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
content/shell/shell_devtools_delegate_android.cc:20: const char*
kFrontendVersion = "21.0.1175.0";
I don't see how this requires an additional feature in the devtools front-end.
It rather needs something like chrome::VersionInfo. For the content shell, it
might be passed as a command line argument.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
content/shell/shell_devtools_delegate_android.cc:36:
base::Bind(&DevToolsHttpHandler::IsUserAllowedToConnect)),
Could you define IsUserAllowedToConnect locally?

[mnaganov (inactive), 2012-08-02 17:09:57]

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
File content/shell/shell_devtools_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
content/shell/shell_devtools_delegate_android.cc:20: const char*
kFrontendVersion = "21.0.1175.0";
On 2012/08/02 17:05:50, pfeldman wrote:
> I don't see how this requires an additional feature in the devtools front-end.
> It rather needs something like chrome::VersionInfo. For the content shell, it
> might be passed as a command line argument.

I remember we have discussed this and have agreed that WebKit revision can be
used instead of Chrome version. I just need to update frontend hosting first.

[pfeldman, 2012-08-02 17:15:16]

> I remember we have discussed this and have agreed that WebKit revision can be
> used instead of Chrome version. I just need to update frontend hosting first.

I don't see why not (like WEBKIT_SVN_REVISION, right?). I now see that you were
talking about the hosting (we associate devtools front-end with the front-end
code).

[Satish, 2012-08-02 17:17:53]

Thanks for the quick review. A few responses below.

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
File chrome/browser/android/devtools_server.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
chrome/browser/android/devtools_server.cc:29:
"http://chrome-devtools-frontend.appspot.com/static/%s/devtools.html";
On 2012/08/02 17:05:50, pfeldman wrote:
> I think entire URL should be specified by the embedder.

In our case this is the embedder since it is in the chrome/ layer and part of
the chrome-for-android browser code. Do you think it should be passed down by
someone else?

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.cc:123: bool
DevToolsHttpHandler::IsUserAllowedToConnect(uid_t uid, gid_t gid) {
On 2012/08/02 17:05:50, pfeldman wrote:
> I don't think this code belongs here. It looks like a random Android-specific
> function to me.

This function is specific to the devtools feature, deciding which specific
process on the device to allow connect to devtools. It is android specific hence
the ifdef, but if it is moved elsewhere it will still be used only for the
purpose of devtools http handler and not something else. So this seems class
seems like the appropriate place, similar to how we have
GetFrontendResourceIds() below which is devtools specific but just iterates over
resources ids.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_br...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_br...
content/shell/shell_browser_main_parts.cc:84: devtools_delegate_ = new
ShellDevToolsDelegate(
On 2012/08/02 17:05:50, pfeldman wrote:
> So you want to always have remote debugging-enabled shell for Android?

For content shell, yes because it is aimed towards developers and not a
shippable product

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
File content/shell/shell_devtools_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
content/shell/shell_devtools_delegate_android.cc:36:
base::Bind(&DevToolsHttpHandler::IsUserAllowedToConnect)),
On 2012/08/02 17:05:50, pfeldman wrote:
> Could you define IsUserAllowedToConnect locally?

I could, but it is also used by chrome/browser/android/devtools_server.cc hence
placed in the common DevToolsHttpHandler (since it decides who can connect to
the http handler)

[pfeldman, 2012-08-02 17:37:53]

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
File chrome/browser/android/devtools_server.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
chrome/browser/android/devtools_server.cc:29:
"http://chrome-devtools-frontend.appspot.com/static/%s/devtools.html";
I just think that hard-coding particular hosting location this deep could lead
to problems. I'd rather keep it in the Android app's settings. I think it is
fine to land it this way, but I'd rather see it fixed in the long run.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.cc:123: bool
DevToolsHttpHandler::IsUserAllowedToConnect(uid_t uid, gid_t gid) {
The code below does not seem to be related to devtools and/or debugging. Placing
it in a separate header or in existing reachable header and giving it a proper
name (CanAccessProcess or something) would be more natural.  Android (vertical
feature) needs it, DevTools (horizontal feature) does not, hence it belongs to
the file matching the ".*android.*" pattern. Does it make sense?

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_br...
File content/shell/shell_browser_main_parts.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_br...
content/shell/shell_browser_main_parts.cc:84: devtools_delegate_ = new
ShellDevToolsDelegate(
Sounds good.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
File content/shell/shell_devtools_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/shell/shell_de...
content/shell/shell_devtools_delegate_android.cc:20: const char*
kFrontendVersion = "21.0.1175.0";
Sounds good.

[Satish, 2012-08-06 10:21:40]

New patch uploaded, PTAL

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
File chrome/browser/android/devtools_server.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/chrome/browser/android...
chrome/browser/android/devtools_server.cc:29:
"http://chrome-devtools-frontend.appspot.com/static/%s/devtools.html";
This URL will be common between various android targets based on chrome.. e.g.
the clank app, android api provider, a simple chrome/ layer browser app and so
on. So it is an implementation detail that I want to contain within
android/devtools_server.cc and not have each caller know about inner workings of
where devtools frontend is hosted.

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
File content/browser/debugger/devtools_http_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/10832112/diff/1/content/browser/debugg...
content/browser/debugger/devtools_http_handler_impl.cc:123: bool
DevToolsHttpHandler::IsUserAllowedToConnect(uid_t uid, gid_t gid) {
On 2012/08/02 17:37:53, pfeldman wrote:
> The code below does not seem to be related to devtools and/or debugging.
Placing
> it in a separate header or in existing reachable header and giving it a proper
> name (CanAccessProcess or something) would be more natural.  Android (vertical
> feature) needs it, DevTools (horizontal feature) does not, hence it belongs to
> the file matching the ".*android.*" pattern. Does it make sense?

Done.

[pfeldman, 2012-08-06 12:12:38]

devtools lgtm.

[Satish, 2012-08-06 12:14:41]

jam@, could you please look at the content api changes? Other changes have been
reviewed by pfeldman and mnaganov

[jam, 2012-08-06 18:30:16]

lgtm

[commit-bot: I haz the power, 2012-08-07 11:08:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satish@chromium.org/10832112/11001

[commit-bot: I haz the power, 2012-08-07 12:52:21]

Change committed as 150336