[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


Simon Hausmann <hausmann at webkit.org> changed:

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




--- Comment #3 from Simon Hausmann <hausmann at webkit.org>  2010-09-02 23:38:32 PST ---
(From update of attachment 66438)
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.

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