Bug 81701

Summary:

[QT][WK2] webview API doc

Product:

WebKit

Reporter:

Mike Sierra <letmespellitoutforyou>

Component:

WebKit API

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hausmann, jturcotte, kenneth, laszlo.gombos, maheshk, menard, vestbo, webkit.review.bot, zoltan

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch features inserted comment regions of qdoc markup	none
revised patch to add changelog & fix other comment item	none
fixed changelog	hausmann: review-, hausmann: commit-queue-
incorporates review comments	hausmann: review+

Mike Sierra

Reported 2012-03-20 14:32:30 PDT

qdoc applies to revised Qt5 public webview API

Attachments
patch features inserted comment regions of qdoc markup (12.08 KB, patch) 2012-03-20 14:34 PDT, Mike Sierra	no flags	Details Formatted Diff Diff
revised patch to add changelog & fix other comment item (12.65 KB, patch) 2012-03-28 11:30 PDT, Mike Sierra	no flags	Details Formatted Diff Diff
fixed changelog (12.92 KB, patch) 2012-03-29 13:52 PDT, Mike Sierra	hausmann: review- hausmann: commit-queue-	Details Formatted Diff Diff
incorporates review comments (13.07 KB, patch) 2012-03-30 07:45 PDT, Mike Sierra	hausmann: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mike Sierra

Comment 1 2012-03-20 14:34:49 PDT

Created attachment 132901 [details] patch features inserted comment regions of qdoc markup

Mahesh Kulkarni

Comment 2 2012-03-20 14:42:01 PDT

Mike, please include change logs as well. Others, thoughts?

Mike Sierra

Comment 3 2012-03-21 07:11:05 PDT

Changes: * Most are insertions that represent the complete up-to-date Qt5 API. * Removed 2 existing qdoc regions, one for loadHtml(), which referenced the deprecated load() method. * Also replaced existing onNavigationRequested() that referenced DownloadRequest, which is not included in the latest API. * Appended doc for signals & enums; placed summary doc near top; otherwise placed each chunk of doc near corresponding declarations within code.

Alexis Menard (darktears)

Comment 4 2012-03-27 14:10:54 PDT

Comment on attachment 132901 [details] patch features inserted comment regions of qdoc markup View in context: https://bugs.webkit.org/attachment.cgi?id=132901&action=review Please look at http://www.webkit.org/coding/contributing.html It's missing the Changelog. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1241 > +If possible, stop loading the contents of page. Why if possible?

Mike Sierra

Comment 5 2012-03-28 11:30:47 PDT

Created attachment 134349 [details] revised patch to add changelog & fix other comment item Appended revised patch. Thanks for feedback.

Mike Sierra

Comment 6 2012-03-28 11:32:42 PDT

If more info is needed in change log, please let me know. Thanks

WebKit Review Bot

Comment 7 2012-03-29 13:41:54 PDT

Attachment 134349 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebKit2/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Mike Sierra

Comment 8 2012-03-29 13:52:31 PDT

Created attachment 134659 [details] fixed changelog

Simon Hausmann

Comment 9 2012-03-30 00:05:10 PDT

Comment on attachment 134659 [details] fixed changelog View in context: https://bugs.webkit.org/attachment.cgi?id=134659&action=review Mike, thanks a lot for working on this! I have a few comments that I think justify another iteration :) > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:96 > + url: "http://www.example.com" How about using a real url, such as http://www.qt-project.org/ ? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:101 > + onNavigationHistoryChanged: { > + console.log("Can go back: " + canGoBack + "\n can go forward: " + canGoForward); > + } I'm not sure it is a common thing to connect to this signal and therefore bring it up in this overview part of the documentation. I'm inclined to say that it's more likely that people will use the canGoBack, etc. properties directly in bindings. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:104 > + var schemaRE = /^\w+:/; > + if (schemaRE.test(request.url)) { I think a comment here would make it easier to understand what this example tries to accomplish, i.e. detect a url schema. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:107 > + } > + else { I think the more common coding style here would put the else on the same line as the closing bracket. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1241 > +Stop loading the contents of page. I think there's a "the" missing. But how about "Stop loading the current page." ? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1302 > +The location of the currently displaying HTML page icon. This > +read-only URL corresponds to the image used within a browser > +application to represent a bookmarked page on the device's home > +screen. To specify an icon, apply the \c{apple-touch-icon} link > +relation within the HTML's \c{<head>} region: I wonder if it would be easier to explain with an example snippet how the icon property works. The current documentation feels like it emphasizes the url representation of the icon, whereas I feel the important aspect of this property is that it can be used as a source to an Image element, i.e. Image { id: favIcon source: webView.icon != "" ? webView.icon : "fallbackFavIcon.png"; ... } > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1321 > +percentage in the range from \c{0} to \c{100}. (This value does not > +account for externally referenced files.) I suggest to leave out the part about externally referenced files, because it's not quite true. If a page consists of text and a lot of externally loaded images, then the loadProgress will only reach 100 if all externally referenced images have completed loading. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1637 > +\qmlsignal WebView::onTitleChanged(title) We have removed the title parameter of this signal, for consistency with all the other fooChanged signals and to avoid parameter shadowing in any slots connected to the signal, which makes the code harder to read. (I can explain a bit more in detail if you want :) > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1696 > +Within a mouse-driven interface, occurs when a mouse pointer passes How about "Within a mouse-driven interface, this signal is emitted when..." ?

