[webkit-reviews] review denied: [Bug 45145] [Qt] V8 port for QT platform: v8 binding changes : [Attachment 66438] v8 binding changes

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Vlad <vladbph at gmail.com>'s
request for review:
Bug 45145: [Qt] V8 port for QT platform: v8 binding changes
https://bugs.webkit.org/show_bug.cgi?id=45145

Attachment 66438: v8 binding changes
https://bugs.webkit.org/attachment.cgi?id=66438&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.