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

Issue 688077 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Monochrome contains about:credits twice (300kb)

Project Member Reported by agrieve@chromium.org, Feb 2 2017

Issue description

Looks to be used here:
https://cs.corp.google.com/clankium/src/android_webview/glue/java/src/com/android/webview/chromium/LicenseContentProvider.java?type=cs&q=webview_licenses.notice&sq=package:clankium&l=36

It's in the apk directly:
 1883096  Defl:N   317478  83% 2001-01-01 00:00 9049bec8  assets/webview_licenses.notice

It's also within resources.pak as a 70kb brotli-compressed file.

For Monochrome, we should omit the file, load up the native library in the content provider, and serve the brotli-compressed version from the .pak file.
 
The content are same? If so, why WebView has its own way to generate the licenses.notice.

FYI, the WebView license is used in android's settings-> about phone->legal information-> System WebView licenses

Comment 2 by torne@chromium.org, Feb 3 2017

Loading the native code in the content provider seems like it might be a huge pain; we don't have any components in the webview package that actually load the library yet. The other way around might work better, but I guess we don't have a brotli decompressor available in java..

Also, as Michael notes, we have a separate script for generating it I think, so I'm not sure the contents are actually the same (they might be all the same licenses, but are the bytes literally identical?), and we may need to check what's different first to see if it's important :)
Certainly the contents are a bit different. Webview is plain text, whereas about:credits is html.

They are approximately the same uncompressed size though, so I suspect the licenses are the same. At some point I saw mention of webview licenses having to be extracted while webview was a part of the Android tree, so perhaps that's the reason for the different scripts.

If we did some pre-processing on the licenses (e.g. consistent whitespace / line-wrapping / remove comment markers), then I'm betting we vastly reduce the size of them without needing brotli (is this allowed)?.


Perhaps we could store them in a stand-alone (zip-compressed) .json file and then have the content provider format them as a big string, as well as have about:credits fetch them with javascript.

If we store by json, we should try and see if sorting them by size will cause similar (nay, identical) licenses to appear close together, helping out gzip.

Comment 4 by torne@chromium.org, Feb 6 2017

As long as they're stored in a form that the WebView content provider can read with just Java code then that's definitely ok. If we do find that the best way to save bytes is brotli, then we can look into whether it's sufficiently straightforward to bring up enough of native to do that.
It is probably not a big deal to load native code, because the activity uses WebView to display license, WebView will be loaded anyway; there still has extra cost to get license by loading native code, but this feature is seldom used.

Comment 6 by torne@chromium.org, Feb 7 2017

No, that's not correct. The license activity doesn't use WebView - it's just a forwarding activity with no UI which launches a HTMLViewer intent.

It's not the performance cost of loading the native library I'm worried about, it's the fact that 1) we've never loaded the native library into WebView's own process before, and we'd want to be careful about how we do this to make sure we get the benefits of webview library loading, and 2) we don't currently have any obvious way to bind native functions to Java code that's unrelated to webview itself. We could work this out and it might not be that tricky, but if there's a reasonable way to solve the actual problem without needing to do this it'd be easier :)
I didn't talk about license activity, instead, the activity (it should be HTML Viewer) loads the WebView.

The thing I don't understand is '1) we've never loaded the native library into WebView's own process before'? Could you give more information?

Comment 8 by torne@chromium.org, Feb 7 2017

HTMLViewer doesn't load the content from the APK, though - it loads it from the content provider that *does* run in the webview package, and does not currently load the native code or use webview at all. There's no way for HTMLViewer to open an asset in the webview APK directly.

I'm not sure what more information there is to give: the actual process that runs for the webview package to launch the content provider, the license forwarding activity, and now things like the crash uploading service, has never loaded the webview native code. All those things are implemented in pure Java and don't themselves use WebView, so this would be the first time we have actually loaded up the library in the process belonging to the webview package instead of just loading it in a client app.
Thanks to explain, I missed the fact that the process is running on WebView package.
Labels: Performance-Browser
Labels: -apk-size binary-size
Summary: Monochrome contains about:credits twice (300kb) (was: Monochrome contains about:credits twice)
Labels: Quick-Win
Labels: intern
Owner: yipengw@chromium.org
Yipeng has started to look at this. Findings so far:
1. Sorting licenses by size makes them gzip to 200kb rather than 300kb (.bro is the same)
2. GVR is now our largest license (a concatenation of them). It is 87kb gzipped! Next largest is bionic, which is 11kb gzipped.
3. about:credits differs from webview credits in that it has javascript. Webview could have <script> as well I think, but we'd need to use nonce-based <script> in about:chrome to appease Content Security Policy restrictions.

