Bug 47716 - [Qt] WebKit2 history, QWKHistory
Summary: [Qt] WebKit2 history, QWKHistory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-15 02:31 PDT by Juha Savolainen
Modified: 2010-10-21 02:40 PDT (History)
4 users (show)

See Also:


Attachments
Initial implementation of QWKHistory. (9.49 KB, patch)
2010-10-15 02:42 PDT, Juha Savolainen
no flags Details | Formatted Diff | Diff
Initial implementation of QWKHistory. (9.37 KB, patch)
2010-10-15 04:51 PDT, Juha Savolainen
kenneth: review-
juha.savolainen: commit-queue?
Details | Formatted Diff | Diff
patch (11.70 KB, patch)
2010-10-18 03:10 PDT, Juha Savolainen
kenneth: review-
Details | Formatted Diff | Diff
patch (12.17 KB, patch)
2010-10-19 02:41 PDT, Juha Savolainen
kenneth: review-
Details | Formatted Diff | Diff
patch (11.98 KB, patch)
2010-10-20 10:45 PDT, Juha Savolainen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Juha Savolainen 2010-10-15 02:31:57 PDT
WebKit2-qt needs history, which represents the history of a QWKPage and makes it possible to navigate it.
Comment 1 Juha Savolainen 2010-10-15 02:42:05 PDT
Created attachment 70844 [details]
Initial implementation of QWKHistory.

This is initial implementation of QWKHistory. The QWKHistory class represents the history of a QWKPage.
More functionality(including QWKForwardListItem) will be added if this feature/patch is accepted.
All comments/suggestions are welcome.
Comment 2 Juha Savolainen 2010-10-15 04:44:29 PDT
Comment on attachment 70844 [details]
Initial implementation of QWKHistory.

Obsoleted. EWS failed to run.
Comment 3 Juha Savolainen 2010-10-15 04:51:23 PDT
Created attachment 70853 [details]
Initial implementation of QWKHistory.

Just fixed directories in patch so EWS could run this.
Comment 4 WebKit Review Bot 2010-10-15 04:53:58 PDT
Attachment 70853 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/qt/qwkhistory.cpp:25:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/qt/qwkhistory.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/UIProcess/API/qt/qwkhistory.h:25:  #ifndef header guard has wrong style, please use: qwkhistory_h  [build/header_guard] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Kenneth Rohde Christiansen 2010-10-15 06:00:51 PDT
Comment on attachment 70853 [details]
Initial implementation of QWKHistory.

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

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:31
> +QWKHistory::QWKHistory(WebBackForwardList* list)

Doesn't look right from an ABI perspective... add a constructor to the private QWKHistoryPrivate::create(WebBackForwardList* list) or so

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:32
> +: m_backForwardList(list)

needs indentaton

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:52
> +    return backListCount()+forwardListCount();

needs spaces around +
Comment 6 Juha Savolainen 2010-10-18 03:10:43 PDT
Created attachment 71013 [details]
patch

Changes after Kenneth's comments.
Created QWKHistoryPrivate-class which have one static function to create QWKHistory.
Comment 7 Kenneth Rohde Christiansen 2010-10-18 12:19:08 PDT
Comment on attachment 71013 [details]
patch

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

> WebKit2/UIProcess/API/qt/qwkhistory.cpp:39
> +QWKHistory::QWKHistory(WebKit::WebBackForwardList* list)

This is not BC compatible. Our public classes (even in private: section) cannot refer to WebCore / WebKit internals

> WebKit2/UIProcess/API/qt/qwkhistory.h:49
> +    WebKit::WebBackForwardList* m_backForwardList;

This class should take a QWKHistoryPrivate* d pointer, and the WebKit::WebBackForwardList* should be inside that private class.
Comment 8 Juha Savolainen 2010-10-19 02:41:56 PDT
Created attachment 71146 [details]
patch

Thank you Kenneth for a informative comments.

Should we make methods also available to QWKHistoryPrivate, eg. 
int QWKHistoryPrivate::backListCount() const
{
    return WKBackForwardListGetBackListCount(toAPI(m_backForwardList));
}
Comment 9 Kenneth Rohde Christiansen 2010-10-19 06:40:07 PDT
Comment on attachment 71146 [details]
patch

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

Almost there!

> WebKit2/UIProcess/API/qt/qwkhistory.h:45
> +    QWKHistory(QWKHistoryPrivate* historyPrivate);

Please leave this as QWKHistory(); Just make it a friend and you can set the d pointer manually.

> WebKit2/UIProcess/API/qt/qwkpage.cpp:486
> +    return d->m_history;

in privates we do not add m_.

> WebKit2/UIProcess/API/qt/qwkpage_p.h:88
> +    QWKHistory* m_history;

make this QWKHistory* history;
Comment 10 Kenneth Rohde Christiansen 2010-10-19 06:40:51 PDT
(In reply to comment #8)
> Created an attachment (id=71146) [details]
> patch
> 
> Thank you Kenneth for a informative comments.
> 
> Should we make methods also available to QWKHistoryPrivate, eg. 
> int QWKHistoryPrivate::backListCount() const
> {
>     return WKBackForwardListGetBackListCount(toAPI(m_backForwardList));
> }

That is not really needed, we only do that for helper methods.
Comment 11 Juha Savolainen 2010-10-19 23:31:33 PDT
(In reply to comment #9)
> Almost there!

So near, so near, I can almost feel it!

> > WebKit2/UIProcess/API/qt/qwkhistory.h:45
> > +    QWKHistory(QWKHistoryPrivate* historyPrivate);
> 
> Please leave this as QWKHistory(); Just make it a friend and you can set the d pointer manually.

Just one clarification, do you mean that we set d pointer from QWKPagePrivate or QWKHistoryPrivate::createHistory? I quess in QWKHistoryPrivate.
Comment 12 Kenneth Rohde Christiansen 2010-10-20 07:52:00 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > Almost there!
> 
> So near, so near, I can almost feel it!
> 
> > > WebKit2/UIProcess/API/qt/qwkhistory.h:45
> > > +    QWKHistory(QWKHistoryPrivate* historyPrivate);
> > 
> > Please leave this as QWKHistory(); Just make it a friend and you can set the d pointer manually.
> 
> Just one clarification, do you mean that we set d pointer from QWKPagePrivate or QWKHistoryPrivate::createHistory? I quess in QWKHistoryPrivate.

The latter ofcourse :-) Just take a look at WebKit/qt/Api - I'm sure we do this various places :-)
Comment 13 Juha Savolainen 2010-10-20 10:45:53 PDT
Created attachment 71307 [details]
patch

fixed patch.
Comment 14 Kenneth Rohde Christiansen 2010-10-20 10:58:41 PDT
Comment on attachment 71307 [details]
patch

Great, keep up the good work!
Comment 15 WebKit Commit Bot 2010-10-20 18:16:38 PDT
The commit-queue encountered the following flaky tests while processing attachment 71307 [details]:

http/tests/navigation/post-goback-same-url.html
java/lc3/JSObject/ToObject-001.html

Please file bugs against the tests.  The author(s) of the test(s) have been CCed on this bug.  The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2010-10-21 02:39:56 PDT
Comment on attachment 71307 [details]
patch

Clearing flags on attachment: 71307

Committed r70218: <http://trac.webkit.org/changeset/70218>
Comment 17 WebKit Commit Bot 2010-10-21 02:40:02 PDT
All reviewed patches have been landed.  Closing bug.