[webkit-reviews] review denied: [Bug 32554] Create injected script instance per inspected frame context : [Attachment 46120] patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 32554: Create injected script instance per inspected frame context
https://bugs.webkit.org/show_bug.cgi?id=32554

Attachment 46120: patch
https://bugs.webkit.org/attachment.cgi?id=46120&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
No ChangeLog.

This code is fantastically complicated but there are no tests!	How will we
avoid breaking this in the future?

 82	v8::Handle<v8::Context> utilityContext = scriptState->context();
 83	v8::Context::Scope contextScope(utilityContext);

It's important to use local handles in v8::Context::Scope.  Is
scriptState->context() a local handle?	The risk is the persistent handle might
be disposed while this context is still on the stack -> disaster.

 1380	var backslashesRe = /\\["\\\/bfnrtu]/g;
 1381	var simpleValuesRe =
 1382	   
/"[^"\\\n\r\u2028\u2029\x00-\x08\x10-\x1f\x80-\x9f]*"|true|false|null|-?\d+(?:\
.\d*)?(?:[eE][+\-]?\d+)?/g;
 1383	var openBracketsRe = /(?:^|:|,)(?:[\s\u2028\u2029]*\[)+/g;
 1384	var remainderRe = /^[\],:{}\s\u2028\u2029]*$/;

Why aren't we using the native JSON parser?

I have no idea whether the rest is correct, but hopefully we have other folks
who can review it.  R- for no ChangeLog.

I'm concerned about the lack of tests in this area.