[Webkit-unassigned] [Bug 45148] [Qt] V8 port for QT platform: QT API implementation changes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45148


Andreas Kling <andreas.kling at nokia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66443|review?                     |review-
               Flag|                            |




--- Comment #2 from Andreas Kling <andreas.kling at nokia.com>  2010-09-03 03:39:08 PST ---
(From update of attachment 66443)
> +#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 :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.