Blink-in-JS: Make error messages thrown in private scripts more verbose

Blink-in-JS: Make error messages thrown in private scripts more verbose

- Split compilation of private scripts from running installClass(), in order to distinguish a compile error with a runtime error that happens in installClass().

- Added ScriptOrigin to private scripts. By doing this, dumpV8Message() can print a file name and a line number where an error is triggered.

- Refactored PrivateScriptRunner.cpp a bit.

This just affects fprintf(stderr) and doesn't change any web-exposed behavior.

BUG=341031
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180180

[haraken, 2014-08-13 11:46:31]

jl@: PTAL

[Jens Widell, 2014-08-13 12:08:38]

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:44: static v8::Handle<v8::Value>
compilePrivateScript(v8::Isolate* isolate, String className, const unsigned
char* source, size_t size)
I think "compileAndRun" was a bit more accurate. Any reason to call this just
"compile" when it also runs?

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:48: String fileName = className
+ ".js";
This isn't necessarily correct for a partial interface, is it? It wouldn't be
entirely helpful if the error message said "Document.js (line 15)" when the
error is actually in DocumentXSLT.js.

[haraken, 2014-08-13 12:37:57]

Thanks for review.

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:44: static v8::Handle<v8::Value>
compilePrivateScript(v8::Isolate* isolate, String className, const unsigned
char* source, size_t size)
On 2014/08/13 12:08:38, Jens Lindström wrote:
> I think "compileAndRun" was a bit more accurate. Any reason to call this just
> "compile" when it also runs?

Done.

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:48: String fileName = className
+ ".js";
On 2014/08/13 12:08:38, Jens Lindström wrote:
> This isn't necessarily correct for a partial interface, is it? It wouldn't be
> entirely helpful if the error message said "Document.js (line 15)" when the
> error is actually in DocumentXSLT.js.

Good point. I changed the code so that |className| becomes a class name of a
partial interface.

However, the current variable names (e.g., |className| and
|dependencyClassName|) are a bit confusing. I'll rename them as follows in a
follow-up CL.

- |className| means a class name of a dependency interface (e.g., "Document").

- |partialClassName| means a class name of a partial interface (e.g.,
"DocumentXSLT").

https://codereview.chromium.org/465283002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/465283002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:27: // implemented in private
scripts? In that case, currently ResourceName() returns an empty handle.

Actually I couldn't figure out a way to get a resource name when a private
script is invoked by Function::Call(). It looks like that a resource name is
available only when the private script is run by Script::Run().

[Jens Widell, 2014-08-13 12:56:19]

LGTM

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/465283002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:48: String fileName = className
+ ".js";
On 2014/08/13 12:37:57, haraken wrote:
> On 2014/08/13 12:08:38, Jens Lindström wrote:
> > This isn't necessarily correct for a partial interface, is it? It wouldn't
be
> > entirely helpful if the error message said "Document.js (line 15)" when the
> > error is actually in DocumentXSLT.js.
> 
> Good point. I changed the code so that |className| becomes a class name of a
> partial interface.
> 
> However, the current variable names (e.g., |className| and
> |dependencyClassName|) are a bit confusing. I'll rename them as follows in a
> follow-up CL.
> 
> - |className| means a class name of a dependency interface (e.g., "Document").
> 
> - |partialClassName| means a class name of a partial interface (e.g.,
> "DocumentXSLT").

Sounds like a good plan.

[haraken, 2014-08-13 12:59:52]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:01:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/465283002/140001

[commit-bot: I haz the power, 2014-08-13 13:59:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19643)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22540)
  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/41103)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46321)

[commit-bot: I haz the power, 2014-08-13 14:04:09]

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

[commit-bot: I haz the power, 2014-08-13 14:04:10]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19649)

[haraken, 2014-08-13 14:09:04]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-13 14:09:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/465283002/160001

[Jens Widell, 2014-08-13 14:09:34]

https://codereview.chromium.org/465283002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/465283002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:89:
compileAndRunPrivateScript(isolate,
kPrivateScriptSourcesForTesting[index].className,
kPrivateScriptSources[index].source, kPrivateScriptSources[index].size);
Ah, shouldn't be *ForTesting here.

[haraken, 2014-08-13 14:10:36]

https://codereview.chromium.org/465283002/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/465283002/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:89:
compileAndRunPrivateScript(isolate,
kPrivateScriptSourcesForTesting[index].className,
kPrivateScriptSources[index].source, kPrivateScriptSources[index].size);
On 2014/08/13 14:09:34, Jens Lindström wrote:
> Ah, shouldn't be *ForTesting here.

Yes, fixed :)

[commit-bot: I haz the power, 2014-08-13 15:09:34]

Committed patchset #9 (160001) as 180180