Bug 31115

Summary: [Qt] QWebView, QGraphicsWebView and QWebFrame setHtml() should be better documented
Product: WebKit Reporter: Jędrzej Nowacki <jedrzej.nowacki>
Component: WebKit QtAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hausmann, tonikitoo, vestbo
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 29457, 29461, 29876, 31552    
Attachments:
Description Flags
documentation change v1
jedrzej.nowacki: review-, jedrzej.nowacki: commit-queue-
Patch for the doc fix, with QGraphicsWebView as well
kenneth: review-
Patch for the doc fix, with QGraphicsWebView as well - v2 none

Jędrzej Nowacki
Reported 2009-11-04 06:20:39 PST
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)
Attachments
documentation change v1 (2.35 KB, patch)
2009-11-25 09:11 PST, Jędrzej Nowacki 		jedrzej.nowacki: review-
jedrzej.nowacki: commit-queue-
DetailsFormatted DiffDiff
Patch for the doc fix, with QGraphicsWebView as well (3.19 KB, patch)
2010-07-29 07:36 PDT, Alexis Menard (darktears) 		kenneth: review-
DetailsFormatted DiffDiff
Patch for the doc fix, with QGraphicsWebView as well - v2 (3.20 KB, patch)
2010-07-29 08:01 PDT, Alexis Menard (darktears) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Antonio Gomes
Comment 1 2009-11-20 19:01:49 PST
I agree that merging setHtml and setContent might be a good thing to be done at some point.
Simon Hausmann
Comment 2 2009-11-21 00:56:35 PST
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
Comment 3 2009-11-25 09:11:10 PST
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
Comment 4 2009-11-25 14:19:52 PST
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)
Comment 5 2010-07-29 07:36:42 PDT
Created attachment 62944 [details] Patch for the doc fix, with QGraphicsWebView as well
Kenneth Rohde Christiansen
Comment 6 2010-07-29 07:40:19 PDT
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)
Comment 7 2010-07-29 08:01:26 PDT
Created attachment 62946 [details] Patch for the doc fix, with QGraphicsWebView as well - v2
Antonio Gomes
Comment 8 2010-07-29 08:13:44 PDT
Comment on attachment 62946 [details] Patch for the doc fix, with QGraphicsWebView as well - v2 r=me
Alexis Menard (darktears)
Comment 9 2010-07-29 08:17:29 PDT
*** Bug 29461 has been marked as a duplicate of this bug. ***
WebKit Commit Bot
Comment 10 2010-07-29 09:30:02 PDT
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
Comment 11 2010-07-29 09:30:07 PDT
All reviewed patches have been landed. Closing bug.
Simon Hausmann
Comment 12 2010-08-04 03:39:31 PDT
Revision r64284 cherry-picked into qtwebkit-2.1 with commit 20c90a1df97b9ca403a562e19624b2245de4c09e
Note You need to log in before you can comment on or make changes to this bug.