Mike Sierra

Comment 10 2012-03-30 07:45:36 PDT

Created attachment 134815 [details] incorporates review comments OK, thanks for all the feedback. Incorporated all changes. My slightly longer comment on "schemaRE" suggests it's conceivable (but unlikely) that you might use a scheme prefix to refer to pages on your own website. I changed your "favIcon" to "appIcon" to avoid confusion with the tiny 16x16 favicons that are deployed differently in desktop browsers, and added a note to clarify that point. There are several variables to how apple-touch-icons work on various browsers ("size," "precomposed" and whether a fixed filename is placed in the server root rather than specified in markup) but I didn't think that worth getting into in this context. I'll take your word for it on parameter shadowing. ;-)

Kenneth Rohde Christiansen

Comment 11 2012-04-01 04:56:09 PDT

Comment on attachment 134815 [details] incorporates review comments View in context: https://bugs.webkit.org/attachment.cgi?id=134815&action=review Nice > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1267 > +/*! > +\qmlproperty url WebView::url > + > +The location of the currently displaying HTML page. This writable > +property offers the main interface to load a page into a web view. > +It functions the same as the \c{window.location} DOM property. > + > +\sa WebView::loadHtml(), WebView::onUrlChanged > +*/ Maybe some info here about handling errors, what happens if the url cannot be loaded directly by us and will be downloaded ? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1293 > +\qmlproperty url WebView::icon Some more info about the sizes, animated icons etc? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1295 > +The location of the currently displaying HTML page icon. This This is commonly known as favicons. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1334 > +\qmlproperty int WebView::loadProgress > + > +The amount of the page that has been loaded, expressed as an integer > +percentage in the range from \c{0} to \c{100}. > + > +\sa WebView::onLoadProgressChanged So if it is fully loaded it will be 100? I think this should be clarified > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1617 > +\qmlmethod void WebView::loadHtml(html, baseUrl) > + > +Loads the specified \a{html} string as the content of the web view. > +(This method offers a lower-level alternative to the \c{url} property, > +which references HTML pages via URL.) > > - External objects such as stylesheets or images referenced in the HTML > - document are located relative to \a baseUrl. > +External objects such as stylesheets or images referenced by the HTML > +string are located relative to the \a baseUrl. I think an example of the base url would make sense > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1646 > +\qmlsignal WebView::onNavigationHistoryChanged() > + > +Occurs when session history changes. > +*/ > + reference to goBack etc? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1669 > +\o \c{status}: Reflects one of three load states: > +\c{LoadStartedStatus}, \c{LoadSucceededStatus}, or > +\c{LoadFailedStatus}. See \c{WebView::LoadStatus}. > + > +\o \c{errorString}: description of load error. I think there were a bug adding yet another state here. LoadStoppedStatus or so > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1694 > +\qmlsignal WebView::onUrlChanged() > + > +Occurs when a URL changes in any way. > +*/ What about redirections? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1710 > +that are not canceled with \c{preventDefault()}.) The \a{hoveredUrl} cancelled ?

Kenneth Rohde Christiansen

Comment 12 2012-04-01 05:03:52 PDT

Maybe explain about data: url's? Most people don't know about them and they are quite useful. http://en.wikipedia.org/wiki/Data_URI_scheme

Simon Hausmann

Comment 13 2012-08-30 00:46:55 PDT

Comment on attachment 134815 [details] incorporates review comments I'm going to go ahead and land these changes manually together with Kenneth's suggestions as a starting point. From there we can then take things incrementally and hopefully with the help of some of the doc folks in Oslo.

Simon Hausmann

Comment 14 2012-08-30 04:09:49 PDT

Comment on attachment 134815 [details] incorporates review comments View in context: https://bugs.webkit.org/attachment.cgi?id=134815&action=review >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1293 >> +\qmlproperty url WebView::icon > > Some more info about the sizes, animated icons etc? I think I'll add more here in a second round of review with the doc folks. >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1295 >> +The location of the currently displaying HTML page icon. This > > This is commonly known as favicons. Good idea. Fixed. >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1334 >> +\sa WebView::onLoadProgressChanged > > So if it is fully loaded it will be 100? I think this should be clarified From what I can tell from the *Loader sources, the signal will be emitted with 100% right before the emission of loadingChanged (in our API). I'm inclined to say that's intuitive behaviour and an implementation detail we may not have to document. >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1617 >> +string are located relative to the \a baseUrl. > > I think an example of the base url would make sense Good idea, added. >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1669 >> +\o \c{errorString}: description of load error. > > I think there were a bug adding yet another state here. LoadStoppedStatus or so Not yet :) >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1694 >> +*/ > > What about redirections? I'm leaving out the docs for this and other signals that are just notifications. We usually only document the property as it's implicit that every property has a onPropertyNameChanged signal that is emitted whenever the property changes. >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1710 >> +that are not canceled with \c{preventDefault()}.) The \a{hoveredUrl} > > cancelled ? Fixed, we indeed use the UK English.

Simon Hausmann

Comment 15 2012-08-30 04:33:45 PDT

Committed r127125: <http://trac.webkit.org/changeset/127125>

Note You need to log in before you can comment on or make changes to this bug.