New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 701938 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Task



Sign in to add a comment

Rename android_webview_apk

Project Member Reported by aelias@chromium.org, Mar 15 2017

Issue description

android_webview_apk/AndroidWebView.apk is actually the "test shell" sample app embedding a WebView.  This is confusing -- it sounds like this should be the WebView itself (which is actually system_webview_apk/system_webview_google_apk).  

In comparison, for CCT, the similar sample client app is called "custom_tabs_client_example_apk".  So I'd suggest renaming this one to something like android_webview_client_example_apk, or android_webview_embedder_sample_apk.
 

Comment 1 by aelias@chromium.org, Mar 15 2017

Cc: torne@chromium.org tobiasjs@chromium.org
Wait, I got this confused with system_webview_shell_apk, which is the actual lightweight client talking to the system WebView.  Whereas android_webview_apk is the one that statically links the entire WebView in addition to the shell.  So it should be called something like nonsystem_webview_with_shell_apk.

Another question is whether android_webview_apk is something that needs to exist at all anymore.  It's a legacy of KitKat where WebView was system-bundled.  At that time it was the only upstream target, accounting for its generic name, and it was the only way to conveniently test WebView changes without a reinstall.  We now have other upstream targets and other ways to quickly install.  Instead of renaming, could this target be retired?
Cc: ntfschr@chromium.org
We (I) have long griped about this. I'd be all for renaming it to something like "builtin_webview_shell_apk" (to mirror system_webview_shell_apk). However, the dominant opinion on the team has usually been that it's not worth the trouble.

Comment 3 by boliu@chromium.org, Mar 15 2017

I'm in the "I support this, but won't volunteer" camp..
I don't know how hard this is to change. I'm still relatively new to WebView though. As a newcomer, here are my first impressions of these things, based on their names only:

 - system_webview_apk: It's webview
 - system_webview_google_apk: It's webview... with proprietary Google stuff
 - system_webview_shell_apk: It's webview... with proprietary Shell stuff... wait, this is actually a browser?
 - android_webview_apk: Wait, this *also* isn't a webview? This is a shell? But I thought "shell" meant browser!
 - android_webview_test_apk: Ok, I know this has something to do with tests. Is this the test cases, or is this the webview we test against?

A modest suggestion:
 - system_webview_apk and system_webview_google_apk: I have no major gripes
 - webview_browser_client_apk: It's a webview browser client
 - webview_for_tests_apk: It's the webview-ish thing we use during testing
 - webview_instrumentation_test_apk: It's the instrumentation tests for WebView

I don't know how consistent this is with chromium, but an experienced chromium engineer (aelias@) got confused enough by our names that he was confused about which thing to file a bug against. To me this sounds like we have a problem. Maybe we can alleviate this via documentation/comments, or maybe it's reasonable to provide consistent & sensible naming conventions.

Comment 5 by sgu...@chromium.org, Mar 15 2017

I am not sure if I really liked the name 
nonsystem_webview_with_shell_apk 

system_webview_shell_apk is kind a clear to me. we could call it webview_browser_apk too.  don't se the point of adding client.

nate's other suggestions looks reasonable to me. But it is a seemingly trivial work (just a rename) with lots of time needed to fix all dependencies. I would rather spend this time on more urgent code health needs, really. (like for example fixing disabled tests?)
webview_browser_apk SGTM (I added "client" as per aelias@'s suggested names)

Comment 7 by aelias@chromium.org, Mar 15 2017

Trybots are green on a rename of android_webview_apk https://codereview.chromium.org/2750183002.  I really don't think there's a huge nest of dependencies, for this particular target.

> I would rather spend this time on more urgent code health needs, really. (like for example fixing disabled tests?)

This kind of thing is an obstacle to getting other Chrome engineers to work on WebView.  It's worth the time in my opinion.

Comment 8 by boliu@chromium.org, Mar 15 2017

> Trybots are green on a rename of android_webview_apk https://codereview.chromium.org/2750183002.  I really don't think there's a huge nest of dependencies, for this particular target.

we have a lot more bot configurations than what runs on the trybots though; in particular, should definitely double check this doesn't break release bots. and then there are things like telemetry. and docs of course

Comment 9 by sgu...@chromium.org, Mar 15 2017

yeah docs should definitely be updated, otherwise it will create more confusion than it fixes.
Cc: ctzsm@chromium.org
Shimi, since you are new it can be a good introduction to our trybot/buildbot configurations and you are probably going over our documents too :) 

