Summary: | [Qt] WebKit2 history, QWKHistory | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Juha Savolainen <juha.savolainen> | ||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Enhancement | CC: | commit-queue, fishd, kenneth, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Juha Savolainen
2010-10-15 02:31:57 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 on attachment 70844 [details]
Initial implementation of QWKHistory.
Obsoleted. EWS failed to run.
Created attachment 70853 [details]
Initial implementation of QWKHistory.
Just fixed directories in patch so EWS could run this.
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 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 + Created attachment 71013 [details]
patch
Changes after Kenneth's comments.
Created QWKHistoryPrivate-class which have one static function to create QWKHistory.
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. 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 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; (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. (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. (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 :-) Created attachment 71307 [details]
patch
fixed patch.
Comment on attachment 71307 [details]
patch
Great, keep up the good work!
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 on attachment 71307 [details] patch Clearing flags on attachment: 71307 Committed r70218: <http://trac.webkit.org/changeset/70218> All reviewed patches have been landed. Closing bug. |