Bug 81701 - [QT][WK2] webview API doc
Summary: [QT][WK2] webview API doc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2012-03-20 14:32 PDT by Mike Sierra
Modified: 2012-08-30 04:33 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Sierra 2012-03-20 14:32:30 PDT
qdoc applies to revised Qt5 public webview API
Comment 1 Mike Sierra 2012-03-20 14:34:49 PDT
Created attachment 132901 [details]
patch features inserted comment regions of qdoc markup
Comment 2 Mahesh Kulkarni 2012-03-20 14:42:01 PDT
Mike, please include change logs as well. 
Others, thoughts?
Comment 3 Mike Sierra 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.
Comment 4 Alexis Menard (darktears) 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?
Comment 5 Mike Sierra 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.
Comment 6 Mike Sierra 2012-03-28 11:32:42 PDT
If more info is needed in change log, please let me know. Thanks
Comment 7 WebKit Review Bot 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.
Comment 8 Mike Sierra 2012-03-29 13:52:31 PDT
Created attachment 134659 [details]
fixed changelog
Comment 9 Simon Hausmann 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..." ?
Comment 10 Mike Sierra 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. ;-)
Comment 11 Kenneth Rohde Christiansen 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 ?
Comment 12 Kenneth Rohde Christiansen 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
Comment 13 Simon Hausmann 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.
Comment 14 Simon Hausmann 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.
Comment 15 Simon Hausmann 2012-08-30 04:33:45 PDT
Committed r127125: <http://trac.webkit.org/changeset/127125>