Comment 12 by torne@chromium.org, Mar 16 2017

re comment 1, all our instrumentation tests run against this target so we can't really get rid of it. Rewriting them all to use just the public WebView API would be quite hard I think..
> re comment 1, all our instrumentation tests run against this target so we can't really get rid of it. Rewriting them all to use just the public WebView API would be quite hard I think..

Yeah, on further examination, agreed.  It would also mean that the test runners would only work on userdebug devices, with the proper keys, with different installation flow for N+, and none of that is worth the trouble.  So let's just rename it.
Owner: ctzsm@chromium.org
Sorry I forgot writing a suggested name, please do changes as below:

android_webview_apk -> webview_instrumentation_apk
android_webview_test_apk -> webview_instrumentation_test_apk

If somebody has a suggestion please add here. However, let's not waste too much time thinking about the name so please speak only if you have strong objection :)
SGTM
Status: Started (was: Available)
Project Member

Comment 19 by bugdroid1@chromium.org, May 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/30d8d501de8b35a7e97060ed6b03f737dd424652

commit 30d8d501de8b35a7e97060ed6b03f737dd424652
Author: Shimi Zhang <ctzsm@google.com>
Date: Thu May 04 17:06:30 2017

Rename android_webview_test_apk to webview_instrumentation_test_apk

Bug:  701938 
Change-Id: I7804b523a996149f1cc3ca1ec62e2811595d2e33
Reviewed-on: https://chromium-review.googlesource.com/495631
Commit-Queue: Shimi Zhang <ctzsm@google.com>
Reviewed-by: John Budorick <jbudorick@chromium.org>