It doesn't look like we'll get below 200kb if sticking with gzip, which still seems unacceptably large to me :(.

I'd like to pursue using the brotli copy only for Monochrome (not webview). 

Concrete steps to get there:
1. Change chrome://credits to use inline <script> tags for webview, but external ones for chrome.
  https://cs.chromium.org/chromium/src/components/about_ui/resources/about_credits.tmpl
   * Truncate it just before the <script> tags
  https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/about_ui.cc?type=cs&q=IDR_ABOUT_UI_CREDITS_JS&l=713
   * Extract the logic of creating the std::string for the html into a function, which takes a boolean for whether to inline the scripts
   * If false, insert the two externs <script> tags by appending to the string
   * If true, manually append the contents of IDR_ABOUT_UI_CREDITS_JS in a <script>

2. Make Monochrome use the same copy of credits:
  https://cs.chromium.org/chromium/src/android_webview/glue/java/src/com/android/webview/chromium/LicenseContentProvider.java
    * Look for webview_licenses.notice within assets, and if there, continue as normal
    * If not there (monochrome case):
      * Init native library via:
            LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER).ensureInitialized();
            BrowserStartupController.get(LibraryProcessType.PROCESS_BROWSER)
                    .startBrowserProcessesSync(false);
      * Use JNI to call the function from #1, with <script> inlined

3. Remove webview_licenses.notice asset when building monochrome_public_apk.
  https://cs.chromium.org/chromium/src/android_webview/BUILD.gn?q=webview_licenses&sq=package:chromium&l=128
    * Remove generate_webview_license_notice and webview_license_path from "monochrome_webview_assets"

Comment 16 by torne@chromium.org, Apr 25 2017

Looks like a good start; we can possibly go further later and also do this for webview but there are more complex cases with loading the library (especially in the new webview stub). Just having it work in Monochrome would be great.

Comment 17 by torne@chromium.org, Apr 25 2017

Cc: torne@chromium.org
Note: filed bug 715147 about gvr's license file
Project Member

Comment 19 by bugdroid1@chromium.org, Apr 27 2017

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

commit c54fcea42957f1effa94fedcb2d985f11488888c
Author: Yipeng Wang <yipengw@chromium.org>
Date: Thu Apr 27 01:08:29 2017

[about:credits] Improve compression by sorting licenses

Restore the alphabetical order in javascript.

Saves around 100k when gzip is used, but only 2k for brotli.
Intention is to have android_webview to use the same copy
of about:credits in a follow-up, in which case this optimization
becomes relevant.

BUG= 688077 

Change-Id: I24d62a81c61c2c53f5740f90d0c8094d3b04352e
Reviewed-on: https://chromium-review.googlesource.com/487725
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Yipeng Wang <yipengw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#467537}
[modify] https://crrev.com/c54fcea42957f1effa94fedcb2d985f11488888c/components/about_ui/resources/about_credits.js
[modify] https://crrev.com/c54fcea42957f1effa94fedcb2d985f11488888c/components/about_ui/resources/about_credits.tmpl
[modify] https://crrev.com/c54fcea42957f1effa94fedcb2d985f11488888c/tools/licenses.py

Project Member

Comment 20 by bugdroid1@chromium.org, Apr 28 2017

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

commit ba3fb344a4ad428165507ebbd82be1b2ffe48c6c
Author: Andrew Grieve <agrieve@chromium.org>
Date: Fri Apr 28 22:13:41 2017

Revert "[about:credits] Improve compression by sorting licenses"

This reverts commit c54fcea42957f1effa94fedcb2d985f11488888c.

Reason for revert: Broke downstream monochrome build.
https://bugs.chromium.org/p/chromium/issues/detail?id=716425

Original change's description:
> [about:credits] Improve compression by sorting licenses
> 
> Restore the alphabetical order in javascript.
> 
> Saves around 100k when gzip is used, but only 2k for brotli.
> Intention is to have android_webview to use the same copy
> of about:credits in a follow-up, in which case this optimization
> becomes relevant.
> 
> BUG= 688077 
> 
> Change-Id: I24d62a81c61c2c53f5740f90d0c8094d3b04352e
> Reviewed-on: https://chromium-review.googlesource.com/487725
> Reviewed-by: Richard Coles <torne@chromium.org>
> Reviewed-by: Dan Beam <dbeam@chromium.org>
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Commit-Queue: Yipeng Wang <yipengw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#467537}

TBR=dbeam@chromium.org,torne@chromium.org,agrieve@chromium.org,yipengw@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
BUG= 688077 

