RESOLVED FIXED 113853
[Qt] Add API in QWebSettings for setting the CSS media type
https://bugs.webkit.org/show_bug.cgi?id=113853
Summary [Qt] Add API in QWebSettings for setting the CSS media type
Jose Lejin PJ
Reported 2013-04-03 01:13:06 PDT
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.
Attachments
Test App (558 bytes, text/html)
2013-04-03 01:14 PDT, Jose Lejin PJ
no flags
Patch (2.04 KB, patch)
2013-04-04 01:34 PDT, Jose Lejin PJ
no flags
Patch (2.03 KB, patch)
2013-04-04 01:58 PDT, Jose Lejin PJ
no flags
Patch (2.71 KB, patch)
2013-04-08 05:43 PDT, Jose Lejin PJ
no flags
Patch (3.84 KB, patch)
2013-04-08 22:47 PDT, Jose Lejin PJ
no flags
Patch (7.12 KB, patch)
2013-04-09 10:54 PDT, Jose Lejin PJ
no flags
Patch (7.60 KB, patch)
2013-04-10 05:14 PDT, Jose Lejin PJ
no flags
Jose Lejin PJ
Comment 1 2013-04-03 01:14:16 PDT
Created attachment 196296 [details] Test App
Jose Lejin PJ
Comment 2 2013-04-04 01:34:18 PDT
WebKit Review Bot
Comment 3 2013-04-04 01:37:12 PDT
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
Comment 4 2013-04-04 01:58:10 PDT
Jose Lejin PJ
Comment 5 2013-04-04 01:59:15 PDT
(In reply to comment #4) > Created an attachment (id=196455) [details] > Patch In this corrected style check error.
Build Bot
Comment 6 2013-04-04 19:24:50 PDT
Jocelyn Turcotte
Comment 7 2013-04-08 02:51:52 PDT
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
Comment 8 2013-04-08 04:12:35 PDT
(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
Comment 9 2013-04-08 04:56:17 PDT
(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
Comment 10 2013-04-08 05:22:37 PDT
(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
Comment 11 2013-04-08 05:43:23 PDT
Created attachment 196845 [details] Patch Updated as per comments
Jocelyn Turcotte
Comment 12 2013-04-08 06:46:18 PDT
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
Comment 13 2013-04-08 06:46:53 PDT
(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
Comment 14 2013-04-08 22:47:48 PDT
Created attachment 196999 [details] Patch Updated patch
Jocelyn Turcotte
Comment 15 2013-04-09 00:49:06 PDT
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
Comment 16 2013-04-09 02:07:26 PDT
(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
Comment 17 2013-04-09 02:11:33 PDT
Simon, Allan: Please tell if you see anything wrong with this added API.
Jose Lejin PJ
Comment 18 2013-04-09 10:54:19 PDT
Created attachment 197145 [details] Patch Updated patch with test cases and documentation
Jocelyn Turcotte
Comment 19 2013-04-10 02:30:49 PDT
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
Comment 20 2013-04-10 02:47:35 PDT
(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
Comment 21 2013-04-10 05:14:20 PDT
Created attachment 197249 [details] Patch Updated patch
Jose Lejin PJ
Comment 22 2013-04-10 05:17:09 PDT
(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 23 2013-04-10 05:47:11 PDT
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 24 2013-04-10 07:22:21 PDT
Comment on attachment 197249 [details] Patch cq requested by arunprasadr on IRC
WebKit Commit Bot
Comment 25 2013-04-10 08:08:38 PDT
Comment on attachment 197249 [details] Patch Clearing flags on attachment: 197249 Committed r148095: <http://trac.webkit.org/changeset/148095>
WebKit Commit Bot
Comment 26 2013-04-10 08:08:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.