Bug 57784 – [Qt][WK2] Qt port needs load-from-history implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Jamie Cooley]

QtWebKit1 had this api in QWebHistory.

WK2 implements this via
WKPageGoToBackForwardListItem(WKPageRef page, WKBackForwardListItemRef item); 

It needs to be added to Qt APIs.

[Jamie Cooley]

Created attachment 88120 [details]
Proposed patch to add load from history

Proposed patch adds the api to QWKPage - A little different from how it was in WK1, since in '2, history knows nothing about the current page. Also added an access method for WKBackForwardListItemRef in QWKHistoryItem... similar thing is done with QWKPage and pageRef.

[Benjamin Poulain]

Comment on attachment 88120 [details]
Proposed patch to add load from history

Missing tests and doc.
I know most methods do in WK2, but that is no excuse for new addition :)

[Jamie Cooley]

Very well, I've created 57850 to write tests for (existing) qwkhistory (which doesn't have any). I think that this needs to be done before I can even address test cases for this bug.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 88120 [details])
> Missing tests and doc.
> I know most methods do in WK2, but that is no excuse for new addition :)

I don't think it's necessary to add doc for the Qt/WK2 APIs at this stage.
We will need a ChangeLog entry though. :)

On another note, this API is rather dirty.

- The name implies that a load will happen, whereas it may as well be a cached page restoration (depending on availability.) The old QWebHistory::goToItem() made more sense in this regard.

- What if the QWKHistoryItem passed to QWKPage::loadFromHistory() comes from another QWKPage's history()?

Since the QWKHistory points to a WebBackForwardList, which internally has a WebPageProxy*, I think it would make more sense that the QWKHistory be aware of the QWKPage that owns it. This way we could put a WK1-style goToItem() into QWKHistory instead.

[Jamie Cooley]

> Since the QWKHistory points to a WebBackForwardList, which internally has a WebPageProxy*, I think it would make more sense that the QWKHistory be aware of the QWKPage that owns it. This way we could put a WK1-style goToItem() into QWKHistory instead.

Now that the other bug to add test content is committed, I'll start to re-work this as you are suggesting. This means that QWKHistory will have to maintain a pointer to it's owner page? I'll write it up and see what you all think.

I always felt like the way I did it here was a little funny, but didn't want to change things around too much. Let's see...

[Jamie Cooley]

Created attachment 95784 [details]
Patch

[Jamie Cooley]

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

So I've implemented something like what you've suggested. For this time I chose the easiest route, and just passed a copy of the (public) QWKPage into history so it has a copy of it (as opposed to granting/enabling access to private or something like this).

I toyed with the idea of changing the QWKHistory instantiation to pass in QWKPage instead of a pointer to the WKBackForward list, and fetching the list inside of QWKHistory's constructor. However, amongst all the toAPI and toImpl and stuff in the WK2API I couldn't figure out how to fetch it. If you would rather I do something like this instead, please suggest how to fetch it.

I named the public API "goToItemAt" after "itemAt" and use the integer-style api. Currently it silently fails if int is out of range. Don't know how you feel about this either, but it's easy to change.

[Jamie Cooley]

Created attachment 98347 [details]
Patch

[Andreas Kling]

Comment on attachment 98347 [details]
Patch

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

Looks mostly great, just some minor itches:

> Source/WebKit2/ChangeLog:8
> +        Created "random access" ForwardBackHistory public API method,

Typo, BackForwardHistory.

> Source/WebKit2/ChangeLog:17
> +        Updated createHistroy method and QWKHistoryPrivate constructor to take a pointer

Typo, createHistory.

> Source/WebKit2/ChangeLog:18
> +        to the owning QWKPage in addtion to the WebBackForwardList. This will be saved so that the

Typo, addition.

> Source/WebKit2/UIProcess/API/qt/qwkhistory.h:51
> +    WKBackForwardListItemRef itemRef() const;

We shouldn't use WK2 types in our APIs, QWKHistory and QWKHistoryItem should be friends and grab at each others private members directly instead.

[Jamie Cooley]

Created attachment 98377 [details]
Patch - respun changed after klings comments.

[Jamie Cooley]

Respun changes after kling's comments.

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

[Andreas Kling]

Comment on attachment 98377 [details]
Patch - respun changed after klings comments.

LGTM

[WebKit Review Bot]

Comment on attachment 98377 [details]
Patch - respun changed after klings comments.

Clearing flags on attachment: 98377

Committed r89605: <http://trac.webkit.org/changeset/89605>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.