Bug 27215 – [QT] suppress (un)desired launcher output that can make layout test to fail with stderr

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Antonio Gomes]

4778 test cases (98%) succeeded
9 test cases (<1%) had incorrect layout
34 test cases (<1%) were new
8 test cases (<1%) crashed
18 test cases (<1%) had stderr output


--> Something noticeable and annoying: I got 18 tests w/ "stderr output" but
all of them output either
 
Xlib:  extension "RANDR" missing on display ":0.0".
 
or 
 
** Message: NP_Initialize
** Message: NP_Initialize succeeded
** Message: NP_Initialize
** Message: NP_Initialize succeeded
** Message: NP_Initialize
** Message: NP_Initialize succeeded
** Message: NP_Initialize
** Message: NP_Initialize succeeded

[Kenneth Rohde Christiansen]

I think that we should atleast filter out the NP Plugin messages starting with "** Message:"

[Kenneth Rohde Christiansen]

Created attachment 32733 [details]
For Qt, suppress NP Plugin stderr output that is out of our control

[Antonio Gomes]

as per chat on IRC, that can be not qt specific, but linux 

so gtk could be interested ?

cc'ing kov

[Kenneth Rohde Christiansen]

Actually, this seems linux specific so is useful for Qt, Chromium and Gtk+

[Kenneth Rohde Christiansen]

Created attachment 32735 [details]
For Linux, suppress NP Plugin stderr output that is out of our control

[Adam Treat]

Why is this out of our control?  What exactly is printing this information?  Coming from outside WebKit altogether?  Can plugins not be loaded by Qt DRT as an alternative?  Are you sure that GTK DRT has this problem?

At the very least, you should document why you are suppressing those messages in the source code as it is not obvious.

[Kenneth Rohde Christiansen]

It is because of the PLUGINDEBUGSTR that most netscape plugins use, but just enabling the test plugin for DRT would be better, but depends on a patch to change the default plugin directories.

[Holger Freyther]

(In reply to comment #0)

> ** Message: NP_Initialize
> ** Message: NP_Initialize succeeded
> ** Message: NP_Initialize
> ** Message: NP_Initialize succeeded
> ** Message: NP_Initialize
> ** Message: NP_Initialize succeeded
> ** Message: NP_Initialize
> ** Message: NP_Initialize succeeded

This is coming from the totem plugin. My take on this is that tests do not fail, so we should not bother.

[Kenneth Rohde Christiansen]

Created attachment 32796 [details]
For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH

[Adam Treat]

Comment on attachment 32796 [details]
For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH

I like the approach of the patch and think this will work, but a few problems:

> -PluginDatabase* PluginDatabase::installedPlugins()
> +PluginDatabase* PluginDatabase::installedPlugins(bool populate)

This reason for this populate arg should be documented in code as it is not obvious.

> +void PluginDatabase::clear()
> +{
> +    m_plugins.clear();
> +    m_pluginsByPath.clear();
> +    m_registeredMIMETypes.clear();
> +}

Sure, but no deletion needed?

> +        void setPluginDirectories(const Vector<String>& directories)
> +        {
> +            clear();
> +            m_pluginDirectories = directories;
> +        }

Looks suspect as you're changing the behavior of this method.  Where is this method currently called from?  Any obvious side effects?
  
> +        bool m_pluginSetChanged;

Adding a member variable that is never used or initialized?

> +void QWEBKIT_EXPORT qt_drt_overwritePluginDirectories()
> +{
> +    PluginDatabase *db = PluginDatabase::installedPlugins(/* populate */ false);

The decorator is in wrong place according to webkit coding style guidelines.

With these things fixed I think it'll be in good shape.

Cheers,
Adam

[Kenneth Rohde Christiansen]

> > +void PluginDatabase::clear()
> > +{
> > +    m_plugins.clear();
> > +    m_pluginsByPath.clear();
> > +    m_registeredMIMETypes.clear();
> > +}
> Sure, but no deletion needed?

It does the same as the remove() functions. It also doesn't delete anything.

> > +        void setPluginDirectories(const Vector<String>& directories)
> > +        {
> > +            clear();
> > +            m_pluginDirectories = directories;
> > +        }
> 
> Looks suspect as you're changing the behavior of this method.  Where is this
> method currently called from?  Any obvious side effects?

Only from the installedPlugins() function, so no.

> > +        bool m_pluginSetChanged;
> 
> Adding a member variable that is never used or initialized?

That is a leftover, thanks for spotting.

> > +void QWEBKIT_EXPORT qt_drt_overwritePluginDirectories()
> > +{
> > +    PluginDatabase *db = PluginDatabase::installedPlugins(/* populate */ false);
> 
> The decorator is in wrong place according to webkit coding style guidelines.

decorator? the /* populate */ ? Btw, this in in Qt code and should thus follow the Qt coding style, but it might still be wrong though.

[Adam Treat]

(In reply to comment #11)
> decorator? the /* populate */ ? Btw, this in in Qt code and should thus follow
> the Qt coding style, but it might still be wrong though.

No, the decorator on 'PluginDatabase *db' and a decision was made that all Qt port code, whether in WebKit/qt or in WebCore or where ever in should follow the webkit coding style.

See here:

http://trac.webkit.org/wiki/QtWebKitContrib#CodingStyle

That decision was made after a lengthy meeting and discussion on the topic in #qtwebkit that included Simon, Ariya, Zecke, Torarne, Ben and myself and others.

[Kenneth Rohde Christiansen]

Created attachment 32936 [details]
For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH

[Kenneth Rohde Christiansen]

Created attachment 32937 [details]
For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH

Submitted patch with garbage by mistake. This one should be alright

[Antonio Gomes]

kenne, could you please exactly say how many fake-failing tests are you supposed to be enabling, so I can try to match on my 32-bit box.

[Simon Hausmann]

Comment on attachment 32937 [details]
For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH

r=me

[Adam Treat]

>         (WebCore::PluginDatabase::clear): Make is possible to clear the

s/is/it/

> +        (WebCore::PluginDatabase::clear): Make is possible to clear the

s/is/it/

>      if (!plugins) {
>          plugins = new PluginDatabase;
> -        plugins->setPluginDirectories(PluginDatabase::defaultPluginDirectories());
> -        plugins->refresh();
> +
> +        if (populate) {
> +            plugins->setPluginDirectories(PluginDatabase::defaultPluginDirectories());
> +            plugins->refresh();
> +        }

No reason for the extra line break.

> +    m_plugins.clear();
> +    m_pluginsByPath.clear();
> +
> +    m_registeredMIMETypes.clear();

No reason for the extra line break.

r=me with these minor nits.  Great work!  One more step to getting the DRT into
reasonable shape :)

[Simon Hausmann]

Landed in 46030 with the ChangeLog typo fixup commit in 46031