Bug 30710 - [Qt] QWebHistory::saveState() is inconsistent with the Qt API
Summary: [Qt] QWebHistory::saveState() is inconsistent with the Qt API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
: 26452 (view as bug list)
Depends on:
Blocks: 29843
  Show dependency treegraph
 
Reported: 2009-10-23 01:41 PDT by Benjamin Poulain
Modified: 2009-11-24 07:48 PST (History)
7 users (show)

See Also:


Attachments
Remove the methods (12.81 KB, patch)
2009-10-28 05:40 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Improve the versioning (7.16 KB, patch)
2009-10-28 07:46 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Avoid serializing to immediate QByteArray (2.56 KB, patch)
2009-10-28 15:05 PDT, Kenneth Rohde Christiansen
vestbo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2009-10-23 01:41:22 PDT
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.
Comment 1 Tor Arne Vestbø 2009-10-27 05:38:40 PDT
Agree, let's remove these for now and keep the stream operators.
Comment 2 Kenneth Rohde Christiansen 2009-10-27 15:04:10 PDT
OK, I'm working on this one.
Comment 3 Jędrzej Nowacki 2009-10-28 01:47:55 PDT
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.
Comment 4 Benjamin Poulain 2009-10-28 02:13:37 PDT
(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.
Comment 5 Jędrzej Nowacki 2009-10-28 03:02:35 PDT
(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.
Comment 6 Jędrzej Nowacki 2009-10-28 03:34:25 PDT
(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.
Comment 7 Kenneth Rohde Christiansen 2009-10-28 05:40:34 PDT
Created attachment 42018 [details]
Remove the methods
Comment 8 Kenneth Rohde Christiansen 2009-10-28 05:43:06 PDT
This patch does the 1) part. I think Jedrzej wanted to do the follow up patches.
Comment 9 Holger Freyther 2009-10-28 06:12:54 PDT
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.
Comment 10 Kenneth Rohde Christiansen 2009-10-28 07:46:47 PDT
Created attachment 42027 [details]
Improve the versioning
Comment 11 Kenneth Rohde Christiansen 2009-10-28 07:47:23 PDT
Comment on attachment 42018 [details]
Remove the methods

Landed in 50214.
Comment 12 Tor Arne Vestbø 2009-10-28 10:06:43 PDT
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.
Comment 13 Kenneth Rohde Christiansen 2009-10-28 10:41:50 PDT
(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.
Comment 14 Kenneth Rohde Christiansen 2009-10-28 11:02:55 PDT
(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.
Comment 15 Kenneth Rohde Christiansen 2009-10-28 15:05:05 PDT
Created attachment 42056 [details]
Avoid serializing to immediate QByteArray
Comment 16 Jędrzej Nowacki 2009-10-29 01:49:47 PDT
(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.
Comment 17 Kenneth Rohde Christiansen 2009-10-29 06:17:40 PDT
(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.
Comment 18 Kenneth Rohde Christiansen 2009-10-29 06:54:31 PDT
Landed in 50269
Comment 19 Jędrzej Nowacki 2009-11-24 07:48:06 PST
*** Bug 26452 has been marked as a duplicate of this bug. ***