Load v8 snapshots directly from APK (and store them uncompressed)

Load V8 startup data directly from the APK on Android.

Startup data files are now stored uncompressed within the .apk, increasing size of the apk, but decreasing total disk requirements (since they don't need to be extracted).

BUG=394502
Committed: https://crrev.com/6f3002d9e87d690bb368931faa2c3ba4638f811e
Cr-Commit-Position: refs/heads/master@{#335271}

[agrieve, 2015-05-25 15:42:57]

agrieve@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Yaron, 2015-05-25 18:56:44]

https://codereview.chromium.org/1156873002/diff/20001/chrome/android/chrome_a...
File chrome/android/chrome_apk.gypi (right):

https://codereview.chromium.org/1156873002/diff/20001/chrome/android/chrome_a...
chrome/android/chrome_apk.gypi:47: 'extensions_to_not_compress': 'bin',
Is there any gn support for this?

https://codereview.chromium.org/1156873002/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java
(left):

https://codereview.chromium.org/1156873002/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/ChromeMobileApplication.java:130:
"natives_blob.bin", "snapshot_blob.bin"
Should we add code to clean up old files? It's almost 1mb

https://codereview.chromium.org/1156873002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1156873002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2227: #if
defined(V8_USE_EXTERNAL_STARTUP_DATA) && !defined(OS_ANDROID)
I think a comment is helpful to explain why this isn't necessary on android.

https://codereview.chromium.org/1156873002/diff/20001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1156873002/diff/20001/content/app/content_mai...
content/app/content_main_runner.cc:721: kV8NativesDataDescriptor,
gin::V8Initializer::kNativesFileName));
Isn't this introducing new blocking IO on the main thread? I believe this used
to happen on the process launcher thread when we grab the fd to forward to child
process

https://codereview.chromium.org/1156873002/diff/20001/content/app/content_mai...
content/app/content_main_runner.cc:726: // with child processes via file
descriptors. On Android they are set in
update

[jochen (gone - plz use gerrit), 2015-05-26 07:26:42]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org, oth@chromium.org, rmcilroy@chromium.org

[rmcilroy, 2015-05-26 09:15:15]

+erikcorry

Erik is working on having Chrome generate V8's snapshot on-demand during first
run on Android (aimed for m45). This would mean that the snapshot_blob.bin file
wouldn't be in the APK at all (however  native_blob.bin would still be there). 

Erik: would this change cause you issues with the on-demand generation CL you
are working on?

https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
content/app/content_main_runner.cc:719: // The fds are already mapped in the
case of Android WebView.
Webview? Please update the comment.

Also, where have you added the code to do this pre-mapping? On Android Webview
this is done with AwAssets::RegisterAssetWithGlobalDescriptors [1], but I don't
see you doing anything similar here.

[1]
https://cs.corp.google.com/#clankium/src/android_webview/lib/main/aw_main_del...

