[webkit-reviews] review denied: [Bug 79851] Return value from executed script in Chromium. : [Attachment 133928] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Eriq Augustine
<eaugusti at chromium.org>'s request for review:
Bug 79851: Return value from executed script in Chromium.
https://bugs.webkit.org/show_bug.cgi?id=79851

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133928&action=review


Sorry that it's taken me so long to review this patch.	This looks fine.  A
couple of small suggestions below.  I think the main thing is that we can avoid
converting back and forth between ScriptValues.  If WebCore wants to use this
function at some point, we might need to use ScriptValue, but it doesn't seem
to be needed right now.

> Source/WebCore/ChangeLog:8
> +	   Providing a varaiant of evaluateScriptInIsolatedWorld that

typo: varaiant

> Source/WebKit/chromium/public/WebFrame.h:279
> +    virtual void executeScriptInIsolatedWorld(
> +	   int worldID, const WebScriptSource* sourcesIn, unsigned numSources,
> +	   int extensionGroup, WebVector<v8::Handle<v8::Value> >* results) = 0;


Should we be more specific that these are local handles?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:906
> +    for (unsigned i = 0; i < numSources; ++i) {
> +	   TextPosition
position(OrdinalNumber::fromOneBasedInt(sourcesIn[i].startLine),
OrdinalNumber::first());
> +	   sources.append(ScriptSourceCode(sourcesIn[i].code, sourcesIn[i].url,
position));
> +    }

Is this code copy/pasted from somewhere?  Should we share code instead?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:909
> +	   Vector<ScriptValue> scriptResults;

Do we really need to round-trip through ScriptValue?  Both the caller and the
callee of this function always use V8, so it seems like we could just pass the
vector directly.