[webkit-reviews] review denied: [Bug 81701] [QT][WK2] webview API doc : [Attachment 134659] fixed changelog

Fri Mar 30 00:05:10 PDT 2012

Simon Hausmann <hausmann at webkit.org> has denied Mike Sierra
<mike.sierra at nokia.com>'s request for review:
Bug 81701: [QT][WK2] webview API doc
https://bugs.webkit.org/show_bug.cgi?id=81701

Attachment 134659: fixed changelog
https://bugs.webkit.org/attachment.cgi?id=134659&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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..." ?