Bug 47716 – [Qt] WebKit2 history, QWKHistory

• Component: WebKit2
• Status: RESOLVED FIXED

[Juha Savolainen]

WebKit2-qt needs history, which represents the history of a QWKPage and makes it possible to navigate it.

[Juha Savolainen]

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.

[Juha Savolainen]

Comment on attachment 70844 [details]
Initial implementation of QWKHistory.

Obsoleted. EWS failed to run.

[Juha Savolainen]

Created attachment 70853 [details]
Initial implementation of QWKHistory.

Just fixed directories in patch so EWS could run this.

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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 +

[Juha Savolainen]

Created attachment 71013 [details]
patch

Changes after Kenneth's comments.
Created QWKHistoryPrivate-class which have one static function to create QWKHistory.

[Kenneth Rohde Christiansen]

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.

[Juha Savolainen]

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));
}

[Kenneth Rohde Christiansen]

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;

[Kenneth Rohde Christiansen]

(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.

[Juha Savolainen]

(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.

[Kenneth Rohde Christiansen]

(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 :-)

[Juha Savolainen]

Created attachment 71307 [details]
patch

fixed patch.

[Kenneth Rohde Christiansen]

Comment on attachment 71307 [details]
patch

Great, keep up the good work!

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 71307 [details]
patch

Clearing flags on attachment: 71307

Committed r70218: <http://trac.webkit.org/changeset/70218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.