[v8-dev] Re: Expose function CheckDebugBreak in the debugger api (issue 473913002 by serg...@chromium.org)

2014-08-14 Thread vsevik
lgtm https://chromiumcodereview.appspot.com/473913002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving email

[v8-dev] Re: Add ScriptID field into ScriptOrigin class. (issue 363553005 by kozyatins...@google.com)

2014-07-03 Thread vsevik
Ulan, friendly ping. https://chromiumcodereview.appspot.com/363553005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop r

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002 by ma...@chromium.org)

2014-07-01 Thread vsevik
So the sourceURL is not available in the "before compile" debug event. See the test fix. aandrey@, is this okay from the dev tools side? (yurys@ said previously that the dev tools side should be okay as long as cctest passes, and it does now). AFAIK DevTools only uses BeforeCompile event

[v8-dev] Re: Add OnCompileError handler and v8::CompileError debug event (issue 264333007 by kozyatins...@google.com)

2014-06-24 Thread vsevik
Friendly ping https://codereview.chromium.org/264333007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving email

[v8-dev] Re: Add OnCompileError handler and v8::CompileError debug event (issue 264333007 by kozyatins...@google.com)

2014-06-20 Thread vsevik
https://codereview.chromium.org/264333007/diff/150001/src/parser.cc#newcode3863 > > src/parser.cc:3863: isolate()->Throw(*result, &location); > > Won't Debug::OnException trigger as well? The call to Debug::OnCompileError > > doesn't actually contain the syntax error location, right? So you w

[v8-dev] Re: Add OnCompileError handler and v8::CompileError debug event (issue 264333007)

2014-06-11 Thread vsevik
lgtm https://codereview.chromium.org/264333007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from

[v8-dev] Re: Add OnSyntaxError handler and v8::SyntaxError debug event (issue 264333007)

2014-06-10 Thread vsevik
Looks good to me. yurys@, yangguo@, Could you please take a look on that? https://chromiumcodereview.appspot.com/264333007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-

[v8-dev] Re: Add OnSyntaxError handler and v8::SyntaxError debug event (issue 264333007)

2014-06-10 Thread vsevik
Looks good to me. https://chromiumcodereview.appspot.com/264333007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop rece

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-10 Thread vsevik
https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc#newcode318 src/scanner.cc:318: // Magic comments are of the form \s*name\s*=\s*value\s*.* and this function We might need to allow only single whitespace between @ and sourceURL= and no space between sourceURL and =: ..

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-10 Thread vsevik
The result seems unchanged why the tests would break? Oh, I didn't get it, you are right, this is fine. https://codereview.chromium.org/316173002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed t

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-10 Thread vsevik
https://codereview.chromium.org/316173002/diff/120001/src/messages.js#oldcode561 src/messages.js:561: if (this.hasCachedNameOrSourceURL) { This should be done in a separate patch, otherwise v8 roll is going to break blink (blink can not migrate to new way of getting source_url before the rol

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-10 Thread vsevik
https://codereview.chromium.org/316173002/diff/120001/src/messages.js File src/messages.js (left): https://codereview.chromium.org/316173002/diff/120001/src/messages.js#oldcode561 src/messages.js:561: if (this.hasCachedNameOrSourceURL) { This should be done in a separate patch, otherwise v8 roll

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-06 Thread vsevik
FYI: Here is a blink side patch. It seems to pass all the tests. https://codereview.chromium.org/323523004/ https://codereview.chromium.org/316173002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed

[v8-dev] Re: Add OnSyntaxError handler and v8::ScriptfailedToParse debug event (issue 264333007)

