Bug 31115 – [Qt] QWebView, QGraphicsWebView and QWebFrame setHtml() should be better documented

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

Hi,

  I think that the QWebFrame::setHtml() should be removed from the public API or at least marked as obsolete/internal.

  Justification:
  a.  The name is confusing. Many users think that the method can be used for XHTML too or for HTML with XHTML extensions. Even if sometimes it works (bug 29540), it is wrong, because implementation force a mime type (text/html).
  b.  The method is redundant. It can be easily replaced by the QWebFrame::setContent(), which behave in the same manner with a defaults arguments.
  c.  The name is confusing. The setHtml() "Sets the content of the web view to the specified html" and the setContent() "Sets the content of the web view to the specified content data", so what is difference between specified html and content data? In my opinion "setContent" is a better name.
  d.  There is no way to override default mime type setting, even if given source code is a correct XHTML or SVG file with DOCTYPE information.
  e.  The setContent() method is more flexible. For example it permits to create a SVG based interface.
  f.  The load function is better choice if user have an URL. It is more "intelligent", because it can guess a mime type from file name or HTTP header (the information is lost if user load the file manually). So it maybe worth to encourage users to use the load() if they can.

Connected bugs: 29457, 29461 (29540)

[Antonio Gomes]

I agree that merging setHtml and setContent might be a good thing to be done at some point.

[Simon Hausmann]

While I agree that setHtml() may not be a very "pure" function to have (give the XHTML relation you mentioned for example), I still think it is a good thing to have.

* The function is consistent with the rest of Qt (QTextEdit::setHtml, QTextDocument::setHtml, QGraphicsTextItem::setHtml). Qt developers love the fact that our API is consistent. The unspoken guideline is that an experienced Qt developer shouldn't have to read the documentation of a class in order to use it. That is why completely different classes like QTabBar, QComboBox or QListWidget have a count() function. They're not called numTabs(), numItems() or numListItems(), which would be a slightly tighter name. Instead they share the same name so that the API remains concise.

* setHtml() is also very convenient. We say that with the Qt API common things should be easy and special tasks should be possible. Populating a widget to render web content with a given string of HTML falls clearly into the former category. It is less error prone than calling setContent(htmlString, "text/htlm"). Oh, oops, that should've been "text/html", but I only found out at run-time because my text didn't show :)

* To satisfy the use-case of a more advanced task we added setContent(), which is harder to use but also more flexible.

[Jędrzej Nowacki]

Created attachment 43851 [details]
documentation change v1

(In reply to comment #2)
> * The function is consistent with the rest of Qt (QTextEdit::setHtml,
> QTextDocument::setHtml, QGraphicsTextItem::setHtml). Qt developers love the
> fact that our API is consistent.
This is nice point :-), but on the other hand classes mentioned by you are much simpler (implementation & functionality). I think it's clear that you shouldn't put SVG into the QTextEdit, but for the QWebPage it is not so obvious. Moreover Qt had to be not only simple, but should "force" right application design. As setHtml is a nice shortcut, I don't think that it must be used on the first place. Developers need to know what actually they want to process (SVG, HTML, XHTML...).

> * setHtml() is also very convenient. We say that with the Qt API common things
> should be easy and special tasks should be possible. Populating a widget to
> render web content with a given string of HTML falls clearly into the former
> category.
Yes it should be, but as we don't provide unified access for XHTML and HTML it is a source of problems.

> * To satisfy the use-case of a more advanced task we added setContent(), which
> is harder to use but also more flexible.
So I prepared a little patch for it. I don't think, we should close the discussion.

[Antonio Gomes]

jedrzej, i like the docs and think this change is enough to sort the confusion out. I just think you should change QGraphicsWebView's setHtml content docs too :-) in WebKit/qt/Api/qwebview.cpp


changing bug subject accordingly to the direction the solution is taking ...

[Alexis Menard (darktears)]

Created attachment 62944 [details]
Patch for the doc fix, with QGraphicsWebView as well

[Kenneth Rohde Christiansen]

Comment on attachment 62944 [details]
Patch for the doc fix, with QGraphicsWebView as well

WebKit/qt/Api/qgraphicswebview.cpp:715
 +      setContent() should be used instead
Misses a dot at the end - both places it is used.

WebKit/qt/Api/qgraphicswebview.cpp:714
 +      \warning This function works only for HTML, for other mime types (ex. XHTML, SVG)
I would use 'ie.' instead of ex. I believe that is more English.

[Alexis Menard (darktears)]

Created attachment 62946 [details]
Patch for the doc fix, with QGraphicsWebView as well - v2

[Antonio Gomes]

Comment on attachment 62946 [details]
Patch for the doc fix, with QGraphicsWebView as well - v2

r=me

[Alexis Menard (darktears)]

*** Bug 29461 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 62946 [details]
Patch for the doc fix, with QGraphicsWebView as well - v2

Clearing flags on attachment: 62946

Committed r64284: <http://trac.webkit.org/changeset/64284>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r64284 cherry-picked into qtwebkit-2.1 with commit 20c90a1df97b9ca403a562e19624b2245de4c09e