Bug 57784 - [Qt][WK2] Qt port needs load-from-history implementation
Summary: [Qt][WK2] Qt port needs load-from-history implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jamie Cooley
URL:
Keywords: Qt, QtTriaged
Depends on: 57850
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-04 13:30 PDT by Jamie Cooley
Modified: 2011-06-23 12:54 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch to add load from history (2.24 KB, patch)
2011-04-04 13:59 PDT, Jamie Cooley
no flags Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2011-06-02 12:18 PDT, Jamie Cooley
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2011-06-23 07:24 PDT, Jamie Cooley
no flags Details | Formatted Diff | Diff
Patch - respun changed after klings comments. (9.00 KB, patch)
2011-06-23 12:00 PDT, Jamie Cooley
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jamie Cooley 2011-04-04 13:30:12 PDT
QtWebKit1 had this api in QWebHistory.

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

It needs to be added to Qt APIs.
Comment 1 Jamie Cooley 2011-04-04 13:59:58 PDT
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.
Comment 2 Benjamin Poulain 2011-04-05 05:09:24 PDT
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 :)
Comment 3 Jamie Cooley 2011-04-05 08:10:06 PDT
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.
Comment 4 Andreas Kling 2011-04-21 18:07:16 PDT
(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.
Comment 5 Jamie Cooley 2011-05-31 08:01:02 PDT
> 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...
Comment 6 Jamie Cooley 2011-06-02 12:18:39 PDT
Created attachment 95784 [details]
Patch
Comment 7 Jamie Cooley 2011-06-02 12:25:41 PDT
(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.
Comment 8 Jamie Cooley 2011-06-23 07:24:33 PDT
Created attachment 98347 [details]
Patch
Comment 9 Andreas Kling 2011-06-23 08:13:46 PDT
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.
Comment 10 Jamie Cooley 2011-06-23 12:00:02 PDT
Created attachment 98377 [details]
Patch - respun changed after klings comments.
Comment 11 Jamie Cooley 2011-06-23 12:01:17 PDT
Respun changes after kling's comments.

(In reply to comment #10)
> Created an attachment (id=98377) [details]
> Patch
Comment 12 Andreas Kling 2011-06-23 12:12:28 PDT
Comment on attachment 98377 [details]
Patch - respun changed after klings comments.

LGTM
Comment 13 WebKit Review Bot 2011-06-23 12:54:40 PDT
Comment on attachment 98377 [details]
Patch - respun changed after klings comments.

Clearing flags on attachment: 98377

Committed r89605: <http://trac.webkit.org/changeset/89605>
Comment 14 WebKit Review Bot 2011-06-23 12:54:49 PDT
All reviewed patches have been landed.  Closing bug.