Bug 37880 – [Qt] Patch for checking content types supported by the QWebPage...

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Dawit A.]

The patch provided here adds a function to query the frame for the content it supports.
It can be used to query QtWebKit whether or not it can handle a specific content type.

The motivation for this patch is to address a crucial QtWebKit/KDE integration issue. 
The issue stems from the fact that the KDE integration of QtWebKit changes QtWebKit
to use KDE's KIO subsytem for network communication. In doing so, it causes an issue
we cannot simply workaround in KDE whenever we need to determine content type and
pause the ioslave and transfer/resume it using another request/job. I can provide a detailed
analysis of this issue, but essentially all QtWebKit based KDE browsers (e.g. reKonq) are
afflicted with this problem. Originally it was thought the problem can be resolved without
requiring changes upstream, but further investigation showed that was not possible.

If this patch is not acceptable as it stand, it can easily be modified and perhaps be added
as a protected member function of QWebPage. Perhaps that would be more acceptable.
Regardless, if we are unable to query QtWebKit for the content that it can/cannot handle
the issue downstream will never be resolved...

[Dawit A.]

Created attachment 53863 [details]
Function for checking content-type supported by QWebFrame...

[Dawit A.]

The patch provided here adds a function to query the frame for the content it supports. It can be used to query QtWebKit whether or not it can handle a specific content type.

The motivation for this patch is to address a crucial QtWebKit/KDE integration issue. The issue stems from the fact that the KDE integration of QtWebKit changes QtWebKit to use KDE's KIO subsytem for network communication. In doing so, it causes an issue we cannot simply workaround in KDE whenever we need to determine content type and pause the ioslave and transfer/resume it using another request/job. I can provide a detailed analysis of this issue, but essentially all QtWebKit based KDE browsers (e.g. reKonq) are afflicted with this problem. Originally it was thought the problem can be resolved without requiring changes upstream, but further investigation showed that was not possible.

If this patch is not acceptable as it stand, it can easily be modified and perhaps be added as a protected member function of QWebPage. Perhaps that would be more acceptable. Regardless, if we are unable to query QtWebKit for the content that it can/cannot handle the issue downstream will never be resolved...

[Simon Hausmann]

Comment on attachment 53863 [details]
Function for checking content-type supported by QWebFrame...

I suggest to send an email to [e-mail] to see what the others thing. But I'm okay with having this in the API.

r- because it should be static and probably in a different class. See the implementaion in FrameLoaderClientQt, it only calls static functions :)

[Dawit A.]

(In reply to comment #3)
> (From update of attachment 53863 [details])
> I suggest to send an email to [e-mail] to see what the others thing. But I'm okay with having this in the API.
> 
> r- because it should be static and probably in a different class. See the 
> implementaion in FrameLoaderClientQt, it only calls static functions :)

Which class ? QWebSettings or QWebPage ? There is no other place to put it other than that. Also if the function is made static, it would mean duplicating the code and hence all the necessary headers from FrameLoaderClientQt::canShowMIMEType. Is that acceptable thing to do ?

[Dawit A.]

Created attachment 64726 [details]
Function for checking content-type supported by QWebFrame II...

Updated patch:

* Changed the function name to isSupportedContentType.
* Added a new function, supportedContentTypes() to return all supported content types.

@simon: I cannot make these functions static because I will not be able to access the frame's local QWebSettings (d->frame->settings()) to
obtain the mimetypes supported by any uploaded plugins just like in FrameLoaderClientQt::canShowMIMEType. Also, you did not say which class these functions might belong to other than where they are, QWebSettings, QWebPage ?? Once these issues are resolved, I can provide unit test cases for these function.

[Andreas Kling]

Comment on attachment 64726 [details]
Function for checking content-type supported by QWebFrame II...

>WebKit/qt/Api/qwebframe.cpp:1416
> +   *  Returns true if the frame can handle the given mimeType, false otherwise.
Missing \a before "mimeType"
And the typical Qt documentation idiom seems to be "Returns true if X; otherwise returns false."

