[webkit-reviews] review denied: [Bug 30078] [GTK] Gtk-directfb and plugins : [Attachment 40780] cummulative patch, Changelog filled with detailed comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 8 16:03:31 PDT 2009


Gustavo Noronha (kov) <gns at gnome.org> has denied vladimir
<smagellan at gmail.com>'s request for review:
Bug 30078: [GTK] Gtk-directfb and plugins
https://bugs.webkit.org/show_bug.cgi?id=30078

Attachment 40780: cummulative patch, Changelog filled with detailed comments
https://bugs.webkit.org/attachment.cgi?id=40780&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> Index: WebCore/plugins/npapi.cpp
> ===================================================================
> --- WebCore/plugins/npapi.cpp (revision 49240)
> +++ WebCore/plugins/npapi.cpp (working copy)
> @@ -108,9 +108,11 @@ const char* NPN_UserAgent(NPP instance)
>  {
>      PluginView* view = pluginViewForInstance(instance);
>  
> +#if ENABLE(NETSCAPE_PLUGIN_API)
>	if (!view)
>	    return PluginView::userAgentStatic();
>   
> +#endif
>      return view->userAgent();
>  }
>  
> @@ -138,8 +140,10 @@ NPError NPN_GetValue(NPP instance, NPNVa
>  {
>      PluginView* view = pluginViewForInstance(instance);
>  
> +#if ENABLE(NETSCAPE_PLUGIN_API)
>	if (!view)
>	    return PluginView::getValueStatic(variable, value);
> +#endif
>  
>      return pluginViewForInstance(instance)->getValue(variable, value);
>  }

These two changes look a bit weird to me. This file is supposed to implement
the netscape plugin API, so disabling it should probably not include the files
at all, or have empty implementations for everything? Can these checks be moved
up?

The changelog is pretty detailed, thanks for that! I think you'll want to wrap
the lines at a reasonable width, though, say, between 80 and 100 characters.

> +#if ENABLE(NETSCAPE_PLUGIN_API)
>      initializeBrowserFuncs();
> +#endif

This is a bit strange, too, specially given that just a few lines bellow NPAPI
seems to be being called:
>  
>  #if defined(XP_UNIX)
>      npErr = NP_Initialize(&m_browserFuncs, &m_pluginFuncs);


Thanks,


More information about the webkit-reviews mailing list