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
I think that we should atleast filter out the NP Plugin messages starting with "** Message:"
Created attachment 32733 [details] For Qt, suppress NP Plugin stderr output that is out of our control
as per chat on IRC, that can be not qt specific, but linux so gtk could be interested ? cc'ing kov
Actually, this seems linux specific so is useful for Qt, Chromium and Gtk+
Created attachment 32735 [details] For Linux, suppress NP Plugin stderr output that is out of our control
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.
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.
(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.
Created attachment 32796 [details] For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH
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
> > +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.
(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.
Created attachment 32936 [details] For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH
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
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.
Comment on attachment 32937 [details] For Qt,overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH r=me
> (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 :)
Landed in 46030 with the ChangeLog typo fixup commit in 46031