>WebKit/qt/Api/qwebframe.cpp:1418
> +  bool QWebFrame::isSupportedContentType(const QString &mimeType) const
Coding style, should be "const QString& mimeType"

>WebKit/qt/Api/qwebframe.cpp:1453
> +          const Vector<PluginPackage*> &plugins = PluginDatabase::installedPlugins()->plugins();
Coding style, should be "const Vector<PluginPackage*>& plugins"

>WebKit/qt/Api/qwebframe.h:201
> +      bool isSupportedContentType(const QString &mimeType) const;
Coding style, should be "const QString& mimeType"

There is also some inconsistency wrt tab size (should always be 4 spaces.)

[Antonio Gomes]

Comment on attachment 64726 [details]
Function for checking content-type supported by QWebFrame II...

Generally, looks good. Please fix kling's suggest and also add a changelog entry.

I am asking myself if these method should go to QWebFrame. Personally I thing they do not belong to a particularly frame of your page, so I'd rather add them to QWebPage.

> +/*!
> + *  \since 4.8
> + * 
> + *  Returns true if the frame can handle the given mimeType, false otherwise.
> + */
> +bool QWebFrame::isSupportedContentType(const QString &mimeType) const

Maybe naming it like: QWeb{Frame or Page}::supportsContentType(XXX) ? not sure ...

Please block the meta bug for API change in Qt4.8/QtWebKit2.1 : bug 31552

> +/*!
> + *  \since 4.8
> + *
> + *  Returns the list of all content types supported by this frame.
> + */
> +QStringList QWebFrame::supportedContentTypes() const
> +{
> +    QStringList mimeTypes;
> +    
> +    extractContentTypeFromHash(MIMETypeRegistry::getSupportedImageMIMETypes(), mimeTypes);
> +    extractContentTypeFromHash(MIMETypeRegistry::getSupportedNonImageMIMETypes(), mimeTypes);

why not making a static helper function for the block below, similarly to what you did above?

> +    if (d->frame && d->frame->settings() && d->frame->settings()->arePluginsEnabled()) {
> +        const Vector<PluginPackage*> &plugins = PluginDatabase::installedPlugins()->plugins();
> +        for (unsigned int i = 0; i < plugins.size(); ++i) {            
> +            MIMEToDescriptionsMap::const_iterator map_it = plugins[i]->mimeToDescriptions().begin();
> +            MIMEToDescriptionsMap::const_iterator map_end = plugins[i]->mimeToDescriptions().end();
> +            for (; map_it != map_end; ++map_it)
> +              mimeTypes << map_it->first;            
> +        }        
> +    }
> +    
> +    return mimeTypes;
> +}

[Dawit A.]