https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
content/app/content_main_runner.cc:720: if
(g_fds->MaybeGet(kV8NativesDataDescriptor) == -1) {
Please use the switches::kV8NativesPassedByFD and 
switches::kV8SnapshotPassedByFD and reuse the code below (you will need to
ensure you set the switches correctly). Otherwise this will cause issues with
Erik's on-demand snapshot generation where the kSnapshotFileName might not be
already mapped when the renderer starts depending on whether it has been
generated yet or not.

[agrieve, 2015-05-26 14:04:59]

On 2015/05/26 09:15:15, rmcilroy wrote:
> +erikcorry
> 
> Erik is working on having Chrome generate V8's snapshot on-demand during first
> run on Android (aimed for m45). This would mean that the snapshot_blob.bin
file
> wouldn't be in the APK at all (however  native_blob.bin would still be there).

> 
> Erik: would this change cause you issues with the on-demand generation CL you
> are working on?
> 
>
https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
> File content/app/content_main_runner.cc (right):
> 
>
https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
> content/app/content_main_runner.cc:719: // The fds are already mapped in the
> case of Android WebView.
> Webview? Please update the comment.
> 
> Also, where have you added the code to do this pre-mapping? On Android Webview
> this is done with AwAssets::RegisterAssetWithGlobalDescriptors [1], but I
don't
> see you doing anything similar here.
> 
> [1]
>
https://cs.corp.google.com/#clankium/src/android_webview/lib/main/aw_main_del...
> 
>
https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
> content/app/content_main_runner.cc:720: if
> (g_fds->MaybeGet(kV8NativesDataDescriptor) == -1) {
> Please use the switches::kV8NativesPassedByFD and 
> switches::kV8SnapshotPassedByFD and reuse the code below (you will need to
> ensure you set the switches correctly). Otherwise this will cause issues with
> Erik's on-demand snapshot generation where the kSnapshotFileName might not be
> already mapped when the renderer starts depending on whether it has been
> generated yet or not.


Yaron pointed out that there is more IO happening in OpenApkAsset() than I
originally thought (on content's main thread), so I'm going to put this patch on
hold and go back to having the FDs passed in from
chrome_content_browser_client.cc.

[agrieve, 2015-05-27 19:14:01]

On 2015/05/26 14:04:59, agrieve wrote:
> On 2015/05/26 09:15:15, rmcilroy wrote:
> > +erikcorry
> > 
> > Erik is working on having Chrome generate V8's snapshot on-demand during
first
> > run on Android (aimed for m45). This would mean that the snapshot_blob.bin
> file
> > wouldn't be in the APK at all (however  native_blob.bin would still be
there).
> 
> > 
> > Erik: would this change cause you issues with the on-demand generation CL
you
> > are working on?
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
> > File content/app/content_main_runner.cc (right):
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
> > content/app/content_main_runner.cc:719: // The fds are already mapped in the
> > case of Android WebView.
> > Webview? Please update the comment.
> > 
> > Also, where have you added the code to do this pre-mapping? On Android
Webview
> > this is done with AwAssets::RegisterAssetWithGlobalDescriptors [1], but I
> don't
> > see you doing anything similar here.
> > 
> > [1]
> >
>
https://cs.corp.google.com/#clankium/src/android_webview/lib/main/aw_main_del...
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/40001/content/app/content_mai...
> > content/app/content_main_runner.cc:720: if
> > (g_fds->MaybeGet(kV8NativesDataDescriptor) == -1) {
> > Please use the switches::kV8NativesPassedByFD and 
> > switches::kV8SnapshotPassedByFD and reuse the code below (you will need to
> > ensure you set the switches correctly). Otherwise this will cause issues
with
> > Erik's on-demand snapshot generation where the kSnapshotFileName might not
be
> > already mapped when the renderer starts depending on whether it has been
> > generated yet or not.
> 
> 
> Yaron pointed out that there is more IO happening in OpenApkAsset() than I
> originally thought (on content's main thread), so I'm going to put this patch
on
> hold and go back to having the FDs passed in from
> chrome_content_browser_client.cc.

yfriedman@ - Please have a preliminary look at the new approach.  I know there
are some debug logs in there and formatting issues, but wanted to see if I'm
doing anything fundamentally wrong here.

[Yaron, 2015-05-27 20:59:43]

high-level seems fine although lots going on - maybe worth breaking up.

Thinking more about this, I'm wondering if we can only do this for L+? There was
a bug (crbug.com/451660) when trying to load the shared library causing a user
dialog on certain samsung devices. Wouldn't this cause the same problem?

[Yaron, 2015-05-27 21:54:23]

Ross: looks like you were involved with that bug, do you know whether this could
be a problem or perhaps it was just related to loading a shared library?

[Torne, 2015-05-28 09:54:47]

On 2015/05/27 21:54:23, Yaron wrote:
> Ross: looks like you were involved with that bug, do you know whether this
could
> be a problem or perhaps it was just related to loading a shared library?

If the V8 snapshot gets mmap()ed with PROT_EXEC then it will hit the same issue
as when loading shared libraries. The problem was that some devices on older
android versions didn't allow the APK to be mapped with execute permissions.

[rmcilroy, 2015-05-28 11:47:12]

On 2015/05/28 09:54:47, Torne wrote:
> On 2015/05/27 21:54:23, Yaron wrote:
> > Ross: looks like you were involved with that bug, do you know whether this
> could
> > be a problem or perhaps it was just related to loading a shared library?
> 
> If the V8 snapshot gets mmap()ed with PROT_EXEC then it will hit the same
issue
> as when loading shared libraries. The problem was that some devices on older
> android versions didn't allow the APK to be mapped with execute permissions.

The V8 snapshot doesn't get mmapped with PROT_EXEC so I don't think we should
have issues like we were seeing in crbug.com/451660.

[rmcilroy, 2015-05-28 12:03:29]

However, this change seems to be doing way more than the previous one, and from
a brief glance I think it would break Erik's CL which generates the snapshot
on-demand (which should provide bigger space/download savings than loading the
snapshot from within the APK). Could you work with Erik to ensure that both
these changes can work together please.

https://codereview.chromium.org/1156873002/diff/60001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1156873002/diff/60001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:201: OpenV8FilesForChildProcesses(&files);
I don't think you should be calling OpenV8FilesForChildProcesses here - you
should only call this once from the browser process and the caller should keep
track of the opened file descriptors to pass them to the child processes. This
is to avoid situations on Linux where an update which Chrome is running might
update the underlying snapshot files, but we still want to pass an FD to the
pre-update snapshot files to renderer processes since they are still running the
pre-update executable (see crbug.com/457656)

[rmcilroy, 2015-05-28 12:47:37]

rmcilroy@chromium.org changed reviewers:
+ erikcorry@chromium.org

[agrieve, 2015-05-28 18:42:55]

On 2015/05/28 12:03:29, rmcilroy wrote:
> However, this change seems to be doing way more than the previous one, and
from
> a brief glance I think it would break Erik's CL which generates the snapshot
> on-demand (which should provide bigger space/download savings than loading the
> snapshot from within the APK). Could you work with Erik to ensure that both
> these changes can work together please.
> 
> https://codereview.chromium.org/1156873002/diff/60001/gin/v8_initializer.cc
> File gin/v8_initializer.cc (right):
> 
>
https://codereview.chromium.org/1156873002/diff/60001/gin/v8_initializer.cc#n...
> gin/v8_initializer.cc:201: OpenV8FilesForChildProcesses(&files);
> I don't think you should be calling OpenV8FilesForChildProcesses here - you
> should only call this once from the browser process and the caller should keep
> track of the opened file descriptors to pass them to the child processes. This
> is to avoid situations on Linux where an update which Chrome is running might
> update the underlying snapshot files, but we still want to pass an FD to the
> pre-update snapshot files to renderer processes since they are still running
the
> pre-update executable (see crbug.com/457656)

I've moved the child process to a separate review:
https://codereview.chromium.org/1156183003/ (feel free to wait on this one first
- will comment again when that's set).

Also renamed OpenV8FilesForChildProcesses -> OpenV8Files, and made it operate on
ScopedFd rather than File.

[jochen (gone - plz use gerrit), 2015-06-05 12:07:47]

I'm waiting for Ross to review this, happy to rubberstamp after he's done

[rmcilroy, 2015-06-05 15:16:41]

On 2015/06/05 12:07:47, jochen wrote:
> I'm waiting for Ross to review this, happy to rubberstamp after he's done

Erik's change is just landing in https://codereview.chromium.org/1164483003/.
Could you rebase on top of that and I'll have another look, thanks.

[Erik Corry Chromium.org, 2015-06-08 13:27:51]

Looks like my change, mentioned by Ross in the previous message, stuck.

[agrieve, 2015-06-08 14:42:35]

On 2015/06/08 13:27:51, Erik Corry Chromium.org wrote:
> Looks like my change, mentioned by Ross in the previous message, stuck.

Awesome! I'll rebase and ping back when it's ready.

[agrieve, 2015-06-08 20:35:41]

On 2015/06/08 14:42:35, agrieve wrote:
> On 2015/06/08 13:27:51, Erik Corry http://Chromium.org wrote:
> > Looks like my change, mentioned by Ross in the previous message, stuck.
> 
> Awesome! I'll rebase and ping back when it's ready.

This is now ready for review :).

[rmcilroy, 2015-06-09 11:52:55]

Looks good overall. A couple of comments but mostly nits.


Also, could you update the CL description to be "Load V8 startup data directly
from the APK on Android", and mention the uncompressed storage in the CL
description.

https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
ResourceExtractor.setMandatoryPaksToExtract("");
Can we just remove this call to setMandatoryPaksToExtract now?

https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
File content/shell/browser/shell_content_browser_client.h (right):

https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
content/shell/browser/shell_content_browser_client.h:114:
gin::V8Initializer::V8Files v8_files_;
Previously the fds were being stored as ScopedFD which meant they would be
closed when this object was destructed - this is no longer the case (they are
stored as raw PlatformFiles).  I think it should be OK to just make the fields
in V8Files base::ScopedFDs then the FDs will be closed automatically when the
V8Files object is destructed.

https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
File extensions/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
extensions/shell/browser/shell_content_browser_client.cc:268: #if
defined(OS_ANDROID)
I don't think this needs to be #ifdef ANDROID - all other platforms should
return kWholeFile which will the be fine if passed to the LoadV8NativesFromFD.

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
align arguments (or run "git cl format" :))

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
nit - usual convention is to suffix with "_out" rather than prefix (i.e.,
region_out here)

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:110: int flags = base::File::FLAG_OPEN |
base::File::FLAG_READ;
nit - move flags down below with OpenV8FileResult result and add a newline
between "GetV8FilePath..." and "OpenV8FileResult result ..."

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:213: // start up (slower) without the snapshot.
nit - probably clearer as "Failure to load the V8 snapshot is not necessarily an
error. V8 can start up (slower) without the snapshot" (no need to mention
natives here I don't think.

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:288: V8Initializer::V8Files::V8Files() {
initialize the fds to "-1"

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
gin/v8_initializer.h:55: struct V8Files {
Make struct "final" (or destructor should be virtual)

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
gin/v8_initializer.h:55: struct V8Files {
nit - V8StartupData

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
gin/v8_initializer.h:58: base::ScopedFD natives_fd;
nit - newline between fields and constructor / destructor

https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
gin/v8_initializer.h:62: private:
nit - newline before "private"

[agrieve, 2015-06-11 15:07:53]

On 2015/06/09 11:52:55, rmcilroy wrote:
> Looks good overall. A couple of comments but mostly nits.
> 
> 
> Also, could you update the CL description to be "Load V8 startup data directly
> from the APK on Android", and mention the uncompressed storage in the CL
> description.
> 
>
https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
> File
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
> (right):
> 
>
https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
> ResourceExtractor.setMandatoryPaksToExtract("");
> Can we just remove this call to setMandatoryPaksToExtract now?
> 
>
https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
> File content/shell/browser/shell_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
> content/shell/browser/shell_content_browser_client.h:114:
> gin::V8Initializer::V8Files v8_files_;
> Previously the fds were being stored as ScopedFD which meant they would be
> closed when this object was destructed - this is no longer the case (they are
> stored as raw PlatformFiles).  I think it should be OK to just make the fields
> in V8Files base::ScopedFDs then the FDs will be closed automatically when the
> V8Files object is destructed.
> 
>
https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
> File extensions/shell/browser/shell_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
> extensions/shell/browser/shell_content_browser_client.cc:268: #if
> defined(OS_ANDROID)
> I don't think this needs to be #ifdef ANDROID - all other platforms should
> return kWholeFile which will the be fine if passed to the LoadV8NativesFromFD.
> 
> https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc
> File gin/v8_initializer.cc (right):
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
> align arguments (or run "git cl format" :))
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
> nit - usual convention is to suffix with "_out" rather than prefix (i.e.,
> region_out here)
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> gin/v8_initializer.cc:110: int flags = base::File::FLAG_OPEN |
> base::File::FLAG_READ;
> nit - move flags down below with OpenV8FileResult result and add a newline
> between "GetV8FilePath..." and "OpenV8FileResult result ..."
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> gin/v8_initializer.cc:213: // start up (slower) without the snapshot.
> nit - probably clearer as "Failure to load the V8 snapshot is not necessarily
an
> error. V8 can start up (slower) without the snapshot" (no need to mention
> natives here I don't think.
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> gin/v8_initializer.cc:288: V8Initializer::V8Files::V8Files() {
> initialize the fds to "-1"
> 
> https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h
> File gin/v8_initializer.h (right):
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> gin/v8_initializer.h:55: struct V8Files {
> Make struct "final" (or destructor should be virtual)
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> gin/v8_initializer.h:55: struct V8Files {
> nit - V8StartupData
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> gin/v8_initializer.h:58: base::ScopedFD natives_fd;
> nit - newline between fields and constructor / destructor
> 
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> gin/v8_initializer.h:62: private:
> nit - newline before "private"

Thanks Ross! Due to some feedback on https://codereview.chromium.org/1147213004/
(which this change depends on), I've had to refactor much of the v8 start-up to
be accessed from content/ (https://codereview.chromium.org/1182443003/). Made
you a reviewer for that as well since I've moved many of the changes to this CL
over to it.

I'll rebase this change once the other issue gets a bit of traction (once I know
it's not completely wrong :P).

[rmcilroy, 2015-06-11 17:32:15]

On 2015/06/11 15:07:53, agrieve wrote:
> On 2015/06/09 11:52:55, rmcilroy wrote:
> > Looks good overall. A couple of comments but mostly nits.
> > 
> > 
> > Also, could you update the CL description to be "Load V8 startup data
directly
> > from the APK on Android", and mention the uncompressed storage in the CL
> > description.
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
> > File
> >
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
> >
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
> > ResourceExtractor.setMandatoryPaksToExtract("");
> > Can we just remove this call to setMandatoryPaksToExtract now?
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
> > File content/shell/browser/shell_content_browser_client.h (right):
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
> > content/shell/browser/shell_content_browser_client.h:114:
> > gin::V8Initializer::V8Files v8_files_;
> > Previously the fds were being stored as ScopedFD which meant they would be
> > closed when this object was destructed - this is no longer the case (they
are
> > stored as raw PlatformFiles).  I think it should be OK to just make the
fields
> > in V8Files base::ScopedFDs then the FDs will be closed automatically when
the
> > V8Files object is destructed.
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
> > File extensions/shell/browser/shell_content_browser_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
> > extensions/shell/browser/shell_content_browser_client.cc:268: #if
> > defined(OS_ANDROID)
> > I don't think this needs to be #ifdef ANDROID - all other platforms should
> > return kWholeFile which will the be fine if passed to the
LoadV8NativesFromFD.
> > 
> > https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc
> > File gin/v8_initializer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
> > align arguments (or run "git cl format" :))
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
> > nit - usual convention is to suffix with "_out" rather than prefix (i.e.,
> > region_out here)
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > gin/v8_initializer.cc:110: int flags = base::File::FLAG_OPEN |
> > base::File::FLAG_READ;
> > nit - move flags down below with OpenV8FileResult result and add a newline
> > between "GetV8FilePath..." and "OpenV8FileResult result ..."
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > gin/v8_initializer.cc:213: // start up (slower) without the snapshot.
> > nit - probably clearer as "Failure to load the V8 snapshot is not
necessarily
> an
> > error. V8 can start up (slower) without the snapshot" (no need to mention
> > natives here I don't think.
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > gin/v8_initializer.cc:288: V8Initializer::V8Files::V8Files() {
> > initialize the fds to "-1"
> > 
> > https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h
> > File gin/v8_initializer.h (right):
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > gin/v8_initializer.h:55: struct V8Files {
> > Make struct "final" (or destructor should be virtual)
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > gin/v8_initializer.h:55: struct V8Files {
> > nit - V8StartupData
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > gin/v8_initializer.h:58: base::ScopedFD natives_fd;
> > nit - newline between fields and constructor / destructor
> > 
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > gin/v8_initializer.h:62: private:
> > nit - newline before "private"
> 
> Thanks Ross! Due to some feedback on
https://codereview.chromium.org/1147213004/
> (which this change depends on), I've had to refactor much of the v8 start-up
to
> be accessed from content/ (https://codereview.chromium.org/1182443003/). Made
> you a reviewer for that as well since I've moved many of the changes to this
CL
> over to it.
> 
> I'll rebase this change once the other issue gets a bit of traction (once I
know
> it's not completely wrong :P).

Sounds good, thanks.

[agrieve, 2015-06-13 02:20:41]

On 2015/06/11 17:32:15, rmcilroy wrote:
> On 2015/06/11 15:07:53, agrieve wrote:
> > On 2015/06/09 11:52:55, rmcilroy wrote:
> > > Looks good overall. A couple of comments but mostly nits.
> > > 
> > > 
> > > Also, could you update the CL description to be "Load V8 startup data
> directly
> > > from the APK on Android", and mention the uncompressed storage in the CL
> > > description.
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
> > > File
> > >
> >
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/android_webview/test/s...
> > >
> >
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
> > > ResourceExtractor.setMandatoryPaksToExtract("");
> > > Can we just remove this call to setMandatoryPaksToExtract now?
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
> > > File content/shell/browser/shell_content_browser_client.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/content/shell/browser/...
> > > content/shell/browser/shell_content_browser_client.h:114:
> > > gin::V8Initializer::V8Files v8_files_;
> > > Previously the fds were being stored as ScopedFD which meant they would be
> > > closed when this object was destructed - this is no longer the case (they
> are
> > > stored as raw PlatformFiles).  I think it should be OK to just make the
> fields
> > > in V8Files base::ScopedFDs then the FDs will be closed automatically when
> the
> > > V8Files object is destructed.
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
> > > File extensions/shell/browser/shell_content_browser_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/extensions/shell/brows...
> > > extensions/shell/browser/shell_content_browser_client.cc:268: #if
> > > defined(OS_ANDROID)
> > > I don't think this needs to be #ifdef ANDROID - all other platforms should
> > > return kWholeFile which will the be fine if passed to the
> LoadV8NativesFromFD.
> > > 
> > >
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc
> > > File gin/v8_initializer.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > > gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
> > > align arguments (or run "git cl format" :))
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > > gin/v8_initializer.cc:94: base::MemoryMappedFile::Region* out_region) {
> > > nit - usual convention is to suffix with "_out" rather than prefix (i.e.,
> > > region_out here)
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > > gin/v8_initializer.cc:110: int flags = base::File::FLAG_OPEN |
> > > base::File::FLAG_READ;
> > > nit - move flags down below with OpenV8FileResult result and add a newline
> > > between "GetV8FilePath..." and "OpenV8FileResult result ..."
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > > gin/v8_initializer.cc:213: // start up (slower) without the snapshot.
> > > nit - probably clearer as "Failure to load the V8 snapshot is not
> necessarily
> > an
> > > error. V8 can start up (slower) without the snapshot" (no need to mention
> > > natives here I don't think.
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.cc#...
> > > gin/v8_initializer.cc:288: V8Initializer::V8Files::V8Files() {
> > > initialize the fds to "-1"
> > > 
> > >
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h
> > > File gin/v8_initializer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > > gin/v8_initializer.h:55: struct V8Files {
> > > Make struct "final" (or destructor should be virtual)
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > > gin/v8_initializer.h:55: struct V8Files {
> > > nit - V8StartupData
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > > gin/v8_initializer.h:58: base::ScopedFD natives_fd;
> > > nit - newline between fields and constructor / destructor
> > > 
> > >
> >
>
https://codereview.chromium.org/1156873002/diff/200001/gin/v8_initializer.h#n...
> > > gin/v8_initializer.h:62: private:
> > > nit - newline before "private"
> > 
> > Thanks Ross! Due to some feedback on
> https://codereview.chromium.org/1147213004/
> > (which this change depends on), I've had to refactor much of the v8 start-up
> to
> > be accessed from content/ (https://codereview.chromium.org/1182443003/).
Made
> > you a reviewer for that as well since I've moved many of the changes to this
> CL
> > over to it.
> > 
> > I'll rebase this change once the other issue gets a bit of traction (once I
> know
> > it's not completely wrong :P).
> 
> Sounds good, thanks.

This is now rebased and ready for review (and much smaller, yay!)

[agrieve, 2015-06-13 02:21:28]

agrieve@chromium.org changed reviewers:
+ gunsch@chromium.org, torne@chromium.org

[agrieve, 2015-06-13 02:21:29]

gunsch@chromium.org: Please review changes in android_webview

torne@chromium.org: Please review changes in chromecast

https://codereview.chromium.org/1156873002/diff/60001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1156873002/diff/60001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:201: OpenV8FilesForChildProcesses(&files);
On 2015/05/28 12:03:29, rmcilroy wrote:
> I don't think you should be calling OpenV8FilesForChildProcesses here - you
> should only call this once from the browser process and the caller should keep
> track of the opened file descriptors to pass them to the child processes. This
> is to avoid situations on Linux where an update which Chrome is running might
> update the underlying snapshot files, but we still want to pass an FD to the
> pre-update snapshot files to renderer processes since they are still running
the
> pre-update executable (see crbug.com/457656)

I did this just because the two functions are copy & pastes of each other (plus
the extra bit below). How about just dropping the "ForChildProcesses" part?

[gunsch-google, 2015-06-13 18:13:08]

gunsch@google.com changed reviewers:
+ gunsch@google.com

[gunsch-google, 2015-06-13 18:13:09]

rs lgtm for chromecast/

[gunsch, 2015-06-13 18:13:23]

oops, wrong account. rs lgtm for chromecast/

[rmcilroy, 2015-06-15 10:21:02]

Generally looks good. A couple of comments.

https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
ResourceExtractor.setMandatoryPaksToExtract("");
Same question as before - can we just remove this call to
setMandatoryPaksToExtract now?

https://codereview.chromium.org/1156873002/diff/240001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1156873002/diff/240001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:52: const char kSnapshotFileName[] =
"assets/snapshot_blob.bin";
this is effective the file path (within the APK) not the filename, right? I
think it would make more sense to have an #ifdef ANDROID in GetV8FilePath which
returns this. Also it might be worth just moving kV8SnapshotBasePathKey in there
(under the #if !defined(OS_MACOSX)) too since it's the only place this is used.

[agrieve, 2015-06-15 14:22:04]

https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
ResourceExtractor.setMandatoryPaksToExtract("");
On 2015/06/15 10:21:02, rmcilroy wrote:
> Same question as before - can we just remove this call to
> setMandatoryPaksToExtract now?

We can't. ResourceExtractor asserts that setMandatoryPaksToExtract() has been
called rather than defaulting to not doing anything. I might look at changing
that after moving .pak files to being loaded uncompressed since then it should
be rather uncommon to require it.

https://codereview.chromium.org/1156873002/diff/240001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1156873002/diff/240001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:52: const char kSnapshotFileName[] =
"assets/snapshot_blob.bin";
On 2015/06/15 10:21:02, rmcilroy wrote:
> this is effective the file path (within the APK) not the filename, right? I
> think it would make more sense to have an #ifdef ANDROID in GetV8FilePath
which
> returns this. Also it might be worth just moving kV8SnapshotBasePathKey in
there
> (under the #if !defined(OS_MACOSX)) too since it's the only place this is
used.

Done.

[rmcilroy, 2015-06-15 17:39:42]

lgtm, thanks.

https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
ResourceExtractor.setMandatoryPaksToExtract("");
On 2015/06/15 14:22:04, agrieve wrote:
> On 2015/06/15 10:21:02, rmcilroy wrote:
> > Same question as before - can we just remove this call to
> > setMandatoryPaksToExtract now?
> 
> We can't. ResourceExtractor asserts that setMandatoryPaksToExtract() has been
> called rather than defaulting to not doing anything. I might look at changing
> that after moving .pak files to being loaded uncompressed since then it should
> be rather uncommon to require it.

Ok makes sense. Could you add a TODO to tidy this up later then please.

[agrieve, 2015-06-15 19:30:41]

On 2015/06/15 17:39:42, rmcilroy wrote:
> lgtm, thanks.
> 
>
https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
> File
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
> (right):
> 
>
https://codereview.chromium.org/1156873002/diff/240001/android_webview/test/s...
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:49:
> ResourceExtractor.setMandatoryPaksToExtract("");
> On 2015/06/15 14:22:04, agrieve wrote:
> > On 2015/06/15 10:21:02, rmcilroy wrote:
> > > Same question as before - can we just remove this call to
> > > setMandatoryPaksToExtract now?
> > 
> > We can't. ResourceExtractor asserts that setMandatoryPaksToExtract() has
been
> > called rather than defaulting to not doing anything. I might look at
changing
> > that after moving .pak files to being loaded uncompressed since then it
should
> > be rather uncommon to require it.
> 
> Ok makes sense. Could you add a TODO to tidy this up later then please.

Done. (Added to ResourceExtractor.setMandatoryPaksToExtract())

[Yaron, 2015-06-16 03:22:29]

lgtm

[jochen (gone - plz use gerrit), 2015-06-16 13:30:55]

lgtm

[Torne, 2015-06-18 10:20:53]

android_webview LGTM

[agrieve, 2015-06-19 15:40:12]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-19 15:40:13]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@google.com,
gunsch@chromium.org, rmcilroy@chromium.org, jochen@chromium.org,
torne@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1156873002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-19 15:40:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156873002/320001

[commit-bot: I haz the power, 2015-06-19 16:49:16]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-06-19 16:50:20]

Patchset 17 (id:??) landed as
https://crrev.com/6f3002d9e87d690bb368931faa2c3ba4638f811e
Cr-Commit-Position: refs/heads/master@{#335271}