[Qt][WK2] History is not accessible in QML.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

[Qt][WK2] History is not accessible in QML.

[Alexis Menard (darktears)]

Created attachment 116343 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #1)
> Created an attachment (id=116343) [details]
> Patch

First shot for feedback before I write tests :D.

QML code look like this

console.log("YOU WILL NAVIGATE TO" + webView.history.backItem().url)

You can also do this :

ListView {
        id: list
        x: 100
        y: 0
        height : root.height
        width : 500
        model: webView.history.backItems(20)
        z:300

        delegate: Text {
                 text: "url : " + model.modelData.url
        }
}

Please go ahead on criticism.

[Rafael Brandao]

Comment on attachment 116343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116343&action=review

It looks very nice. :)

> Source/WebKit2/UIProcess/API/qt/qwebpagehistory.cpp:97
> +QWebPageHistory::QWebPageHistory()

Not sure, but isn't good practice to start member pointers to zero? Or wouldn't be a good idea to receive the pointer in the constructor and assign your private attribute here? The only place you create a QWebPageHistory you also set it's private attribute just afterwards and the rest of implementation depends on it. I think passing this on argument would make it more clear.

> Source/WebKit2/UIProcess/API/qt/qwebpagehistory_p.h:72
> +    QWebPageHistoryItem* backItem() const;

It may be just me but when I read 'backItem', I have the feeling you'll push all items in the list one step backwards. But this is the one that return the previous value, right? Wouldn't 'previousItem' sound better?

> Source/WebKit2/UIProcess/API/qt/qwebpagehistory_p.h:76
> +    QVariant backItems(int maxItems) const;

Same feeling here.

> ChangeLog:8
> +        Add the new QWebPageHistory in the map file/

style ('/' in the end).

[Tor Arne Vestbø]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=116343) [details] [details]
> > Patch
> 
> First shot for feedback before I write tests :D.
> 
> QML code look like this
> 
> console.log("YOU WILL NAVIGATE TO" + webView.history.backItem().url)
> 

I think the property should be named navigationHistory, to signify that it's dealing with back-forward-history and not browsing history.

Also, I think we should consider doing a more model-like API here, meaning a single naviagtionHistory property, which is a model, and can be dropped in a ListView. naviagtionHistory.at(0) is the previous page in the history (the "back" item).


> You can also do this :
> 
> ListView {
>         id: list
>         x: 100
>         y: 0
>         height : root.height
>         width : 500
>         model: webView.history.backItems(20)
>         z:300
> 
>         delegate: Text {
>                  text: "url : " + model.modelData.url
>         }
> }
> 
> Please go ahead on criticism.

[Alexis Menard (darktears)]

Created attachment 116531 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #5)
> Created an attachment (id=116531) [details]
> Patch

Example of usage :
    ListView {
        id: list
        x: 100
        y: 0
        height : root.height
        width : 300
        model: webView.navigationHistory.backItems
        z:300

        delegate: Text {
                        color:"black"
                        text: "title : " + title

                MouseArea {
                    anchors.fill: parent
                    onClicked: {
                            webView.navigationHistory.navigateBackTo(index)
                        }
                }
        }
    }

    ListView {
        id: list2
        x: list.x + list.width
        y: 0
        height : root.height
        width : 300
        model: pageWidget.webView.navigationHistory.forwardItems
        z:300

        delegate: Text {
                color:"black"
                text: "title : " + title
                MouseArea {
                    anchors.fill: parent
                    onClicked {  webView.navigationHistory.navigateForwardTo(index)
                        }
                }
        }
    }

[Alexis Menard (darktears)]

I'll write tests when you guys think the API is cool :D.

