Move V8 snapshot loading code from isolate_holder to gin/v8_startup_data.{h,cc}.

Move V8 snapshot loading code from isolate_holder to gin/v8_startup_data.{h,cc}.

isolate_holder.h is public and used by blink and therefore cannot refer to types from base. As part of porting this to other platforms we needed to load snapshots from BaseFile descriptors.

BUG=421063
Committed: https://crrev.com/05c26fde422600a25247a22f9670463216d0519a
Cr-Commit-Position: refs/heads/master@{#323886}

[oth, 2015-03-26 09:57:23]

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

[oth, 2015-03-26 09:57:23]

Please take a look over this, first CL, thanks!

[rmcilroy, 2015-03-26 10:50:09]

https://codereview.chromium.org/1011133006/diff/1/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1011133006/diff/1/gin/isolate_holder.cc#newco...
gin/isolate_holder.cc:15: #include "crypto/sha2.h"
I think you can drop crypto/sha2.h now

https://codereview.chromium.org/1011133006/diff/1/gin/isolate_holder.cc#newco...
gin/isolate_holder.cc:95: gin::V8StartupData::Initialize();
I don't think we want to add an Initialize function to V8StartupData (since this
is the only place it should ever be called and it's not really something we
would want to expose to the rest of Chrome).

I'm think the solution would be to pull the whole IsolateHolder::Initialize (and
associated) functions out into the new V8StartupData class (and rename the class
V8Initializer or similar).

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc
File gin/v8_startup_data.cc (left):

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#oldc...
gin/v8_startup_data.cc:11: #include "base/logging.h"
You should have base/logging.h I think

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#oldc...
gin/v8_startup_data.cc:16: #include "crypto/sha2.h"
I think you still need this for VerifyV8SnapshotFile

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc
File gin/v8_startup_data.cc (right):

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#newc...
gin/v8_startup_data.cc:17: #include "gin/v8_startup_data.h"
put this header at the very top (with a newline after it before the other
includes)

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#newc...
gin/v8_startup_data.cc:18: #include "v8/include/v8.h"
group with other non-ifdefed includes

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#newc...
gin/v8_startup_data.cc:144: #else   // !defined(OS_MACOSX)
nit - drop extra whitespace

[oth, 2015-03-30 09:42:00]

This change incorporates comments in the last round.

There is a failure in the Mac debug build, but I'm as-yet unclear about the root
cause.

http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_dbg_...

https://codereview.chromium.org/1011133006/diff/1/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1011133006/diff/1/gin/isolate_holder.cc#newco...
gin/isolate_holder.cc:15: #include "crypto/sha2.h"
On 2015/03/26 10:50:08, rmcilroy wrote:
> I think you can drop crypto/sha2.h now

Done.

https://codereview.chromium.org/1011133006/diff/1/gin/isolate_holder.cc#newco...
gin/isolate_holder.cc:95: gin::V8StartupData::Initialize();
On 2015/03/26 10:50:08, rmcilroy wrote:
> I don't think we want to add an Initialize function to V8StartupData (since
this
> is the only place it should ever be called and it's not really something we
> would want to expose to the rest of Chrome).
> 
> I'm think the solution would be to pull the whole IsolateHolder::Initialize
(and
> associated) functions out into the new V8StartupData class (and rename the
class
> V8Initializer or similar).

Yes, IsolateHolder::Initialize() has been moved to V8Initializer::Initialize().
It looks like multiple steps needed to push this so in first step,
IsolateHolder::Initializer() still exists, but just calls
V8Initializer::Initialize().

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc
File gin/v8_startup_data.cc (left):

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#oldc...
gin/v8_startup_data.cc:11: #include "base/logging.h"
On 2015/03/26 10:50:08, rmcilroy wrote:
> You should have base/logging.h I think

Done.

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc
File gin/v8_startup_data.cc (right):

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#newc...
gin/v8_startup_data.cc:17: #include "gin/v8_startup_data.h"
On 2015/03/26 10:50:08, rmcilroy wrote:
> put this header at the very top (with a newline after it before the other
> includes)

Done.

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#newc...
gin/v8_startup_data.cc:18: #include "v8/include/v8.h"
On 2015/03/26 10:50:08, rmcilroy wrote:
> group with other non-ifdefed includes

