Bug 54739 – [Qt] Add a Method to QWebHistory to let the browser add items to the tab's history

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Anton Kreuzkamp]

Created attachment 82940 [details]
patch as descriped above

The attached patch adds a Method to QWebHistory for adding an item to the tab's history. It is needed for restoring a tab including it's history.

[Benjamin Poulain]

Comment on attachment 82940 [details]
patch as descriped above

You forgot the flags :)

[WebKit Review Bot]

Attachment 82940 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebhistory.cpp', u'S..." exit_code: 1

Source/WebKit/qt/Api/qwebhistory.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/Api/qwebhistory.cpp:404:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 82940 [details]
patch as descriped above

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

And this needs good autotest coverage.

> WebKit/qt/Api/qwebhistory.cpp:400
> +  Adds an item to the history.

This is not good enough documentation. The doc also has to mention what are the argument for example.

> WebKit/qt/Api/qwebhistory.cpp:402
> +void QWebHistory::addItem(QString &url, QString &title, double lastVisited)

This should be ref-to-const for url and title.
The type for lastVisited is just mirroring WebKit internal, this is not good for Qt APIs.

[Benjamin Poulain]

Why do you need this at all? Why not serialize the history to QDataStream and restore it?

[Anton Kreuzkamp]

>You forgot the flags :)
which flags?

>The type for lastVisited is just mirroring WebKit internal, this is not good for Qt APIs.
which one to take? qreal?

>And this needs good autotest coverage.
What do you mean by that? (sorry, my english isn't the best)

>Why do you need this at all? Why not serialize the history to QDataStream and >restore it?
Well, I need to be able to save it in and restore it from a textfile.

[Benjamin Poulain]

(In reply to comment #5)
> >You forgot the flags :)
> which flags?

"R?" and "C?".
See http://trac.webkit.org/wiki/QtWebKitContrib

> >The type for lastVisited is just mirroring WebKit internal, this is not good for Qt APIs.
> which one to take? qreal?

QDateTime?...

> >And this needs good autotest coverage.
> What do you mean by that? (sorry, my english isn't the best)

When you provide a patch, you should also provide the unittest proving your patch is correct.
Those tests are called "autotests" or "API test" in the context of Qt APIs. You can find the existing tests in Source/WebKit/qt/tests/.


> >Why do you need this at all? Why not serialize the history to QDataStream and >restore it?
> Well, I need to be able to save it in and restore it from a textfile.

We will need something a bit more compelling that that :)
Saving that to a text file is useful if the user can modify the values with a  text editor. What is the use case for that?

[Jędrzej Nowacki]

I am not convinced by proposed API changes because of two issues:
1. It allows and encourage to fake history. I don't see real use case for this kind manipulation.
2. It seems that QDataStream should be enough for saving and restoring a application history.

As I said I'm not convinced. It doesn't mean that the change doesn't have sens. More use cases are needed :-)

[Anton Kreuzkamp]

(In reply to comment #6)
> > >Why do you need this at all? Why not serialize the history to QDataStream and >restore it?
> > Well, I need to be able to save it in and restore it from a textfile.
> 
> We will need something a bit more compelling that that :)
> Saving that to a text file is useful if the user can modify the values with a  text editor. What is the use case for that?

I simply need a way to restore the history after a restart of the browser.

[Benjamin Poulain]

(In reply to comment #8)
> I simply need a way to restore the history after a restart of the browser.

The datastream should be good enough then. I close the task.