(In reply to comment #6)
> (From update of attachment 64726 [details])
> >WebKit/qt/Api/qwebframe.cpp:1418
> > +  bool QWebFrame::isSupportedContentType(const QString &mimeType) const
> Coding style, should be "const QString& mimeType"

There are a lot of already implemented functions in that class that do not follow this guideline. In fact, I wrote the functions that way because the functions immediately above it did the same exact thing. It is rather confusing though :(  Anyhow, I will fix my patch accordingly.

[Dawit A.]

(In reply to comment #7)
> (From update of attachment 64726 [details])
> Generally, looks good. Please fix kling's suggest and also add a changelog entry.
> 
> I am asking myself if these method should go to QWebFrame. Personally I thing they do not belong to a particularly frame of your page, so I'd rather add them to QWebPage.
> 
> > +/*!
> > + *  \since 4.8
> > + * 
> > + *  Returns true if the frame can handle the given mimeType, false otherwise.
> > + */
> > +bool QWebFrame::isSupportedContentType(const QString &mimeType) const
> 
> Maybe naming it like: QWeb{Frame or Page}::supportsContentType(XXX) ? not sure ...
> 
> Please block the meta bug for API change in Qt4.8/QtWebKit2.1 : bug 31552
> 
> > +/*!
> > + *  \since 4.8
> > + *
> > + *  Returns the list of all content types supported by this frame.
> > + */
> > +QStringList QWebFrame::supportedContentTypes() const
> > +{
> > +    QStringList mimeTypes;
> > +    
> > +    extractContentTypeFromHash(MIMETypeRegistry::getSupportedImageMIMETypes(), mimeTypes);
> > +    extractContentTypeFromHash(MIMETypeRegistry::getSupportedNonImageMIMETypes(), mimeTypes);
> 
> why not making a static helper function for the block below, similarly to what you did above?
> 
> > +    if (d->frame && d->frame->settings() && d->frame->settings()->arePluginsEnabled()) {
> > +        const Vector<PluginPackage*> &plugins = PluginDatabase::installedPlugins()->plugins();
> > +        for (unsigned int i = 0; i < plugins.size(); ++i) {            
> > +            MIMEToDescriptionsMap::const_iterator map_it = plugins[i]->mimeToDescriptions().begin();
> > +            MIMEToDescriptionsMap::const_iterator map_end = plugins[i]->mimeToDescriptions().end();
> > +            for (; map_it != map_end; ++map_it)
> > +              mimeTypes << map_it->first;            
> > +        }        
> > +    }
> > +    
> > +    return mimeTypes;
> > +}

[Dawit A.]

(In reply to comment #7)
> (From update of attachment 64726 [details])
> Generally, looks good. Please fix kling's suggest and also add a changelog entry.
> 
> I am asking myself if these method should go to QWebFrame. Personally I thing they do not belong to a particularly frame of your page, so I'd rather add them to QWebPage.
> 
> > +/*!
> > + *  \since 4.8
> > + * 
> > + *  Returns true if the frame can handle the given mimeType, false otherwise.
> > + */
> > +bool QWebFrame::isSupportedContentType(const QString &mimeType) const
> 
> Maybe naming it like: QWeb{Frame or Page}::supportsContentType(XXX) ? not sure ...

Well I can name the function "canHandleContentType(xxx)" which is what I originally wanted to name it because it reflects exactly what the function does... 

As to where to add these functions, I settled on QWebFrame because by default QtWebKit sets the current frame as the originating object when constructing QNetworkRequest objects...

[Dawit A.]

Created attachment 64781 [details]
Function for checking content-type supported by QWebFrame III...

Fixed all the issues pointed out by the reviews. Also renamed "isSupportedContentType(xxx)" to "canHandleMimeType(xxx)".

I also wonder whether or not there should be some mechanism to prevent QtWebKit from handling certain mimetypes.
For example, by default QtWebKit (read: webkit) handles all "text/" mime-types. However, in places where such
option is configurable, e.g. KDE, there is no way to prevent QtWebKit from doing this. In fact, a bug report has already
been filed against kwebkit about this very same issue. See https://bugs.kde.org/show_bug.cgi?id=242417.

[Dawit A.]

Created attachment 65275 [details]
Function for checking content-type supported by QWebPage I...

Moved the function to QWebPage as suggested by Antonio in comment #7...

[Kenneth Rohde Christiansen]

Comment on attachment 65275 [details]
Function for checking content-type supported by QWebPage I...

WebKit/qt/Api/qwebpage.cpp:2164
 +  static void extractContentTypeFromHash(const HashSet<String>& types, QStringList& list)
I dont like not being able to see that is being written to. The name doesn't help making that clear.

In Qt, we prefer pointers for out values as that makes the code more clear:

ex. extractContentTypeFromHash(types, &outList);

[Antonio Gomes]

Comment on attachment 65275 [details]
Function for checking content-type supported by QWebPage I...

> diff --git a/WebKit/qt/Api/qwebpage.cpp b/WebKit/qt/Api/qwebpage.cpp
> index 5eee21c..3b86582 100644
> --- a/WebKit/qt/Api/qwebpage.cpp
> +++ b/WebKit/qt/Api/qwebpage.cpp
> @@ -85,6 +85,9 @@
>  #include "PageClientQt.h"
>  #include "WorkerThread.h"
>  #include "wtf/Threading.h"
> +#include "MIMETypeRegistry.h"
> +#include "PluginDatabase.h"
> +#include "PluginPackage.h"
>  
>  #include <QApplication>
>  #include <QBasicTimer>
> @@ -2158,6 +2161,63 @@ QObject *QWebPage::createPlugin(const QString &classid, const QUrl &url, const Q
>      return 0;
>  }
>  
> +static void extractContentTypeFromHash(const HashSet<String>& types, QStringList& list)
> +{
> +    HashSet<String>::const_iterator endIt = types.end();
> +    for (HashSet<String>::const_iterator it = types.begin(); it != endIt; ++it)
> +        list << *it;
> +}
> +
> +static void extractContentTypeFromPluginList(const Vector<PluginPackage*>& plugins, QStringList& list)
> +{
> +    for (unsigned int i = 0; i < plugins.size(); ++i) {
> +        MIMEToDescriptionsMap::const_iterator map_it = plugins[i]->mimeToDescriptions().begin();
> +        MIMEToDescriptionsMap::const_iterator map_end = plugins[i]->mimeToDescriptions().end();
> +        for (; map_it != map_end; ++map_it)
> +            list << map_it->first;
> +    }
> +}
> +
> +/*!
> + *  \since 4.8
> + *
> + *  Returns the list of all content types supported by this frame.
> + */
> +QStringList QWebPage::supportedContentTypes() const
> +{

"by this frame" sounds out of date now :)

other than that, looks fine. See kenneth's suggestion too.

[Dawit A.]

Created attachment 65320 [details]
Function for checking content-type supported by QWebPage II...

Updated per review suggestions above...

[Antonio Gomes]

Comment on attachment 65320 [details]
Function for checking content-type supported by QWebPage II...

Almost there! One minor change I would like to see yet. See it below.


> +/*!
> + *  \since 4.8
> + *
> + *  Returns the list of all content types supported by QWebPage.
> + */
> +QStringList QWebPage::supportedContentTypes() const
> +{
> +    QStringList mimeTypes;
> +
> +    WebCore::Frame *frame = d->page->focusController()->focusedOrMainFrame();
> +    extractContentTypeFromHash(MIMETypeRegistry::getSupportedImageMIMETypes(), &mimeTypes);
> +    extractContentTypeFromHash(MIMETypeRegistry::getSupportedNonImageMIMETypes(), &mimeTypes);
> +    if (frame && frame->settings() && frame->settings()->arePluginsEnabled())
> +        extractContentTypeFromPluginList(PluginDatabase::installedPlugins()->plugins(), &mimeTypes);

Frame::settings() is just a shortcut for Page::settings.

Lets use the later, please.

if (d->page->settings()->arePluginsEnabled()) should work. or maybe with an additional null check for settings.

> +
> +    WebCore::Frame *frame = d->page->focusController()->focusedOrMainFrame();
> +    if (frame && frame->settings() && frame->settings()->arePluginsEnabled() &&
> +        PluginDatabase::installedPlugins()->isMIMETypeRegistered(type))

Ditto.

Other than that, it looks fine to me.

[Dawit A.]

Created attachment 65418 [details]
Function for checking content-type supported by QWebPage III...

Changed frame->settings() to d->page->settings() as suggested in comment #16.

[Antonio Gomes]

(In reply to comment #17)
> Created an attachment (id=65418) [details]
> Function for checking content-type supported by QWebPage III...

LGTM.

Simon, Kenneth?

[Dawit A.]

Can someone please review and land this patch or reject it and provide guidance ? We really need this to be available in QtWebKit 2.1 downstream in kdewebkit...

[Kenneth Rohde Christiansen]

Comment on attachment 65418 [details]
Function for checking content-type supported by QWebPage III...

View in context: https://bugs.webkit.org/attachment.cgi?id=65418&action=review

How is this affecting performance?. LGTM.

> WebKit/qt/Api/qwebpage.cpp:2174
> +static void extractContentTypeFromPluginList(const Vector<PluginPackage*>& plugins, QStringList* list)

PluginList? It seems to be a vector.

> WebKit/qt/Api/qwebpage.cpp:2188
> + *  \since 4.8

I don't know if this is right, since this might be part of QtWebKit2.1, but well...

> WebKit/qt/Api/qwebpage.cpp:2213
> +      return true;

indentation seems wrong here

> WebKit/qt/Api/qwebpage.cpp:2216
> +      return true;

identation issue

> WebKit/qt/Api/qwebpage.cpp:2218
> +    if (d->page->settings() && d->page->settings()->arePluginsEnabled() &&

&& should always be at the start of a line according to the style guide

> WebKit/qt/Api/qwebpage.cpp:2220
> +      return true;

indentation issue

[Kenneth Rohde Christiansen]

Btw, how is the being tested?

[Dawit A.]

Created attachment 68952 [details]
65418: Function for checking content-type supported by QWebPage IV...

Added test coverage and addressed the issues in the last review...

[Kenneth Rohde Christiansen]

Comment on attachment 68952 [details]
65418: Function for checking content-type supported by QWebPage IV...

View in context: https://bugs.webkit.org/attachment.cgi?id=68952&action=review

> WebKit/qt/Api/qwebpage.cpp:2213
> + *  \since QtWebKit 2.1

I think I should just leave this out for now. I'm not sure this will actually work or break the documentation.

[Eric Seidel (no email)]

Comment on attachment 65418 [details]
Function for checking content-type supported by QWebPage III...

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 65418 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Jędrzej Nowacki]

Minutes from the first iteration of the API review:
- We agree that the functionality is missing in the current API.
- Function canHandleSupportedTypes() should be renamed to supportsContentType() which would be symmetric to supportedContentType() and more Qtish.
- Both functions should be methods of QWebPage and non static, because current API allows to change supported content types per instance of the QWebPage (by loading a plugin)

Summary:
The function name should be changed, personally I would like to see an autotest that checks what happens if a plugin is added/removed. Apart form that small issues the patch looks good.

[Dawit A.]

(In reply to comment #25)
> Minutes from the first iteration of the API review:
> - We agree that the functionality is missing in the current API.
> - Function canHandleSupportedTypes() should be renamed to supportsContentType() which would be symmetric to supportedContentType() and more Qtish.

That is not a problem...

> - Both functions should be methods of QWebPage and non static, because current API allows to change supported content types per instance of the QWebPage (by loading a plugin)

They are already non-static...

> Summary:
> The function name should be changed, personally I would like to see an autotest that checks what happens if a plugin is added/removed. Apart form that small issues the patch looks good.

That too can be done easily enough...

[Dawit A.]

> > Summary:
> > [...]personally I would like to see an autotest that checks what happens if a plugin is added/removed. Apart form that small issues the patch looks good.

> That too can be done easily enough...

Well I spoke too soon here... What kind of plugins are we talking about here ? Qt specific plugins (application/x-qt-plugin) ? webkit plugins that can be embeded into the page, e.g. NPAPI plugins (flash, java applets) ?

As it stands right now, none of those are handled by these two new functions because those contents will never generate the "unsupportedContent" signal.

[Dawit A.]

Created attachment 69473 [details]
Function for checking content-type supported by QWebPage V...

Renamed "canHandleContentType" to "supportsContentType" as requested by the API review.

[Eric Seidel (no email)]

Comment on attachment 68952 [details]
65418: Function for checking content-type supported by QWebPage IV...

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 68952 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Andreas Kling]

Comment on attachment 69473 [details]
Function for checking content-type supported by QWebPage V...

r=me

We need to do something about the (lack of) \since tags. I'll open a new bug about that.

[WebKit Commit Bot]

Comment on attachment 69473 [details]
Function for checking content-type supported by QWebPage V...

Clearing flags on attachment: 69473

Committed r69825: <http://trac.webkit.org/changeset/69825>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.