[Android] Abandon bundling DevTools frontends for mobile apps

[Android] Abandon bundling DevTools frontends for mobile apps

From now, DevTools frontends are only served from the cloud using
Blink revision number. This enabled all mobile Chromium apps
(Chrome, Chromium TestShell, WebView) to be inspectable from chrome://inspect.

An exception is made for Content Shell, as it will be used for running layout
tests (including Inspector layout tests) on Android and thus it needs the
frontend to be bundled.

BUG=138925
TBR=jochen (to work around not formally having Android owners for chrome/browser/chrome_browser_main_android.*)

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

[pfeldman, 2013-06-18 17:20:01]

https://codereview.chromium.org/17389005/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_remote_frontend_util.cc (right):

https://codereview.chromium.org/17389005/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_remote_frontend_util.cc:17: const char
kServerSocketNameSuffix[] = "devtools_remote";
Does not belong to content imo.

https://codereview.chromium.org/17389005/diff/1/content/public/browser/devtoo...
File content/public/browser/devtools_remote_frontend_util.h (right):

https://codereview.chromium.org/17389005/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_remote_frontend_util.h:17: CONTENT_EXPORT
std::string GetDevToolsFrontendResourceURL(
It sounds like both: consumers and providers for this data is embedders, it is
not used anywhere in content, so it should not be exposed there.

[jochen (gone - plz use gerrit), 2013-06-18 18:01:43]

will this affect the ability of android running inspector layout tests?

[mnaganov (inactive), 2013-06-18 20:24:16]

On 2013/06/18 18:01:43, jochen wrote:
> will this affect the ability of android running inspector layout tests?

jochen@: How do those test work? Can you please give me a pointer to them?

[mnaganov (inactive), 2013-06-19 10:53:55]

jochen@: I've talked to peter@ and we have decided to leave Content Shell as is.
That means, it will not be possible to start remote debugging via
"chrome://inspect" for it, but that shouldn't be an issue.

https://codereview.chromium.org/17389005/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_remote_frontend_util.cc (right):

https://codereview.chromium.org/17389005/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_remote_frontend_util.cc:17: const char
kServerSocketNameSuffix[] = "devtools_remote";
On 2013/06/18 17:20:01, pfeldman wrote:
> Does not belong to content imo.

OK, I've created a component for these functions and also moved devtools_auth
there, on the same grounds.

[pfeldman, 2013-06-20 12:37:11]

https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/a...
File android_webview/browser/aw_devtools_delegate.cc (right):

https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/a...
android_webview/browser/aw_devtools_delegate.cc:29:
devtools_remote_frontend::GetDevToolsServerSocketName("webview");
This only makes sense for android, just hardcode it here.

https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/a...
android_webview/browser/aw_devtools_delegate.cc:37:
devtools_remote_frontend::GetDevToolsFrontendMainResourceURL(),
ditto

https://codereview.chromium.org/17389005/diff/23001/chrome/browser/android/de...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/17389005/diff/23001/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:132: :
socket_name_(devtools_remote_frontend::GetDevToolsServerSocketName(
Simply hardcode values here.

https://codereview.chromium.org/17389005/diff/23001/components/devtools_remot...
File components/devtools_remote_frontend/devtools_remote_frontend_util.cc
(right):

https://codereview.chromium.org/17389005/diff/23001/components/devtools_remot...
components/devtools_remote_frontend/devtools_remote_frontend_util.cc:13: const
char kRemoteFrontendDomain[] = "chrome-devtools-frontend.appspot.com";
I don't think such component should exist at all.

[mnaganov (inactive), 2013-06-20 16:48:08]

https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/a...
File android_webview/browser/aw_devtools_delegate.cc (right):

https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/a...
android_webview/browser/aw_devtools_delegate.cc:29:
devtools_remote_frontend::GetDevToolsServerSocketName("webview");
On 2013/06/20 12:37:11, pfeldman wrote:
> This only makes sense for android, just hardcode it here.

Done.

https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/a...
android_webview/browser/aw_devtools_delegate.cc:37:
devtools_remote_frontend::GetDevToolsFrontendMainResourceURL(),
On 2013/06/20 12:37:11, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/17389005/diff/23001/chrome/browser/android/de...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/17389005/diff/23001/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:132: :
socket_name_(devtools_remote_frontend::GetDevToolsServerSocketName(
On 2013/06/20 12:37:11, pfeldman wrote:
> Simply hardcode values here.

I'm now reusing it from devtools_adb_bridge, since it is also in 'chrome/'

https://codereview.chromium.org/17389005/diff/23001/components/devtools_remot...
File components/devtools_remote_frontend/devtools_remote_frontend_util.cc
(right):

https://codereview.chromium.org/17389005/diff/23001/components/devtools_remot...
components/devtools_remote_frontend/devtools_remote_frontend_util.cc:13: const
char kRemoteFrontendDomain[] = "chrome-devtools-frontend.appspot.com";
On 2013/06/20 12:37:11, pfeldman wrote:
> I don't think such component should exist at all.

OK, removed.

[pfeldman, 2013-06-20 17:03:38]

lgtm

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:189: jstring socketNamePrefix) {
socket_name_prefix ?

[mnaganov (inactive), 2013-06-20 17:10:06]

Thanks, Pavel!

Need an OWNER's review, PTAL:

jamesr@: webkit/
sky@: chrome/

[sky, 2013-06-20 18:07:18]

I'm not a good reviewer for this. Could you get someone that works on android to
review it?

[jamesr, 2013-06-20 18:49:46]

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
File webkit/common/user_agent/user_agent_util.h (right):

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
webkit/common/user_agent/user_agent_util.h:25: WEBKIT_USER_AGENT_EXPORT
std::string GetWebKitRevision();
what does this number mean on release branches?

[mnaganov (inactive), 2013-06-20 20:11:08]

bulach@: Marcus, can you please make an owner's review?

sky@: You are right, almost all 'chrome/' changes are Android-related. But since
Marcus is not an owner for the top-level 'chrome/' will it be possible to TBR
you once I'll obtain his approval?

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
File webkit/common/user_agent/user_agent_util.h (right):

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
webkit/common/user_agent/user_agent_util.h:25: WEBKIT_USER_AGENT_EXPORT
std::string GetWebKitRevision();
On 2013/06/20 18:49:47, jamesr wrote:
> what does this number mean on release branches?

It's always the number of the svn revision. Every svn commit, including commits
in branches does have it. We will have DevTools frontends for every Blink
revision for trunk and all chromium branches.

[sky, 2013-06-20 20:16:19]

Is it possible to find an Android owner for these?

On Thu, Jun 20, 2013 at 1:11 PM,  <mnaganov@chromium.org> wrote:
> bulach@: Marcus, can you please make an owner's review?
>
> sky@: You are right, almost all 'chrome/' changes are Android-related. But
> since
> Marcus is not an owner for the top-level 'chrome/' will it be possible to
> TBR
> you once I'll obtain his approval?
>
>
>
>
https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
> File webkit/common/user_agent/user_agent_util.h (right):
>
>
https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
> webkit/common/user_agent/user_agent_util.h:25: WEBKIT_USER_AGENT_EXPORT
> std::string GetWebKitRevision();
> On 2013/06/20 18:49:47, jamesr wrote:
>>
>> what does this number mean on release branches?
>
>
> It's always the number of the svn revision. Every svn commit, including
> commits in branches does have it. We will have DevTools frontends for
> every Blink revision for trunk and all chromium branches.
>
> https://codereview.chromium.org/17389005/

[mnaganov (inactive), 2013-06-20 20:26:46]

On 2013/06/20 20:16:19, sky wrote:
> Is it possible to find an Android owner for these?
>

Scott, I've asked Marcus (bulach@), he is an owner for 'chrome/android' and
'chrome/browser/android', but there are also Android-related changes to
'chrome/browser' and 'chrome/', and formally, there are no Android owners for
these directories.
 
> On Thu, Jun 20, 2013 at 1:11 PM,  <mailto:mnaganov@chromium.org> wrote:
> > bulach@: Marcus, can you please make an owner's review?
> >
> > sky@: You are right, almost all 'chrome/' changes are Android-related. But
> > since
> > Marcus is not an owner for the top-level 'chrome/' will it be possible to
> > TBR
> > you once I'll obtain his approval?
> >
> >
> >
> >
>
https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
> > File webkit/common/user_agent/user_agent_util.h (right):
> >
> >
>
https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
> > webkit/common/user_agent/user_agent_util.h:25: WEBKIT_USER_AGENT_EXPORT
> > std::string GetWebKitRevision();
> > On 2013/06/20 18:49:47, jamesr wrote:
> >>
> >> what does this number mean on release branches?
> >
> >
> > It's always the number of the svn revision. Every svn commit, including
> > commits in branches does have it. We will have DevTools frontends for
> > every Blink revision for trunk and all chromium branches.
> >
> > https://codereview.chromium.org/17389005/

[sky, 2013-06-20 21:31:43]

How about some per-file owners then? bulach@ should be able to suggest an
appropriate set of folks for these.

Sorry to push on this, but everything goes smoother when the right set of folks
can actually approve changes.

[jamesr, 2013-06-21 00:56:38]

Using a blink revision number as a versioning system seems really hacky.  Will
the server have a copy of the devtools frontend for every single blink rev,
including branches, past and present? What happens when we change to git or
change the way we do blink revisions (say merge repositories or rewrite
history)?

The devtools protocol itself is versioned, can the frontend just figure out what
the best frontend is to connect to a given protocol version?

[mnaganov (inactive), 2013-06-21 08:36:09]

On 2013/06/21 00:56:38, jamesr wrote:

James, thanks for holding a debate, answers are inline.

> Using a blink revision number as a versioning system seems really hacky.  Will
> the server have a copy of the devtools frontend for every single blink rev,
> including branches, past and present?

Yes. As I've stated earlier, we are uploading DevTools frontends for every SVN
revision of Blink that touches 'trunk/' and 'branches/chromium/' branches. We
don't include changes to experimental branches, e.g. 'oilpan' just because they
are only used by a limited number of developers and are never released. We don't
need the revisions from the past, only starting from the moment when this change
will hopefully be committed. The frontends are stored compressed and deduped, so
we are estimating we only need ~1.5G of storage space per 100000 Blink
revisions.

> What happens when we change to git

Then we will be using git commit hashes for identification and will be
monitoring all production branches for changes. This code will even not need to
change, as it doesn't interpret the revision number itself in any way, it simply
passes it to the server.

> or change the way we do blink revisions (say merge repositories or rewrite
> history)?
> 

I'm not sure you really want to be rewriting history on a public repository, as
this will create confusion for all developers.

If blink repository will be merged with, say, chromium repository, we will be
using commit hashes from the resulting repository.

As there will always be a way for identifying the state of the repository, it
will be possible to use it for our purposes of matching DevTools' backend and
frontend revisions.

> The devtools protocol itself is versioned, can the frontend just figure out
what
> the best frontend is to connect to a given protocol version?

The DevTools protocol versions only correspond to the published part of the
protocol. But as it is constantly evolving, it also contains the "unstable"
part. As Pavel has stated, using non strictly matching frontend and backend
revisions can render DevTools remote frontend unusable, especially on the trunk
branch. So we have settled down with this approach.

[bulach, 2013-06-21 09:02:16]

the android bits lgtm, but I have a few questions re: socket name versus
telemetry (and funnily, have a patch about to land that will switch
chromium.perf to use chromium test shell instead of content shell). :)

https://codereview.chromium.org/17389005/diff/20041/chrome/android/testshell/...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(left):

https://codereview.chromium.org/17389005/diff/20041/chrome/android/testshell/...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:64:
mDevToolsServer = new DevToolsServer(true,
"chromium_testshell_devtools_remote");
nit: would this change line 99 of
http://src.chromium.org/viewvc/chrome/trunk/src/tools/telemetry/telemetry/cor...
?

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:41:
"http://chrome-devtools-frontend.appspot.com/serve_rev/%s/devtools.html";
nit: is "serve" correct or should it be serve"r"?

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:42: const char
kDefaultSocketNamePrefix[] = "chrome";
as above, would telemetry need to change? it tries to be backward compatible,
i.e., new telemetry versus old browser, would it be possible to detect this
somehow?

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
File webkit/common/user_agent/user_agent_util.h (right):

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
webkit/common/user_agent/user_agent_util.h:25: WEBKIT_USER_AGENT_EXPORT
std::string GetWebKitRevision();
On 2013/06/20 20:11:09, Mikhail Naganov (Chromium) wrote:
> On 2013/06/20 18:49:47, jamesr wrote:
> > what does this number mean on release branches?
> 
> It's always the number of the svn revision. Every svn commit, including
commits
> in branches does have it. We will have DevTools frontends for every Blink
> revision for trunk and all chromium branches.

isn't it safer to just rely on GetWebKitVersion() above then?

[mnaganov (inactive), 2013-06-21 09:21:45]

> the android bits lgtm, but I have a few questions re: socket name versus
> telemetry (and funnily, have a patch about to land that will switch
> chromium.perf to use chromium test shell instead of content shell). :)

Thanks, Marcus!

Don't worry--the socket names doesn't change, only the way they are constructed.

Also, as telemetry substitutes DevTools frontend, it should be agnostic to
whether the frontend is bundled or not.

https://codereview.chromium.org/17389005/diff/20041/chrome/android/testshell/...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(left):

https://codereview.chromium.org/17389005/diff/20041/chrome/android/testshell/...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:64:
mDevToolsServer = new DevToolsServer(true,
"chromium_testshell_devtools_remote");
On 2013/06/21 09:02:17, bulach wrote:
> nit: would this change line 99 of
>
http://src.chromium.org/viewvc/chrome/trunk/src/tools/telemetry/telemetry/cor...
> ?

No, we don't actually change any socket names here. I have only replaced
providing a full socket name with providing a prefix for it. As in
'chrome://inspect' we are looking for 'devtools_remote' substring in socket
names, I just want to make sure that sockets opened by Chromium backends will
actually contain it.

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
File chrome/browser/android/dev_tools_server.cc (right):

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:41:
"http://chrome-devtools-frontend.appspot.com/serve_rev/%s/devtools.html";
On 2013/06/21 09:02:17, bulach wrote:
> nit: is "serve" correct or should it be serve"r"?

It is correct. It's not a "server revision", but rather a command to "serve by
the revision". But thanks for looking at this!

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:42: const char
kDefaultSocketNamePrefix[] = "chrome";
On 2013/06/21 09:02:17, bulach wrote:
> as above, would telemetry need to change? it tries to be backward compatible,
> i.e., new telemetry versus old browser, would it be possible to detect this
> somehow?

No. Please note from the var name change, that it is now a prefix added to
"_devtools_remote".

https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/de...
chrome/browser/android/dev_tools_server.cc:189: jstring socketNamePrefix) {
On 2013/06/20 17:03:39, pfeldman wrote:
> socket_name_prefix ?

Done. Writing JNI code is always challenging when it comes to following coding
conventions.

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
File webkit/common/user_agent/user_agent_util.h (right):

https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/...
webkit/common/user_agent/user_agent_util.h:25: WEBKIT_USER_AGENT_EXPORT
std::string GetWebKitRevision();
On 2013/06/21 09:02:17, bulach wrote:
> On 2013/06/20 20:11:09, Mikhail Naganov (Chromium) wrote:
> > On 2013/06/20 18:49:47, jamesr wrote:
> > > what does this number mean on release branches?
> > 
> > It's always the number of the svn revision. Every svn commit, including
> commits
> > in branches does have it. We will have DevTools frontends for every Blink
> > revision for trunk and all chromium branches.
> 
> isn't it safer to just rely on GetWebKitVersion() above then?

GetWebKitVersion doesn't provide any more useful information than
GetWebKitRevision, as we have hardcoded the numbers for major and minor versions
since the Blink forking.

[bulach, 2013-06-21 11:54:54]

lgtm, thanks for the explanations! :)

[mnaganov (inactive), 2013-06-24 15:12:58]

jamesr@ ping!

[jamesr, 2013-06-26 20:34:37]

This seems pretty weird, but OK. relucant lgtm for webkit/

[commit-bot: I haz the power, 2013-06-27 02:51:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/17389005/53001

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

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

[commit-bot: I haz the power, 2013-06-27 03:15:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/17389005/53001

[commit-bot: I haz the power, 2013-06-27 14:28:37]

Change committed as 208913