Change-Id: I08d4da9f1c7ab45b15b14973d334feb4485c3ee6
Reviewed-on: https://chromium-review.googlesource.com/490709
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#468152}
[modify] https://crrev.com/ba3fb344a4ad428165507ebbd82be1b2ffe48c6c/components/about_ui/resources/about_credits.js
[modify] https://crrev.com/ba3fb344a4ad428165507ebbd82be1b2ffe48c6c/components/about_ui/resources/about_credits.tmpl
[modify] https://crrev.com/ba3fb344a4ad428165507ebbd82be1b2ffe48c6c/tools/licenses.py

Project Member

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

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

commit 0534ad76ecae8af1ea088bec38536de6d736c887
Author: Yipeng Wang <yipengw@chromium.org>
Date: Mon May 01 20:49:29 2017

Reland: [about:credits] Improve compression by sorting licenses

Reverted in: ba3fb344a4ad428165507ebbd82be1b2ffe48c6c.

Reason for reland: Fixed sort order when two licenses of same length exist.

Restore the alphabetical order in javascript.

Saves around 100k when gzip is used, but only 2k for brotli.
Intention is to have android_webview to use the same copy
of about:credits in a follow-up, in which case this optimization
becomes relevant.

TBR=dbeam@chromium.org
BUG= 688077 
Change-Id: I085626bc2c81933ae973fb9762b6c3694318855b

Change-Id: I085626bc2c81933ae973fb9762b6c3694318855b
Reviewed-on: https://chromium-review.googlesource.com/490718
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#468402}
[modify] https://crrev.com/0534ad76ecae8af1ea088bec38536de6d736c887/components/about_ui/resources/about_credits.js
[modify] https://crrev.com/0534ad76ecae8af1ea088bec38536de6d736c887/components/about_ui/resources/about_credits.tmpl
[modify] https://crrev.com/0534ad76ecae8af1ea088bec38536de6d736c887/tools/licenses.py

Here are the diff between webview and chrome licenses. 

 Skia
 Snappy: A fast compressor/decompressor
 Speech Dispatcher
+Strongtalk
 Sudden Motion Sensor library
 SwiftShader
-The Chromium Project
 The USB ID Repository
 The library to input, validate, and display addresses.
 V8 JavaScript Engine
@@ -101,6 +101,7 @@
 drawElements Quality Program
 dynamic annotations
 etc1
+fdlibm
 ffmpeg
 fips181
 flac

* "The Chromium Project" license is only included in webview. 
* "Strongtalk" and "fdlibm" are only included in chrome
Labels: -binary-size Performance-Size
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 8 2017

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

commit 5671d4fe7016d6a2306db39021fb16d51c6ac543
Author: Yipeng Wang <yipengw@chromium.org>
Date: Thu Jun 08 19:29:40 2017

Make monochrome use the same copy of licenses as chrome

1. Compare chrome and webview licenses, and add the Chromium project license;
2. Get the Brotli decompression function and put it in component/about_ui/;
3. make monochrome call the function via JNI.

Bug:  688077 
Change-Id: I125dce46f85d3b268d9cee6d19249905f79f0209
Reviewed-on: https://chromium-review.googlesource.com/497870
Commit-Queue: Yipeng Wang <yipengw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478052}
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/android_webview/BUILD.gn
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/android_webview/apk/BUILD.gn
[rename] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/android_webview/apk/java/src/com/android/webview/chromium/LicenseActivity.java
[rename] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/android_webview/apk/java/src/com/android/webview/chromium/LicenseContentProvider.java
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/android_webview/glue/BUILD.gn
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/chrome/android/chrome_public_apk_tmpl.gni
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/chrome/android/monochrome/BUILD.gn
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/chrome/android/monochrome/java/DEPS
[copy] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/chrome/android/monochrome/java/src/com/android/webview/chromium/LicenseContentProvider.java
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/chrome/browser/ui/webui/DEPS
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/chrome/browser/ui/webui/about_ui.cc
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/BUILD.gn
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/DEPS
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/android/BUILD.gn
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/android/OWNERS
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/android/java/src/org/chromium/components/aboutui/CreditUtils.java
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/credit_utils.cc
[add] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/credit_utils.h
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/resources/about_credits.js
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/resources/about_credits.tmpl
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/components/about_ui/resources/about_credits_entry.tmpl
[modify] https://crrev.com/5671d4fe7016d6a2306db39021fb16d51c6ac543/tools/licenses.py

Project Member

Comment 25 by bugdroid1@chromium.org, Jun 8 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/d9d677e6c881b7dec9b3f29a85ea10816fbada7e

commit d9d677e6c881b7dec9b3f29a85ea10816fbada7e
Author: Yipeng Wang <yipengw@google.com>
Date: Thu Jun 08 19:54:21 2017

Status: Fixed (was: Available)
Filed a follow-up for applying this to webview & stub:
https://bugs.chromium.org/p/chromium/issues/detail?id=734846

Sign in to add a comment