Infrastructure for reading V8's initial snapshot from external files

Infrastructure for enabling V8's initial snapshot to be loaded from external files as opposed to being statically linked to the binary.

This is not currently supported on any architecture.

BUG=421063
Committed: https://crrev.com/3a3c88a61fc3fc5b001e7bae70e6335d3cf7c997
Cr-Commit-Position: refs/heads/master@{#301595}

[baixo, 2014-09-22 17:56:10]

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

[baixo, 2014-09-22 17:56:11]

Please take a look.

[rmcilroy, 2014-09-23 10:04:20]

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

https://codereview.chromium.org/594603003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:16: #endif
I don't think you need this here?

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

https://codereview.chromium.org/594603003/diff/20001/content/app/content_main...
content/app/content_main_runner.cc:20: #endif
I'm not sure you need this here either?

https://codereview.chromium.org/594603003/diff/20001/content/app/content_main...
content/app/content_main_runner.cc:736: #else
nit - newline before the #else

https://codereview.chromium.org/594603003/diff/20001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:38: base::MemoryMappedFile IsolateHolder::mapped_snapshot;
We only allow POD types (not class objects) as static variables in Chrome code
(there is a limit on static initializers which means we avoid static object's). 
You could make these both pointers (which would avoid the need for the bool
since you could just check whether the points are NULL or not.

Also, please move these into the anonymous namespace above.

https://codereview.chromium.org/594603003/diff/20001/gin/public/isolate_holder.h
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/20001/gin/public/isolate_holde...
gin/public/isolate_holder.h:56: static bool read_snapshot;
You shouldn't have these static members here (you are using static globals in
isolate_holder.cc, and similarly to the comment I made there, we don't allow
static member variables in classes (other than "static const" POD types).

[baixo, 2014-09-23 11:36:47]

Addressed Ross' comments.

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

https://codereview.chromium.org/594603003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:16: #endif
On 2014/09/23 10:04:20, rmcilroy wrote:
> I don't think you need this here?

Done.

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

https://codereview.chromium.org/594603003/diff/20001/content/app/content_main...
content/app/content_main_runner.cc:20: #endif
On 2014/09/23 10:04:20, rmcilroy wrote:
> I'm not sure you need this here either?

Done.

https://codereview.chromium.org/594603003/diff/20001/content/app/content_main...
content/app/content_main_runner.cc:736: #else
On 2014/09/23 10:04:20, rmcilroy wrote:
> nit - newline before the #else

Done.

https://codereview.chromium.org/594603003/diff/20001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/20001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:38: base::MemoryMappedFile IsolateHolder::mapped_snapshot;
On 2014/09/23 10:04:20, rmcilroy wrote:
> We only allow POD types (not class objects) as static variables in Chrome code
> (there is a limit on static initializers which means we avoid static
object's). 
> You could make these both pointers (which would avoid the need for the bool
> since you could just check whether the points are NULL or not.

Done.

> 
> Also, please move these into the anonymous namespace above.

This is not possible because the anonymous namespace does not enclose the
IsolateHolder namespace.

https://codereview.chromium.org/594603003/diff/20001/gin/public/isolate_holder.h
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/20001/gin/public/isolate_holde...
gin/public/isolate_holder.h:56: static bool read_snapshot;
On 2014/09/23 10:04:20, rmcilroy wrote:
> You shouldn't have these static members here (you are using static globals in
> isolate_holder.cc, and similarly to the comment I made there, we don't allow
> static member variables in classes (other than "static const" POD types).

Done.

[rmcilroy, 2014-09-23 14:05:40]

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:16: 
nit - Generally Ifdef'ed defines are placed in a separate section below the
unconditional includes.  Could you do that here and in the other files please.

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:43: base::MemoryMappedFile* IsolateHolder::mapped_snapshot
= NULL;
As mentioned, these should be globals, not static members (as with
g_array_buffer_allocator above).  Please also rename to prefix with "g_" and
move to the above anonymous namespace (which is possible when they are globals
not static members.

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:55: if
(!mapped_natives->Initialize(base::File(natives_fd))) {
create the new mapped_snapshot here rather than above such that it remains null
if the load fails.

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:61: mapped_snapshot = new base::MemoryMappedFile;
ditto

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:103: }
There seems like a lot of duplication here between LoadV8Snapshot and
LoadV8SnapshotFD.  Could you split some code out into shared functions?

https://codereview.chromium.org/594603003/diff/40001/gin/public/isolate_holder.h
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/40001/gin/public/isolate_holde...
gin/public/isolate_holder.h:55: static base::MemoryMappedFile* mapped_snapshot;
As mentioned, remove these from this class and make them globals in
isolate_holder.cc.

https://codereview.chromium.org/594603003/diff/40001/gin/shell/gin_main.cc
File gin/shell/gin_main.cc (right):

https://codereview.chromium.org/594603003/diff/40001/gin/shell/gin_main.cc#ne...
gin/shell/gin_main.cc:66: #endif // V8_USE_EXTERNAL_STARTUP_DATA
nit - move this block to be below gin::IsolateHolder::Initialize (and search for
gin::IsolateHolder::Initialize elsewhere in the codebase and add the
gin::IsolateHolder::LoadV8Snapshot(); below those calls, which should fix the
other test failures you are seeing (and avoid the gin dependency issues you had
before)..

https://codereview.chromium.org/594603003/diff/40001/mojo/apps/js/main.cc
File mojo/apps/js/main.cc (right):

https://codereview.chromium.org/594603003/diff/40001/mojo/apps/js/main.cc#new...
mojo/apps/js/main.cc:17: #endif // V8_USE_EXTERNAL_STARTUP_DATA
Ditto - move this to the call to be below the call to
gin::IsolateHolder::LoadV8Snapshot() in mojo.

[baixo, 2014-09-23 19:30:30]

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:16: 
On 2014/09/23 14:05:40, rmcilroy wrote:
> nit - Generally Ifdef'ed defines are placed in a separate section below the
> unconditional includes.  Could you do that here and in the other files please.

Done.

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:43: base::MemoryMappedFile* IsolateHolder::mapped_snapshot
= NULL;
On 2014/09/23 14:05:40, rmcilroy wrote:
> As mentioned, these should be globals, not static members (as with
> g_array_buffer_allocator above).  Please also rename to prefix with "g_" and
> move to the above anonymous namespace (which is possible when they are globals
> not static members.

Done.

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:55: if
(!mapped_natives->Initialize(base::File(natives_fd))) {
On 2014/09/23 14:05:39, rmcilroy wrote:
> create the new mapped_snapshot here rather than above such that it remains
null
> if the load fails.

We need the created object to initialize the file in first place. So now, if we
are unable to initialize the file, I'm deleting the object and setting the
pointer back to NULL.

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:61: mapped_snapshot = new base::MemoryMappedFile;
On 2014/09/23 14:05:39, rmcilroy wrote:
> ditto

Same as above.

https://codereview.chromium.org/594603003/diff/40001/gin/isolate_holder.cc#ne...
gin/isolate_holder.cc:103: }
On 2014/09/23 14:05:39, rmcilroy wrote:
> There seems like a lot of duplication here between LoadV8Snapshot and
> LoadV8SnapshotFD.  Could you split some code out into shared functions?

Done.

https://codereview.chromium.org/594603003/diff/40001/gin/public/isolate_holder.h
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/40001/gin/public/isolate_holde...
gin/public/isolate_holder.h:55: static base::MemoryMappedFile* mapped_snapshot;
On 2014/09/23 14:05:40, rmcilroy wrote:
> As mentioned, remove these from this class and make them globals in
> isolate_holder.cc.

Done.

https://codereview.chromium.org/594603003/diff/40001/gin/shell/gin_main.cc
File gin/shell/gin_main.cc (right):

https://codereview.chromium.org/594603003/diff/40001/gin/shell/gin_main.cc#ne...
gin/shell/gin_main.cc:66: #endif // V8_USE_EXTERNAL_STARTUP_DATA
On 2014/09/23 14:05:40, rmcilroy wrote:
> nit - move this block to be below gin::IsolateHolder::Initialize (and search
for
> gin::IsolateHolder::Initialize elsewhere in the codebase and add the
> gin::IsolateHolder::LoadV8Snapshot(); below those calls, which should fix the
> other test failures you are seeing (and avoid the gin dependency issues you
had
> before)..

This can't be moved below because gin::IsolateHolder::Initialize depends on
this.

Did add a call to gin::IsolateHolder::LoadV8Snapshot() *before* every call to
gin::IsolateHolder::Initialize, except for net/proxy/proxy_resolver_v8.cc, as
this is not a test and I don't know whether the call would be performed by a
process that can or cannot read from the file system.

Didn't add a call to mojo/apps/js/js_app.cc either, but the one in
mojo/apps/js/main.cc will probably take care of that.

https://codereview.chromium.org/594603003/diff/40001/mojo/apps/js/main.cc
File mojo/apps/js/main.cc (right):

https://codereview.chromium.org/594603003/diff/40001/mojo/apps/js/main.cc#new...
mojo/apps/js/main.cc:17: #endif // V8_USE_EXTERNAL_STARTUP_DATA
On 2014/09/23 14:05:40, rmcilroy wrote:
> Ditto - move this to the call to be below the call to
> gin::IsolateHolder::LoadV8Snapshot() in mojo.

Same as before.

[rmcilroy, 2014-10-06 15:41:03]

In general looks good but I think you can remove a couple of changes due to some
dependencies.

Also, please update the CL's title and description to mention that this is only
for Linux and Android and be more descriptive (also title should be "Read V8's
snapshot from an external file on Linux / Android").

https://codereview.chromium.org/594603003/diff/280001/android_webview/android...
File android_webview/android_webview_tests.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/android_webview/android...
android_webview/android_webview_tests.gypi:60: }],
replace tabs with spaces (throughout)

https://codereview.chromium.org/594603003/diff/280001/android_webview/android...
android_webview/android_webview_tests.gypi:105: '../gin/gin.gyp:gin',
I think we can remove this gin dependency from here and every other target which
also depends on test_support_content since test_support_content itself depends
on gin.

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:118: boolean
isV8SnapshotData = file.equals(V8_SNAPSHOT_DATA_FILENAME);
Make this a single boolean (e.g., isAppDataFile)

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:150: // icudata
needs to be accessed by a renderer process.
nit - update comment

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:357: * so this is
treated as an error as well.
nit - move this up above the ICU data sentence and change comment to "Failing to
update the V8 snapshot files will lead to a version mismatch between V8 and the
loaded snapshot which will cause V8 to crash, so this is treated as an error."

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:365: File
v8_snapshot = new File(getAppDataDir(), V8_SNAPSHOT_DATA_FILENAME);
nit - move this down to line 370.

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi#oldco...
build/common.gypi:2343: 
nit - add back newline

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi#newco...
build/common.gypi:993: 'v8_use_external_startup_data': 0,
Please create a bug on crbug which mentions that you need to add enable this on
windows / mac and reference in a comment here.

https://codereview.chromium.org/594603003/diff/280001/chrome/android/shell/ja...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellApplication.java
(right):

https://codereview.chromium.org/594603003/diff/280001/chrome/android/shell/ja...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellApplication.java:35:
*  V8 used to be statically linked to the binary, but now it's loaded
/s/V8/V8's initial snapshot/

https://codereview.chromium.org/594603003/diff/280001/chrome/android/shell/ja...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellApplication.java:36:
*  from external snapshots. Therefore we need to install such
/s/external snapshots/an external file/

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_android_p...
File chrome/chrome_android_paks.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_android_p...
chrome/chrome_android_paks.gypi:107: ],
fix tabs

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2866: ],
I don't think the changes in this file are necessary because unit_tests_apk
depends on chrome_java, and this already does the packaging in
chrome_android_paks.gypi (pulled in through chrome chrome_android.gypi which is
included in the chrome_java target).  Does it still work without the changes in
this target?  (I think it should because ICU data is not explicitly mentioned on
this target).

https://codereview.chromium.org/594603003/diff/280001/chrome/tools/build/win/...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/594603003/diff/280001/chrome/tools/build/win/...
chrome/tools/build/win/FILES.cfg:122: {
It's probably best to remove the Windows and Mac specific changes in this CL to
keep things simple (but keep them locally so you can add to a win / mac specific
CL).

https://codereview.chromium.org/594603003/diff/280001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/594603003/diff/280001/components/components_t...
components/components_tests.gyp:276: '../gin/gin.gyp:gin',
shouldn't be required (pulled in via test_support_content)

https://codereview.chromium.org/594603003/diff/280001/components/components_t...
components/components_tests.gyp:833: '../gin/gin.gyp:gin',
ditto

https://codereview.chromium.org/594603003/diff/280001/content/public/test/con...
File content/public/test/content_test_suite_base.cc (right):

https://codereview.chromium.org/594603003/diff/280001/content/public/test/con...
content/public/test/content_test_suite_base.cc:70:
gin::IsolateHolder::LoadV8Snapshot();
I'm not sure if this is the right place for this - where is Blink initialized
for ContentTestSuite?

https://codereview.chromium.org/594603003/diff/280001/content/public/test/uni...
File content/public/test/unittest_test_suite.cc (right):

https://codereview.chromium.org/594603003/diff/280001/content/public/test/uni...
content/public/test/unittest_test_suite.cc:26:
gin::IsolateHolder::LoadV8Snapshot();
You are already doing this in TestBlinkWebUnitTestSupport() so this shouldn't be
needed (note - IOS doesn't run V8 so no issue on IOS).

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/l...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java
(right):

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/l...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java:25:
*  snapshots (natives_blob.bin and snapshot.bin) along with pak files.
Same comments as ChromeShellApplication.java

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/s...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java
(right):

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/s...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java:28:
*  snapshots (natives_blob.bin and snapshot.bin) along with pak files.
Same comments as ChromeShellApplication.java

https://codereview.chromium.org/594603003/diff/280001/gin/converter_unittest.cc
File gin/converter_unittest.cc (left):

https://codereview.chromium.org/594603003/diff/280001/gin/converter_unittest....
gin/converter_unittest.cc:27: using v8::Value;
I'm not sure why these changes are required?  Can you remove them (if not they
should be a separate CL)

https://codereview.chromium.org/594603003/diff/280001/mojo/apps/js/js_app.cc
File mojo/apps/js/js_app.cc (right):

https://codereview.chromium.org/594603003/diff/280001/mojo/apps/js/js_app.cc#...
mojo/apps/js/js_app.cc:80: gin::IsolateHolder::LoadV8Snapshot();
Is this required given the change in content_handler_main.cc /
standalone_main.cc?  I think those should be enough since MojoMain should always
be called before this Run() method.

https://codereview.chromium.org/594603003/diff/280001/third_party/android_web...
File third_party/android_webview_glue/android_webview_glue.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/third_party/android_web...
third_party/android_webview_glue/android_webview_glue.gypi:55: }],
fix tabs

[baixo, 2014-10-07 14:53:28]

Addressed latest Ross' comments (thanks, Ross!).

https://codereview.chromium.org/594603003/diff/280001/android_webview/android...
File android_webview/android_webview_tests.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/android_webview/android...
android_webview/android_webview_tests.gypi:60: }],
On 2014/10/06 15:41:02, rmcilroy wrote:
> replace tabs with spaces (throughout)

Done.

https://codereview.chromium.org/594603003/diff/280001/android_webview/android...
android_webview/android_webview_tests.gypi:105: '../gin/gin.gyp:gin',
On 2014/10/06 15:41:02, rmcilroy wrote:
> I think we can remove this gin dependency from here and every other target
which
> also depends on test_support_content since test_support_content itself depends
> on gin.

Done.

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:118: boolean
isV8SnapshotData = file.equals(V8_SNAPSHOT_DATA_FILENAME);
On 2014/10/06 15:41:02, rmcilroy wrote:
> Make this a single boolean (e.g., isAppDataFile)

Done.

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:150: // icudata
needs to be accessed by a renderer process.
On 2014/10/06 15:41:02, rmcilroy wrote:
> nit - update comment

Done.

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:357: * so this is
treated as an error as well.
On 2014/10/06 15:41:02, rmcilroy wrote:
> nit - move this up above the ICU data sentence and change comment to "Failing
to
> update the V8 snapshot files will lead to a version mismatch between V8 and
the
> loaded snapshot which will cause V8 to crash, so this is treated as an error."

Done.

https://codereview.chromium.org/594603003/diff/280001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:365: File
v8_snapshot = new File(getAppDataDir(), V8_SNAPSHOT_DATA_FILENAME);
On 2014/10/06 15:41:02, rmcilroy wrote:
> nit - move this down to line 370.

Done.

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi#oldco...
build/common.gypi:2343: 
On 2014/10/06 15:41:02, rmcilroy wrote:
> nit - add back newline

Done.

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/build/common.gypi#newco...
build/common.gypi:993: 'v8_use_external_startup_data': 0,
On 2014/10/06 15:41:02, rmcilroy wrote:
> Please create a bug on crbug which mentions that you need to add enable this
on
> windows / mac and reference in a comment here.

Done.

https://codereview.chromium.org/594603003/diff/280001/chrome/android/shell/ja...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellApplication.java
(right):

https://codereview.chromium.org/594603003/diff/280001/chrome/android/shell/ja...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellApplication.java:35:
*  V8 used to be statically linked to the binary, but now it's loaded
On 2014/10/06 15:41:02, rmcilroy wrote:
> /s/V8/V8's initial snapshot/

Done.

https://codereview.chromium.org/594603003/diff/280001/chrome/android/shell/ja...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellApplication.java:36:
*  from external snapshots. Therefore we need to install such
On 2014/10/06 15:41:02, rmcilroy wrote:
> /s/external snapshots/an external file/

Done. Just replaced by "external files" instead, since it comes from natives_
and snapshot_blob.bin

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_android_p...
File chrome/chrome_android_paks.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_android_p...
chrome/chrome_android_paks.gypi:107: ],
On 2014/10/06 15:41:02, rmcilroy wrote:
> fix tabs

Done.

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2866: ],
On 2014/10/06 15:41:02, rmcilroy wrote:
> I don't think the changes in this file are necessary because unit_tests_apk
> depends on chrome_java, and this already does the packaging in
> chrome_android_paks.gypi (pulled in through chrome chrome_android.gypi which
is
> included in the chrome_java target).  Does it still work without the changes
in
> this target?  (I think it should because ICU data is not explicitly mentioned
on
> this target).

Unfortunately it doesn't work without these changes. The packaging you mentioned
does not ensure that the snapshot files end up in the unit_tests_apk. (I think
ICU is not called in these tests).

https://codereview.chromium.org/594603003/diff/280001/chrome/tools/build/win/...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/594603003/diff/280001/chrome/tools/build/win/...
chrome/tools/build/win/FILES.cfg:122: {
On 2014/10/06 15:41:02, rmcilroy wrote:
> It's probably best to remove the Windows and Mac specific changes in this CL
to
> keep things simple (but keep them locally so you can add to a win / mac
specific
> CL).

Done.

https://codereview.chromium.org/594603003/diff/280001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/594603003/diff/280001/components/components_t...
components/components_tests.gyp:276: '../gin/gin.gyp:gin',
On 2014/10/06 15:41:03, rmcilroy wrote:
> shouldn't be required (pulled in via test_support_content)

Done.

https://codereview.chromium.org/594603003/diff/280001/components/components_t...
components/components_tests.gyp:833: '../gin/gin.gyp:gin',
On 2014/10/06 15:41:03, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/594603003/diff/280001/content/public/test/con...
File content/public/test/content_test_suite_base.cc (right):

https://codereview.chromium.org/594603003/diff/280001/content/public/test/con...
content/public/test/content_test_suite_base.cc:70:
gin::IsolateHolder::LoadV8Snapshot();
On 2014/10/06 15:41:03, rmcilroy wrote:
> I'm not sure if this is the right place for this - where is Blink initialized
> for ContentTestSuite?

Moved this to content/public/test/render_view_test.cc right before
blink::initialize().

This call might be satisfying other tests, in which case we can either move it
back here or find a better place higher up the stack.

https://codereview.chromium.org/594603003/diff/280001/content/public/test/uni...
File content/public/test/unittest_test_suite.cc (right):

https://codereview.chromium.org/594603003/diff/280001/content/public/test/uni...
content/public/test/unittest_test_suite.cc:26:
gin::IsolateHolder::LoadV8Snapshot();
On 2014/10/06 15:41:03, rmcilroy wrote:
> You are already doing this in TestBlinkWebUnitTestSupport() so this shouldn't
be
> needed (note - IOS doesn't run V8 so no issue on IOS).

Done.

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/l...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java
(right):

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/l...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java:25:
*  snapshots (natives_blob.bin and snapshot.bin) along with pak files.
On 2014/10/06 15:41:03, rmcilroy wrote:
> Same comments as ChromeShellApplication.java

Done.

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/s...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java
(right):

https://codereview.chromium.org/594603003/diff/280001/content/shell/android/s...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java:28:
*  snapshots (natives_blob.bin and snapshot.bin) along with pak files.
On 2014/10/06 15:41:03, rmcilroy wrote:
> Same comments as ChromeShellApplication.java

Done.

https://codereview.chromium.org/594603003/diff/280001/gin/converter_unittest.cc
File gin/converter_unittest.cc (left):

https://codereview.chromium.org/594603003/diff/280001/gin/converter_unittest....
gin/converter_unittest.cc:27: using v8::Value;
On 2014/10/06 15:41:03, rmcilroy wrote:
> I'm not sure why these changes are required?  Can you remove them (if not they
> should be a separate CL)

Done.

https://codereview.chromium.org/594603003/diff/280001/mojo/apps/js/js_app.cc
File mojo/apps/js/js_app.cc (right):

https://codereview.chromium.org/594603003/diff/280001/mojo/apps/js/js_app.cc#...
mojo/apps/js/js_app.cc:80: gin::IsolateHolder::LoadV8Snapshot();
On 2014/10/06 15:41:03, rmcilroy wrote:
> Is this required given the change in content_handler_main.cc /
> standalone_main.cc?  I think those should be enough since MojoMain should
always
> be called before this Run() method.

Done.

https://codereview.chromium.org/594603003/diff/280001/third_party/android_web...
File third_party/android_webview_glue/android_webview_glue.gypi (right):

https://codereview.chromium.org/594603003/diff/280001/third_party/android_web...
third_party/android_webview_glue/android_webview_glue.gypi:55: }],
On 2014/10/06 15:41:03, rmcilroy wrote:
> fix tabs

