Return value from executed script in Chromium.
Created attachment 129365 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 129365 [details] Patch Attachment 129365 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11695043
Comment on attachment 129365 [details] Patch Attachment 129365 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11686353
Comment on attachment 129365 [details] Patch Looks like you've got some compilation trouble.
Comment on attachment 129365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129365&action=review > LayoutTests/ChangeLog:11 > + * platform/chromium/http/tests/misc/execute-and-return-value-expected.txt: Added. > + * platform/chromium/http/tests/misc/execute-and-return-value.html: Added. If you think layoutTestController.evaluateScriptInIsolatedWorldAndReturnValue() is chromium-specific, we don't need to provide empty implementations of it for other platforms. If not, we should put the test to the common place (not platform/chromium/).
Created attachment 129796 [details] Patch
Comment on attachment 129796 [details] Patch Attachment 129796 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11769722
Comment on attachment 129796 [details] Patch Attachment 129796 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11767730
Comment on attachment 129796 [details] Patch Attachment 129796 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11766735
Created attachment 130463 [details] Patch
Comment on attachment 130463 [details] Patch Attachment 130463 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11838596
Created attachment 130473 [details] Patch
Comment on attachment 130473 [details] Patch Attachment 130473 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11834746 New failing tests: fast/xmlhttprequest/xmlhttprequest-gc.html
Created attachment 132067 [details] Patch
Comment on attachment 132067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132067&action=review This patch uses a bunch of risky memory management patterns. Using OwnPtr and PassOwnPtr is clearly a win inside WebKit. It's less clear to me what the best way of returning v8 handles across the WebKit API is. Do we do this in other places that might establish a pattern that we should follow? > Source/WebCore/bindings/v8/ScriptController.cpp:166 > + WTF::Vector<v8::Handle<v8::Value> >* v8Results = m_proxy->evaluateInIsolatedWorld(worldID, sources, 0); You shouldn't need to use the WTF namespace here. > Source/WebCore/bindings/v8/ScriptController.cpp:167 > + WTF::Vector<ScriptValue>* results = new WTF::Vector<ScriptValue>(); We should never call "new" without immediately calling adoptPtr or adoptRef. In this case, we should call adoptPtr. > Source/WebCore/bindings/v8/ScriptController.cpp:171 > + for (itr = v8Results->begin(); itr != v8Results->end(); ++itr) > + results->append(ScriptValue(*itr)); This code is incorrectly indented. > Source/WebCore/bindings/v8/ScriptController.cpp:173 > + delete v8Results; We should never be calling delete manually. Do we need to use an OwnPtr and PassOwnPtr to manage the lifetime of this object? > Source/WebCore/bindings/v8/V8Proxy.cpp:266 > + evaluationResults->append(v8::Persistent<v8::Value>::New(evaluate(sources[i], 0))); Creating a new v8::Persistent value is dangerous because it can easily lead to memory leaks. Perhaps the caller should be responsible for creating a v8::HandleScope? It's not clear to me what the best way of returning a value is without risking memory leaks.
Also, consider using out parameters rather than returning heap-allocated vectors. That makes it easier for the function to know whether its caller cares about the return value.
Created attachment 132400 [details] Patch
Comment on attachment 132400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132400&action=review This is much better than the previous iterations. > Source/WebCore/bindings/v8/V8Proxy.cpp:265 > + if (results) Can we move this test outside of the loop? I guess the compiler will probably do that optimization of us... > Source/WebKit/chromium/src/WebFrameImpl.cpp:906 > + sources.append(ScriptSourceCode( > + sourcesIn[i].code, sourcesIn[i].url, position)); No need to wrap at 80 cols > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1350 > + // TODO: There are many more types that can be handled. TODO => FIXME
Did you want to mark this patch for review? It looks pretty good. If you'd like me to review it, I can study it in more detail.
Comment on attachment 132400 [details] Patch Yes, that would be great. I think that the constant re-packaging of the evaluated values is a bit unfortunate, but I can't see a way around it that follows the conventions of the code around it.
Created attachment 133928 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 133928 [details] Patch 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.
Humm... if we pass the Vector of v8::Handle<v8::Value>'s, do we need the Handle to be Persistent? I could be mistaken, but doesn't the result of the eval get placed into a Local which is managed by the HandleScope created in V8Proxy::evaluateInIsolatedWorld(). I think it was fine before when I was using ScriptValue because ScriptValue uses Persistent. Sorry if I am totally off, it's all a bit confusing for a first dive into WebKit.
I'm not 100% sure either. Certainly, if we're returning local handles, we'll need the caller to have their own HandleScope. I'm not sure what happens if you have two HandleScopes on the stack. Are locals bound to the most recent HandleScope or to the last to unwind?
Comment on attachment 133928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133928&action=review >> Source/WebCore/ChangeLog:8 >> + Providing a varaiant of evaluateScriptInIsolatedWorld that > > typo: varaiant done. >> Source/WebKit/chromium/public/WebFrame.h:279 >> + int extensionGroup, WebVector<v8::Handle<v8::Value> >* results) = 0; > > Should we be more specific that these are local handles? Yes, I think we should. Doing this should make it clear to the caller that they need their own HandleScope. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:906 >> + } > > Is this code copy/pasted from somewhere? Should we share code instead? It's from WebFrameImpl::executeScriptInIsolatedWorld. Do you think it is worth it? >> 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. Discussed in IRC. ScriptValue bundles the value in a Persistent which helps the hand-off to the caller's HandleScope. This is only an issue because the Vector is passed by reference instead of by value.
Created attachment 136572 [details] Patch
Comment on attachment 136572 [details] Patch Attachment 136572 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12384609
Created attachment 136682 [details] Patch
Comment on attachment 136682 [details] Patch Attachment 136682 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12387136
Created attachment 136721 [details] Patch
Comment on attachment 136721 [details] Patch Did you mean to mark this patch for review? The bubbles are all green. I think we're all set here.
Yes I did.
Comment on attachment 136721 [details] Patch Clearing flags on attachment: 136721 Committed r114974: <http://trac.webkit.org/changeset/114974>
All reviewed patches have been landed. Closing bug.