[webkit-reviews] review denied: [Bug 45148] [Qt] V8 port for QT platform: QT API implementation changes : [Attachment 66443] QT API implementation changes

[bugzilla-daemon at webkit.org]

Andreas Kling <andreas.kling at nokia.com> has denied Vlad <vladbph at gmail.com>'s
request for review:
Bug 45148: [Qt] V8 port for QT platform: QT API implementation changes
https://bugs.webkit.org/show_bug.cgi?id=45148

Attachment 66443: QT API implementation changes
https://bugs.webkit.org/attachment.cgi?id=66443&action=review

------- Additional Comments from Andreas Kling <andreas.kling at nokia.com>
> +#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 :-)