Done.

[rmcilroy, 2014-10-07 16:16:11]

https://codereview.chromium.org/594603003/diff/320001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/594603003/diff/320001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:355: * is treated
as an error.
nit - remove extra newline here.

https://codereview.chromium.org/594603003/diff/320001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/320001/build/common.gypi#newco...
build/common.gypi:990: # TODO(baixo): Eliminate this OS dependency by enabling
nit - this is not a dependency.  I would just write: 
"# TODO(baixo): Enable v8_use_external_startup_data for Windows and Mac -
http://crbug.com/421063"

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

https://codereview.chromium.org/594603003/diff/320001/content/app/content_mai...
content/app/content_main_runner.cc:730: if (v8_natives_fd != -1 &&
v8_snapshot_fd != -1)
use "{" when a block takes more than one line (even if only a single statment. 
Add braces for both the if and else block

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.g...
content/content_tests.gypi:1037: '../gin/gin.gyp:gin',
not required (pulled in by test_support_content dependency)

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.g...
content/content_tests.gypi:1402: '../gin/gin.gyp:gin',
ditto

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.g...
content/content_tests.gypi:1441: '../gin/gin.gyp:gin',
ditto

https://codereview.chromium.org/594603003/diff/320001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/320001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:25: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
nit - remove the double #ifdef V8_USE_EXTERNAL_STARTUP_DATA - just:
#ifdef V8_USE_EXTERNAL_STARTUP_DATA
#include "base/path_service.h"
#ifdef OS_MACOSX
#include "base/mac/bundle_locations.h"
#endif  // OS_MACOSX
#endif  // V8_USE_EXTERNAL_STARTUP_DATA

https://codereview.chromium.org/594603003/diff/320001/gin/public/isolate_hold...
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/320001/gin/public/isolate_hold...
gin/public/isolate_holder.h:14: #include "base/files/memory_mapped_file.h"
Do you need this include in the header?

https://codereview.chromium.org/594603003/diff/320001/gin/public/isolate_hold...
gin/public/isolate_holder.h:38: // initialize V8 and Gin.
Please add a comment here that if V8_USE_EXTERNAL_STARTUP_DATA then
LoadV8Snapshot should be called before calling Initialize

https://codereview.chromium.org/594603003/diff/320001/gin/test/v8_test.cc
File gin/test/v8_test.cc (right):

https://codereview.chromium.org/594603003/diff/320001/gin/test/v8_test.cc#new...
gin/test/v8_test.cc:30: 
nit - remove newline

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/applicatio...
File mojo/apps/js/application_delegate_impl.cc (right):

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/applicatio...
mojo/apps/js/application_delegate_impl.cc:25: 
nit - remove newline

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/test/js_to...
File mojo/apps/js/test/js_to_cpp_unittest.cc (right):

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/test/js_to...
mojo/apps/js/test/js_to_cpp_unittest.cc:410: 
nit - remove newline

https://codereview.chromium.org/594603003/diff/320001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/594603003/diff/320001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:780: 
nit - remove newline

[baixo, 2014-10-08 11:28:55]

Addressed Ross' comments

https://codereview.chromium.org/594603003/diff/320001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/594603003/diff/320001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:355: * is treated
as an error.
On 2014/10/07 16:16:10, rmcilroy wrote:
> nit - remove extra newline here.

Done.

https://codereview.chromium.org/594603003/diff/320001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/320001/build/common.gypi#newco...
build/common.gypi:990: # TODO(baixo): Eliminate this OS dependency by enabling
On 2014/10/07 16:16:10, rmcilroy wrote:
> nit - this is not a dependency.  I would just write: 
> "# TODO(baixo): Enable v8_use_external_startup_data for Windows and Mac -
> http://crbug.com/421063%22

Done.

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

https://codereview.chromium.org/594603003/diff/320001/content/app/content_mai...
content/app/content_main_runner.cc:730: if (v8_natives_fd != -1 &&
v8_snapshot_fd != -1)
On 2014/10/07 16:16:10, rmcilroy wrote:
> use "{" when a block takes more than one line (even if only a single statment.

> Add braces for both the if and else block

Done.

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.g...
content/content_tests.gypi:1037: '../gin/gin.gyp:gin',
On 2014/10/07 16:16:11, rmcilroy wrote:
> not required (pulled in by test_support_content dependency)

Done.

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.g...
content/content_tests.gypi:1402: '../gin/gin.gyp:gin',
On 2014/10/07 16:16:11, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/594603003/diff/320001/content/content_tests.g...
content/content_tests.gypi:1441: '../gin/gin.gyp:gin',
On 2014/10/07 16:16:10, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/594603003/diff/320001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/320001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:25: #ifdef V8_USE_EXTERNAL_STARTUP_DATA
On 2014/10/07 16:16:11, rmcilroy wrote:
> nit - remove the double #ifdef V8_USE_EXTERNAL_STARTUP_DATA - just:
> #ifdef V8_USE_EXTERNAL_STARTUP_DATA
> #include "base/path_service.h"
> #ifdef OS_MACOSX
> #include "base/mac/bundle_locations.h"
> #endif  // OS_MACOSX
> #endif  // V8_USE_EXTERNAL_STARTUP_DATA

Done.

https://codereview.chromium.org/594603003/diff/320001/gin/public/isolate_hold...
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/320001/gin/public/isolate_hold...
gin/public/isolate_holder.h:14: #include "base/files/memory_mapped_file.h"
On 2014/10/07 16:16:11, rmcilroy wrote:
> Do you need this include in the header?

No. But I need base/files/file_path.h :).

https://codereview.chromium.org/594603003/diff/320001/gin/public/isolate_hold...
gin/public/isolate_holder.h:38: // initialize V8 and Gin.
On 2014/10/07 16:16:11, rmcilroy wrote:
> Please add a comment here that if V8_USE_EXTERNAL_STARTUP_DATA then
> LoadV8Snapshot should be called before calling Initialize

Done.

https://codereview.chromium.org/594603003/diff/320001/gin/test/v8_test.cc
File gin/test/v8_test.cc (right):

https://codereview.chromium.org/594603003/diff/320001/gin/test/v8_test.cc#new...
gin/test/v8_test.cc:30: 
On 2014/10/07 16:16:11, rmcilroy wrote:
> nit - remove newline

Done.

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/applicatio...
File mojo/apps/js/application_delegate_impl.cc (right):

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/applicatio...
mojo/apps/js/application_delegate_impl.cc:25: 
On 2014/10/07 16:16:11, rmcilroy wrote:
> nit - remove newline

Done.

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/test/js_to...
File mojo/apps/js/test/js_to_cpp_unittest.cc (right):

https://codereview.chromium.org/594603003/diff/320001/mojo/apps/js/test/js_to...
mojo/apps/js/test/js_to_cpp_unittest.cc:410: 
On 2014/10/07 16:16:11, rmcilroy wrote:
> nit - remove newline

Done.

https://codereview.chromium.org/594603003/diff/320001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/594603003/diff/320001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:780: 
On 2014/10/07 16:16:11, rmcilroy wrote:
> nit - remove newline

Done.

[jochen (gone - plz use gerrit), 2014-10-08 15:16:05]

cool

I would recommend to rip out the mac/win specific code from this CL, so it's
more contained

I would also expect that you have to add the blobs to the various .isolate files
to make the test runners happy

https://codereview.chromium.org/594603003/diff/340001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/340001/build/common.gypi#newco...
build/common.gypi:991: # http://crbug.com/421063
what about chromeos?

https://codereview.chromium.org/594603003/diff/340001/chrome/chrome_dll_bundl...
File chrome/chrome_dll_bundle.gypi (right):

https://codereview.chromium.org/594603003/diff/340001/chrome/chrome_dll_bundl...
chrome/chrome_dll_bundle.gypi:305: ['v8_use_external_startup_data==1', {
not required yet?

https://codereview.chromium.org/594603003/diff/340001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/594603003/diff/340001/content/DEPS#newcode30
content/DEPS:30: "+gin",
i don't think we want that at the top level, rather in the subdirs that actually
use gin

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

https://codereview.chromium.org/594603003/diff/340001/content/app/content_mai...
content/app/content_main_runner.cc:730: if (v8_natives_fd != -1 &&
v8_snapshot_fd != -1) {
shouldn't this be ifdef'd to OS_ANDROID?

https://codereview.chromium.org/594603003/diff/340001/gin/public/isolate_hold...
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/340001/gin/public/isolate_hold...
gin/public/isolate_holder.h:72: static bool MapV8Files(
this method can be in an anonymous namespace in isolate_holder.cc, no?

[baixo, 2014-10-14 16:36:42]

Addressed Jochen's comments (thanks, Jochen!).

https://codereview.chromium.org/594603003/diff/340001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/340001/build/common.gypi#newco...
build/common.gypi:991: # http://crbug.com/421063
On 2014/10/08 15:16:05, jochen wrote:
> what about chromeos?

Done.

https://codereview.chromium.org/594603003/diff/340001/chrome/chrome_dll_bundl...
File chrome/chrome_dll_bundle.gypi (right):

https://codereview.chromium.org/594603003/diff/340001/chrome/chrome_dll_bundl...
chrome/chrome_dll_bundle.gypi:305: ['v8_use_external_startup_data==1', {
On 2014/10/08 15:16:05, jochen wrote:
> not required yet?

Done.

https://codereview.chromium.org/594603003/diff/340001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/594603003/diff/340001/content/DEPS#newcode30
content/DEPS:30: "+gin",
On 2014/10/08 15:16:05, jochen wrote:
> i don't think we want that at the top level, rather in the subdirs that
actually
> use gin

Done.

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

https://codereview.chromium.org/594603003/diff/340001/content/app/content_mai...
content/app/content_main_runner.cc:730: if (v8_natives_fd != -1 &&
v8_snapshot_fd != -1) {
On 2014/10/08 15:16:05, jochen wrote:
> shouldn't this be ifdef'd to OS_ANDROID?

It is, right? (Line 717)

https://codereview.chromium.org/594603003/diff/340001/gin/public/isolate_hold...
File gin/public/isolate_holder.h (right):

https://codereview.chromium.org/594603003/diff/340001/gin/public/isolate_hold...
gin/public/isolate_holder.h:72: static bool MapV8Files(
On 2014/10/08 15:16:05, jochen wrote:
> this method can be in an anonymous namespace in isolate_holder.cc, no?

Done.

[jochen (gone - plz use gerrit), 2014-10-15 15:40:14]

almost there.

you'll need somebody to review the mojo bits and whatever other subdir I'm not
an owner of

https://codereview.chromium.org/594603003/diff/430001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/430001/build/common.gypi#newco...
build/common.gypi:990: # TODO(baixo): Enable v8_use_external_startup_data for
Windows and Mac -
nit. the comment should say "... Windows, Linux, Mac, and ChromeOS"

https://codereview.chromium.org/594603003/diff/430001/build/get_landmines.py
File build/get_landmines.py (right):

https://codereview.chromium.org/594603003/diff/430001/build/get_landmines.py#...
build/get_landmines.py:59: print 'Need clobber build for V8 external snapshot
changes: crbug.com/421063'
only required for android, no? Just move it down below the if platform() ==
'android':

https://codereview.chromium.org/594603003/diff/430001/chrome/installer/mini_i...
File chrome/installer/mini_installer.gypi (right):

https://codereview.chromium.org/594603003/diff/430001/chrome/installer/mini_i...
chrome/installer/mini_installer.gypi:182: ['v8_use_external_startup_data == 1',
{
is this required on android? I think this gyp file is windows only

https://codereview.chromium.org/594603003/diff/430001/chrome/tools/build/linu...
File chrome/tools/build/linux/FILES.cfg (right):

https://codereview.chromium.org/594603003/diff/430001/chrome/tools/build/linu...
chrome/tools/build/linux/FILES.cfg:59: 'filename': 'natives_blob.bin',
shouldn't be required yet

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

https://codereview.chromium.org/594603003/diff/430001/content/app/content_mai...
content/app/content_main_runner.cc:734:
CHECK(gin::IsolateHolder::LoadV8Snapshot());
what is this branch for?

https://codereview.chromium.org/594603003/diff/430001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/430001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:24: #ifdef OS_MACOSX
remove non-android bits for now?

[baixo1, 2014-10-16 10:10:28]

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

[baixo1, 2014-10-16 10:10:30]

Addressed Jochen's comments.

https://codereview.chromium.org/594603003/diff/430001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/430001/build/common.gypi#newco...
build/common.gypi:990: # TODO(baixo): Enable v8_use_external_startup_data for
Windows and Mac -
On 2014/10/15 15:40:14, jochen wrote:
> nit. the comment should say "... Windows, Linux, Mac, and ChromeOS"

Done.

https://codereview.chromium.org/594603003/diff/430001/build/get_landmines.py
File build/get_landmines.py (right):

https://codereview.chromium.org/594603003/diff/430001/build/get_landmines.py#...
build/get_landmines.py:59: print 'Need clobber build for V8 external snapshot
changes: crbug.com/421063'
On 2014/10/15 15:40:14, jochen wrote:
> only required for android, no? Just move it down below the if platform() ==
> 'android':

It's the opposite, actually. We do not want a snapshot_blob.bin file hanging
around when v8_use_external_startup_data is disabled.

Moved this into a new "if platform() != 'android':"

https://codereview.chromium.org/594603003/diff/430001/chrome/installer/mini_i...
File chrome/installer/mini_installer.gypi (right):

https://codereview.chromium.org/594603003/diff/430001/chrome/installer/mini_i...
chrome/installer/mini_installer.gypi:182: ['v8_use_external_startup_data == 1',
{
On 2014/10/15 15:40:14, jochen wrote:
> is this required on android? I think this gyp file is windows only

Done.

https://codereview.chromium.org/594603003/diff/430001/chrome/tools/build/linu...
File chrome/tools/build/linux/FILES.cfg (right):

https://codereview.chromium.org/594603003/diff/430001/chrome/tools/build/linu...
chrome/tools/build/linux/FILES.cfg:59: 'filename': 'natives_blob.bin',
On 2014/10/15 15:40:14, jochen wrote:
> shouldn't be required yet

Done.

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

https://codereview.chromium.org/594603003/diff/430001/content/app/content_mai...
content/app/content_main_runner.cc:734:
CHECK(gin::IsolateHolder::LoadV8Snapshot());
On 2014/10/15 15:40:14, jochen wrote:
> what is this branch for?

On Android, the browser process may reach this point before the descriptors have
been pushed.

https://codereview.chromium.org/594603003/diff/430001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/430001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:24: #ifdef OS_MACOSX
On 2014/10/15 15:40:14, jochen wrote:
> remove non-android bits for now?

Done. Removed non-android bits from the entire file.

[rmcilroy, 2014-10-16 11:09:40]

rmcilroy@chromium.org changed reviewers:
+ derat@chromium.org, gavinp@chromium.org, mkosiba@chromium.org,
qsr@chromium.org

[rmcilroy, 2014-10-16 11:09:41]

Adding OWNERS for changes outside content.

derat@chromium.org: Please review changes in src/ash

qsr@chromium.org: Please review changes in src/mojo

mkosiba@chromium.org: Please review changes in src/android_webview

gavinp@chromium.org: Please review changes in src/net

Thanks!

[Daniel Erat, 2014-10-16 15:19:17]

i don't have any state here, but i'm surprised that ash/ has a dependency on v8
-- i don't see any references to it in the source. can you give a bit of
background about why this is needed?

[mkosiba (inactive), 2014-10-16 15:21:32]

android_webview/ LGTM

It would be cool if this did not require the files to be extracted out of the
APK though. See:
  https://codereview.chromium.org/394313002/
  https://codereview.chromium.org/401743003/
  https://codereview.chromium.org/402603006/
For how we did this for the .pak files.

You'll probably need to figure out how to pass in this new kind of FD from the
browser process to the sandboxed renderer

https://codereview.chromium.org/594603003/diff/470001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/470001/build/common.gypi#newco...
build/common.gypi:992: ['OS=="android"', {
OS == "android" and android_webview_build==0

otherwise you've broken all the WebViews, and that's not fun.

[baixo1, 2014-10-16 16:43:33]

Disabled external snapshot for webview (thanks Martin!) and removed ash
dependency (thanks Daniel!).

https://codereview.chromium.org/594603003/diff/470001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/594603003/diff/470001/build/common.gypi#newco...
build/common.gypi:992: ['OS=="android"', {
On 2014/10/16 15:21:32, mkosiba wrote:
> OS == "android" and android_webview_build==0
> 
> otherwise you've broken all the WebViews, and that's not fun.

Done.

[qsr, 2014-10-17 11:32:30]

qsr@chromium.org changed reviewers:
+ abarth@chromium.org

[qsr, 2014-10-17 11:32:31]

-qsr
+abarth for mojo/ that knows a lot more about js that I do. I cannot decide with
my knowledge if depending on files on disk is an issue or not for mojo js
applications.

[rmcilroy, 2014-10-17 13:10:04]

/build and /base/android LGTM.

[jochen (gone - plz use gerrit), 2014-10-17 13:38:35]

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

https://codereview.chromium.org/594603003/diff/490001/content/app/content_mai...
content/app/content_main_runner.cc:734:
CHECK(gin::IsolateHolder::LoadV8Snapshot());
then you don't need the call in the network stack, no?

https://codereview.chromium.org/594603003/diff/490001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/490001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:52: LOG(ERROR) << "Couldn't mmap v8 natives data file";
nit. that should be LOG(FATAL), as we can't use v8 without natives

[baixo1, 2014-10-20 10:44:23]

Addresses Jochen's comments.

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

https://codereview.chromium.org/594603003/diff/490001/content/app/content_mai...
content/app/content_main_runner.cc:734:
CHECK(gin::IsolateHolder::LoadV8Snapshot());
On 2014/10/17 13:38:34, jochen wrote:
> then you don't need the call in the network stack, no?

Yes. That call is only needed for the tests. Therefore, that call was moved to
net/test/run_all_unittests.cc

https://codereview.chromium.org/594603003/diff/490001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/594603003/diff/490001/gin/isolate_holder.cc#n...
gin/isolate_holder.cc:52: LOG(ERROR) << "Couldn't mmap v8 natives data file";
On 2014/10/17 13:38:34, jochen wrote:
> nit. that should be LOG(FATAL), as we can't use v8 without natives

Done.

[jochen (gone - plz use gerrit), 2014-10-20 13:55:33]

lgtm

https://codereview.chromium.org/594603003/diff/530001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

https://codereview.chromium.org/594603003/diff/530001/tools/gypv8sh.py#newcode40
tools/gypv8sh.py:40: if os.path.exists(v8nativesfile):
can you add a commandline flag to this tool, and set it from gyp instead of just
looking whether the files exist?

so we avoid setting the command line flag if there are stale blobs from previous
builds

[abarth-chromium, 2014-10-20 16:57:50]

abarth@chromium.org changed reviewers:
+ hansmuller@chromium.org

[hansmuller, 2014-10-20 18:22:35]

I've only looked at the v8 and mojo changes. The gin and mojo apps only exist to
run V8 JS code. What's the advantage of dynamically loading a V8 snapshot
instead of statically linking? Is it to give the individual binaries a smaller
disk footprint?

[jochen (gone - plz use gerrit), 2014-10-20 21:31:06]

yes.

e.g. on windows, the chrome binary ships three copies of this data (3x 4MB)

to be able to build all targets with this configuration, all v8 embedders in
chrome need to support this feature.

[jochen (gone - plz use gerrit), 2014-10-20 21:31:27]

yes.

e.g. on windows, the chrome binary ships three copies of this data (3x 4MB)

to be able to build all targets with this configuration, all v8 embedders in
chrome need to support this feature.

[hansmuller, 2014-10-20 23:29:43]

On 2014/10/20 21:31:27, jochen wrote:
> yes.
> 
> e.g. on windows, the chrome binary ships three copies of this data (3x 4MB)
> 
> to be able to build all targets with this configuration, all v8 embedders in
> chrome need to support this feature.

OK.

I don't think mojo/apps/js/standalone_main.cc and
mojo/apps/js/content_handler_main.cc need to call
gin::IsolateHolder::LoadV8Snapshot(). They both effectively create an
ApplicationDelegateImpl object which now does the work in its Initialize
method().

Other than that, this change LGTM.

[baixo1, 2014-10-21 14:00:59]

Addressed comments from Jochen and Hans.

https://codereview.chromium.org/594603003/diff/530001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

https://codereview.chromium.org/594603003/diff/530001/tools/gypv8sh.py#newcode40
tools/gypv8sh.py:40: if os.path.exists(v8nativesfile):
On 2014/10/20 13:55:33, jochen wrote:
> can you add a commandline flag to this tool, and set it from gyp instead of
just
> looking whether the files exist?
> 
> so we avoid setting the command line flag if there are stale blobs from
previous
> builds

Done.

[abarth-chromium, 2014-10-21 16:30:46]

mojo/ LGTM

[baixo1, 2014-10-22 17:19:03]

gavinp, could you please take a look or pass it to someone else?

[rmcilroy, 2014-10-23 10:09:39]

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

[rmcilroy, 2014-10-23 10:09:41]

cbentzel: Could you please take a look at the changes in /net

[jochen (gone - plz use gerrit), 2014-10-23 14:20:33]

i'd suggest eroman@ for net/

[baixo1, 2014-10-23 15:28:33]

baixo@google.com changed reviewers:
+ eroman@chromium.org
- cbentzel@chromium.org, derat@chromium.org

[baixo1, 2014-10-23 15:28:35]

eroman: Could you please take a look at the changes in /net?

[eroman, 2014-10-23 18:39:15]

net lgtm

[baixo1, 2014-10-23 19:32:06]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-10-23 19:32:44]

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

[baixo1, 2014-10-23 19:33:11]

The CQ bit was unchecked by baixo@google.com

[baixo1, 2014-10-24 09:24:21]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-10-24 09:24:42]

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

[commit-bot: I haz the power, 2014-10-24 09:29:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 09:29:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[baixo1, 2014-10-24 09:47:05]

baixo@google.com changed reviewers:
+ jamesr@chromium.org

[baixo1, 2014-10-24 09:47:07]

jamesr: Could you please approve /mojo?

[rmcilroy, 2014-10-24 10:44:04]

On 2014/10/24 09:47:07, baixo1 wrote:
> jamesr: Could you please approve /mojo?

jamesr: this was approved LGTMed for Mojo by abarth before the repo split, but
only got approved by other owners post-repo split.  We can't land this without
the Mojo changes since that will break the mojo tests in the chromium waterfall.
 What is the process for this? Could you land the mojo changes in the mojo repo
and then land this CL alongside a roll of mojo?

[jamesr, 2014-10-24 15:44:20]

On 2014/10/24 10:44:04, rmcilroy wrote:
> On 2014/10/24 09:47:07, baixo1 wrote:
> > jamesr: Could you please approve /mojo?
> 
> jamesr: this was approved LGTMed for Mojo by abarth before the repo split, but
> only got approved by other owners post-repo split.  We can't land this without
> the Mojo changes since that will break the mojo tests in the chromium
waterfall.
>  What is the process for this? Could you land the mojo changes in the mojo
repo
> and then land this CL alongside a roll of mojo?

Which tests?

[baixo1, 2014-10-24 16:18:58]

On 2014/10/24 15:44:20, jamesr wrote:
> On 2014/10/24 10:44:04, rmcilroy wrote:
> > On 2014/10/24 09:47:07, baixo1 wrote:
> > > jamesr: Could you please approve /mojo?
> > 
> > jamesr: this was approved LGTMed for Mojo by abarth before the repo split,
but
> > only got approved by other owners post-repo split.  We can't land this
without
> > the Mojo changes since that will break the mojo tests in the chromium
> waterfall.
> >  What is the process for this? Could you land the mojo changes in the mojo
> repo
> > and then land this CL alongside a roll of mojo?
> 
> Which tests?

mojo_apps_js_unittests  and  mojo_js_unittests

[jamesr, 2014-10-24 17:05:42]

On 2014/10/24 16:18:58, baixo1 wrote:
> On 2014/10/24 15:44:20, jamesr wrote:
> > On 2014/10/24 10:44:04, rmcilroy wrote:
> > > On 2014/10/24 09:47:07, baixo1 wrote:
> > > > jamesr: Could you please approve /mojo?
> > > 
> > > jamesr: this was approved LGTMed for Mojo by abarth before the repo split,
> but
> > > only got approved by other owners post-repo split.  We can't land this
> without
> > > the Mojo changes since that will break the mojo tests in the chromium
> > waterfall.
> > >  What is the process for this? Could you land the mojo changes in the mojo
> > repo
> > > and then land this CL alongside a roll of mojo?
> > 
> > Which tests?
> 
> mojo_apps_js_unittests  and  mojo_js_unittests

OK. You can just land this patch and be done. Confusingly those tests and the
mojo code you're patching here are being maintained in the chromium repo, not
the mojo repo. This is in part to minimize the cost of maintaining c++ / js
bindings code which can be pretty complex. I hour to update the directory
structure soon to make this clearer.

[rmcilroy, 2014-10-24 17:07:12]

On 2014/10/24 17:05:42, jamesr wrote:
> On 2014/10/24 16:18:58, baixo1 wrote:
> > On 2014/10/24 15:44:20, jamesr wrote:
> > > On 2014/10/24 10:44:04, rmcilroy wrote:
> > > > On 2014/10/24 09:47:07, baixo1 wrote:
> > > > > jamesr: Could you please approve /mojo?
> > > > 
> > > > jamesr: this was approved LGTMed for Mojo by abarth before the repo
split,
> > but
> > > > only got approved by other owners post-repo split.  We can't land this
> > without
> > > > the Mojo changes since that will break the mojo tests in the chromium
> > > waterfall.
> > > >  What is the process for this? Could you land the mojo changes in the
mojo
> > > repo
> > > > and then land this CL alongside a roll of mojo?
> > > 
> > > Which tests?
> > 
> > mojo_apps_js_unittests  and  mojo_js_unittests
> 
> OK. You can just land this patch and be done. Confusingly those tests and the
> mojo code you're patching here are being maintained in the chromium repo, not
> the mojo repo. This is in part to minimize the cost of maintaining c++ / js
> bindings code which can be pretty complex. I hour to update the directory
> structure soon to make this clearer.

Thanks James, could you LGTM as a mojo OWNER please so that the presubmit
passes?

[jamesr, 2014-10-24 17:09:20]

lgtm

[baixo1, 2014-10-24 17:37:01]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-10-24 17:37:59]

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

[commit-bot: I haz the power, 2014-10-24 20:19:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 20:19:35]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[baixo1, 2014-10-27 16:55:20]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-10-27 16:56:15]

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

[commit-bot: I haz the power, 2014-10-27 17:02:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-27 17:02:48]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73985)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/78935)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[baixo1, 2014-10-27 17:09:30]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-10-27 17:10:40]

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

[commit-bot: I haz the power, 2014-10-27 17:16:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-27 17:16:11]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73989)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/78946)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[baixo1, 2014-10-27 18:01:29]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-10-27 18:02:02]

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

[commit-bot: I haz the power, 2014-10-27 20:01:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-27 20:01:19]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/79021)

[rmcilroy, 2014-10-27 20:26:29]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2014-10-27 20:27:39]

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

[commit-bot: I haz the power, 2014-10-27 22:12:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-27 22:12:58]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/79021)

[baixo1, 2014-10-28 10:30:22]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-10-28 10:31:56]

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

[commit-bot: I haz the power, 2014-10-28 11:52:33]

Committed patchset #39 (id:770001)

[commit-bot: I haz the power, 2014-10-28 11:53:21]

Patchset 39 (id:??) landed as
https://crrev.com/3a3c88a61fc3fc5b001e7bae70e6335d3cf7c997
Cr-Commit-Position: refs/heads/master@{#301595}