Apply ScriptPreprocessor to Web page scripts only.

Apply ScriptPreprocessor to Web page scripts only.

Move ScriptPreprocessor from ScriptDebugServer to ScriptController.
Evaluate preprocessor in isolatedWorld.
Preprocess event listeners and <script> elements before entering V8.
Preprocess eval source in V8 beforeCompile event.

Add Debugger.getScriptCompilationTypeInfo() to extract the
V8 compilation script info, to seperate eval scripts from
e.g. devtools command line evaluations.

Report exceptions in the ScriptPreprocessor compilation.
Add new test-cases for these error paths.

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

[johnjbarton, 2013-07-16 17:19:12]

This patch is the same as 
https://codereview.chromium.org/13575004/#ps128001
which has LGTM but CQ will not process it because it has a Base URL from before
the blink fork.

[abarth-chromium, 2013-07-16 22:30:28]

lgtm

[commit-bot: I haz the power, 2013-07-16 22:35:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnjbarton@chromium.org/19284011/1

[commit-bot: I haz the power, 2013-07-16 22:35:09]

Failed to apply patch for Source/bindings/v8/ScriptDebugServer.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/v8/ScriptDebugServer.h
  Hunk #2 succeeded at 79 (offset 1 line).
  Hunk #3 succeeded at 93 (offset 1 line).
  Hunk #4 succeeded at 120 (offset 1 line).
  Hunk #5 FAILED at 127.
  1 out of 5 hunks FAILED -- saving rejects to file
Source/bindings/v8/ScriptDebugServer.h.rej

Patch:       Source/bindings/v8/ScriptDebugServer.h
Index: Source/bindings/v8/ScriptDebugServer.h
diff --git a/Source/bindings/v8/ScriptDebugServer.h
b/Source/bindings/v8/ScriptDebugServer.h
index
80fb9a6416db8ac1b51c3806c5d737fdc1277cf0..8f8fa04ff6dc4f907e60cb4c86697a77d6bdefd8
100644
--- a/Source/bindings/v8/ScriptDebugServer.h
+++ b/Source/bindings/v8/ScriptDebugServer.h
@@ -45,6 +45,7 @@
 
 namespace WebCore {
 
+class ScriptController;
 class ScriptDebugListener;
 class ScriptObject;
 class ScriptState;
@@ -77,8 +78,6 @@ public:
     bool setScriptSource(const String& sourceID, const String& newContent, bool
preview, String* error, RefPtr<TypeBuilder::Debugger::SetScriptSourceError>&,
ScriptValue* newCallFrames, ScriptObject* result);
     void updateCallStack(ScriptValue* callFrame);
 
-    void setScriptPreprocessor(const String& preprocessorBody);
-
     class Task {
     public:
         virtual ~Task() { }
@@ -93,16 +92,16 @@ public:
     v8::Local<v8::Value> functionScopes(v8::Handle<v8::Function>);
     v8::Local<v8::Value> getInternalProperties(v8::Handle<v8::Object>&);
     v8::Handle<v8::Value> setFunctionVariableValue(v8::Handle<v8::Value>
functionValue, int scopeNumber, const String& variableName,
v8::Handle<v8::Value> newValue);
-
+    v8::Local<v8::Value> callDebuggerMethod(const char* functionName, int argc,
v8::Handle<v8::Value> argv[]);
 
     virtual void compileScript(ScriptState*, const String& expression, const
String& sourceURL, String* scriptId, String* exceptionMessage);
     virtual void clearCompiledScripts();
     virtual void runScript(ScriptState*, const String& scriptId, ScriptValue*
result, bool* wasThrown, String* exceptionMessage);
+    virtual ScriptController* scriptController(v8::Handle<v8::Context>);
 
 protected:
     explicit ScriptDebugServer(v8::Isolate*);
     virtual ~ScriptDebugServer();
-    
     ScriptValue currentCallFrame();
 
     virtual ScriptDebugListener*
getDebugListenerForContext(v8::Handle<v8::Context>) = 0;
@@ -120,10 +119,6 @@ protected:
 
     void ensureDebuggerScriptCompiled();
 
-    v8::Local<v8::Value> callDebuggerMethod(const char* functionName, int argc,
v8::Handle<v8::Value> argv[]);
-
-    String preprocessSourceCode(const String& sourceCode);
-
     PauseOnExceptionsState m_pauseOnExceptionsState;
     ScopedPersistent<v8::Object> m_debuggerScript;
     ScopedPersistent<v8::Object> m_executionState;
@@ -132,10 +127,7 @@ protected:
     ScopedPersistent<v8::FunctionTemplate> m_breakProgramCallbackTemplate;
     HashMap<String, OwnPtr<ScopedPersistent<v8::Script> > > m_compiledScripts;
     v8::Isolate* m_isolate;
-    
 private:
-    class ScriptPreprocessor;
-    OwnPtr<ScriptPreprocessor> m_scriptPreprocessor;
     bool m_runningNestedMessageLoop;
 };

[commit-bot: I haz the power, 2013-07-16 22:51:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnjbarton@chromium.org/19284011/6001

[haraken, 2013-07-16 23:31:36]

LGTM

[commit-bot: I haz the power, 2013-07-17 00:56:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-17 16:12:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnjbarton@chromium.org/19284011/21001

[commit-bot: I haz the power, 2013-07-17 18:47:35]

Change committed as 154410

[pfeldman, 2013-07-22 18:33:05]

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
File Source/bindings/v8/DebuggerScript.js (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
Source/bindings/v8/DebuggerScript.js:312:
DebuggerScript._getScriptCompilationTypeInfo = function(script)
no _get prefixes in Blink

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
Source/bindings/v8/DebuggerScript.js:317: result += 'eval from ';
Use double quotes

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
Source/bindings/v8/DebuggerScript.js:344: return exc + '';
No abbreviations in Blink

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
File Source/bindings/v8/V8LazyEventListener.cpp (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
Source/bindings/v8/V8LazyEventListener.cpp:138: String listenerSource =
frame->script()->preprocess(m_code, m_functionName);
That's not the way inspector is typically handling overrides, we are using
InspectorInstrumentation for that. It makes sure we are not affecting
performance.

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/core/inspec...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/core/inspec...
Source/core/inspector/PageDebuggerAgent.cpp:144:
frame->script()->setScriptPreprocessor(m_pageAgent->scriptPreprocessor());
Ditto. This might affect performance.

[johnjbarton, 2013-07-22 18:45:33]

Pavel, I need a couple of hints:

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
File Source/bindings/v8/V8LazyEventListener.cpp (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
Source/bindings/v8/V8LazyEventListener.cpp:138: String listenerSource =
frame->script()->preprocess(m_code, m_functionName);
On 2013/07/22 18:33:05, pfeldman wrote:
> That's not the way inspector is typically handling overrides, we are using
> InspectorInstrumentation for that. It makes sure we are not affecting
> performance.

Can you point me to an example that might be similar?

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/core/inspec...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/core/inspec...
Source/core/inspector/PageDebuggerAgent.cpp:144:
frame->script()->setScriptPreprocessor(m_pageAgent->scriptPreprocessor());
On 2013/07/22 18:33:05, pfeldman wrote:
> Ditto. This might affect performance.

Even in the case that scriptPreprocessor isEmpty() ?

[pfeldman, 2013-07-23 07:51:05]

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
File Source/bindings/v8/ScriptPreprocessor.cpp (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
Source/bindings/v8/ScriptPreprocessor.cpp:120: String scriptName =
toWebCoreStringWithUndefinedOrNullCheck(debugServer->callDebuggerMethod("getScriptName",
WTF_ARRAY_LENGTH(argvEventData), argvEventData));
You should only talk to DebuggerScript from within ScriptDebugServer.

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
File Source/bindings/v8/V8LazyEventListener.cpp (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/bindings/v8...
Source/bindings/v8/V8LazyEventListener.cpp:138: String listenerSource =
frame->script()->preprocess(m_code, m_functionName);
Sure. You should not have extracted preprocessor and made it accessible through
the ScriptController - that's not how inspector gets into things. You put here:

InspectorInstrumentation::prepareEventListenerObject(frame, m_code,
m_functionName, &listenerSource);

Then you introduce corresponding instrumentation in InspectorInstrumentation.idl
and route it to the InspectorDebuggerAgent. From there, you call
ScriptDebugServer. It bails out in case debugger agent is not engaged and I
think that's how you save 3-4% time that has regressed.

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/core/inspec...
File Source/core/inspector/PageDebuggerAgent.cpp (right):

https://chromiumcodereview.appspot.com/19284011/diff/21001/Source/core/inspec...
Source/core/inspector/PageDebuggerAgent.cpp:144:
frame->script()->setScriptPreprocessor(m_pageAgent->scriptPreprocessor());
Here you simply go into ScriptDebugServer.