[Tor Arne Vestbø]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=116531) [details] [details]
> > Patch
> 
> Example of usage :
>                 MouseArea {
>                     anchors.fill: parent
>                     onClicked: {
>                             webView.navigationHistory.navigateBackTo(index)
>                         }

Looking at this I'm thinking the action-part of the api should perhaps be on the view:

webView.goBack(index), where goBack(int count = 0) so our current goBack() would do the same thing

The webView.navigationHistory property sounds read only to me, so doing webView.navigationHistory.navigateBackTo() would be a bit strange.

[Tor Arne Vestbø]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created an attachment (id=116531) [details] [details] [details]
> > > Patch
> > 
> > Example of usage :
> >                 MouseArea {
> >                     anchors.fill: parent
> >                     onClicked: {
> >                             webView.navigationHistory.navigateBackTo(index)
> >                         }
> 
> Looking at this I'm thinking the action-part of the api should perhaps be on the view:
> 
> webView.goBack(index), where goBack(int count = 0) so our current goBack() would do the same thing
> 
> The webView.navigationHistory property sounds read only to me, so doing webView.navigationHistory.navigateBackTo() would be a bit strange.

sorry, goBack(int index = 0)

[Alexis Menard (darktears)]

Created attachment 118105 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #10)
> Created an attachment (id=118105) [details]
> Patch

Require declarative to be fixed first. But works otherwise.

[Simon Hausmann]

Would it perhaps make sense to modify the mini browser to also use this functionality?

[Alexis Menard (darktears)]

(In reply to comment #12)
> Would it perhaps make sense to modify the mini browser to also use this functionality?

I was thinking too. I wanted to implement some kind of long press on the navigation buttons but popups are broken in Qt5 and popups in QML require some C++ QWindow. Not sure it's worth having this.

[Tor Arne Vestbø]

(In reply to comment #13)
> (In reply to comment #12)
> > Would it perhaps make sense to modify the mini browser to also use this functionality?
> 
> I was thinking too. I wanted to implement some kind of long press on the navigation buttons but popups are broken in Qt5 and popups in QML require some C++ QWindow. Not sure it's worth having this.

You don't need a real window. Just do a fake popup using regular QML?

[Alexis Menard (darktears)]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Would it perhaps make sense to modify the mini browser to also use this functionality?
> > 
> > I was thinking too. I wanted to implement some kind of long press on the navigation buttons but popups are broken in Qt5 and popups in QML require some C++ QWindow. Not sure it's worth having this.
> 
> You don't need a real window. Just do a fake popup using regular QML?

As discussed on IRC, I'll make an additional patch for it.

[Simon Hausmann]

Comment on attachment 118105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118105&action=review

> Source/WebKit/qt/declarative/experimental/plugin.cpp:48
> +        qmlRegisterUncreatableType<QWebNavigationListModel>(uri, 3, 0, "WebNavigationListModel", QObject::tr("Cannot create separate instance of WebNavigationListModel"));
> +        qmlRegisterUncreatableType<QWebNavigationHistory>(uri, 3, 0, "PageHistory", QObject::tr("Cannot create separate instance of PageHistory"));

Hm, any particular reason for the naming inconsistency? The classes are called QWebNavigation* but the QML types are WebNavigationListMode and PageHistory (no Web prefix and Page instead).

> Source/WebKit2/UIProcess/API/qt/qwebnavigationhistory.cpp:81
> +void QWebNavigationHistoryPrivate::goBackTo(int index)
> +{
> +    WKRetainPtr<WKBackForwardListItemRef> itemRef = WKBackForwardListGetItemAtIndex(toAPI(m_backForwardList), -(index + 1));
> +    if (itemRef && m_page)
> +        WKPageGoToBackForwardListItem(m_page->pageRef(), itemRef.get());
> +}

One thing that just feels wrong about the history implementation is our mix of internal and C-API usage. Having toAPI in our internal code to get something done just doesn't feel right. Either we use internal types and functions or we use only public API and also store the FooRef public API types.

Anyway, this isn't your fault of course, this is how the "old" QWKHistory code was written. Think of my comment as something that maybe be worth addressing either now or in the future.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_navigationHistory.qml:83
> +            webView.goBack(1)

I admit that I'm not a fan of this API, I don't find it very readable because the meaning of the number isn't totally clear. Is it relative or absolute? I'd prefer goBack() / goForward() and instead of a clearer name for the case where you want to jump to a specific index in the back or forward history. Consider this a bikeshedding comment :)

What I'm thinking of for example is

webView.goBackToIndex(42);

What do you think?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:69
> +    connect(qmlWebView, SIGNAL(urlChanged(QUrl)), m_navigationHistory, SLOT(_q_reset()));

Is there perhaps a smarter way of doing this? It feels a bit strange to go all the way up to the public API layer to get a "notification" when we have to reset our history models. What about using WKPageLoaderClient's didChangeBackForwardList instead?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:81
> -    delete m_history;
> +    delete m_navigationHistory;

Doesn't have to be in this patch, but it would be nice to get rid of the manual deletes and use smart pointers instead, following the WebKit pattern using OwnPtr.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:174
> -    QWKHistory* m_history;
> +    QWebNavigationHistory* m_navigationHistory;

It would be nice to combine this refactoring with also moving the code out of QtWebPageProxy, which we're trying to get rid of after all. Doesn't haaaaave to be in this patch though.

[Tor Arne Vestbø]

Comment on attachment 118105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118105&action=review

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_navigationHistory.qml:83
>> +            webView.goBack(1)
> 
> I admit that I'm not a fan of this API, I don't find it very readable because the meaning of the number isn't totally clear. Is it relative or absolute? I'd prefer goBack() / goForward() and instead of a clearer name for the case where you want to jump to a specific index in the back or forward history. Consider this a bikeshedding comment :)
> 
> What I'm thinking of for example is
> 
> webView.goBackToIndex(42);
> 
> What do you think?

The goBack(1) form looks weird, but 1 should be the default, so you'd normally do goBack(). The special case would be where you passed the index of a history item, and then the code would look like:

goBack(clickedHistoryItem.index)

Which is somewhat more explanatory. 

Another option would be to base the alternative form on the item itself::

goBack(clickedHistoryItem)

or even 

goBackTo(clickedHistoryItem)

[Alexis Menard (darktears)]

Created attachment 118605 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #17)
> (From update of attachment 118105 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118105&action=review
> 
> >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_navigationHistory.qml:83
> >> +            webView.goBack(1)
> > 
> > I admit that I'm not a fan of this API, I don't find it very readable because the meaning of the number isn't totally clear. Is it relative or absolute? I'd prefer goBack() / goForward() and instead of a clearer name for the case where you want to jump to a specific index in the back or forward history. Consider this a bikeshedding comment :)
> > 
> > What I'm thinking of for example is
> > 
> > webView.goBackToIndex(42);
> > 
> > What do you think?
> 
> The goBack(1) form looks weird, but 1 should be the default, so you'd normally do goBack(). The special case would be where you passed the index of a history item, and then the code would look like:
> 
> goBack(clickedHistoryItem.index)
> 
> Which is somewhat more explanatory. 
> 
> Another option would be to base the alternative form on the item itself::
> 
> goBack(clickedHistoryItem)
> 
> or even 
> 
> goBackTo(clickedHistoryItem)

For example (concrete usage) :

    ListView {
        id: list
        x: 100
        y: 0
        height : root.height
        width : 300
        model: pageWidget.webView.experimental.navigationHistory.backItems
        z:300

        delegate: Text {
                        color:"black"
                        text: "title : " + title

                MouseArea {
                    anchors.fill: parent
                    onClicked: {
                            pageWidget.webView.goBack(index)
                        }
                }
        }

}

It is insanely convenient.

[Alexis Menard (darktears)]

Created attachment 118616 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 118784 [details]
Patch

[Simon Hausmann]

Comment on attachment 118784 [details]
Patch

r=me_and_torarne :)

[Alexis Menard (darktears)]

Committed r102582: <http://trac.webkit.org/changeset/102582>

[Alexis Menard (darktears)]

Committed r102597: <http://trac.webkit.org/changeset/102597>