Bug 113853 – [Qt] Add API in QWebSettings for setting the CSS media type

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jose Lejin PJ]

API needed in QWebSettings for setting the CSS media type.

Refer http://www.w3schools.com/tags/att_style_media.asp
WebKit should know in what media type it is running(tv/screen/ handheld/projection/print etc.) to automatically behave.

WebCore has the method(refer Settings.h) to change the media type, which can be exposed in Qt WebKit API.

[Jose Lejin PJ]

Created attachment 196296 [details]
Test App

[Jose Lejin PJ]

Created attachment 196453 [details]
Patch

[WebKit Review Bot]

Attachment 196453 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebsettings.cpp', u'Source/WebKit/qt/Api/qwebsettings.h', u'Source/WebKit/qt/ChangeLog']" exit_code: 1
Source/WebKit/qt/Api/qwebsettings.h:167:  The parameter name "mediaType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jose Lejin PJ]

Created attachment 196455 [details]
Patch

[Jose Lejin PJ]

(In reply to comment #4)
> Created an attachment (id=196455) [details]
> Patch

In this corrected style check error.

[Build Bot]

Comment on attachment 196455 [details]
Patch

Attachment 196455 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17522053

[Jocelyn Turcotte]

Comment on attachment 196455 [details]
Patch

Did you test this well?
By reading the code it seems like this only applies to the current FrameView (which is only used for the currently navigated page).
If you click a link to navigate to a second page, it feels like this setting won't be applied unless you call setMediaTypeOverride again.

A safer way to implement this would be to do like setMediaStyle is implemented in Source/WebKit/mac/WebView/WebView.mm.
It could still live in QWebSettings, but instead of forwarding the value to WebCore::Settings, it would be fetched by our implementation of FrameLoaderClientQt::overrideMediaType in Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp.

[Jose Lejin PJ]

(In reply to comment #7)
> (From update of attachment 196455 [details])
> Did you test this well?
> By reading the code it seems like this only applies to the current FrameView (which is only used for the currently navigated page).
> If you click a link to navigate to a second page, it feels like this setting won't be applied unless you call setMediaTypeOverride again.
> 
> A safer way to implement this would be to do like setMediaStyle is implemented in Source/WebKit/mac/WebView/WebView.mm.
> It could still live in QWebSettings, but instead of forwarding the value to WebCore::Settings, it would be fetched by our implementation of FrameLoaderClientQt::overrideMediaType in Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp.


Thanks for the comment. You are right ! It will work for current frame view only.

String FrameView::mediaType() const
{
    // See if we have an override type.
    String overrideType = m_frame->loader()->client()->overrideMediaType();
    InspectorInstrumentation::applyEmulatedMedia(m_frame.get(), &overrideType);
    if (!overrideType.isNull())
        return overrideType;
    return m_mediaType;
}

Since overrideType is coming null, for next frameview "screen" default value set in m_mediaType is taken.


But is this _intentionally_ done ? Example is I am browsing a page in "screen" type and I want to print that page. In browser I will set that to "print" type. When I transition to next page I assume same "screen" type behavior instead "print" css type.

If we get type from FrameLoaderClientQt::overrideMediaType(), next page also will be in "print" css mode. Then it will become opposite to WebCore::Settings behavior. 

Please suggest !
(If application want to retain this css media type, they can do it in page transition by setting again as per need.)

[Jocelyn Turcotte]

(In reply to comment #8)
"print" is already handled by QtWebKit since it can know better when it is getting printed than the application can itself. Also, the media type is then not confined to the scope of a navigated page, but rather in the scope of a layout done in preparation to render in a printing QPainter.

So let's assume that this feature only applies to handheld, projector, tv, etc. media styles.
I think that in this case, the media type won't change between navigations, and requiring the application to reset it on every page load would be a very cumbersome API.

[Jose Lejin PJ]

(In reply to comment #9)
> (In reply to comment #8)
> "print" is already handled by QtWebKit since it can know better when it is getting printed than the application can itself. Also, the media type is then not confined to the scope of a navigated page, but rather in the scope of a layout done in preparation to render in a printing QPainter.
> 
> So let's assume that this feature only applies to handheld, projector, tv, etc. media styles.
> I think that in this case, the media type won't change between navigations, and requiring the application to reset it on every page load would be a very cumbersome API.

OK. We will return current media type set in FrameLoaderClientQt::overrideMediaType() by reading from WebCore::Settings. The value need to be set in  Settings::setMediaTypeOverride(const String& mediaTypeOverride) since recalc style automatically happens if user changes style after page loads also.

I will upload the new patch.

[Jose Lejin PJ]

Created attachment 196845 [details]
Patch

Updated as per comments

[Jocelyn Turcotte]

Comment on attachment 196845 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1568
> +        mediaType = m_frame->settings()->mediaTypeOverride();

Please don't do that, have a separate boolean in QWebSettingsPrivate.
WebCore::Settings::mediaTypeOverride() seems to be used by testing code and this would be mixing layers that are not designed in that way.
This also needs to work with QWebSettings::globalSettings(), which wouldn't in this case.

Also, please add tests for this feature in Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp

[Jocelyn Turcotte]

(In reply to comment #12)
> Please don't do that, have a separate boolean in QWebSettingsPrivate.

I meant a separate QString, sorry.

[Jose Lejin PJ]

Created attachment 196999 [details]
Patch

Updated patch

[Jocelyn Turcotte]

Comment on attachment 196999 [details]
Patch

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

This is starting to look pretty clean, but as stated in Comment #12, we needs a test for this in Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp. Testing this setting both thouth a non-global and through globalSettings() would be great.

> Source/WebKit/qt/ChangeLog:3
> +        Add API in QWebSettings for setting the CSS media type

For Qt specific bugs, we add the "[Qt] " prefix in front of our changelog headlines.

> Source/WebKit/qt/Api/qwebsettings.cpp:938
> +    Sets the CSS media type.

Please provide enough documentation for people to understand how this feature work.
The documentation could match the one for mediaStyle and setMediaStyle in Source/WebKit/mac/WebView/WebView.h.

Specifically, it should state that setting this to a null QString will revert to the default one, and that cssMediaType will only return the value set through setCSSMediaType and not the one used internally.

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1572
> +    // If type is null try to get value from QWebSettings::globalSettings() 
> +    if (type.isNull())
> +        type = QWebSettings::globalSettings()->cssMediaType();

This case should already be handled by your code in QWebSettings::apply(). If not, something is wrong.

[Jose Lejin PJ]

(In reply to comment #15)
> (From update of attachment 196999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196999&action=review
> 
> This is starting to look pretty clean, but as stated in Comment #12, we needs a test for this in Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp. Testing this setting both thouth a non-global and through globalSettings() would be great.
> 
> > Source/WebKit/qt/ChangeLog:3
> > +        Add API in QWebSettings for setting the CSS media type
> 
> For Qt specific bugs, we add the "[Qt] " prefix in front of our changelog headlines.
> 
> > Source/WebKit/qt/Api/qwebsettings.cpp:938
> > +    Sets the CSS media type.
> 
> Please provide enough documentation for people to understand how this feature work.
> The documentation could match the one for mediaStyle and setMediaStyle in Source/WebKit/mac/WebView/WebView.h.
> 
> Specifically, it should state that setting this to a null QString will revert to the default one, and that cssMediaType will only return the value set through setCSSMediaType and not the one used internally.
> 
> > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1572
> > +    // If type is null try to get value from QWebSettings::globalSettings() 
> > +    if (type.isNull())
> > +        type = QWebSettings::globalSettings()->cssMediaType();
> 
> This case should already be handled by your code in QWebSettings::apply(). If not, something is wrong.


Thanks for review. I will add needed test cases and documentation. 
Yes.. QWebSettings::apply() is taking care of returning proper media type set by globalSettings. I will remove this in patch.

[Jocelyn Turcotte]

Simon, Allan: Please tell if you see anything wrong with this added API.

[Jose Lejin PJ]

Created attachment 197145 [details]
Patch

Updated patch with test cases and documentation

[Jocelyn Turcotte]

Comment on attachment 197145 [details]
Patch

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

A few minor issues:

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1571
> +    String type;
> +    if (m_webFrame && m_webFrame->pageAdapter && m_webFrame->pageAdapter->settings)
> +        type = m_webFrame->pageAdapter->settings->cssMediaType();
> +
> +    return type;

To follow a bit more the style in the rest of the file:

if (...)
    return ...->cssMediaType();
return String();

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3296
> +    page = new TestPage();

Try to use the existing m_page member like other tests do, you should be able to use the same page for all chunks.
If that helps, you can separate the global and per-page setting in two different tests.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3298
> +    QTest::qWait(500);

QVERIFY(::waitForSignal(m_view, SIGNAL(loadFinished(bool))));

[Jose Lejin PJ]

(In reply to comment #19)
> (From update of attachment 197145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197145&action=review
> 
> A few minor issues:
> 
> > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1571
> > +    String type;
> > +    if (m_webFrame && m_webFrame->pageAdapter && m_webFrame->pageAdapter->settings)
> > +        type = m_webFrame->pageAdapter->settings->cssMediaType();
> > +
> > +    return type;
> 
> To follow a bit more the style in the rest of the file:
> 
> if (...)
>     return ...->cssMediaType();
> return String();
> 

When I uploaded first patch style checker showed error on multiple return with webkit-patch upload script. I will correct this.

> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3296
> > +    page = new TestPage();
> 
> Try to use the existing m_page member like other tests do, you should be able to use the same page for all chunks.
> If that helps, you can separate the global and per-page setting in two different tests.
> 
> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3298
> > +    QTest::qWait(500);
> 
> QVERIFY(::waitForSignal(m_view, SIGNAL(loadFinished(bool))));

I will update test cases as per comments.

[Jose Lejin PJ]

Created attachment 197249 [details]
Patch

Updated patch

[Jose Lejin PJ]

(In reply to comment #19)
> 
> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3298
> > +    QTest::qWait(500);
> 
> QVERIFY(::waitForSignal(m_view, SIGNAL(loadFinished(bool))));

QVERIFY(::waitForSignal(m_view, SIGNAL(loadFinished(bool)))) fails since html content is too small and loads immediately on m_view->setHtml itself. So using QSignalSpy loadSpy(m_view, SIGNAL(loadFinished(bool)))

[Jocelyn Turcotte]

Comment on attachment 197249 [details]
Patch

Looks good to me!
Please click on "Details" on the attachment and also set the cq flag to "?" when you're ready for this patch to land.

[Jocelyn Turcotte]

Comment on attachment 197249 [details]
Patch

cq requested by arunprasadr on IRC

[WebKit Commit Bot]

Comment on attachment 197249 [details]
Patch

Clearing flags on attachment: 197249

Committed r148095: <http://trac.webkit.org/changeset/148095>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.