Done.

https://codereview.chromium.org/1011133006/diff/1/gin/v8_startup_data.cc#newc...
gin/v8_startup_data.cc:144: #else   // !defined(OS_MACOSX)
On 2015/03/26 10:50:08, rmcilroy wrote:
> nit - drop extra whitespace

Done.

[rmcilroy, 2015-03-30 10:29:22]

This is looking good!

Please update the description and mention this is part-one of a series of
patches.

A couple more comments and then we will send it out to a wider audience.

https://codereview.chromium.org/1011133006/diff/100001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1011133006/diff/100001/gin/isolate_holder.cc#...
gin/isolate_holder.cc:68:
V8Initializer::Initialize((V8Initializer::ScriptMode)mode, allocator);
nit - let's just make IsolateHolder::ScriptMode a typedef of
V8Initializer::ScriptMode during the transition.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:35: base::MemoryMappedFile* g_mapped_snapshot = NULL;
nit - could you update these to nullptr's too since your here (to be consistent
with the g_array_buffer_allocator you updated above).

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:137: return g_array_buffer_allocator;
let's keep the g_array_buffer_allocator in IsolateHolder.cc and set it from
here.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:19: enum ScriptMode { kNonStrictMode, kStrictMode };
note for future cleanup (or now if it's easy) we should make this an "emum
class" now that we have C++11 compiler support.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:28: // LoadV8SnapshotFd) before calling Initialize.
update "LoadV8SnapshotFd" in comment with name chosen below

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:34: static void GetV8ExternalSnapshotData(const char**
natives_data_out,
Could you document these methods please (you can copy the comments from
https://codereview.chromium.org/1019483002/diff/120001/gin/public/isolate_hol...)

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:41: static bool LoadV8SnapshotFromFile(base::PlatformFile
natives_fd,
Both this and LoadV8Snapshot load it from a file, so let's make this
LoadV8SnapshotFromFileDescriptor (or LoadV8SnapshotFromFD for succinctness) -

[oth, 2015-03-30 15:55:19]

Comments for review comments from patch set 6 incorporated into patch set 7.

https://codereview.chromium.org/1011133006/diff/100001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1011133006/diff/100001/gin/isolate_holder.cc#...
gin/isolate_holder.cc:68:
V8Initializer::Initialize((V8Initializer::ScriptMode)mode, allocator);
On 2015/03/30 10:29:22, rmcilroy wrote:
> nit - let's just make IsolateHolder::ScriptMode a typedef of
> V8Initializer::ScriptMode during the transition.

In patch set 7, there is just gin::IsolateHolder::ScriptMode.

The V8Initializer class has base file types in that may not be directly
available to all consumers of the IsolateHolder class so leaving the enum there
for now.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:35: base::MemoryMappedFile* g_mapped_snapshot = NULL;
On 2015/03/30 10:29:22, rmcilroy wrote:
> nit - could you update these to nullptr's too since your here (to be
consistent
> with the g_array_buffer_allocator you updated above).

Done.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:19: enum ScriptMode { kNonStrictMode, kStrictMode };
On 2015/03/30 10:29:22, rmcilroy wrote:
> note for future cleanup (or now if it's easy) we should make this an "emum
> class" now that we have C++11 compiler support.

Yes, good point. In the near term, the code now just uses
gin::IsolateHolder::ScriptMode enum.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:28: // LoadV8SnapshotFd) before calling Initialize.
On 2015/03/30 10:29:22, rmcilroy wrote:
> update "LoadV8SnapshotFd" in comment with name chosen below

Done and comment moved to IsolateHolder.h for now.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:34: static void GetV8ExternalSnapshotData(const char**
natives_data_out,
On 2015/03/30 10:29:22, rmcilroy wrote:
> Could you document these methods please (you can copy the comments from
>
https://codereview.chromium.org/1019483002/diff/120001/gin/public/isolate_hol...)

Done.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:34: static void GetV8ExternalSnapshotData(const char**
natives_data_out,
On 2015/03/30 10:29:22, rmcilroy wrote:
> Could you document these methods please (you can copy the comments from
>
https://codereview.chromium.org/1019483002/diff/120001/gin/public/isolate_hol...)

Done.

https://codereview.chromium.org/1011133006/diff/100001/gin/v8_initializer.h#n...
gin/v8_initializer.h:41: static bool LoadV8SnapshotFromFile(base::PlatformFile
natives_fd,
On 2015/03/30 10:29:22, rmcilroy wrote:
> Both this and LoadV8Snapshot load it from a file, so let's make this
> LoadV8SnapshotFromFileDescriptor (or LoadV8SnapshotFromFD for succinctness) - 

Done.

[rmcilroy, 2015-03-30 17:44:17]

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

[rmcilroy, 2015-03-30 17:44:18]

Looks great, just a couple of gin::V8Initializer::Initialize() calls to be
turned back to gin::IsolateHolder::Initialize().

Jochen: could you take a look and see if this is going in a direction you're
happy with?

https://codereview.chromium.org/1011133006/diff/100001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1011133006/diff/100001/gin/isolate_holder.cc#...
gin/isolate_holder.cc:68:
V8Initializer::Initialize((V8Initializer::ScriptMode)mode, allocator);
On 2015/03/30 15:55:19, orion wrote:
> On 2015/03/30 10:29:22, rmcilroy wrote:
> > nit - let's just make IsolateHolder::ScriptMode a typedef of
> > V8Initializer::ScriptMode during the transition.
> 
> In patch set 7, there is just gin::IsolateHolder::ScriptMode.
> 
> The V8Initializer class has base file types in that may not be directly
> available to all consumers of the IsolateHolder class so leaving the enum
there
> for now.

sounds good.

https://codereview.chromium.org/1011133006/diff/120001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1011133006/diff/120001/gin/isolate_holder.cc#...
gin/isolate_holder.cc:72: g_array_buffer_allocator = allocator;
nit - add a CHECK(allocator) here (I know it's done in V8Initializer, but just
for safety in case that code gets changed in the future.

https://codereview.chromium.org/1011133006/diff/120001/gin/test/file_runner.cc
File gin/test/file_runner.cc (right):

https://codereview.chromium.org/1011133006/diff/120001/gin/test/file_runner.c...
gin/test/file_runner.cc:66:
gin::V8Initializer::Initialize(gin::IsolateHolder::kStrictMode,
I think this should still be gin::IsolateHolder::Initialize() (and similarly
throughout).

https://codereview.chromium.org/1011133006/diff/120001/gin/v8_initializer.h
File gin/v8_initializer.h (right):

https://codereview.chromium.org/1011133006/diff/120001/gin/v8_initializer.h#n...
gin/v8_initializer.h:25: v8::ArrayBuffer::Allocator* allocator);
It would be good if we could avoid exposing this method as part of the public
API, but I can't think of a good way of doing this without having IsolateHolder
friend V8Initializer, which I'm not particularly keen to do. Any thoughts
anyone?

[oth, 2015-03-31 09:34:47]

Thanks!

https://codereview.chromium.org/1011133006/diff/120001/gin/isolate_holder.cc
File gin/isolate_holder.cc (right):

https://codereview.chromium.org/1011133006/diff/120001/gin/isolate_holder.cc#...
gin/isolate_holder.cc:72: g_array_buffer_allocator = allocator;
On 2015/03/30 17:44:18, rmcilroy wrote:
> nit - add a CHECK(allocator) here (I know it's done in V8Initializer, but just
> for safety in case that code gets changed in the future.

Done.

https://codereview.chromium.org/1011133006/diff/120001/gin/test/file_runner.cc
File gin/test/file_runner.cc (right):

https://codereview.chromium.org/1011133006/diff/120001/gin/test/file_runner.c...
gin/test/file_runner.cc:66:
gin::V8Initializer::Initialize(gin::IsolateHolder::kStrictMode,
On 2015/03/30 17:44:18, rmcilroy wrote:
> I think this should still be gin::IsolateHolder::Initialize() (and similarly
> throughout).

Done.

[jochen (gone - plz use gerrit), 2015-03-31 10:35:38]

i'm happy

https://codereview.chromium.org/1011133006/diff/140001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1011133006/diff/140001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:109: "+gin/v8_initializer.h",
we could just add "+gin",

[oth, 2015-03-31 16:39:22]

oth@chromium.org changed reviewers:
+ eroman@chromium.org, raymes@chromium.org

[oth, 2015-03-31 16:39:24]

eroman@chromium.org: Please review changes in net

raymes@chromium.org: Please review changes in ppapi

https://codereview.chromium.org/1011133006/diff/140001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1011133006/diff/140001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:109: "+gin/v8_initializer.h",
On 2015/03/31 10:35:38, jochen wrote:
> we could just add "+gin",

Done.

[rmcilroy, 2015-03-31 16:59:03]

lgtm, thanks!

https://codereview.chromium.org/1011133006/diff/160001/chrome/plugin/DEPS
File chrome/plugin/DEPS (right):

https://codereview.chromium.org/1011133006/diff/160001/chrome/plugin/DEPS#new...
chrome/plugin/DEPS:4: "+gin/v8_initializer.h",
maybe just +gin here too

https://codereview.chromium.org/1011133006/diff/160001/content/app/DEPS
File content/app/DEPS (right):

https://codereview.chromium.org/1011133006/diff/160001/content/app/DEPS#newcode8
content/app/DEPS:8: "+gin/v8_initializer.h",
ditto

https://codereview.chromium.org/1011133006/diff/160001/content/shell/DEPS
File content/shell/DEPS (right):

https://codereview.chromium.org/1011133006/diff/160001/content/shell/DEPS#new...
content/shell/DEPS:3: "+gin/v8_initializer.h",
ditto

[eroman, 2015-03-31 18:19:52]

net/* LGTM

[raymes, 2015-03-31 22:51:23]

rubberstamp lgtm ppapi/

[oth, 2015-04-01 13:19:46]

On 2015/03/31 10:35:38, jochen wrote:
> i'm happy
> 
> https://codereview.chromium.org/1011133006/diff/140001/chrome/browser/DEPS
> File chrome/browser/DEPS (right):
> 
>
https://codereview.chromium.org/1011133006/diff/140001/chrome/browser/DEPS#ne...
> chrome/browser/DEPS:109: "+gin/v8_initializer.h",
> we could just add "+gin",

Thanks Jochen, DEPS change done.

[jochen (gone - plz use gerrit), 2015-04-01 14:51:49]

lgtm

[oth, 2015-04-01 15:37:05]

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

[commit-bot: I haz the power, 2015-04-01 15:37:30]

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

[commit-bot: I haz the power, 2015-04-01 15:54:01]

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

[commit-bot: I haz the power, 2015-04-01 15:54:03]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oth, 2015-04-02 11:10:44]

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

[oth, 2015-04-02 11:10:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
eroman@chromium.org, raymes@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1011133006/#ps180001
(title: "Fix ifdef scope for chromeos and rebase.")

[commit-bot: I haz the power, 2015-04-02 11:10:54]

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

[commit-bot: I haz the power, 2015-04-02 11:15:28]

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

[commit-bot: I haz the power, 2015-04-02 11:30:39]

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

[commit-bot: I haz the power, 2015-04-02 11:45:44]

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

[commit-bot: I haz the power, 2015-04-02 12:00:30]

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

[commit-bot: I haz the power, 2015-04-02 12:15:35]

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

[commit-bot: I haz the power, 2015-04-02 12:30:21]

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

[commit-bot: I haz the power, 2015-04-02 12:45:51]

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

[commit-bot: I haz the power, 2015-04-02 13:00:32]

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

[commit-bot: I haz the power, 2015-04-02 13:15:32]

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

[commit-bot: I haz the power, 2015-04-02 13:30:41]

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

[commit-bot: I haz the power, 2015-04-02 13:45:50]

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

[commit-bot: I haz the power, 2015-04-02 14:01:17]

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

[commit-bot: I haz the power, 2015-04-02 14:15:48]

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

[commit-bot: I haz the power, 2015-04-02 14:30:45]

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

[commit-bot: I haz the power, 2015-04-02 14:45:35]

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

[commit-bot: I haz the power, 2015-04-02 15:00:38]

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

[commit-bot: I haz the power, 2015-04-02 15:15:30]

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

[commit-bot: I haz the power, 2015-04-02 15:30:36]

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

[commit-bot: I haz the power, 2015-04-02 15:46:31]

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

[commit-bot: I haz the power, 2015-04-02 16:01:41]

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

[commit-bot: I haz the power, 2015-04-02 16:17:10]

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

[commit-bot: I haz the power, 2015-04-02 16:30:39]

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

[rmistry, 2015-04-02 16:39:04]

The CQ bit was unchecked by rmistry@google.com

[rmistry, 2015-04-02 17:02:39]

The CQ bit was checked by rmistry@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-02 17:04:07]

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

[rmistry, 2015-04-02 17:06:04]

On 2015/04/02 17:04:07, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1011133006/180001

jochen@ pointed me to this CL. There was a bug in the CQ that kept posting the
same messages for dry runs. I am watching this CL to see if it is now fixed.

[rmistry, 2015-04-02 19:05:05]

+phajdan +sergiyb

I do not understand why this CL is still going. The CQ should have abandoned it
because of the tryjob failures, this does not seem related to dry runs. Is there
an infinite retry set somewhere?

[Sergiy Byelozyorov, 2015-04-02 19:09:43]

On 2015/04/02 19:05:05, rmistry wrote:
> +phajdan +sergiyb
> 
> I do not understand why this CL is still going. The CQ should have abandoned
it
> because of the tryjob failures, this does not seem related to dry runs. Is
there
> an infinite retry set somewhere?

Looks like it stopped 2 hours ago, didn't it?

[rmistry, 2015-04-02 19:11:02]

On 2015/04/02 19:09:43, Sergiy Byelozyorov wrote:
> On 2015/04/02 19:05:05, rmistry wrote:
> > +phajdan +sergiyb
> > 
> > I do not understand why this CL is still going. The CQ should have abandoned
> it
> > because of the tryjob failures, this does not seem related to dry runs. Is
> there
> > an infinite retry set somewhere?
> 
> Looks like it stopped 2 hours ago, didn't it?

The Commit checkbox is still checked, and I do not see a msg from commit-bot
saying trybots failed.

[rmistry, 2015-04-02 20:25:15]

On 2015/04/02 19:11:02, rmistry wrote:
> On 2015/04/02 19:09:43, Sergiy Byelozyorov wrote:
> > On 2015/04/02 19:05:05, rmistry wrote:
> > > +phajdan +sergiyb
> > > 
> > > I do not understand why this CL is still going. The CQ should have
abandoned
> > it
> > > because of the tryjob failures, this does not seem related to dry runs. Is
> > there
> > > an infinite retry set somewhere?
> > 
> > Looks like it stopped 2 hours ago, didn't it?
> 
> The Commit checkbox is still checked, and I do not see a msg from commit-bot
> saying trybots failed.

I am going to uncheck the checkbox. There are failures anyway that look like
need to be fixed. Lets see what happens when the non dry run CQ is later
clicked.

[rmistry, 2015-04-02 20:25:24]

The CQ bit was unchecked by rmistry@google.com

[chromium-reviews, 2015-04-02 21:14:32]

The commit checkbox was not set here - I'm a Noogler and wanted to test first on
more platforms. CQ helped identify some problems with the changelist.

I submitted a dry run yesterday, but that also appeared to have the commit bit
set, but this wasn't requested.

-----Original Message-----
From: "rmistry@google.com" <rmistry@google.com>
Sent: ‎02/‎04/‎2015 20:11
To: "oth@chromium.org" <oth@chromium.org>; "jochen@chromium.org"
<jochen@chromium.org>; "eroman@chromium.org" <eroman@chromium.org>;
"raymes@chromium.org" <raymes@chromium.org>; "rmcilroy@chromium.org"
<rmcilroy@chromium.org>
Cc: "cbentzel+watch@chromium.org" <cbentzel+watch@chromium.org>;
"chromium-reviews@chromium.org" <chromium-reviews@chromium.org>;
"darin-cc@chromium.org" <darin-cc@chromium.org>; "jam@chromium.org"
<jam@chromium.org>; "phajdan.jr@chromium.org" <phajdan.jr@chromium.org>
Subject: Re: Move V8 snapshot loading code from isolate_holder
togin/v8_startup_data.{h,cc}. (issue 1011133006 by oth@chromium.org)

On 2015/04/02 19:09:43, Sergiy Byelozyorov wrote:
> On 2015/04/02 19:05:05, rmistry wrote:
> > +phajdan +sergiyb
> >
> > I do not understand why this CL is still going. The CQ should have  
> abandoned
> it
> > because of the tryjob failures, this does not seem related to dry runs.  
> Is
> there
> > an infinite retry set somewhere?

> Looks like it stopped 2 hours ago, didn't it?

The Commit checkbox is still checked, and I do not see a msg from commit-bot
saying trybots failed.

https://codereview.chromium.org/1011133006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmistry, 2015-04-03 11:04:44]

On 2015/04/02 21:14:32, chromium-reviews wrote:
> The commit checkbox was not set here - I'm a Noogler and wanted to test first
on
> more platforms. CQ helped identify some problems with the changelist.
> 
> I submitted a dry run yesterday, but that also appeared to have the commit bit
> set, but this wasn't requested.

Dry runs automatically check the commit bot because it needs to go to the CQ.
You will see a "(dry run)" message show up next to the CQ checkbox for such
cases.

> 
> -----Original Message-----
> From: mailto:"rmistry@google.com" <mailto:rmistry@google.com>
> Sent: ‎02/‎04/‎2015 20:11
> To: mailto:"oth@chromium.org" <mailto:oth@chromium.org>;
mailto:"jochen@chromium.org"
> <mailto:jochen@chromium.org>; mailto:"eroman@chromium.org"
<mailto:eroman@chromium.org>;
> mailto:"raymes@chromium.org" <mailto:raymes@chromium.org>;
mailto:"rmcilroy@chromium.org"
> <mailto:rmcilroy@chromium.org>
> Cc: mailto:"cbentzel+watch@chromium.org" <mailto:cbentzel+watch@chromium.org>;
> mailto:"chromium-reviews@chromium.org" <mailto:chromium-reviews@chromium.org>;
> mailto:"darin-cc@chromium.org" <mailto:darin-cc@chromium.org>;
mailto:"jam@chromium.org"
> <mailto:jam@chromium.org>; mailto:"phajdan.jr@chromium.org"
<mailto:phajdan.jr@chromium.org>
> Subject: Re: Move V8 snapshot loading code from isolate_holder
> togin/v8_startup_data.{h,cc}. (issue 1011133006 by mailto:oth@chromium.org)
> 
> On 2015/04/02 19:09:43, Sergiy Byelozyorov wrote:
> > On 2015/04/02 19:05:05, rmistry wrote:
> > > +phajdan +sergiyb
> > >
> > > I do not understand why this CL is still going. The CQ should have  
> > abandoned
> > it
> > > because of the tryjob failures, this does not seem related to dry runs.  
> > Is
> > there
> > > an infinite retry set somewhere?
> 
> > Looks like it stopped 2 hours ago, didn't it?
> 
> The Commit checkbox is still checked, and I do not see a msg from commit-bot
> saying trybots failed.
> 
> https://codereview.chromium.org/1011133006/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[oth, 2015-04-05 12:01:26]

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

[oth, 2015-04-05 12:01:27]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
raymes@chromium.org, eroman@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1011133006/#ps200001
(title: "Fix missing #ifdef in V8Initializer::Initialize.")

[commit-bot: I haz the power, 2015-04-05 12:01:34]

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

[commit-bot: I haz the power, 2015-04-05 12:58:23]

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

[commit-bot: I haz the power, 2015-04-05 12:58:25]

This issue passed the CQ dry run.

[oth, 2015-04-05 14:30:03]

The CQ bit was checked by oth@chromium.org

[commit-bot: I haz the power, 2015-04-05 14:30:08]

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

[commit-bot: I haz the power, 2015-04-05 14:31:11]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-04-05 14:32:00]

Patchset 11 (id:??) landed as
https://crrev.com/05c26fde422600a25247a22f9670463216d0519a
Cr-Commit-Position: refs/heads/master@{#323886}