Oxide

Merge ~osomon/oxide:fix-1649861-serialized-navigation-entries into oxide:master

  1. Git
  2. lp:~osomon/oxide
  3. fix-1649861-serialized-navigation-entries
  4. Merge into master
Proposed by Olivier Tilloy
Status: Merged
Approved by: Chris Coulson
Approved revision: b90c7c8c4bb669c2a64fe7d8e1e67e6bbd3dfc08
Merged at revision: f40a17d2841c51309f605fab1a229b08e85d5305
Proposed branch: ~osomon/oxide:fix-1649861-serialized-navigation-entries
Merge into: oxide:master
Diff against target: 341 lines (+245/-14)
5 files modified
qt/core/browser/oxide_qt_web_view.cc (+148/-14)
qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html (+5/-0)
qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html (+5/-0)
qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html (+5/-0)
qt/tests/qmltests/api/tst_WebView_save_restore_state.qml (+82/-0)
Reviewer Review Type Date Requested Status
Chris Coulson Approve
Review via email: mp+313310@code.qastaging.launchpad.net

Commit message

Fix session save/restore across oxide versions (LP: #1649861).

Make session save/restore future-proof by storing each navigation entry separately.

To post a comment you must log in.
Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Thanks for this - I've left a comment inline.

Also, there needs to be some tests for the upgrade path too.

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks for the review. I’ve replied to your comment with a (hopefully) thorough analysis of the potential issue, and I’ll add a comment in the code for future reference. I’ll also look into adding tests for the upgrade path.

Revision history for this message
Olivier Tilloy (osomon) wrote :

Updated with a comment for future reference and a unit test for the upgrade path.

Revision history for this message
Santosh (santoshbit2007) :
Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks for your review Santosh. See my answers to your comments inline.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Ok, I've replied inline.

One other thing to consider - 1.19 has already shipped to the archive with this bug. I'm not sure if there's a way to handle upgrades from the current 1.19 release without breaking restoration for a second time, as state saved with the current 1.19 release at least has an extra integer for each navigation entry. Given the extra integer is always zero, and each subsequent serialized navigation entry starts with the index (which should always be non-zero), then there's probably a clever way to detect this...

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

> Ok, I've replied inline.
>
> One other thing to consider - 1.19 has already shipped to the archive with
> this bug. I'm not sure if there's a way to handle upgrades from the current
> 1.19 release without breaking restoration for a second time, as state saved
> with the current 1.19 release at least has an extra integer for each
> navigation entry. Given the extra integer is always zero, and each subsequent
> serialized navigation entry starts with the index (which should always be non-
> zero), then there's probably a clever way to detect this...

Also, given this, it might be worth serializing the Oxide version in the new format.

Revision history for this message
Olivier Tilloy (osomon) wrote :

All addressed. Comments welcome.

Revision history for this message
Chris Coulson (chrisccoulson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches