V8 Implementation of QWebFrame::addToJavaScriptWindowObject() QWebFrame::evaluateJavaScript()
Created attachment 66443 [details] QT API implementation changes
Comment on attachment 66443 [details] QT API implementation changes > +#if !USE(JSC) > +using namespace V8::Bindings; > +#endif This is backwards, it should be #if USE(V8) if anything. Also, like I said earlier I think we should always explicitly say V8 instead of having it implied by !USE(JSC), example: #if USE(JSC) ... #elif USE(V8) ... #endif > + // Create scope handler > + v8::HandleScope hs; > + // Get proxy from scriptcontroller > + V8Proxy* proxy = scriptController->proxy(); > + // Ask the context from proxy > + v8::Handle<v8::Context> context = proxy->context(); This style of commenting detracts from readability and should be avoided. > + if (object.IsEmpty()) { > + return QVariant(); > + } Coding style, remove the { and }. > +#if defined(WTF_USE_V8) && WTF_USE_V8 Use the USE macro from wtf/Platform.h instead. > +#if USE(JSC) > void QWebFrame::addToJavaScriptWindowObject(const QString &name, QObject *object, QScriptEngine::ValueOwnership ownership) Since the function signature has no engine-specific parameters, the preprocessor conditional should be inside the function. > + v8::HandleScope handle_scope; ... > + v8::Context::Scope conxtext_scope(v8Context); we_dont_use_this_style, weUseThisStyle :-)
(In reply to comment #2) > (From update of attachment 66443 [details]) > > +#if !USE(JSC) > > +using namespace V8::Bindings; > > +#endif > > This is backwards, it should be #if USE(V8) if anything. Also, like I said earlier I think we should always explicitly say V8 instead of having it implied by !USE(JSC), example: > > #if USE(JSC) > ... > #elif USE(V8) > ... > #endif > OK > > > + // Create scope handler > > + v8::HandleScope hs; > > + // Get proxy from scriptcontroller > > + V8Proxy* proxy = scriptController->proxy(); > > + // Ask the context from proxy > > + v8::Handle<v8::Context> context = proxy->context(); > OK > This style of commenting detracts from readability and should be avoided. > > > + if (object.IsEmpty()) { > > + return QVariant(); > > + } > This would break compilation if header is used outside of webkit. > Coding style, remove the { and }. > > > +#if defined(WTF_USE_V8) && WTF_USE_V8 > > Use the USE macro from wtf/Platform.h instead. > OK > > +#if USE(JSC) > > void QWebFrame::addToJavaScriptWindowObject(const QString &name, QObject *object, QScriptEngine::ValueOwnership ownership) > > Since the function signature has no engine-specific parameters, the preprocessor conditional should be inside the function. style check missed that :) OK > > > + v8::HandleScope handle_scope; > ... > > + v8::Context::Scope conxtext_scope(v8Context); > > we_dont_use_this_style, weUseThisStyle :-)
Created attachment 66577 [details] Review comments changes #if defined(WTF_USE_V8) && WTF_USE_V8 left in the code b/c the qwebelement.h can be used as public header and USE() macro could not be defined.
Comment on attachment 66577 [details] Review comments changes LGTM
(In reply to comment #4) > Created an attachment (id=66577) [details] > Review comments changes > > #if defined(WTF_USE_V8) && WTF_USE_V8 left in the code b/c the qwebelement.h can be used as public header and USE() macro could not be defined. Small nit: setupScriptContext() still uses #else instead of #elif USE(V8)
Committed r67301: <http://trac.webkit.org/changeset/67301>