Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(474)

Issue 17389005: [Android] Abandon bundling DevTools frontends for mobile apps (Closed)

Created:
7 years, 6 months ago by mnaganov (inactive)
Modified:
7 years, 5 months ago
CC:
chromium-reviews, vsevik, jam, yurys, joi+watch-content_chromium.org, darin-cc_chromium.org, jochen+watch_chromium.org, android-webview-reviews_chromium.org, pfeldman
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Moved remote frontend code into a component, restored bundling for Content Shell #

Patch Set 3 : Added an OWNERS file for the component #

Patch Set 4 : Rebased #

Total comments: 8

Patch Set 5 : Simpler approach, no component #

Total comments: 12

Patch Set 6 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -58 lines) Patch
M android_webview/android_webview.gyp View 1 2 3 4 5 2 chunks +0 lines, -2 lines 0 comments Download
M android_webview/browser/aw_devtools_delegate.cc View 1 2 3 4 3 chunks +6 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/DevToolsServer.java View 2 chunks +3 lines, -5 lines 0 comments Download
M chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellApplication.java View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/android/dev_tools_server.h View 2 chunks +1 line, -3 lines 0 comments Download
M chrome/browser/android/dev_tools_server.cc View 1 2 3 4 5 8 chunks +17 lines, -24 lines 0 comments Download
M chrome/browser/chrome_browser_main_android.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/chrome_browser_main_android.cc View 1 chunk +0 lines, -15 lines 0 comments Download
M chrome/browser/devtools/devtools_adb_bridge.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/devtools/devtools_adb_bridge.cc View 1 2 3 4 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/chrome_android.gypi View 2 chunks +0 lines, -2 lines 0 comments Download
content/shell/shell_devtools_delegate.cc View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M webkit/common/user_agent/user_agent_util.h View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 27 (0 generated)
mnaganov (inactive)
7 years, 6 months ago (2013-06-18 13:14:48 UTC) #1
pfeldman
https://codereview.chromium.org/17389005/diff/1/content/browser/devtools/devtools_remote_frontend_util.cc File content/browser/devtools/devtools_remote_frontend_util.cc (right): https://codereview.chromium.org/17389005/diff/1/content/browser/devtools/devtools_remote_frontend_util.cc#newcode17 content/browser/devtools/devtools_remote_frontend_util.cc:17: const char kServerSocketNameSuffix[] = "devtools_remote"; Does not belong to ...
7 years, 6 months ago (2013-06-18 17:20:01 UTC) #2
jochen (gone - plz use gerrit)
will this affect the ability of android running inspector layout tests?
7 years, 6 months ago (2013-06-18 18:01:43 UTC) #3
mnaganov (inactive)
On 2013/06/18 18:01:43, jochen wrote: > will this affect the ability of android running inspector ...
7 years, 6 months ago (2013-06-18 20:24:16 UTC) #4
mnaganov (inactive)
jochen@: I've talked to peter@ and we have decided to leave Content Shell as is. ...
7 years, 6 months ago (2013-06-19 10:53:55 UTC) #5
pfeldman
https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/aw_devtools_delegate.cc File android_webview/browser/aw_devtools_delegate.cc (right): https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/aw_devtools_delegate.cc#newcode29 android_webview/browser/aw_devtools_delegate.cc:29: devtools_remote_frontend::GetDevToolsServerSocketName("webview"); This only makes sense for android, just hardcode ...
7 years, 6 months ago (2013-06-20 12:37:11 UTC) #6
mnaganov (inactive)
https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/aw_devtools_delegate.cc File android_webview/browser/aw_devtools_delegate.cc (right): https://codereview.chromium.org/17389005/diff/23001/android_webview/browser/aw_devtools_delegate.cc#newcode29 android_webview/browser/aw_devtools_delegate.cc:29: devtools_remote_frontend::GetDevToolsServerSocketName("webview"); On 2013/06/20 12:37:11, pfeldman wrote: > This only ...
7 years, 6 months ago (2013-06-20 16:48:08 UTC) #7
pfeldman
lgtm https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/dev_tools_server.cc File chrome/browser/android/dev_tools_server.cc (right): https://codereview.chromium.org/17389005/diff/20041/chrome/browser/android/dev_tools_server.cc#newcode189 chrome/browser/android/dev_tools_server.cc:189: jstring socketNamePrefix) { socket_name_prefix ?
7 years, 6 months ago (2013-06-20 17:03:38 UTC) #8
mnaganov (inactive)
Thanks, Pavel! Need an OWNER's review, PTAL: jamesr@: webkit/ sky@: chrome/
7 years, 6 months ago (2013-06-20 17:10:06 UTC) #9
sky
I'm not a good reviewer for this. Could you get someone that works on android ...
7 years, 6 months ago (2013-06-20 18:07:18 UTC) #10
jamesr
https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/user_agent_util.h File webkit/common/user_agent/user_agent_util.h (right): https://codereview.chromium.org/17389005/diff/20041/webkit/common/user_agent/user_agent_util.h#newcode25 webkit/common/user_agent/user_agent_util.h:25: WEBKIT_USER_AGENT_EXPORT std::string GetWebKitRevision(); what does this number mean on ...
7 years, 6 months ago (2013-06-20 18:49:46 UTC) #11
mnaganov (inactive)
bulach@: Marcus, can you please make an owner's review? sky@: You are right, almost all ...
7 years, 6 months ago (2013-06-20 20:11:08 UTC) #12
sky
Is it possible to find an Android owner for these? On Thu, Jun 20, 2013 ...
7 years, 6 months ago (2013-06-20 20:16:19 UTC) #13
mnaganov (inactive)
On 2013/06/20 20:16:19, sky wrote: > Is it possible to find an Android owner for ...
7 years, 6 months ago (2013-06-20 20:26:46 UTC) #14
sky
How about some per-file owners then? bulach@ should be able to suggest an appropriate set ...
7 years, 6 months ago (2013-06-20 21:31:43 UTC) #15
jamesr
Using a blink revision number as a versioning system seems really hacky. Will the server ...
7 years, 6 months ago (2013-06-21 00:56:38 UTC) #16
mnaganov (inactive)
On 2013/06/21 00:56:38, jamesr wrote: James, thanks for holding a debate, answers are inline. > ...
7 years, 6 months ago (2013-06-21 08:36:09 UTC) #17
bulach
the android bits lgtm, but I have a few questions re: socket name versus telemetry ...
7 years, 6 months ago (2013-06-21 09:02:16 UTC) #18
mnaganov (inactive)
> the android bits lgtm, but I have a few questions re: socket name versus ...
7 years, 6 months ago (2013-06-21 09:21:45 UTC) #19
bulach
lgtm, thanks for the explanations! :)
7 years, 6 months ago (2013-06-21 11:54:54 UTC) #20
mnaganov (inactive)
jamesr@ ping!
7 years, 6 months ago (2013-06-24 15:12:58 UTC) #21
jamesr
This seems pretty weird, but OK. relucant lgtm for webkit/
7 years, 6 months ago (2013-06-26 20:34:37 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/17389005/53001
7 years, 6 months ago (2013-06-27 02:51:36 UTC) #23
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=12663
7 years, 6 months ago (2013-06-27 03:03:19 UTC) #24
mnaganov (inactive)
7 years, 6 months ago (2013-06-27 03:15:45 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/17389005/53001
7 years, 6 months ago (2013-06-27 03:15:57 UTC) #26
commit-bot: I haz the power
7 years, 5 months ago (2013-06-27 14:28:37 UTC) #27
Message was sent while issue was closed.
Change committed as 208913

Powered by Google App Engine
This is Rietveld 408576698