[Webkit-unassigned] [Bug 81701] [QT][WK2] webview API doc

Fri Mar 30 00:05:11 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=81701

Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #134659|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #9 from Simon Hausmann <hausmann at webkit.org>  2012-03-30 00:05:10 PST ---
(From update of attachment 134659)
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..." ?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.