Added methods to QWebFrame for setting and consulting the CSS2 media type
Created attachment 48656 [details] Patch
I think QPrinter references are suppose to be behind QT_NO_PRINTER guards - see qwebframe.* and qwebview.*.
Maybe we should use an enum instead?
Comment on attachment 48656 [details] Patch > @@ -1439,6 +1466,9 @@ void QWebFrame::print(QPrinter *printer) const > } > > printContext.end(); > + > + QWebFrame* that = const_cast<QWebFrame*>(this); > + that->setMediaType(currentMediaType); > } > #endif // QT_NO_PRINTER Wouldn't it be nicer if the media type was preserved in Frame::setPrinting?
> Wouldn't it be nicer if the media type was preserved in Frame::setPrinting? It would, and I should fix that.
Comment on attachment 48656 [details] Patch Other than that I like the API. I think it should be a Q_PROPERTY. It would be nice perhaps if the media type could be propagated down to child frames. Theoretically the frame is the correct place, but given that child frames can appear and disappear at will it only makes sense to set this property on the persistent main frame, at which point to avoid disambigouties a property on the page might be clearer. I'm going to say r- because of the missing Q_PROPERTY and more importantly because I feel it's not clear that this is the right place. Would love to hear input from others on this. Mailing list? :)
I'm still not sure if we should use an enum instead of a string, to not allow people to add non-standard media types. Media types is only one part of media as we have media features as well, and their support depend on the parser recognizing the right keywords, so it would be impossible extending the media features via a public api (it would also be hard as we would need callback methods, knowing about the type, css etc), and that is probably not something that we would like to do anyway. My the current patch, the user of the api can add whatever media type he/she likes and I think we should disencourage that. What is your take on that Simon?
(In reply to comment #6) > It would be nice perhaps if the media type could be propagated down to child > frames. Theoretically the frame is the correct place, but given that child > frames can appear and disappear at will it only makes sense to set this > property on the persistent main frame, at which point to avoid disambigouties a > property on the page might be clearer. > > I'm going to say r- because of the missing Q_PROPERTY and more importantly > because I feel it's not clear that this is the right place. Would love to hear > input from others on this. Mailing list? :) I totally agree with Simon. When I saw this patch, my first thought was that this should be a QWebPage property, because we are not planning on supporting different media types per frame.
Created attachment 55304 [details] Second try, up for comments
(In reply to comment #9) > Created an attachment (id=55304) [details] > Second try, up for comments goog patch. +static void setMediaTypeRecursively(WebCore::Frame* frame, WebCore::String media) const WebCore::String& nitpick: - m_frame->createView(m_webFrame->page()->viewportSize(), + m_frame->createView(page->viewportSize(), it is indeed right, but does it need to happen in this patch? :) + \since 4.7 +*/ you are proposing it for 4.7, so it should block the qtwebkit-2.0 release.
Comment on attachment 55304 [details] Second try, up for comments WebKit/qt/Api/qwebpage.cpp:1821 + d->mainFrame->d->setMediaType(d->mediaType); This should probably call mainFrame() instead of accessing d->mainFrame directly, to avoid crashing when d->mainFrame is a null pointer. WebKit/qt/Api/qwebpage.cpp:1807 + QString QWebPage::mediaType() const Since this is a Q_PROPERTY the document must use the \property syntax, instead of individual documentation for the setter and getter. WebKit/qt/Api/qwebpage_p.h:173 + QString mediaType; Do we really need to store a copy of the media type in WebKit? I haven't looked deeply, just wondering if we can't just keep the string in WebCore?
(In reply to comment #10) > + \since 4.7 > +*/ > > you are proposing it for 4.7, so it should block the qtwebkit-2.0 release. That's a good point. Do we really need this feature in the release? We have 43 bugs blocking the release and 23 triaged priority one bugs.....
(In reply to comment #11) > (From update of attachment 55304 [details]) > WebKit/qt/Api/qwebpage.cpp:1821 > + d->mainFrame->d->setMediaType(d->mediaType); > This should probably call mainFrame() instead of accessing d->mainFrame > directly, to avoid crashing when d->mainFrame is a null pointer. OK, fine. > > WebKit/qt/Api/qwebpage.cpp:1807 > + QString QWebPage::mediaType() const > Since this is a Q_PROPERTY the document must use the \property syntax, instead > of individual documentation for the setter and getter. OK, so not individual documentation then. > > WebKit/qt/Api/qwebpage_p.h:173 > + QString mediaType; > Do we really need to store a copy of the media type in WebKit? I haven't looked > deeply, just wondering if we can't just keep the string in WebCore? We do, as you can set it before a frame is loaded and because you can move a frame from one page to another.
(In reply to comment #12) > (In reply to comment #10) > > + \since 4.7 > > +*/ > > > > you are proposing it for 4.7, so it should block the qtwebkit-2.0 release. > > That's a good point. Do we really need this feature in the release? > > We have 43 bugs blocking the release and 23 triaged priority one bugs..... I didn't add it as a blocker, but that could be done it it enters trunk. Btw, 43 includes the tracker bugs, and I am actually trying to triage the bugs right now :-)
Created attachment 55417 [details] Patch
Comment on attachment 55417 [details] Patch >+static void setMediaTypeRecursively(WebCore::Frame* frame, const WebCore::String& media) >+{ >+ WebCore::FrameView* view = frame->view(); >+ if (view->mediaType() != media) { >+ view->setMediaType(media); >+ frame->document()->updateStyleSelector(); >+ frame->view()->layout(); >+ } >+ >+ for (Frame* child = frame->tree()->firstChild(); child; child = child->tree()->nextSibling()) >+ setMediaTypeRecursively(child, media); >+} >+ >+void QWebFramePrivate::setMediaType(const QString& media) >+{ >+ setMediaTypeRecursively(frame, media); >+} If it is specified in the specs that an outer frame should propagate its media type value to its inner frames, the recursive step (read method setMediaTypeRecursively) should go to Frame.cpp. That way you would be ensuring that other ports that implement setMediaType-like method in their API would also follow the same recurvive assignment behaviour. In other words, adding setMediaTypeRecursively in WebKit side can create diverging behaviors between webkit ports if one does the recursive assignment and other(s) do(es) not. + \note A frame inherits the media type of its owner, thus either another frame in the case it is a sub + frame, or else the page itself. When a frame is moved from one page to another, the media type is + automatically updated. + it would be so great to autotest that as well :) , but it can come in a follow up test I think.
Antonio, this is a C++ side API decision. The spec doesn't define how the user agent/browser should create it's API for setting the media type, that is up to us.
Comment on attachment 55417 [details] Patch r=me However before landing I think this property should be moved to QWebSettings
Created attachment 56110 [details] Patch (Add as a setting)
Comment on attachment 55417 [details] Patch clearing r+ from obsolete patch.
Ping review?
Comment on attachment 56110 [details] Patch (Add as a setting) WebCore/page/Settings.h:311 + Page* page() { return m_page; } This should probably be const. WebKit/qt/Api/qwebframe.cpp:225 + void QWebFramePrivate::applyMediaType(WebCore::Frame* frame, const WebCore::String& mediaType, bool recursive) I don't see where false is passed from any caller of this method, so I think you can remove the argument and call the function applyMediaTypeRecursively (Webkittish ;) WebKit/qt/Api/qwebsettings.cpp:584 + void QWebSettings::setStyleSheetMediaType(const QString& mediaType) The implementation of this function should fall back to the media type of the global QWebSettings object. The usual pattern is to do the changes in apply(), which will also be be called if you change settings in the global QWebSettings object. WebKit/qt/Api/qwebsettings.cpp:591 + QString QWebSettings::styleSheetMediaType() const I think the implementation of this function should returnd->styleSheetMediaType straight, instead of automatically falling back. See the same pattern for example with QWebSettings::userStyleSheetUrl and setUserStyleSheetUrl.
Chilling the prio on this a bit, since it hasn't been touched in almost a year.
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.