Allow startup with missing V8 snapshot file.

Allow startup with missing V8 snapshot file.

We want to stop shipping the snapshot file, and instead we want
to generate it on the client. This will reduce the download size.
But since snapshot generation will be asynchronous in a utility
process, it might not be present on the first few runs of the
browser. This means we have to be able to start up without the
snapshot file (just with the natives source file). This CL
fixes Blink to cope with a missing snapshot file (V8 could
already cope).

R=rmcilroy@chromium.org, sky@chromium.org
BUG=

Committed: https://crrev.com/c94eff1e7e8b2bd5973d7211c320d9f025c980c1
Cr-Commit-Position: refs/heads/master@{#333258}

[rmcilroy, 2015-06-01 14:06:32]

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
ash/shell/content_client/shell_content_browser_client.cc:80:
DCHECK(v8_natives_fd_.get() != -1);
nit - please add a comment saying why its OK if v8_snapshot_fd doesn't get
opened.

https://codereview.chromium.org/1164483003/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1164483003/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:746: gin::V8Initializer::LoadV8Snapshot();
Maybe we should make it obvious from the function name that this is OK to fail?
Or maybe just return true if it fails in a way we expect (i.e., file is not
available) and only return false for unexpected errors (e.g., map errors on a
file which exists? Then you could CHECK it everytime?

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

https://codereview.chromium.org/1164483003/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:45: // V8Initializer::LoadV8Natives) before calling
this method.
Also mention that snapshot should be loaded if available before calling this
function

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.cc#newco...
gin/v8_initializer.cc:178: GetV8FilePaths(&natives_data_path,
&snapshot_data_path);
Maybe create seperate functions for natives and snapshot to avoid having to
define an unused variable.

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h#newcode39
gin/v8_initializer.h:39: static bool LoadV8NativesFromFD(base::PlatformFile
natives_fd,
could you add WARN_UNUSED_RESULT on this (and possibly loadSnapshot if you
change it so it returns true when it fails in a way we expect).

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h#newcode49
gin/v8_initializer.h:49: static bool LoadV8Natives();
WARN_UNUSED_RESULT here too.

https://codereview.chromium.org/1164483003/diff/1/net/proxy/proxy_resolver_v8.cc
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1164483003/diff/1/net/proxy/proxy_resolver_v8...
net/proxy/proxy_resolver_v8.cc:371: gin::V8Initializer::LoadV8Natives();
CHECK? (and for all LoadV8Natives)

https://codereview.chromium.org/1164483003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1164483003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:45054: +</histogram>
According to Orion the current UMA didn't show much since crashes prevent the
UMA from being sent. I guess we would like to keep UMA for the snapshot loading
to check how often it doesn't find the snapshot, but we probably don't need to
add the natives UMA, WDYT?

[Erik Corry Chromium.org, 2015-06-02 11:18:53]

Changed version not uploaded yet

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
ash/shell/content_client/shell_content_browser_client.cc:80:
DCHECK(v8_natives_fd_.get() != -1);
On 2015/06/01 14:06:31, rmcilroy wrote:
> nit - please add a comment saying why its OK if v8_snapshot_fd doesn't get
> opened.

Done.

https://codereview.chromium.org/1164483003/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1164483003/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:746: gin::V8Initializer::LoadV8Snapshot();
On 2015/06/01 14:06:31, rmcilroy wrote:
> Maybe we should make it obvious from the function name that this is OK to
fail?
> Or maybe just return true if it fails in a way we expect (i.e., file is not
> available) and only return false for unexpected errors (e.g., map errors on a
> file which exists? Then you could CHECK it everytime?

Not sure I want to CHECK it.  Lets say it's there, but can't be mapped or
verified.  I think it's more helpful to log it in UMA and carry on.

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

https://codereview.chromium.org/1164483003/diff/1/gin/public/isolate_holder.h...
gin/public/isolate_holder.h:45: // V8Initializer::LoadV8Natives) before calling
this method.
On 2015/06/01 14:06:31, rmcilroy wrote:
> Also mention that snapshot should be loaded if available before calling this
> function

Done.

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.cc#newco...
gin/v8_initializer.cc:178: GetV8FilePaths(&natives_data_path,
&snapshot_data_path);
On 2015/06/01 14:06:31, rmcilroy wrote:
> Maybe create seperate functions for natives and snapshot to avoid having to
> define an unused variable.

Done.

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h#newcode39
gin/v8_initializer.h:39: static bool LoadV8NativesFromFD(base::PlatformFile
natives_fd,
On 2015/06/01 14:06:31, rmcilroy wrote:
> could you add WARN_UNUSED_RESULT on this (and possibly loadSnapshot if you
> change it so it returns true when it fails in a way we expect).

See below

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h#newcode49
gin/v8_initializer.h:49: static bool LoadV8Natives();
On 2015/06/01 14:06:31, rmcilroy wrote:
> WARN_UNUSED_RESULT here too.

IT currently contains a LOG(FATAL) if it doesn't return true.  I think that's
better because a LOG might be more informative than a failed assert.  If we
stick with that, there's no point in having a return value.

https://codereview.chromium.org/1164483003/diff/1/net/proxy/proxy_resolver_v8.cc
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1164483003/diff/1/net/proxy/proxy_resolver_v8...
net/proxy/proxy_resolver_v8.cc:371: gin::V8Initializer::LoadV8Natives();
On 2015/06/01 14:06:31, rmcilroy wrote:
> CHECK? (and for all LoadV8Natives)

See above

https://codereview.chromium.org/1164483003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1164483003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:45054: +</histogram>
On 2015/06/01 14:06:31, rmcilroy wrote:
> According to Orion the current UMA didn't show much since crashes prevent the
> UMA from being sent. I guess we would like to keep UMA for the snapshot
loading
> to check how often it doesn't find the snapshot, but we probably don't need to
> add the natives UMA, WDYT?

Sounds right

[rmcilroy, 2015-06-02 11:27:07]

https://codereview.chromium.org/1164483003/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1164483003/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:746: gin::V8Initializer::LoadV8Snapshot();
On 2015/06/02 11:18:52, Erik Corry Chromium.org wrote:
> On 2015/06/01 14:06:31, rmcilroy wrote:
> > Maybe we should make it obvious from the function name that this is OK to
> fail?
> > Or maybe just return true if it fails in a way we expect (i.e., file is not
> > available) and only return false for unexpected errors (e.g., map errors on
a
> > file which exists? Then you could CHECK it everytime?
> 
> Not sure I want to CHECK it.  Lets say it's there, but can't be mapped or
> verified.  I think it's more helpful to log it in UMA and carry on.

As discussed below, let's get rid of the return values.

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1164483003/diff/1/gin/v8_initializer.h#newcode49
gin/v8_initializer.h:49: static bool LoadV8Natives();
On 2015/06/02 11:18:52, Erik Corry Chromium.org wrote:
> On 2015/06/01 14:06:31, rmcilroy wrote:
> > WARN_UNUSED_RESULT here too.
> 
> IT currently contains a LOG(FATAL) if it doesn't return true.  I think that's
> better because a LOG might be more informative than a failed assert.  If we
> stick with that, there's no point in having a return value.

Right makes sense. OK, let's get rid of the return values for both 
LoadV8Natives and LoadV8Snapshot and just LOG(FATAL) within the functions if we
can't continue.

[picksi, 2015-06-02 11:44:36]

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

[picksi, 2015-06-02 11:44:37]

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
ash/shell/content_client/shell_content_browser_client.cc:54: if
(v8_natives_fd_.get() != -1) {
"v8_[natives|snapshot]_fd_.get() != -1" appears a lot, is it possible to create
wrapper functions called (something like) natives_exists() & snapshot_exists()?

[Erik Corry Chromium.org, 2015-06-02 14:29:32]

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

[Erik Corry Chromium.org, 2015-06-02 14:29:47]

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
ash/shell/content_client/shell_content_browser_client.cc:54: if
(v8_natives_fd_.get() != -1) {
On 2015/06/02 11:44:37, picksi wrote:
> "v8_[natives|snapshot]_fd_.get() != -1" appears a lot, is it possible to
create
> wrapper functions called (something like) natives_exists() &
snapshot_exists()?

Done.

[picksi, 2015-06-02 14:56:51]

On 2015/06/02 14:29:47, Erik Corry Chromium.org wrote:
>
https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
> File ash/shell/content_client/shell_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
> ash/shell/content_client/shell_content_browser_client.cc:54: if
> (v8_natives_fd_.get() != -1) {
> On 2015/06/02 11:44:37, picksi wrote:
> > "v8_[natives|snapshot]_fd_.get() != -1" appears a lot, is it possible to
> create
> > wrapper functions called (something like) natives_exists() &
> snapshot_exists()?
> 
> Done.

Thanks!

[picksi, 2015-06-02 14:56:51]

On 2015/06/02 14:29:47, Erik Corry Chromium.org wrote:
>
https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
> File ash/shell/content_client/shell_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/1164483003/diff/1/ash/shell/content_client/sh...
> ash/shell/content_client/shell_content_browser_client.cc:54: if
> (v8_natives_fd_.get() != -1) {
> On 2015/06/02 11:44:37, picksi wrote:
> > "v8_[natives|snapshot]_fd_.get() != -1" appears a lot, is it possible to
> create
> > wrapper functions called (something like) natives_exists() &
> snapshot_exists()?
> 
> Done.

Thanks!

[rmcilroy, 2015-06-02 15:20:28]

https://codereview.chromium.org/1164483003/diff/40001/ash/shell/content_clien...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/ash/shell/content_clien...
ash/shell/content_client/shell_content_browser_client.cc:54: if
(natives_fd_exists()) {
Just DCHECK(natives_fd_exists() here since it should always be true from below
(and for all other ContentBrowserClient subclasses).

https://codereview.chromium.org/1164483003/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:360: // start up (slower)
without the snapshot.
Can you add this comment to all ContentBrowserClient subclasses please.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:53: void GetV8FilePath(base::FilePath* path, const char*
file_name) {
nit - /s/path/path_out and move file_name to first param.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:68: bool MapV8File(base::MemoryMappedFile**
mmapped_file_return,
nit - /s/mmapped_file_return/mmapped_file_out and move to end of parameter list
(this will mean we need to the default argument values for region, but we should
probably do that anyway since they are frowned apon by the style guide)

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:68: bool MapV8File(base::MemoryMappedFile**
mmapped_file_return,
Keep as static?

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:72: base::MemoryMappedFile* mmapped_file =
*mmapped_file_return =
DCHECK(*mmapped_file_out == NULL) before allocating here.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:74: if (!mmapped_file->IsValid()) {
I don't think this is necessary right? mmapped_file->IsValid() is always going
to be false for a newly allocated MemoryMappedFile.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:211: }
Could you share the code from lines 168-185 with 194-211 with a helper function
- e.g:

  LoadV8FileResult result = LoadV8StartupFile(&g_mapped_natives, 
kNativesFileName, g_natives_fingerprint);

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:234: return MapV8File(&g_mapped_snapshot,
base::File(snapshot_pf),
Please do the same UMA histograms here as in LoadV8Snapshot (this is actually
the one we care about on Android).

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:253: CHECK(MapV8File(&g_mapped_natives,
base::File(natives_pf), natives_region));
LOG(FATAL) here like LoadV8Natives for consistency.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.h#ne...
gin/v8_initializer.h:38: int64 snapshot_size) WARN_UNUSED_RESULT;
Remove bool return value here to be consistent with LoadV8Snapshot?

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.h#ne...
gin/v8_initializer.h:56: static bool
OpenV8FilesForChildProcesses(base::PlatformFile* natives_fd_out,
I realize this is not your change, but could you add WARN_RESULT_UNUSED please.

[picksi, 2015-06-02 16:02:15]

a couple of readability nits/bikeshedding

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:232: }
nit: would the following be more readable :

base::MemoryMappedFile::Region snapshot_region;

if (snapshot_size == 0 && snapshot_offset == 0)
{
  snapshot_region = base::MemoryMappedFile::Region::kWholeFile;
}
else
{
 snapshot_region = base::MemoryMappedFile::Region(snapshot_offset,
snapshot_size);
}

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:323: if (!g_mapped_natives) {
nit: remove the ! and swap if/else body?

[jam, 2015-06-02 17:57:09]

which files do you want me to look at?

[Erik Corry Chromium.org, 2015-06-04 11:40:41]

Hi

I need some help to land this since it touches a lot of files.  Jam can approve
it all, since he is a top level owner, but if others could take a look it would
surely help:

sky is an owner of:
ash/
chrome/plugin
content/public/test
content/test
components/html_viewer
content/shell/browser

jochen:
gin
gin/public
gin/shell
gin/test


I don't have anyone other than jam to look at
chromecast/browser
content/app
extensions/shell/browser
net/proxy

Thanks!

https://codereview.chromium.org/1164483003/diff/40001/ash/shell/content_clien...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/ash/shell/content_clien...
ash/shell/content_client/shell_content_browser_client.cc:54: if
(natives_fd_exists()) {
On 2015/06/02 15:20:27, rmcilroy wrote:
> Just DCHECK(natives_fd_exists() here since it should always be true from below
> (and for all other ContentBrowserClient subclasses).

Unfortunately this is also used for the GPU process, which does not have V8 and
so does not have the natives file open.

https://codereview.chromium.org/1164483003/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:360: // start up (slower)
without the snapshot.
On 2015/06/02 15:20:27, rmcilroy wrote:
> Can you add this comment to all ContentBrowserClient subclasses please.

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:53: void GetV8FilePath(base::FilePath* path, const char*
file_name) {
On 2015/06/02 15:20:27, rmcilroy wrote:
> nit - /s/path/path_out and move file_name to first param.

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:68: bool MapV8File(base::MemoryMappedFile**
mmapped_file_return,
On 2015/06/02 15:20:27, rmcilroy wrote:
> Keep as static?

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:68: bool MapV8File(base::MemoryMappedFile**
mmapped_file_return,
On 2015/06/02 15:20:27, rmcilroy wrote:
> nit - /s/mmapped_file_return/mmapped_file_out and move to end of parameter
list
> (this will mean we need to the default argument values for region, but we
should
> probably do that anyway since they are frowned apon by the style guide)

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:72: base::MemoryMappedFile* mmapped_file =
*mmapped_file_return =
On 2015/06/02 15:20:27, rmcilroy wrote:
> DCHECK(*mmapped_file_out == NULL) before allocating here.

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:74: if (!mmapped_file->IsValid()) {
On 2015/06/02 15:20:27, rmcilroy wrote:
> I don't think this is necessary right? mmapped_file->IsValid() is always going
> to be false for a newly allocated MemoryMappedFile.

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:211: }
On 2015/06/02 15:20:27, rmcilroy wrote:
> Could you share the code from lines 168-185 with 194-211 with a helper
function
> - e.g:
> 
>   LoadV8FileResult result = LoadV8StartupFile(&g_mapped_natives, 
> kNativesFileName, g_natives_fingerprint);

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:232: }
On 2015/06/02 16:02:15, picksi wrote:
> nit: would the following be more readable :
> 
> base::MemoryMappedFile::Region snapshot_region;
> 
> if (snapshot_size == 0 && snapshot_offset == 0)
> {
>   snapshot_region = base::MemoryMappedFile::Region::kWholeFile;
> }
> else
> {
>  snapshot_region = base::MemoryMappedFile::Region(snapshot_offset,
> snapshot_size);
> }

Some compilers are very annoying about uninitialized variables, so I'd rather
avoid them.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:234: return MapV8File(&g_mapped_snapshot,
base::File(snapshot_pf),
On 2015/06/02 15:20:27, rmcilroy wrote:
> Please do the same UMA histograms here as in LoadV8Snapshot (this is actually
> the one we care about on Android).

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:253: CHECK(MapV8File(&g_mapped_natives,
base::File(natives_pf), natives_region));
On 2015/06/02 15:20:27, rmcilroy wrote:
> LOG(FATAL) here like LoadV8Natives for consistency.

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.cc#n...
gin/v8_initializer.cc:323: if (!g_mapped_natives) {
On 2015/06/02 16:02:15, picksi wrote:
> nit: remove the ! and swap if/else body?

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.h#ne...
gin/v8_initializer.h:38: int64 snapshot_size) WARN_UNUSED_RESULT;
On 2015/06/02 15:20:27, rmcilroy wrote:
> Remove bool return value here to be consistent with LoadV8Snapshot?

Done.

https://codereview.chromium.org/1164483003/diff/40001/gin/v8_initializer.h#ne...
gin/v8_initializer.h:56: static bool
OpenV8FilesForChildProcesses(base::PlatformFile* natives_fd_out,
On 2015/06/02 15:20:28, rmcilroy wrote:
> I realize this is not your change, but could you add WARN_RESULT_UNUSED
please.

Done.

[Erik Corry Chromium.org, 2015-06-04 11:41:04]

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

[rmcilroy, 2015-06-04 12:54:30]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-04 12:54:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164483003/80001

[commit-bot: I haz the power, 2015-06-04 13:31:40]

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

[commit-bot: I haz the power, 2015-06-04 13:31:41]

Dry run: Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[rmcilroy, 2015-06-04 14:03:59]

https://codereview.chromium.org/1164483003/diff/40001/ash/shell/content_clien...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/40001/ash/shell/content_clien...
ash/shell/content_client/shell_content_browser_client.cc:54: if
(natives_fd_exists()) {
On 2015/06/04 11:40:40, Erik Corry Chromium.org wrote:
> On 2015/06/02 15:20:27, rmcilroy wrote:
> > Just DCHECK(natives_fd_exists() here since it should always be true from
below
> > (and for all other ContentBrowserClient subclasses).
> 
> Unfortunately this is also used for the GPU process, which does not have V8
and
> so does not have the natives file open.

I'm not sure this is to do with the gpu process - the ChildProcessLauncher
should be calling GetAdditionalMappedFilesForChildProcess no matter the process
type so this natives_fd_exists() should be true here even if the process in
question isn't going to use it.  The thing I'm worried about is that the process
launcher calls AppendExtraCommandLineSwitches before
GetAdditionalMappedFilesForChildProcess and therefore the first process launched
doesn't get the ::switches::kV8NativesPassedByFD in it's command line when it
should (which would cause it to fallback to loading directly from the
filesystem, which works on Linux but leads to issues during updates, and would
probably break on Android.

[sky, 2015-06-04 19:36:42]

LGTM

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
ash/shell/content_client/shell_content_browser_client.cc:57: if
(snapshot_fd_exists()) {
nit: no {}

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
File ash/shell/content_client/shell_content_browser_client.h (right):

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
ash/shell/content_client/shell_content_browser_client.h:51: bool
natives_fd_exists() { return v8_natives_fd_ != -1; }
fields than members. Also, can't you use v8_natives_fd_.is_valid() ?

[jam, 2015-06-05 04:26:42]

lgtm

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

lgtm once Ross is happy

[Erik Corry Chromium.org, 2015-06-05 13:15:17]

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
File ash/shell/content_client/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
ash/shell/content_client/shell_content_browser_client.cc:57: if
(snapshot_fd_exists()) {
On 2015/06/04 19:36:42, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
File ash/shell/content_client/shell_content_browser_client.h (right):

https://codereview.chromium.org/1164483003/diff/100001/ash/shell/content_clie...
ash/shell/content_client/shell_content_browser_client.h:51: bool
natives_fd_exists() { return v8_natives_fd_ != -1; }
On 2015/06/04 19:36:42, sky wrote:
> fields than members. Also, can't you use v8_natives_fd_.is_valid() ?

Done.

[rmcilroy, 2015-06-05 13:24:52]

lgtm, thanks!

https://codereview.chromium.org/1164483003/diff/140001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1164483003/diff/140001/content/browser/child_...
content/browser/child_process_launcher.cc:166:
GetContentClient()->browser()->AppendMappedFileCommandLineSwitches(cmd_line);
nit - there is another call to GetAdditionalMappedFilesForChildProcess in
battery_monitor_integration_browsertest.cc - could you add the
AppendMappedFileCommandLineSwitches call there too please.

[Erik Corry Chromium.org, 2015-06-05 13:44:48]

The CQ bit was checked by erikcorry@chromium.org

[Erik Corry Chromium.org, 2015-06-05 13:44:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jam@chromium.org, sky@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1164483003/#ps180001
(title: "fix battery_monitor_integration_browsertest")

[commit-bot: I haz the power, 2015-06-05 13:45:00]

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

[Erik Corry Chromium.org, 2015-06-05 15:14:44]

The CQ bit was checked by erikcorry@chromium.org

[Erik Corry Chromium.org, 2015-06-05 15:14:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jam@chromium.org, sky@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1164483003/#ps240001
(title: "Fix2 for Windows")

[commit-bot: I haz the power, 2015-06-05 15:15:37]

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

[commit-bot: I haz the power, 2015-06-05 16:36:08]

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

[commit-bot: I haz the power, 2015-06-05 16:36:10]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Erik Corry Chromium.org, 2015-06-08 10:25:23]

The CQ bit was checked by erikcorry@chromium.org

[Erik Corry Chromium.org, 2015-06-08 10:25:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jam@chromium.org, sky@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1164483003/#ps260001
(title: "Fix misplaced ifdef for Windows")

[commit-bot: I haz the power, 2015-06-08 10:25:54]

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

[commit-bot: I haz the power, 2015-06-08 11:29:26]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-06-08 11:30:35]

Patchset 14 (id:??) landed as
https://crrev.com/c94eff1e7e8b2bd5973d7211c320d9f025c980c1
Cr-Commit-Position: refs/heads/master@{#333258}

[Erik Corry Chromium.org, 2015-06-08 13:31:03]

https://codereview.chromium.org/1164483003/diff/140001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1164483003/diff/140001/content/browser/child_...
content/browser/child_process_launcher.cc:166:
GetContentClient()->browser()->AppendMappedFileCommandLineSwitches(cmd_line);
On 2015/06/05 13:24:51, rmcilroy wrote:
> nit - there is another call to GetAdditionalMappedFilesForChildProcess in
> battery_monitor_integration_browsertest.cc - could you add the
> AppendMappedFileCommandLineSwitches call there too please.

In the end I didn't do this becasue the command line is const, and the call to
GetAdditionalMappedFilesForChildProcess is inside another implementation of
GetAdditionalMappedFilesForChildProcess, so the caller of the outer version can
be expected to call AppendMappedFileCommandLineSwitches

[rmcilroy, 2015-06-08 17:26:12]

https://codereview.chromium.org/1164483003/diff/140001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1164483003/diff/140001/content/browser/child_...
content/browser/child_process_launcher.cc:166:
GetContentClient()->browser()->AppendMappedFileCommandLineSwitches(cmd_line);
On 2015/06/08 13:31:03, Erik Corry Chromium.org wrote:
> On 2015/06/05 13:24:51, rmcilroy wrote:
> > nit - there is another call to GetAdditionalMappedFilesForChildProcess in
> > battery_monitor_integration_browsertest.cc - could you add the
> > AppendMappedFileCommandLineSwitches call there too please.
> 
> In the end I didn't do this becasue the command line is const, and the call to
> GetAdditionalMappedFilesForChildProcess is inside another implementation of
> GetAdditionalMappedFilesForChildProcess, so the caller of the outer version
can
> be expected to call AppendMappedFileCommandLineSwitches

Ahh well, if it works I guess this is fine.