Introduce gin_shell

Introduce gin_shell

This CL adds a simple shell program for Gin to make edit/test/debug cycle
faster. The shell excutes a list of scripts from the command line and loads any
requested AMD modules relative to the current working directory.

This CL will also let us remove the ugly code in
https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/test/run_js_tests.cc
because we now know how to file modules via the file system. Eventually for
Mojo, we'll want to use a net_module_provider (instead of the
file_module_provider included in this CL) to load additional AMD modules off
the network.

BUG=317398
R=jochen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235750

[abarth-chromium, 2013-11-17 18:20:24]

Please take a look.  Thanks!

[jochen (gone - plz use gerrit), 2013-11-18 12:46:05]

nice!

https://codereview.chromium.org/74753002/diff/1/gin/gin.gyp
File gin/gin.gyp (right):

https://codereview.chromium.org/74753002/diff/1/gin/gin.gyp#newcode60
gin/gin.gyp:60: ],
you could add

'msvs_settings': {
  'VCLinkerTool': {
    'SubSystem': '1', # /SUBSYSTEM:CONSOLE
  },
},

for windows

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
File gin/modules/file_module_provider.cc (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:17: void
AttempToLoadModule(base::WeakPtr<Runner> runner,
should all these parameters be const references?

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:20: if (!runner.get())
no get() needed

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:26: for (size_t i = 0; i <
components.size(); ++i) {
no { } needed

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:27: path = path.AppendASCII(components[i]);
the id is actually utf8, isn't it?

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:42: }
add // namespace

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
File gin/modules/file_module_provider.h (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.h:10: #include "base/files/file_path.h"
nit. empty line before this one

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.h:18: virtual ~FileModuleProvider();
why is this virtual? Did you intend to have a ModuleProvider base class?

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.h:20: void AttempToLoadModules(Runner* runner,
const std::set<std::string>& ids);
should this have some kind of callback that is invoked if the module was loaded
successfully or not?

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.cc (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:28: using v8::Value;
is there a reason why you use all those names? I find it pretty confusing to
have generic names like Handle without the v8::

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:33: PendingModule();
Do you ever construct a PendingModule without setting all three data members?
Why not initialize them already in the constructor

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:130: context->Global()->SetHiddenValue(key,
External::New(registry));
I just changed this API to require isolate as first parameter

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.h (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:11: #include "base/compiler_specific.h"
nit empty line before this line

https://codereview.chromium.org/74753002/diff/40001/gin/runner.cc
File gin/runner.cc (right):

https://codereview.chromium.org/74753002/diff/40001/gin/runner.cc#newcode63
gin/runner.cc:63: script->Run();
consider locking the isolate and entering before running scripts.

This code currently relies on the default isolate being correctly setup and
everything. It would be nicer to do this explicitly.

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

https://codereview.chromium.org/74753002/diff/40001/gin/shell/gin_main.cc#new...
gin/shell/gin_main.cc:47: };
nit. private: DISALLOW_COPY_AND_ASSIGN(ShellRunnerDelegate)

https://codereview.chromium.org/74753002/diff/40001/gin/shell/gin_main.cc#new...
gin/shell/gin_main.cc:55: gin::Initialize();
in gin::Initialize, you should invoke base::i18n::InitializeICU() instead of
V8::InitializeICU

https://codereview.chromium.org/74753002/diff/40001/gin/try_catch.h
File gin/try_catch.h (right):

https://codereview.chromium.org/74753002/diff/40001/gin/try_catch.h#newcode23
gin/try_catch.h:23: };
DISALLOW_COPY_AND_ASSIGN

[abarth-chromium, 2013-11-18 15:33:05]

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
File gin/modules/file_module_provider.cc (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:17: void
AttempToLoadModule(base::WeakPtr<Runner> runner,
On 2013/11/18 12:46:05, jochen wrote:
> should all these parameters be const references?

Done.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:20: if (!runner.get())
On 2013/11/18 12:46:05, jochen wrote:
> no get() needed

Done.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:27: path = path.AppendASCII(components[i]);
On 2013/11/18 12:46:05, jochen wrote:
> the id is actually utf8, isn't it?

I've added a TODO about this issue.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.cc:42: }
On 2013/11/18 12:46:05, jochen wrote:
> add // namespace

Done.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
File gin/modules/file_module_provider.h (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.h:10: #include "base/files/file_path.h"
On 2013/11/18 12:46:05, jochen wrote:
> nit. empty line before this one

Done.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.h:18: virtual ~FileModuleProvider();
On 2013/11/18 12:46:05, jochen wrote:
> why is this virtual? Did you intend to have a ModuleProvider base class?

Yeah, I had one in an earlier iteration of the CL and forgot to remove the
virtual destructor.  We'll probably have one eventually to generalize better the
local and remote case, but I've removed the virtual destructor for now.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/file_module_p...
gin/modules/file_module_provider.h:20: void AttempToLoadModules(Runner* runner,
const std::set<std::string>& ids);
On 2013/11/18 12:46:05, jochen wrote:
> should this have some kind of callback that is invoked if the module was
loaded
> successfully or not?

We could certainly add that, but it wouldn't be used currently.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.cc (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:28: using v8::Value;
On 2013/11/18 12:46:05, jochen wrote:
> is there a reason why you use all those names? I find it pretty confusing to
> have generic names like Handle without the v8::

If we don't do this, there are a bunch of lines that hit the 80 col limit and
have really ugly line breaks.  I can remove it if you like, but we've been doing
this in files that make heavy use of the V8 API.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:33: PendingModule();
On 2013/11/18 12:46:05, jochen wrote:
> Do you ever construct a PendingModule without setting all three data members?
> Why not initialize them already in the constructor

I didn't do it because the naming of the arguments is really awkward.  The first
argument wants to be called "id", but that's also the name of the member
variable...  I can do it if you want, but I double it makes a measurable
performance difference.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.cc:130: context->Global()->SetHiddenValue(key,
External::New(registry));
On 2013/11/18 12:46:05, jochen wrote:
> I just changed this API to require isolate as first parameter

Nice!  It was odd that it didn't before.  Fixed.

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
File gin/modules/module_registry.h (right):

https://codereview.chromium.org/74753002/diff/40001/gin/modules/module_regist...
gin/modules/module_registry.h:11: #include "base/compiler_specific.h"
On 2013/11/18 12:46:05, jochen wrote:
> nit empty line before this line

Done.

https://codereview.chromium.org/74753002/diff/40001/gin/runner.cc
File gin/runner.cc (right):

https://codereview.chromium.org/74753002/diff/40001/gin/runner.cc#newcode63
gin/runner.cc:63: script->Run();
On 2013/11/18 12:46:05, jochen wrote:
> consider locking the isolate and entering before running scripts.
> 
> This code currently relies on the default isolate being correctly setup and
> everything. It would be nicer to do this explicitly.

Done.  Is there some way to enable an ASSERT in V8 to make sure we're always
doing this?

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

https://codereview.chromium.org/74753002/diff/40001/gin/shell/gin_main.cc#new...
gin/shell/gin_main.cc:47: };
On 2013/11/18 12:46:05, jochen wrote:
> nit. private: DISALLOW_COPY_AND_ASSIGN(ShellRunnerDelegate)

Done.

https://codereview.chromium.org/74753002/diff/40001/gin/shell/gin_main.cc#new...
gin/shell/gin_main.cc:55: gin::Initialize();
On 2013/11/18 12:46:05, jochen wrote:
> in gin::Initialize, you should invoke base::i18n::InitializeICU() instead of
> V8::InitializeICU

Done.

[abarth-chromium, 2013-11-18 15:34:37]

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

https://codereview.chromium.org/74753002/diff/40001/gin/shell/gin_main.cc#new...
gin/shell/gin_main.cc:55: gin::Initialize();
On 2013/11/18 15:33:06, abarth wrote:
> On 2013/11/18 12:46:05, jochen wrote:
> > in gin::Initialize, you should invoke base::i18n::InitializeICU() instead of
> > V8::InitializeICU
> 
> Done.

If I do that, gin_shell doesn't link.  Maybe I need to pull in ICU via base
instead of via V8?

[abarth-chromium, 2013-11-18 15:36:39]

https://codereview.chromium.org/74753002/diff/40001/gin/runner.cc
File gin/runner.cc (right):

https://codereview.chromium.org/74753002/diff/40001/gin/runner.cc#newcode63
gin/runner.cc:63: script->Run();
On 2013/11/18 15:33:06, abarth wrote:
> On 2013/11/18 12:46:05, jochen wrote:
> > consider locking the isolate and entering before running scripts.
> > 
> > This code currently relies on the default isolate being correctly setup and
> > everything. It would be nicer to do this explicitly.
> 
> Done.  Is there some way to enable an ASSERT in V8 to make sure we're always
> doing this?

Adding a v8::Locker causes an ASSERT.  I've left it off for now.  We can add it
in a future CL.

[abarth-chromium, 2013-11-18 15:40:00]

Patch updated, PTAL.

I've also rolled https://codereview.chromium.org/60613003/ into this CL so that
I can actually send it to the try bots.  Thanks!

[jochen (gone - plz use gerrit), 2013-11-18 15:54:42]

lgtm, thx

[commit-bot: I haz the power, 2013-11-18 16:00:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/74753002/190001

[commit-bot: I haz the power, 2013-11-18 16:20:37]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[abarth-chromium, 2013-11-18 17:09:40]

Committed patchset #4 manually as r235750.