[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Asan_Builder_Tests__dbg_.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/telemetry_browser_tests_failures.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/asan_setup_failure.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipes/chromium_trybot.expected/full_tryserver_chromium_android_android_coverage.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/timestamp_as_point_id_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/restart_usb_builder_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_tests_failure.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/keep_data_install_tester_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tombstones_m53.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/gerrit_try_builder_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_with_step_warning.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/use_devil_adb_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/webview_cts_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/basic_builder_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_tests_infra_failure.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_adb_vendor_keys_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/device_file_format_mismatch.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/stackwalker_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_failing_host_info.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/webview_tester_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Find_Annotated_Test.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_runner_allow_high_battery_temp_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_blacklisted_devices.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/asan_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/enable_platform_mode_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Builder__dbg_.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_no_devices_during_status.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_other_device_failure_during_status.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_tests/tests/steps/find_annotated_test.expected/basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_tests_reference_failure.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/device_flags_builder_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/slow_tester_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/downgrade_install_tester_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_no_devices_during_recovery.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/no_cache_builder_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_runner_allow_low_battery_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_runner_user_build_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/json_results_file_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/result_details_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/resource_size_builder_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/gerrit_refs.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Coverage__dbg_.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.py
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_runner_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/no_strict_mode_tester_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_tests_infra_reference_failure.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/perf_runner_disable_location_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/coverage_builder_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_offline_devices.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/upload_result_details_failures.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/tester_other_device_failure_during_recovery.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/render_results_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/remove_system_vrcore_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/last_known_devices_basic.json
[modify] https://crrev.com/30d8d501de8b35a7e97060ed6b03f737dd424652/scripts/slave/recipe_modules/chromium_android/example.expected/telemetry_browser_tests_tester_basic.json

Project Member

Comment 21 by bugdroid1@chromium.org, May 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25d1b2958dad54c9310305dad0218c8f3fdbef98

commit 25d1b2958dad54c9310305dad0218c8f3fdbef98
Author: ctzsm <ctzsm@chromium.org>
Date: Mon May 15 22:27:58 2017

Rename android-webview-shell to android-webview-instrumentation

This CL is a follow up of crrev/2878493003.

Since we are renaming AndroidWebView.apk to WebViewInstrumentation.apk,
it doesn't make sense we still call the corresponding enty
android-webview-shell, so it is better to change this name as well.

BUG= 701938 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2884643003
Cr-Commit-Position: refs/heads/master@{#471924}

[modify] https://crrev.com/25d1b2958dad54c9310305dad0218c8f3fdbef98/content/test/gpu/gpu_tests/test_expectations.py
[modify] https://crrev.com/25d1b2958dad54c9310305dad0218c8f3fdbef98/content/test/gpu/gpu_tests/test_expectations_unittest.py
[modify] https://crrev.com/25d1b2958dad54c9310305dad0218c8f3fdbef98/content/test/gpu/gpu_tests/webgl_conformance_expectations.py
[modify] https://crrev.com/25d1b2958dad54c9310305dad0218c8f3fdbef98/content/test/gpu/gpu_tests/webgl_conformance_integration_test.py
[modify] https://crrev.com/25d1b2958dad54c9310305dad0218c8f3fdbef98/tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py

Project Member

Comment 22 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23a57a1539f1ebf22f42d01a2978ed5b67654201

commit 23a57a1539f1ebf22f42d01a2978ed5b67654201
Author: ctzsm <ctzsm@chromium.org>
Date: Wed May 17 01:05:43 2017

Remove android_webview_apk and android_webview_test_apk

This CL removes both android_webview_apk and android_webview_test_apk
build targets as we are deprecating those two names.

Please refer the crbug for more info, and the new target names are
webview_instrumentation_apk and webview_instrumentation_test_apk
correspondingly.

New build names are WebViewInstrumentation.apk and
WebViewInstrumentationTest.apk

BUG= 701938 

Review-Url: https://codereview.chromium.org/2879723002
Cr-Commit-Position: refs/heads/master@{#472276}

[modify] https://crrev.com/23a57a1539f1ebf22f42d01a2978ed5b67654201/android_webview/test/BUILD.gn

Thanks!
Thanks, Shimi! The new names are a lot clearer
Project Member

Comment 25 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26d71b4f8cbf1fd071ae1817918857613d9338f7

commit 26d71b4f8cbf1fd071ae1817918857613d9338f7
Author: catapult-deps-roller@chromium.org <catapult-deps-roller@chromium.org>
Date: Wed May 17 05:16:56 2017

Roll src/third_party/catapult/ 37015fb47..dab2aa44c (13 commits)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/37015fb470d8..dab2aa44c6cd

$ git log 37015fb47..dab2aa44c --date=short --no-merges --format='%ad %ae %s'
2017-05-16 nednguyen Revert of Remove webrtc_rendering_stats TBMv1 metrics (patchset #2 id:20001 of https://codereview.chromium.org/2890443002/ )
2017-05-16 rnephew Revert of [Telemetry][CodeHealth] Make unittests with fake platforms use fakes.FakePlatform (patchset #3 id:60001 of https://codereview.chromium.org/2889583003/ )
2017-05-16 rnephew [Telemetry][CodeHealth] Make unittests with fake platforms use fakes.FakePlatform
2017-05-16 ctzsm Remove android-webview-shell
2017-05-16 rnephew [Telemetry] Add Expectation module that enables disabling benchmarks/stories
2017-05-16 perezju [common/battor] Fix errors while logging
2017-05-16 nednguyen Remove webrtc_rendering_stats TBMv1 metrics
2017-05-16 perezju [Telemetry] Stop tracing/metrics before closing the browser
2017-05-16 nednguyen Remove v8_gc_latency TBMv1 metric
2017-05-16 ulan Revert of Fix main frame detection in loading metrics. (patchset #7 id:120001 of https://codereview.chromium.org/2862103002/ )
2017-05-15 jreck Quick fix to avoid corruption from binder_parser
2017-05-15 ctzsm Rename AndroidWebView.apk to WebViewInstrumentation.apk
2017-05-15 ulan Fix main frame detection in loading metrics.

Created with:
  roll-dep src/third_party/catapult
BUG= 701938 ,711065, 719447 , 720317 , 692112 , 701938 , 692112 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=sullivan@chromium.org

Change-Id: I01ecaf24a0ad46e599b4e1272e8bb6204cabf75f
Reviewed-on: https://chromium-review.googlesource.com/507032
Reviewed-by: <catapult-deps-roller@chromium.org>
Commit-Queue: <catapult-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#472336}
[modify] https://crrev.com/26d71b4f8cbf1fd071ae1817918857613d9338f7/DEPS

Comment 26 by ctzsm@chromium.org, May 22 2017

Status: Fixed (was: Started)

Sign in to add a comment