Bug 30710 – [Qt] QWebHistory::saveState() is inconsistent with the Qt API

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Benjamin Poulain]

In Qt, there is classically two ways to serialize an object to a QByteArray: Q...::saveState() and QDataStream & operator<<(Q...).

The function saveState is used for visible object like widgets (QFileDialog, QSplitter, etc). The bytearray returned is typically small, and is saved with QSettings.

For all other objects (QColor, QImage, etc), QDataStream & operator<<(Q...) is preferred since it can be used to write directly on the filesystem.


Since, QWebHistory is not a Widget, the functions QWebHistory::saveState() and QWebHistory::restoreState() should be removed.

[Tor Arne Vestbø]

Agree, let's remove these for now and keep the stream operators.

[Kenneth Rohde Christiansen]

OK, I'm working on this one.

[Jędrzej Nowacki]

Few month ago there was a discussion in the Oslo office about these functions. The main argument for having saveState() and restoreState() was to keep coherence with other Qt classes (like QApplication, QMainWindow...). Second issue was a problem of multiple version of QWebHistory. The QDataSteream version was changed on most of Qt minor, but not in bug fix releases and the version format is X.Y (enum QDataStream::Version). As most of the QtWebkit bug fix are done by upstreaming the Webkit part, there is a chance that we will have to bump the version in a bug fix release. Qt's and Webkit's version number are out of sync.

