Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1687853002:
Make AW state_serializer handle restoring also legacy format (Closed)
DescriptionMake AW state_serializer handle restoring also legacy format
This ensures that even if an app attempts to restore a state saved
before upgrading WebView (using the new state version) the old version
will still be successfully restored.
BUG=584693
Committed: https://crrev.com/85ddabfdb82d0eb28addaf22f0f2c689cffeac46
Cr-Commit-Position: refs/heads/master@{#375418}
Patch Set 1 #Patch Set 2 : Fix signed/unsigned int compile issue #
Total comments: 12
Patch Set 3 : Tweaks according to review suggestions #
Total comments: 4
Patch Set 4 : Minor fixups and adding additional header tests #Patch Set 5 : Fix crashing unittests #Patch Set 6 : Fix signed/unsigned int mismatch #
Messages
Total messages: 19 (6 generated)
The CQ bit was checked by davve@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687853002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687853002/20001
sbergner@opera.com changed reviewers: + mnaganov@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The idea looks good, just have some questions for the implementation. https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... File android_webview/native/state_serializer.cc (right): https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer.cc:39: const uint32_t SUPPORTED_VERSIONS[] = { Do we really need an array? Since there just 2 possible values, perhaps it's too early to generalize yet. Also the version validity checker function will become a one-liner. https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer.cc:43: const uint32_t NUM_SUPPORTED_VERSIONS = nit: there is `arraysize` in base/macros.h https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:143: entry->SetHttpStatusCode(http_status_code); I would suggest extracting entry initialization code into a function to avoid copy-pasting it. https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:152: result = internal::RestoreNavigationEntryFromPickle(20130814, &iterator, Shouldn't this actually be a test where we save a pickle in the old format and then restore in the new format? We will not be saving using the old format in the real life anymore.
https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... File android_webview/native/state_serializer.cc (right): https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer.cc:39: const uint32_t SUPPORTED_VERSIONS[] = { On 2016/02/10 16:37:45, mnaganov wrote: > Do we really need an array? Since there just 2 possible values, perhaps it's too > early to generalize yet. Also the version validity checker function will become > a one-liner. Sure, makes sense. https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:143: entry->SetHttpStatusCode(http_status_code); On 2016/02/10 16:37:45, mnaganov wrote: > I would suggest extracting entry initialization code into a function to avoid > copy-pasting it. Acknowledged. https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:152: result = internal::RestoreNavigationEntryFromPickle(20130814, &iterator, On 2016/02/10 16:37:45, mnaganov wrote: > Shouldn't this actually be a test where we save a pickle in the old format and > then restore in the new format? We will not be saving using the old format in > the real life anymore. What this test aims to check is that we can still actually load the legacy format as well. To do that I create a legacy pickle (with the WriteNavigationEntryToPickle(20130814,...)) so when reading that pickle the state_version of the pickle is the 20130814 version. I'm trying to mimic what happens in reality when a state has been saved with the old WebView version (pickle version=20130814) and then restored with this new version.
https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:152: result = internal::RestoreNavigationEntryFromPickle(20130814, &iterator, On 2016/02/11 07:23:28, sbergner wrote: > On 2016/02/10 16:37:45, mnaganov wrote: > > Shouldn't this actually be a test where we save a pickle in the old format and > > then restore in the new format? We will not be saving using the old format in > > the real life anymore. > > What this test aims to check is that we can still actually load the legacy > format as well. To do that I create a legacy pickle (with the > WriteNavigationEntryToPickle(20130814,...)) so when reading that pickle the > state_version of the pickle is the 20130814 version. I'm trying to mimic what > happens in reality when a state has been saved with the old WebView version > (pickle version=20130814) and then restored with this new version. Right, I've got confused by this direct version specification in the call to "Restore...". I have a suggestion how to make this more clear (and perhaps even closer to the real case) -- when saving, write both the header and the contents to the pickle, and then on restore use the public "RestoreFromPickle" to test that it will read the header and restore the entry using the saved version. WDYT? https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:171: It also makes sense to add a test that uses a pickle with some non-supported version in the header, and verify that we will reject it. https://codereview.chromium.org/1687853002/diff/40001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/40001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:24: namespace { nit: please put a blank line after "namespace {" https://codereview.chromium.org/1687853002/diff/40001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:67: return entry; I guess you need to return std::move(entry) here.
https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:152: result = internal::RestoreNavigationEntryFromPickle(20130814, &iterator, On 2016/02/11 17:21:48, mnaganov wrote: > On 2016/02/11 07:23:28, sbergner wrote: > > On 2016/02/10 16:37:45, mnaganov wrote: > > > Shouldn't this actually be a test where we save a pickle in the old format > and > > > then restore in the new format? We will not be saving using the old format > in > > > the real life anymore. > > > > What this test aims to check is that we can still actually load the legacy > > format as well. To do that I create a legacy pickle (with the > > WriteNavigationEntryToPickle(20130814,...)) so when reading that pickle the > > state_version of the pickle is the 20130814 version. I'm trying to mimic what > > happens in reality when a state has been saved with the old WebView version > > (pickle version=20130814) and then restored with this new version. > > Right, I've got confused by this direct version specification in the call to > "Restore...". I have a suggestion how to make this more clear (and perhaps even > closer to the real case) -- when saving, write both the header and the contents > to the pickle, and then on restore use the public "RestoreFromPickle" to test > that it will read the header and restore the entry using the saved version. > WDYT? Hm, I do agree that actually trying to test the complete pickle restore (incl. header) would be good. However I believe that means we have to do one of two things: 1. Construct a WebContents object to feed into a new internal::WriteToPickle() which also takes a state_version 2. Duplicate the data writing of selection index and entry count so we would call: WriteHeaderToPickle(20130814, ..); pickle->WriteInt(1); // entry count pickle->WriteInt(0); // selected index WriteNavigationEntryToPickle(20130814, ..); Regarding option 1, I am not very familiar with content::WebContents but initial feeling was that it wasn't very easy to construct that object and get it to contain the wanted navigation entries. For option 2 I don't really like the fact that we have to rely on knowledge about implementation details of how WriteToPickle works. Do you see other alternatives or think any of the above is a good way to go? https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:171: On 2016/02/11 17:21:48, mnaganov wrote: > It also makes sense to add a test that uses a pickle with some non-supported > version in the header, and verify that we will reject it. Acknowledged.
PTAL https://codereview.chromium.org/1687853002/diff/40001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/40001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:67: return entry; On 2016/02/11 17:21:48, mnaganov wrote: > I guess you need to return std::move(entry) here. Changed it at first but on second thought I don't think it is needed
LGTM Please fix the clang issue and you are good to go! https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/20001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:152: result = internal::RestoreNavigationEntryFromPickle(20130814, &iterator, On 2016/02/12 07:53:31, sbergner wrote: > On 2016/02/11 17:21:48, mnaganov wrote: > > On 2016/02/11 07:23:28, sbergner wrote: > > > On 2016/02/10 16:37:45, mnaganov wrote: > > > > Shouldn't this actually be a test where we save a pickle in the old format > > and > > > > then restore in the new format? We will not be saving using the old format > > in > > > > the real life anymore. > > > > > > What this test aims to check is that we can still actually load the legacy > > > format as well. To do that I create a legacy pickle (with the > > > WriteNavigationEntryToPickle(20130814,...)) so when reading that pickle the > > > state_version of the pickle is the 20130814 version. I'm trying to mimic > what > > > happens in reality when a state has been saved with the old WebView version > > > (pickle version=20130814) and then restored with this new version. > > > > Right, I've got confused by this direct version specification in the call to > > "Restore...". I have a suggestion how to make this more clear (and perhaps > even > > closer to the real case) -- when saving, write both the header and the > contents > > to the pickle, and then on restore use the public "RestoreFromPickle" to test > > that it will read the header and restore the entry using the saved version. > > WDYT? > > Hm, I do agree that actually trying to test the complete pickle restore (incl. > header) would be good. However I believe that means we have to do one of two > things: > 1. Construct a WebContents object to feed into a new internal::WriteToPickle() > which also takes a state_version > 2. Duplicate the data writing of selection index and entry count so we would > call: > WriteHeaderToPickle(20130814, ..); > pickle->WriteInt(1); // entry count > pickle->WriteInt(0); // selected index > WriteNavigationEntryToPickle(20130814, ..); > > Regarding option 1, I am not very familiar with content::WebContents but initial > feeling was that it wasn't very easy to construct that object and get it to > contain the wanted navigation entries. > > For option 2 I don't really like the fact that we have to rely on knowledge > about implementation details of how WriteToPickle works. > > Do you see other alternatives or think any of the above is a good way to go? Yeah, involving an instance of a real or mocked out WebContents would be too heavy. I think the resulting set of tests is good enough. Thanks! https://codereview.chromium.org/1687853002/diff/40001/android_webview/native/... File android_webview/native/state_serializer_unittest.cc (right): https://codereview.chromium.org/1687853002/diff/40001/android_webview/native/... android_webview/native/state_serializer_unittest.cc:67: return entry; On 2016/02/12 13:00:10, sbergner wrote: > On 2016/02/11 17:21:48, mnaganov wrote: > > I guess you need to return std::move(entry) here. > > Changed it at first but on second thought I don't think it is needed I was thinking that the unit tests were crashing because of it, but you've found the actual cause.
clang compile issue addressed, please take a look
The CQ bit was checked by sbergner@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from mnaganov@chromium.org Link to the patchset: https://codereview.chromium.org/1687853002/#ps100001 (title: "Fix signed/unsigned int mismatch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687853002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687853002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Make AW state_serializer handle restoring also legacy format This ensures that even if an app attempts to restore a state saved before upgrading WebView (using the new state version) the old version will still be successfully restored. BUG=584693 ========== to ========== Make AW state_serializer handle restoring also legacy format This ensures that even if an app attempts to restore a state saved before upgrading WebView (using the new state version) the old version will still be successfully restored. BUG=584693 Committed: https://crrev.com/85ddabfdb82d0eb28addaf22f0f2c689cffeac46 Cr-Commit-Position: refs/heads/master@{#375418} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/85ddabfdb82d0eb28addaf22f0f2c689cffeac46 Cr-Commit-Position: refs/heads/master@{#375418} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
