Bug 45145 – [Qt] V8 port for QT platform: v8 binding changes

• Component: New Bugs
• Status: RESOLVED FIXED

[Vlad]

V8 binding changes for QT

[Vlad]

Created attachment 66437 [details]
v8 binding changes

[Vlad]

Created attachment 66438 [details]
v8 binding changes

[Simon Hausmann]

Comment on attachment 66438 [details]
v8 binding changes

View in context: https://bugs.webkit.org/attachment.cgi?id=66438&action=prettypatch

> WebCore/bindings/v8/npruntime_internal.h:30
> -#include "npapi.h"
> +#if PLATFORM(QT)
> +#include <bridge/npapi.h>
> +#endif
Why is this PLATFORM(QT) guard added here? (the ChangeLog doesn't explain)

Won't the missing inclusion break the Chromium build?

> WebCore/bindings/v8/NPV8Object.cpp:254
> +#if PLATFORM(QT)
> +    bool popupsAllowed = false;
> +#else
It looks like this is at least missing a FIXME.

Then again, maybe it's not too difficult to fix. What about this?

PluginView* view = pluginViewForInstance(npp);
bool popupsAllowed = view ? view->arePopupsAllowed() : false;

> WebCore/bindings/v8/ScriptCachedFrameData.h:54
> +#elif PLATFORM(ANDROID) || PLATFORM(QT)
Any explanation why this is needed for the Qt port but not for Chromium for example? (I'm just curious :)

> WebCore/bindings/v8/V8Binding.cpp:75
> +#if !PLATFORM(QT)
>          : m_plainString(string)
>          , m_atomicString(string)
> +#endif
>      {
>  #ifndef NDEBUG
>          m_threadId = WTF::currentThread();
>  #endif
> +#if PLATFORM(QT)
> +        m_plainString = string;
> +        m_atomicString = string;
> +#endif
Why is the initialization done different for the Qt port?

Or is this not actually Qt specific but specific to rvct?

> WebCore/config.h:190
> +#if PLATFORM(QT)
> +#include <bridge/npruntime_internal.h>
> +#endif
What compile errors did you encounter?

I'm going to say r- because I think some of the #ifdefs are not correct (such as the string init one) and there's a little bit of context missing for some of the changes.

In general it looks good though.

[Vlad]

(In reply to comment #3)
> (From update of attachment 66438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66438&action=prettypatch
This should be fixed by changing inclusion order. At the time of the port it was a problem.
> 
> > WebCore/bindings/v8/npruntime_internal.h:30
> > -#include "npapi.h"
> > +#if PLATFORM(QT)
> > +#include <bridge/npapi.h>
> > +#endif
> Why is this PLATFORM(QT) guard added here? (the ChangeLog doesn't explain)
> 
OK
> Won't the missing inclusion break the Chromium build?
> 
> > WebCore/bindings/v8/NPV8Object.cpp:254
> > +#if PLATFORM(QT)
> > +    bool popupsAllowed = false;
> > +#else
> It looks like this is at least missing a FIXME.
> 
> Then again, maybe it's not too difficult to fix. What about this?
> 
> PluginView* view = pluginViewForInstance(npp);
> bool popupsAllowed = view ? view->arePopupsAllowed() : false;

Comment from Chromium says: // We don't use WebKit's page caching, so this implementation is just a stub.
I chose to use caching :)
> 
> > WebCore/bindings/v8/ScriptCachedFrameData.h:54
> > +#elif PLATFORM(ANDROID) || PLATFORM(QT)
> Any explanation why this is needed for the Qt port but not for Chromium for example? (I'm just curious :)

Original code crashes at runtime. Most likely wrong constructor is called. This is again may be related to CString.h inclusion. Will check it if #ifdef can be removed now.
> 
> > WebCore/bindings/v8/V8Binding.cpp:75
> > +#if !PLATFORM(QT)
> >          : m_plainString(string)
> >          , m_atomicString(string)
> > +#endif
> >      {
> >  #ifndef NDEBUG
> >          m_threadId = WTF::currentThread();
> >  #endif
> > +#if PLATFORM(QT)
> > +        m_plainString = string;
> > +        m_atomicString = string;
> > +#endif
> Why is the initialization done different for the Qt port?
> 
> Or is this not actually Qt specific but specific to rvct?

Compilation errors appear on maemo. Will double check this.
> 
> > WebCore/config.h:190
> > +#if PLATFORM(QT)
> > +#include <bridge/npruntime_internal.h>
> > +#endif
> What compile errors did you encounter?
> 
> I'm going to say r- because I think some of the #ifdefs are not correct (such as the string init one) and there's a little bit of context missing for some of the changes.
> 
> In general it looks good though.

[Vlad]

Just checked removing #if !PLATFORM(QT). It has to be 
    explicit WebCoreStringResource(const AtomicString& string)
        : m_plainString(reinterpret_cast<const WTF::String &>(string))
        , m_atomicString(reinterpret_cast<const WTF::String &>(string))
to specify which to use
"WTF::String::String(const QString &) or
"WTF::String::String(const WTF::String &)

[Nate Chapin]

Comment on attachment 66438 [details]
v8 binding changes


> +#if PLATFORM(QT)
> +    if (widget->isPluginView()) {
> +        PluginView* pluginView = static_cast<PluginView*>(widget);
> +        // Create script inscance (v8 object)
> +        PassScriptInstance instance = pluginView->bindingInstance();
> +        if (!instance)
> +            return 0;
> +        // Get npobject
> +        NPObject* npObject = static_cast<NPObject*>(instance->instance()->GetPointerFromInternalField(v8DOMWrapperObjectIndex));

I'm confused by this. It looks like you're accessing an NPObject that is stored on a PassScriptInstance and creating a new PassScriptInstance for it? Why wouldn't we just want the original PassScriptInstance?

I think I'm probably missing some subtlety :)

> +        // Track the plugin object. We've been given a reference to the object.
> +        m_pluginObjects.set(widget, npObject);
> +        return instance;
> +    }
>  
> +    QWidget* platformWidget = widget->platformWidget();
> +    if (!platformWidget)
> +        return 0;
> +
> +    Frame* activeFrame = V8Proxy::retrieveFrameForEnteredContext();
> +    if (!activeFrame)
> +        return 0;
> +
> +    V8Proxy* activeProxy = activeFrame->script()->proxy();
> +    v8::HandleScope handleScope;
> +    v8::Handle<v8::Context> v8Context = V8Proxy::mainWorldContext(activeFrame);
> +    if (v8Context.IsEmpty())
> +        return 0;
> +
> +    v8::Context::Scope scope(v8Context);
> +    v8::Handle<v8::Object> result = 
> +        V8::Bindings::QtInstance::getQtInstance(platformWidget, v8Context, "", QScriptEngine::QtOwnership)->getV8Object();
> +    return V8ScriptInstance::create(result);
> +

Would it be possible to put the QT-specific code in a WebCore/platform/qt/PlatformBridge.h and make use of the PlatformBridge::pluginScriptableObject() call that Android and Chromium use?

[Vlad]

I'm trying as we speak :) Complication is that QT has QT plugins.
if (widget->isPluginView())() is for qt plugins and pluginView->bindingInstance() returns v8 object already and then returns:)

[Vlad]

I'm trying to make it look like this

PassScriptInstance ScriptController::createScriptInstanceForWidget(Widget* widget)
{
...
#if PLATFORM(QT)
    // Create v8 object for QT plugins first
    PassScriptInstance instance = PlatformBridge::pluginScriptableObjectInstance(widget);
    if (instance)
        return instance;
#endif
    if (widget->isFrameView())
        return 0;
    NPObject* npObject = PlatformBridge::pluginScriptableObject(widget);
    if (!npObject)
        return 0;
...

[Vlad]

Confusing part for me is that JSC implementation does:
_NPN_ReleaseObject() in PassScriptInstance PluginView::bindingInstance() after npobject retrieved from PluginView. 
V8 implementation in 
PassScriptInstance ScriptController::createScriptInstanceForWidget() 
doesnt do that... Does this mean that Chromium PlatformBridge::pluginScriptableObject(widget) already has ref counter decreased for this npobject?

[Vlad]

Created attachment 66571 [details]
Moved most of scriptcontroller changes to PlatformBridge: pluginScriptableObject(), pluginScriptableObjectInstance(), popupsAllowed()

[WebKit Review Bot]

Attachment 66571 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/qt/platformBridge.cpp:23:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/platform/qt/platformBridge.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/qt/PlatformBridge.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/qt/PlatformBridge.h:77:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/qt/PlatformBridge.h:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vlad]

Created attachment 66574 [details]
Included plugins changes b/c of dependency

[Adam Barth]

Comment on attachment 66574 [details]
Included plugins changes b/c of dependency 

View in context: https://bugs.webkit.org/attachment.cgi?id=66574&action=prettypatch

> WebCore/bindings/v8/ScriptController.cpp:74
> +#if PLATFORM(QT)
> +PassRefPtr<DOMWrapperWorld> ScriptController::createWorld()
> +{
> +    return DOMWrapperWorld::create();
> +}
> +#endif
This can't be right.  It's not Qt-specific.  We need to integrate V8 with DOMWrapperWorld in genera.

> WebCore/bindings/v8/V8Binding.cpp:54
> -        : m_plainString(string)
> +        : m_plainString(reinterpret_cast<const WTF::String &>(string))
I don't understand this code.  Does Qt use different string types than other ports?  reinterpret_cast is almost always wrong.

> WebCore/plugins/win/PluginViewWin.cpp:661
> +#if USE(JSC)
>      JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);
> +#endif
This ifdefs point to a missing abstraction.  Perhaps a ScriptLock object?

[anton muhin]

(In reply to comment #9)
> Confusing part for me is that JSC implementation does:
> _NPN_ReleaseObject() in PassScriptInstance PluginView::bindingInstance() after npobject retrieved from PluginView. 
> V8 implementation in 
> PassScriptInstance ScriptController::createScriptInstanceForWidget() 
> doesnt do that... Does this mean that Chromium PlatformBridge::pluginScriptableObject(widget) already has ref counter decreased for this npobject?

I cannot say for JSC, but V8 bindings introduce a weak reference to a wrapper which owns the NPObject and would deref NPObject when it gets unreachable via V8.

[anton muhin]

(In reply to comment #14)
> (In reply to comment #9)
> > Confusing part for me is that JSC implementation does:
> > _NPN_ReleaseObject() in PassScriptInstance PluginView::bindingInstance() after npobject retrieved from PluginView. 
> > V8 implementation in 
> > PassScriptInstance ScriptController::createScriptInstanceForWidget() 
> > doesnt do that... Does this mean that Chromium PlatformBridge::pluginScriptableObject(widget) already has ref counter decreased for this npobject?
> 
> I cannot say for JSC, but V8 bindings introduce a weak reference to a wrapper which owns the NPObject and would deref NPObject when it gets unreachable via V8.

Forgotten URL: http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/WebCore/bindings/v8/V8NPObject.cpp&q=createV8ObjectForNPObject&sa=N&cd=1&ct=rc

[anton muhin]

Comment on attachment 66574 [details]
Included plugins changes b/c of dependency 

View in context: https://bugs.webkit.org/attachment.cgi?id=66574&action=prettypatch

> WebCore/platform/qt/PlatformBridge.cpp:67
> +        V8::Bindings::QtInstance::getQtInstance(platformWidget, v8Context, "", QScriptEngine::QtOwnership)->getV8Object();
can returned result by empty (on some failure path?)  Or V8ScriptInstance will take care of this?

[Vlad]

(In reply to comment #13)
> (From update of attachment 66574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66574&action=prettypatch
> 
> > WebCore/bindings/v8/ScriptController.cpp:74
> > +#if PLATFORM(QT)
> > +PassRefPtr<DOMWrapperWorld> ScriptController::createWorld()
> > +{
> > +    return DOMWrapperWorld::create();
> > +}
> > +#endif
> This can't be right.  It's not Qt-specific.  We need to integrate V8 with DOMWrapperWorld in genera.
It is not Qt specific code and ifdef should be removed. I was trying to isolate Qt+v8 related changes.

> 
> > WebCore/bindings/v8/V8Binding.cpp:54
> > -        : m_plainString(string)
> > +        : m_plainString(reinterpret_cast<const WTF::String &>(string))
> I don't understand this code.  Does Qt use different string types than other ports?  reinterpret_cast is almost always wrong.

As I said earlier without explicit cast compiler gives an error b/c of confusion b/n
"WTF::String::String(const QString &) or
"WTF::String::String(const WTF::String &)

> 
> > WebCore/plugins/win/PluginViewWin.cpp:661
> > +#if USE(JSC)
> >      JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);
> > +#endif
> This ifdefs point to a missing abstraction.  Perhaps a ScriptLock object?

[Simon Hausmann]

(In reply to comment #17)
> > > WebCore/bindings/v8/V8Binding.cpp:54
> > > -        : m_plainString(string)
> > > +        : m_plainString(reinterpret_cast<const WTF::String &>(string))
> > I don't understand this code.  Does Qt use different string types than other ports?  reinterpret_cast is almost always wrong.
> 
> As I said earlier without explicit cast compiler gives an error b/c of confusion b/n
> "WTF::String::String(const QString &) or
> "WTF::String::String(const WTF::String &)

We have solved this one differently now:

http://trac.webkit.org/changeset/67326

[Simon Hausmann]

These changes are not needed anymore, after Andreas and I separated them in the trunk.