(In reply to comment #0)
> The function saveState is used for visible object like widgets (QFileDialog,
> QSplitter, etc). The bytearray returned is typically small, and is saved with
> QSettings.
> 
> For all other objects (QColor, QImage, etc), QDataStream & operator<<(Q...) is
> preferred since it can be used to write directly on the filesystem.
The QApplication is not a widget and it is hard to say that it is visible part of an application.
QWebHistory is not so big, so comparison to QImage is an abuse. I don't think that writing history is a bottle neck, and it have to be done as a real stream. But of course optimizations are welcomed :-).

Summary:
The case is not so obvious, but I prefer streaming operators too. And I think that removing of save&restore functions is perfectly save for the first Qt 4.6 release.

[Benjamin Poulain]

(In reply to comment #3)
> Few month ago there was a discussion in the Oslo office about these functions.
> The main argument for having saveState() and restoreState() was to keep
> coherence with other Qt classes (like QApplication, QMainWindow...). Second
> issue was a problem of multiple version of QWebHistory. The QDataSteream
> version was changed on most of Qt minor, but not in bug fix releases and the
> version format is X.Y (enum QDataStream::Version). As most of the QtWebkit bug
> fix are done by upstreaming the Webkit part, there is a chance that we will
> have to bump the version in a bug fix release. Qt's and Webkit's version number
> are out of sync.

To be able to work around a possible bug, maybe we should introduce a version number in the stream?
This way we can deal with the bug silently by using this version number and we will not ask the user to change an arbitrary version number for bugs of Webkit

> The QApplication is not a widget and it is hard to say that it is visible part
> of an application.

QApplication::saveState() is different. This function is used for session management, it does not return a QByteArray.

[Jędrzej Nowacki]

(In reply to comment #4)
> To be able to work around a possible bug, maybe we should introduce a version
> number in the stream?
> This way we can deal with the bug silently by using this version number and we
> will not ask the user to change an arbitrary version number for bugs of Webkit
No and yes. If you look at QWebHistory implementation there is an QWebHistory version saved in a stream. Current API permit you to save a history in specified version and protect you from shooting in the foot by restoring a bad version. I found it a bit more safe than the QDataStream API, witch give you full control on versions. What is sens of loading a history version 3 with an algorithm version 1, apart from nasty side effects? The real problem is in the stream class that for CPU and memory reasons doesn't include the stream version.
The problem isn't new. I think that most of Qt users know that they need to save somehow the QDataStream version. So I strongly believe that if we want to get rid of the saveState() and the restoreState() we shouldn't include a version number in a stream. Just use the standard enum QDataStream::Version. In case of problems we will add new value to the enum changing the version on bug fix releases too.

> > The QApplication is not a widget and it is hard to say that it is visible part
> > of an application.
>
> QApplication::saveState() is different. This function is used for session
> management, it does not return a QByteArray.
It is more complex and have different signature, true, but purpose of this function is the same.

[Jędrzej Nowacki]

(In reply to comment #5)
In replay to replay... To clarify...
1. I agree, we should remove the saveState() and the restoreState().
2. We should remove the QWebHistory::HistoryStateVersion and use the QDataStream::Version instead.
3. We should _not_ save version number in a stream.

[Kenneth Rohde Christiansen]

Created attachment 42018 [details]
Remove the methods

[Kenneth Rohde Christiansen]

This patch does the 1) part. I think Jedrzej wanted to do the follow up patches.

[Holger Freyther]

Comment on attachment 42018 [details]
Remove the methods

Okay, based on the promise that the internal QDataStream/QByteArray will go away before Qt4.6 let us progress on this one.

[Kenneth Rohde Christiansen]

Created attachment 42027 [details]
Improve the versioning

[Kenneth Rohde Christiansen]

Comment on attachment 42018 [details]
Remove the methods

Landed in 50214.

[Tor Arne Vestbø]

Comment on attachment 42027 [details]
Improve the versioning

QDataStream's version is for the internal format of QDataStream. We should follow the example in the docs:

http://doc.trolltech.com/4.5/qdatastream.html

Ie, a payload-spesific version where we try to load whatever we can from earlier versions, etc.

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> (From update of attachment 42027 [details])
> QDataStream's version is for the internal format of QDataStream. We should
> follow the example in the docs:
> 
> http://doc.trolltech.com/4.5/qdatastream.html
> 
> Ie, a payload-spesific version where we try to load whatever we can from
> earlier versions, etc.

We should keep the versioning internal, and always save in the latest format. Reading will read only if the version is supported.

This is more or less what me and Tor Arne agreed upon.

If needed in the future, it will still be possible to add support for saving in older formats.

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 42027 [details] [details])
> > QDataStream's version is for the internal format of QDataStream. We should
> > follow the example in the docs:
> > 
> > http://doc.trolltech.com/4.5/qdatastream.html
> > 
> > Ie, a payload-spesific version where we try to load whatever we can from
> > earlier versions, etc.
> 
> We should keep the versioning internal, and always save in the latest format.
> Reading will read only if the version is supported.
> 
> This is more or less what me and Tor Arne agreed upon.
> 
> If needed in the future, it will still be possible to add support for saving in
> older formats.

The above was implemented and landed in 50224.

The only thing missing is making the internal QDataStream/QByteArray in the stream operators go away.

[Kenneth Rohde Christiansen]

Created attachment 42056 [details]
Avoid serializing to immediate QByteArray

[Jędrzej Nowacki]

(In reply to comment #12)
> (From update of attachment 42027 [details])
> QDataStream's version is for the internal format of QDataStream. We should
> follow the example in the docs:
> 
> http://doc.trolltech.com/4.5/qdatastream.html
> 
> Ie, a payload-spesific version where we try to load whatever we can from
> earlier versions, etc.

This example is created for developers using Qt, not for internal Qt development. I have checked a few classes (QImage, QColor, QRect, QVector). None of them save the version into a stream.

[Kenneth Rohde Christiansen]

(In reply to comment #16)
> (In reply to comment #12)
> > (From update of attachment 42027 [details] [details])
> > QDataStream's version is for the internal format of QDataStream. We should
> > follow the example in the docs:
> > 
> > http://doc.trolltech.com/4.5/qdatastream.html
> > 
> > Ie, a payload-spesific version where we try to load whatever we can from
> > earlier versions, etc.
> 
> This example is created for developers using Qt, not for internal Qt
> development. I have checked a few classes (QImage, QColor, QRect, QVector).
> None of them save the version into a stream.

They don't need to (directly) as they are distributed with Qt. But we won't continue being distributed with Qt, so we definately needs this.

[Kenneth Rohde Christiansen]

Landed in 50269

[Jędrzej Nowacki]

*** Bug 26452 has been marked as a duplicate of this bug. ***