2014-06-06 Thread vsevik
https://chromiumcodereview.appspot.com/264333007/diff/20001/include/v8-debug.h File include/v8-debug.h (right): https://chromiumcodereview.appspot.com/264333007/diff/20001/include/v8-debug.h#newcode25 include/v8-debug.h:25: ScriptFailedToParse = 9 SyntaxError https://chromiumcodereview.appspot.

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-06 Thread vsevik
The latest patch set parses both sourceURL and sourceMappingURL, and allows them in the middle of the script too (the later one overrides the previous one, if there are several instances of the same value). Great, thank you! I'm not sure if you'd need more plumbing than this, for example,

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-05 Thread vsevik
s function bar() {} ? I don't like this syntax and would prefer if we didn't support it, but I we support it in our current implementation and it is possible that some of our users would be upset if we change that. We can still change it later (I hope the diff wouldn't be big) vsevi

[v8-dev] Re: Handle "//# sourceURL" comments in the Parser instead of the JS. (issue 316173002)

2014-06-05 Thread vsevik
I believe we should consider only sourceURL that are at the end of file. If there is something like function foo(){} //@ sourceURL = url.js function bar(){} we should skip the url as far as I remember. This is not quite so. For example we want to support a case when the script has both s

[v8-dev] Re: Add GetScriptResourceNameOrSourceURL with tests (issue 265593002)

2014-06-05 Thread vsevik
Now that I looked closer at this API, I see (if I'm not mistaken) two ways to get the script name of where the exception was thrown: - Through the script in the Message object, like exposed in this CL. - Through the stack trace in the Message object. The top most function in the stack trace c

[v8-dev] Re: Add GetScriptResourceNameOrSourceURL with tests (issue 265593002)

2014-06-04 Thread vsevik
Yeah, you are right. We discussed this with Vsevolod and it seems that what we actually need in debugger is a public API to v8::internal::Script object that contains all the data. After discussion with the V8 team it seems that there is no plans to change API to support this case. Do you th

[v8-dev] Re: Don't clear exception pending message if we have externally TryCatch and finally on top of stack. (issue 306463002)

2014-06-02 Thread vsevik
https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc File src/isolate.cc (left): https://chromiumcodereview.appspot.com/306463002/diff/60001/src/isolate.cc#oldcode1157 src/isolate.cc:1157: if (!is_catchable_by_javascript(pending_exception())) { Looking at this once again I

[v8-dev] Re: Don't clear exception pending message if we have externally TryCatch and finally on top of stack. (issue 306463002)

2014-06-02 Thread vsevik
yangguo@, Could you please have a look on this? https://chromiumcodereview.appspot.com/306463002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe

[v8-dev] Re: Don't clear exception pending message if we have externally TryCatch and finally on top of stack. (issue 306463002)

2014-06-02 Thread vsevik
lgtm https://chromiumcodereview.appspot.com/306463002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails

[v8-dev] Re: Don't clear exception pending message if we have externally TryCatch and finally on top of stack. (issue 306463002)

2014-06-02 Thread vsevik
https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc File src/isolate.cc (right): https://chromiumcodereview.appspot.com/306463002/diff/20001/src/isolate.cc#newcode1730 src/isolate.cc:1730: bool has_externaly_caught = HasExternallyCaught(); has_external_try_catch https://c

[v8-dev] Re: Don't clear exception pending message if we have externally TryCatch and finally on top of stack. (issue 306463002)

2014-05-28 Thread vsevik
Can we add a test? https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc File src/isolate.cc (right): https://chromiumcodereview.appspot.com/306463002/diff/1/src/isolate.cc#newcode1727 src/isolate.cc:1727: // Return true. if we don't pending message and can remove it Either com

[v8-dev] Re: Add GetScriptResourceNameOrSourceURL with tests (issue 265593002)

2014-05-05 Thread vsevik
Yury, could you please take a look? https://chromiumcodereview.appspot.com/265593002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this g

[v8-dev] Add GetScriptResourceNameOrSourceURL with tests (issue 265593002)

2014-04-30 Thread vsevik
https://codereview.chromium.org/265593002/diff/40001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/265593002/diff/40001/include/v8.h#newcode1134 include/v8.h:1134: * Returns the resource name for the script from where the function causing Returns the resource name or so

[v8-dev] Stack trace string should use dynamic script sourceURL if present. (issue 143283015)

2014-02-11 Thread vsevik
Reviewers: yurys, dcarney, Message: yurys@, could you please take a look? dcarney@, could you please OWNER review this? Description: Stack trace string should use dynamic script sourceURL if present. BUG=v8:2342 R=yurys Please review this at https://codereview.chromium.org/143283015/ SVN Base

[v8-dev] Add scriptId to StackTrace frames. (issue 23536007)

2013-08-30 Thread vsevik
Reviewers: Yury Semikhatsky, Toon Verwaest, Message: yurys@: Could you please review this change? verwaest@: Could you please OWNER review this change? Description: Add scriptId to StackTrace frames. BUG=v8:2865 Please review this at https://codereview.chromium.org/23536007/ SVN Base: git:

[v8-dev] Re: DevTools: CPUProfiler: provide url for scripts that have sourceURL property. (issue 16035027)

2013-06-11 Thread vsevik
https://codereview.chromium.org/16035027/diff/17001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/16035027/diff/17001/src/compiler.cc#newcode1188 src/compiler.cc:1188: Handle name = GetScriptNameOrSourceURL(script); Can you just do GetScriptNameOrSourceURL here? W

[v8-dev] Add support for //# sourceURL similar to deprecated //@ sourceURL one. (issue 15859010)

2013-05-29 Thread vsevik
Reviewers: Yury Semikhatsky, Description: Add support for //# sourceURL similar to deprecated //@ sourceURL one. BUG=v8:2702 Please review this at https://codereview.chromium.org/15859010/ SVN Base: git://github.com/v8/v8.git@master Affected files: M include/v8.h M src/messages.js M tes

[v8-dev] Re: Supported sourceURL comments for scripts having a name. (issue 10966011)

2012-09-20 Thread vsevik
Reviewers: Yury Semikhatsky, Message: Please take another look Description: Supported sourceURL comments for scripts having a name. sourceURL comment is now preferred script name for all scripts except for those with non zero start position (e.g. inline scripts in HTML). BUG=2342 Please rev