Bug 25144 - [Qt] Adding QWebHistory::addItem
Summary: [Qt] Adding QWebHistory::addItem
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Enhancement
Assignee: Nobody
Keywords: Qt
Depends on:
Reported: 2009-04-11 14:55 PDT by Mohammed Sameer
Modified: 2009-07-14 18:09 PDT (History)
1 user (show)

See Also:

patch (1.81 KB, patch)
2009-04-11 14:57 PDT, Mohammed Sameer
zecke: review-
Details | Formatted Diff | Diff
patch - 2nd try (1.75 KB, patch)
2009-04-13 12:14 PDT, Mohammed Sameer
zecke: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mohammed Sameer 2009-04-11 14:55:33 PDT
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 ;)
Comment 1 Mohammed Sameer 2009-04-11 14:57:25 PDT
Created attachment 29417 [details]
Comment 2 Holger Freyther 2009-04-11 20:32:01 PDT
Comment on attachment 29417 [details]

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.

    - Please use 4 spaces as of our CodingStyle guidelines on webkit.org
    - Please use a lowercase letter for the start of variable names
Comment 3 Mohammed Sameer 2009-04-13 12:14:23 PDT
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.
Comment 4 Mohammed Sameer 2009-04-13 12:16:43 PDT
And thanks for the review and all the tips :)
Comment 5 Mohammed Sameer 2009-04-13 12:20:45 PDT
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.
Comment 6 Holger Freyther 2009-05-11 20:32:30 PDT
(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 7 Holger Freyther 2009-05-22 22:56:31 PDT
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?
Comment 8 Mohammed Sameer 2009-05-23 09:28:33 PDT
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) ;-)
Comment 9 Adam Treat 2009-07-14 18:09:22 PDT
There is work being done that serializes this IIRC.  Marking as invalid given this and the date.