Add RemoteDebuggingController.java and its (native) dependencies.

Add RemoteDebuggingController.java and its (native) dependencies.

This is part of Chrome for Android upstreaming.


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

[mnaganov (inactive), 2012-06-11 12:47:47]

lgtm

[John Grabowski, 2012-06-11 20:17:02]

mostly lg; some questions.

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.cc (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.cc:22:
"http://chrome-devtools-frontend.appspot.com/static/%s/devtools.html";
Is this something for Google internal use, or for public consumption?
If for public consumption, do we need to get on the launch cal?
Do we need to worry about load?

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:6: #define
CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
Does ContentShell support the devtools?

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:24: // This class controls Chrome
Developer Tools remote debugging server.
namespace content {

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/rem...
File content/browser/android/remote_debugging_controller.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/rem...
content/browser/android/remote_debugging_controller.h:9: #include <jni.h>
namespace content {

[mnaganov (inactive), 2012-06-11 21:32:44]

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.cc (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.cc:22:
"http://chrome-devtools-frontend.appspot.com/static/%s/devtools.html";
On 2012/06/11 20:17:02, John Grabowski wrote:
> Is this something for Google internal use, or for public consumption?
> If for public consumption, do we need to get on the launch cal?
> Do we need to worry about load?
> 

It was already launched as CfA hit the market. Current load doesn't seem to
stress AppEngine in any way.

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:6: #define
CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
On 2012/06/11 20:17:02, John Grabowski wrote:
> Does ContentShell support the devtools?

Chromium's content_shell supports it. I haven't look into ours yet.

[John Grabowski, 2012-06-11 23:17:13]

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:6: #define
CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
On 2012/06/11 21:32:44, Mikhail Naganov (Chromium) wrote:
> On 2012/06/11 20:17:02, John Grabowski wrote:
> > Does ContentShell support the devtools?
> 
> Chromium's content_shell supports it. I haven't look into ours yet.

Thanks for the note.

In our internal repo, it looks like Chrome (the app) references this file.  If
that would continue to be true, this header needs to be in content/public.

[Philippe, 2012-06-12 16:55:58]

On 2012/06/11 23:17:13, John Grabowski wrote:
>
http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
> File content/browser/android/devtools_server.h (right):
> 
>
http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
> content/browser/android/devtools_server.h:6: #define
> CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
> On 2012/06/11 21:32:44, Mikhail Naganov (Chromium) wrote:
> > On 2012/06/11 20:17:02, John Grabowski wrote:
> > > Does ContentShell support the devtools?
> > 
> > Chromium's content_shell supports it. I haven't look into ours yet.
> 
> Thanks for the note.
> 
> In our internal repo, it looks like Chrome (the app) references this file.  If
> that would continue to be true, this header needs to be in content/public.

Is that reasonable if externally we move it to content/public/ only once browser
starts using it (which might never happen)?

I see that the file referencing this header internally is part of a GYP target
annotated with a TODO saying "Move this target to content".

[Philippe, 2012-06-13 15:21:12]

I also did a sync.

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:6: #define
CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
On 2012/06/11 23:17:14, John Grabowski wrote:
> On 2012/06/11 21:32:44, Mikhail Naganov (Chromium) wrote:
> > On 2012/06/11 20:17:02, John Grabowski wrote:
> > > Does ContentShell support the devtools?
> > 
> > Chromium's content_shell supports it. I haven't look into ours yet.
> 
> Thanks for the note.
> 
> In our internal repo, it looks like Chrome (the app) references this file.  If
> that would continue to be true, this header needs to be in content/public.

Is that reasonable if externally we move it to content/public/ only once browser
starts using it (which might never happen)?

I see that the file referencing this header internally is part of a GYP target
annotated with a TODO saying "Move this target to content".

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:24: // This class controls Chrome
Developer Tools remote debugging server.
On 2012/06/11 20:17:02, John Grabowski wrote:
> namespace content {

Done.

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/rem...
File content/browser/android/remote_debugging_controller.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/rem...
content/browser/android/remote_debugging_controller.h:9: #include <jni.h>
On 2012/06/11 20:17:02, John Grabowski wrote:
> namespace content {

Done.

[John Grabowski, 2012-06-13 19:54:10]

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:6: #define
CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
On 2012/06/13 15:21:12, Philippe wrote:
> On 2012/06/11 23:17:14, John Grabowski wrote:
> > On 2012/06/11 21:32:44, Mikhail Naganov (Chromium) wrote:
> > > On 2012/06/11 20:17:02, John Grabowski wrote:
> > > > Does ContentShell support the devtools?
> > > 
> > > Chromium's content_shell supports it. I haven't look into ours yet.
> > 
> > Thanks for the note.
> > 
> > In our internal repo, it looks like Chrome (the app) references this file. 
If
> > that would continue to be true, this header needs to be in content/public.
> 
> Is that reasonable if externally we move it to content/public/ only once
browser
> starts using it (which might never happen)?
> 
> I see that the file referencing this header internally is part of a GYP target
> annotated with a TODO saying "Move this target to content".

Reasonable?  Perhaps, but I still think we should put this in the right place. 
Even if "acceptable" to have broken deps downstream, we shouldn't do it once we
see it.

[Philippe, 2012-06-18 10:04:01]

On 2012/06/13 19:54:10, John Grabowski wrote:
>
http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
> File content/browser/android/devtools_server.h (right):
> 
>
http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
> content/browser/android/devtools_server.h:6: #define
> CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
> On 2012/06/13 15:21:12, Philippe wrote:
> > On 2012/06/11 23:17:14, John Grabowski wrote:
> > > On 2012/06/11 21:32:44, Mikhail Naganov (Chromium) wrote:
> > > > On 2012/06/11 20:17:02, John Grabowski wrote:
> > > > > Does ContentShell support the devtools?
> > > > 
> > > > Chromium's content_shell supports it. I haven't look into ours yet.
> > > 
> > > Thanks for the note.
> > > 
> > > In our internal repo, it looks like Chrome (the app) references this file.

> If
> > > that would continue to be true, this header needs to be in content/public.
> > 
> > Is that reasonable if externally we move it to content/public/ only once
> browser
> > starts using it (which might never happen)?
> > 
> > I see that the file referencing this header internally is part of a GYP
target
> > annotated with a TODO saying "Move this target to content".
> 
> Reasonable?  Perhaps, but I still think we should put this in the right place.

> Even if "acceptable" to have broken deps downstream, we shouldn't do it once
we
> see it.

My understanding is that the code referencing this internally should not be in
the app since there is a TODO saying "move this to content". Therefore the right
place does not seem to be in content/public/ if the TODO is still relevant. Am I
wrong? Or perhaps you meant "same place as downstream" instead of "right place"?

I made the change anyway. If you finally think that the header should not be in
content/public/, I can still delete the latest patch set.

[mnaganov (inactive), 2012-06-18 10:18:54]

Philippe, thanks for your efforts!

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
File content/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
content/browser/android/devtools_server.h:6: #define
CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
On 2012/06/13 15:21:12, Philippe wrote:
> On 2012/06/11 23:17:14, John Grabowski wrote:
> > On 2012/06/11 21:32:44, Mikhail Naganov (Chromium) wrote:
> > > On 2012/06/11 20:17:02, John Grabowski wrote:
> > > > Does ContentShell support the devtools?
> > > 
> > > Chromium's content_shell supports it. I haven't look into ours yet.
> > 
> > Thanks for the note.
> > 
> > In our internal repo, it looks like Chrome (the app) references this file. 
If
> > that would continue to be true, this header needs to be in content/public.
> 
> Is that reasonable if externally we move it to content/public/ only once
browser
> starts using it (which might never happen)?
> 
> I see that the file referencing this header internally is part of a GYP target
> annotated with a TODO saying "Move this target to content".

I think, the TODO doesn't imply that the target will still be including the same
set of files after moving to content/public. The one particular file that
references devtools_server.h (the preferences controller) will not be a part of
chromeview as it is only makes sense for the browser.

Moving this header to content/public seems like a good idea.

[Philippe, 2012-06-18 11:20:15]

On 2012/06/18 10:18:54, Mikhail Naganov (Chromium) wrote:
> Philippe, thanks for your efforts!
> 
>
http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
> File content/browser/android/devtools_server.h (right):
> 
>
http://codereview.chromium.org/10540094/diff/7001/content/browser/android/dev...
> content/browser/android/devtools_server.h:6: #define
> CONTENT_BROWSER_ANDROID_DEVTOOLS_SERVER_H_
> On 2012/06/13 15:21:12, Philippe wrote:
> > On 2012/06/11 23:17:14, John Grabowski wrote:
> > > On 2012/06/11 21:32:44, Mikhail Naganov (Chromium) wrote:
> > > > On 2012/06/11 20:17:02, John Grabowski wrote:
> > > > > Does ContentShell support the devtools?
> > > > 
> > > > Chromium's content_shell supports it. I haven't look into ours yet.
> > > 
> > > Thanks for the note.
> > > 
> > > In our internal repo, it looks like Chrome (the app) references this file.

> If
> > > that would continue to be true, this header needs to be in content/public.
> > 
> > Is that reasonable if externally we move it to content/public/ only once
> browser
> > starts using it (which might never happen)?
> > 
> > I see that the file referencing this header internally is part of a GYP
target
> > annotated with a TODO saying "Move this target to content".
> 
> I think, the TODO doesn't imply that the target will still be including the
same
> set of files after moving to content/public. The one particular file that
> references devtools_server.h (the preferences controller) will not be a part
of
> chromeview as it is only makes sense for the browser.
> 
> Moving this header to content/public seems like a good idea.

Thanks Mikhail for the clarification. I made the change in the last patch set.

[jochen (gone - plz use gerrit), 2012-06-18 11:32:39]

drive-by content reviews

http://codereview.chromium.org/10540094/diff/15002/content/browser/android/re...
File content/browser/android/remote_debugging_controller.cc (right):

http://codereview.chromium.org/10540094/diff/15002/content/browser/android/re...
content/browser/android/remote_debugging_controller.cc:12: static void
StartRemoteDebugging(JNIEnv*, jclass) {
what about moving those three functions into an anonymous namespace instead of
making them static?

http://codereview.chromium.org/10540094/diff/15002/content/public/browser/and...
File content/public/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/15002/content/public/browser/and...
content/public/browser/android/devtools_server.h:17: };
no ;

[mnaganov (inactive), 2012-06-18 12:26:52]

http://codereview.chromium.org/10540094/diff/15002/content/browser/android/re...
File content/browser/android/remote_debugging_controller.cc (right):

http://codereview.chromium.org/10540094/diff/15002/content/browser/android/re...
content/browser/android/remote_debugging_controller.cc:12: static void
StartRemoteDebugging(JNIEnv*, jclass) {
On 2012/06/18 11:32:40, jochen wrote:
> what about moving those three functions into an anonymous namespace instead of
> making them static?

They are called from Java code via JNI. This is the convention used by the
bridge code generator.

[Philippe, 2012-06-18 13:28:59]

http://codereview.chromium.org/10540094/diff/15002/content/browser/android/re...
File content/browser/android/remote_debugging_controller.cc (right):

http://codereview.chromium.org/10540094/diff/15002/content/browser/android/re...
content/browser/android/remote_debugging_controller.cc:12: static void
StartRemoteDebugging(JNIEnv*, jclass) {
On 2012/06/18 12:26:52, Mikhail Naganov (Chromium) wrote:
> On 2012/06/18 11:32:40, jochen wrote:
> > what about moving those three functions into an anonymous namespace instead
of
> > making them static?
> 
> They are called from Java code via JNI. This is the convention used by the
> bridge code generator.

Indeed, the JNI generator emits the following prototypes in
jni/remote_debugging_controller_jni.h:

static void StartRemoteDebugging(JNIEnv*, jclass);
static void StopRemoteDebugging(JNIEnv* , jclass);
...

It does not support namespace specification for free functions yet. We have a
bug tracking that internally.

http://codereview.chromium.org/10540094/diff/15002/content/public/browser/and...
File content/public/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/15002/content/public/browser/and...
content/public/browser/android/devtools_server.h:17: };
On 2012/06/18 11:32:40, jochen wrote:
> no ;

Done.

[John Grabowski, 2012-06-18 17:24:57]

LGTM for the issues I raised

[Philippe, 2012-06-20 08:56:30]

On 2012/06/18 17:24:57, John Grabowski wrote:
> LGTM for the issues I raised

Any other LGTM?

[Philippe, 2012-06-20 09:03:28]

On 2012/06/20 08:56:30, Philippe wrote:
> On 2012/06/18 17:24:57, John Grabowski wrote:
> > LGTM for the issues I raised
> 
> Any other LGTM?

Pavel's LGTM would be nice :)

[Philippe, 2012-06-28 10:46:07]

I actually need someone for content/ and content/public/browser/ since I moved
devtools_server.h. I'm adding avi@.

[Philippe, 2012-07-05 15:39:43]

On 2012/06/28 10:46:07, Philippe wrote:
> I actually need someone for content/ and content/public/browser/ since I moved
> devtools_server.h. I'm adding avi@.

Ping :)

[jam, 2012-07-10 20:35:49]

please see http://www.chromium.org/developers/content-module/content-api and the
rest of the files in content/public for inspiration. content/public, like the
WebKit API, only contains interfaces, not concrete classes.

[Philippe, 2012-07-11 10:23:18]

On 2012/07/10 20:35:49, John Abd-El-Malek wrote:
> please see http://www.chromium.org/developers/content-module/content-api and
the
> rest of the files in content/public for inspiration. content/public, like the
> WebKit API, only contains interfaces, not concrete classes.

I uploaded a new patch set which makes DevToolsServer an interface.

[jam, 2012-07-11 15:19:22]

http://codereview.chromium.org/10540094/diff/27009/content/browser/android/de...
File content/browser/android/devtools_server.cc (right):

http://codereview.chromium.org/10540094/diff/27009/content/browser/android/de...
content/browser/android/devtools_server.cc:10: #include <string>
nit: in header for this file, so not needed

http://codereview.chromium.org/10540094/diff/27009/content/browser/android/de...
content/browser/android/devtools_server.cc:64: protocol_handler_ =
content::DevToolsHttpHandler::Start(
nit: here and below, don't need "content::"

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
File content/public/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
content/public/browser/android/devtools_server.h:20: // This class controls
Chrome Developer Tools remote debugging server.
nit: remove the "Chrome"

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
content/public/browser/android/devtools_server.h:43: virtual bool
is_initialized() const = 0;
for this and below, see the google style guide on function names. i.e. these
should be IsInitialized/IsStarted

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
content/public/browser/android/devtools_server.h:49: DevToolsServer() {}
i don't think you need this

[jam, 2012-07-11 15:20:41]

and just to be clear, there's C++ code in src/chrome that will include 	
content/public/browser/android/devtools_server.h? I ask because I also see what
looks like a way to control this through java.

[Philippe, 2012-07-11 15:48:05]

I'm not sure this is what you meant John in your last reply, but we do have
indeed some C++ code on the app side including devtools_server.h and called by
the Java side.

http://codereview.chromium.org/10540094/diff/27009/content/browser/android/de...
File content/browser/android/devtools_server.cc (right):

http://codereview.chromium.org/10540094/diff/27009/content/browser/android/de...
content/browser/android/devtools_server.cc:10: #include <string>
On 2012/07/11 15:19:22, John Abd-El-Malek wrote:
> nit: in header for this file, so not needed

Done.

http://codereview.chromium.org/10540094/diff/27009/content/browser/android/de...
content/browser/android/devtools_server.cc:64: protocol_handler_ =
content::DevToolsHttpHandler::Start(
On 2012/07/11 15:19:22, John Abd-El-Malek wrote:
> nit: here and below, don't need "content::"

Done.

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
File content/public/browser/android/devtools_server.h (right):

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
content/public/browser/android/devtools_server.h:20: // This class controls
Chrome Developer Tools remote debugging server.
On 2012/07/11 15:19:22, John Abd-El-Malek wrote:
> nit: remove the "Chrome"

Done.

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
content/public/browser/android/devtools_server.h:43: virtual bool
is_initialized() const = 0;
On 2012/07/11 15:19:22, John Abd-El-Malek wrote:
> for this and below, see the google style guide on function names. i.e. these
> should be IsInitialized/IsStarted

Done.

http://codereview.chromium.org/10540094/diff/27009/content/public/browser/and...
content/public/browser/android/devtools_server.h:49: DevToolsServer() {}
On 2012/07/11 15:19:22, John Abd-El-Malek wrote:
> i don't think you need this

Right. The client can't instantiate this class anyway since it's abstract.

[jam, 2012-07-11 16:05:54]

On 2012/07/11 15:48:05, Philippe wrote:
> I'm not sure this is what you meant John in your last reply, but we do have
> indeed some C++ code on the app side including devtools_server.h and called by
> the Java side.

To put it another way, who includes
content/public/browser/android/devtools_server.h? i.e. it is it only code in
content (i.e. content/browser/android/remote_debugging_controller.cc), or is
there code that will live outside content that includes this header?

[Philippe, 2012-07-11 16:26:18]

On 2012/07/11 16:05:54, John Abd-El-Malek wrote:
> On 2012/07/11 15:48:05, Philippe wrote:
> > I'm not sure this is what you meant John in your last reply, but we do have
> > indeed some C++ code on the app side including devtools_server.h and called
by
> > the Java side.
> 
> To put it another way, who includes
> content/public/browser/android/devtools_server.h? i.e. it is it only code in
> content (i.e. content/browser/android/remote_debugging_controller.cc), or is
> there code that will live outside content that includes this header?

It seems that we will have  content/shell/shell_devtools_delegate_android.cc
soon which will include devtools_server.h.

Apart from this file, all the clients of devtools_server.h on the Chromium side
are part of this CL (thus in content). However as I said in my previous reply we
do have internally on the app side another client of devtools_server.h.

If I remember correctly, this client on the app side is precisely the reason why
we moved devtools_server.h to content/public/ (See John Grabowski's comment in
the first patch sets). Before that we had devtools_server.h directly under
content/browser/android/.

Please tell me if I'm missing something :)

[jam, 2012-07-11 16:29:51]

lgtm (I didn't see jrg's earlier message that code in src/chrome includes this)

[commit-bot: I haz the power, 2012-07-11 17:16:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10540094/41010

[commit-bot: I haz the power, 2012-07-11 18:38:54]

Failed to apply patch for content/content_browser.gypi:
While running patch -p1 --forward --force;
patching file content/content_browser.gypi
Hunk #1 FAILED at 30.
Hunk #2 FAILED at 200.
2 out of 2 hunks FAILED -- saving rejects to file
content/content_browser.gypi.rej

[commit-bot: I haz the power, 2012-07-12 09:24:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10540094/37013

[commit-bot: I haz the power, 2012-07-12 10:56:59]

Change committed as 146313