Bug 34902 – [Qt] Add methods to QtWebKit API for setting and consulting the CSS2 media type

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Kenneth Rohde Christiansen]

Added methods to QWebFrame for setting and consulting the CSS2 media type

[Kenneth Rohde Christiansen]

Created attachment 48656 [details]
Patch

[Laszlo Gombos]

I think QPrinter references are suppose to be behind QT_NO_PRINTER guards - see qwebframe.* and qwebview.*.

[Kenneth Rohde Christiansen]

Maybe we should use an enum instead?

[Simon Hausmann]

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?

[Kenneth Rohde Christiansen]

> Wouldn't it be nicer if the media type was preserved in Frame::setPrinting?

It would, and I should fix that.

[Simon Hausmann]

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? :)

[Kenneth Rohde Christiansen]

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?

[Yael]

(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.

[Kenneth Rohde Christiansen]

Created attachment 55304 [details]
Second try, up for comments

[Antonio Gomes]

(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.

[Simon Hausmann]

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?

[Simon Hausmann]

(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.....

[Kenneth Rohde Christiansen]

(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.

[Kenneth Rohde Christiansen]

(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 :-)

[Kenneth Rohde Christiansen]

Created attachment 55417 [details]
Patch

[Antonio Gomes]

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.

[Kenneth Rohde Christiansen]

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.

[Simon Hausmann]

Comment on attachment 55417 [details]
Patch

r=me

However before landing I think this property should be moved to QWebSettings

[Kenneth Rohde Christiansen]

Created attachment 56110 [details]
Patch (Add as a setting)

[Adam Barth]

Comment on attachment 55417 [details]
Patch

clearing r+ from obsolete patch.

[Kenneth Rohde Christiansen]

Ping review?

[Simon Hausmann]

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.

[Andreas Kling]

Chilling the prio on this a bit, since it hasn't been touched in almost a year.

[Jocelyn Turcotte]

=== 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.