This patch adds QWebHistory::addItem which allows adding history item to the page BackForwardList. Please bear with me as it's my 1st webkit patch ;)
Created attachment 29417 [details] patch
Comment on attachment 29417 [details] patch First of all welcome and a '-' is not as bad as it might seem like. There are some beginner issues though. I think the API addition does make sense, so it is about technical issues as this point. The comments are inline with the patch. > Index: WebKit/qt/ChangeLog > =================================================================== > --- WebKit/qt/ChangeLog (revision 42423) > +++ WebKit/qt/ChangeLog (working copy) > @@ -1,3 +1,14 @@ > +2009-04-11 Mohammed Sameer <set EMAIL_ADDRESS environment variable> You should set EMAIL_ADDRESS in your environment or change it here. > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Added QWebHistory::addItem that allows adding history items > + to the page BackForwardList Please use the eight spaces, tabs will be rejected by a post commit hook. > /*! > + Adds an item to the history. > + > + Please note that this will "trash" any forward items Some more documentation is needed. E.g. a @since 4.6 tag, description of the parameters (e.g. what timezone is lastVisited), and maybe even a unit test, showing the behavior (specially the trashing one). > +*/ > +void QWebHistory::addItem(const QString &url, const QString &title, const QDateTime &lastVisited) > +{ > + QByteArray utf8 = url.toUtf8(); > + WebCore::String Url(utf8.constData(), utf8.length()); > + utf8 = title.toUtf8(); > + WebCore::String Title(utf8.constData(), utf8.length()); > + d->lst->addItem(WebCore::HistoryItem::create(Url, Title, lastVisited.toTime_t())); > +} > You can write all this as: d->lst->addItem(WebCore::HistoryItem::create(String(url), String(title), last...); Why this approach is better: - Less code - The conversion to Utf8 takes time and extra allocations and your import into String is not losless... This c'tor is ascii only and will lose any non ascii charachters. Style: - Please use 4 spaces as of our CodingStyle guidelines on webkit.org - Please use a lowercase letter for the start of variable names
Created attachment 29433 [details] patch - 2nd try 2nd trial. I decided to do it the Gtk way without a lastVisited argument. I also return the new QWebHistoryItem I'll then create another patch that allows setting various properties on that object.
And thanks for the review and all the tips :)
As for trashing the forward items, this is what BackForwardList does. We can't really control it. The use case I have is a browser restoring the session upon startup. It should add items to the history so trashing the forward items is not really a big issue IMHO.
(In reply to comment #3) > I'll then create another patch that allows setting various properties on that > object. So far, QWebHistoryItem is not mutable by choice. Be prepared it might stay this way.
Comment on attachment 29433 [details] patch - 2nd try Saying r=- right now as the history will unlikely be mutable. Is the constructor you are using everything you need? Do you need any other attributes?
For the use case I have in mind, I want to save all the history item attributes and restore them. The easiest way is to add setters to the QWebHistoryItem returned unless you want a constructor that takes a lot of parameters that will have to be updated whenever the history item gets new attributes (I doubt we want this) ;-)
There is work being done that serializes this IIRC. Marking as invalid given this and the date.