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.
Created attachment 196296 [details] Test App
Created attachment 196453 [details] Patch
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.
Created attachment 196455 [details] Patch
(In reply to comment #4) > Created an attachment (id=196455) [details] > Patch In this corrected style check error.
Comment on attachment 196455 [details] Patch Attachment 196455 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17522053
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.
(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.)
(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.
(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.
Created attachment 196845 [details] Patch Updated as per comments
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
(In reply to comment #12) > Please don't do that, have a separate boolean in QWebSettingsPrivate. I meant a separate QString, sorry.
Created attachment 196999 [details] Patch Updated patch
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.
(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.
Simon, Allan: Please tell if you see anything wrong with this added API.
Created attachment 197145 [details] Patch Updated patch with test cases and documentation
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))));
(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.
Created attachment 197249 [details] Patch Updated patch
(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)))
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.
Comment on attachment 197249 [details] Patch cq requested by arunprasadr on IRC
Comment on attachment 197249 [details] Patch Clearing flags on attachment: 197249 Committed r148095: <http://trac.webkit.org/changeset/148095>
All reviewed patches have been landed. Closing bug.