[Webkit-unassigned] [Bug 45145] [Qt] V8 port for QT platform: v8 binding changes

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Vlad <vladbph at gmail.com>  2010-09-03 11:39:13 PST ---

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

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