Bug 45148 – [Qt] V8 port for Qt platform: Qt API implementation changes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vlad]

V8 Implementation of 
QWebFrame::addToJavaScriptWindowObject()
QWebFrame::evaluateJavaScript()

[Vlad]

Created attachment 66443 [details]
QT API implementation changes

[Andreas Kling]

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 :-)

[Vlad]

(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 :-)

[Vlad]

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.

[Andreas Kling]

Comment on attachment 66577 [details]
Review comments changes

LGTM

[Kent Hansen]

(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)

[Andreas Kling]

Committed r67301: <http://trac.webkit.org/changeset/67301>