Moved serialization out of CRWSessionEntry.

Moved serialization out of CRWSessionEntry.

This CL creates CRWNavigationItemStorage and its associated builder
class, which handles persisting serialized NavigationItem state.

BUG=454984, 619992
Review-Url: https://codereview.chromium.org/2664113003
Cr-Commit-Position: refs/heads/master@{#448063}
Committed: https://chromium.googlesource.com/chromium/src/+/78f08f6f78417131794d7befbf1ecc97eac7877c

[kkhorimoto, 2017-01-31 20:54:17]

The CQ bit was checked by kkhorimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 20:54:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kkhorimoto, 2017-01-31 20:59:52]

kkhorimoto@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-01-31 22:12:26]

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service.mm (left):

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service.mm:23: #import
"ios/web/navigation/crw_session_controller.h"
Could you please cleanup ios/chrome/browser/sessions/DEPS from exceptions and
add crbug.com/619992 to BUG= line.

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service.mm:221:
forClassName:@"CRWSessionEntry"];
Should we use CRWNavigationItemStorage key instead?

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/crw_navi...
File ios/web/navigation/crw_navigation_item_storage_unittest.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/crw_navi...
ios/web/navigation/crw_navigation_item_storage_unittest.mm:14: #import
"ios/testing/ocmock_complex_type_helper.h"
This test tests interaction (that specific methods were called), while it is
always better to test state.
This thread contains different discussion which suggest that in Chromium we
should test state instead of integration:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-KH_IP0rIWQ...

a good test for this class could be:
- assign values to all properties
- call encodeWithCoder:
- create a new object with initWithCoder:
- compare properties

for legacy structure you can:
- construct correct NSCoder object in old format
- create a new object with initWithCoder:
- verify properties

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/crw_navi...
ios/web/navigation/crw_navigation_item_storage_unittest.mm:28: // Set up
|item_storage_|.
Is it possible to move this code to constructor? It's quite easy to forget
super::SetUp() call.

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_item_impl.h (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_impl.h:147: friend
NavigationItemStorageBuilder;
nit: Do you want to move this to the top (before all ivars)?

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_item_storage_builder.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_storage_builder.mm:3: // found in the LICENSE
file.
Could you please add a unit test for this class.

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
File ios/web/public/crw_navigation_item_storage.h (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.h:72: // Exposes serialization helper
functions for tests.
From Google Testing Dev Guide: "Test code using real APIs - don’t define back
doors only for tests."
goto.google.com/unit-test-practices?cl=head#public-apis

It's quite easy to avoid using private API if tests test state, not interaction.

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
File ios/web/public/crw_navigation_item_storage.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.mm:7: #import
"base/strings/sys_string_conversions.h"
nit: s/import/include

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.mm:117: return [object class] ==
[self class] && [object hash] == [self hash];
Equality of hashes does not mean objects equality, so this will not always work.
Is this method used outside of tests? If no, can we instead create a helper test
only function?

[commit-bot: I haz the power, 2017-01-31 23:47:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 23:47:01]

Dry run: This issue passed the CQ dry run.

[kkhorimoto, 2017-02-03 00:35:52]

Description was changed from

==========
Moved serialization out of CRWSessionEntry.

This CL creates CRWNavigationItemStorage and its associated builder
class, which handles persisting serialized NavigationItem state.

BUG=454984
==========

to

==========
Moved serialization out of CRWSessionEntry.

This CL creates CRWNavigationItemStorage and its associated builder
class, which handles persisting serialized NavigationItem state.

BUG=454984, 619992
==========

[kkhorimoto, 2017-02-03 00:36:09]

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service.mm (left):

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service.mm:23: #import
"ios/web/navigation/crw_session_controller.h"
On 2017/01/31 22:12:26, Eugene But wrote:
> Could you please cleanup ios/chrome/browser/sessions/DEPS from exceptions and
> add crbug.com/619992 to BUG= line.

Done.

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
File ios/chrome/browser/sessions/session_service.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/chrome/browser/sessions...
ios/chrome/browser/sessions/session_service.mm:221:
forClassName:@"CRWSessionEntry"];
On 2017/01/31 22:12:26, Eugene But wrote:
> Should we use CRWNavigationItemStorage key instead?

The unarchiver already knows to use CRWNavigationItemStorage's |-initWithCoder:|
when it attempts to deserialize objects with that class name.  This is just
added for backward compatibility so that if the unarchiver sees a serialized
CRWSessionEntry, it knows to use CRWNavigationItemStorage.

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/crw_navi...
File ios/web/navigation/crw_navigation_item_storage_unittest.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/crw_navi...
ios/web/navigation/crw_navigation_item_storage_unittest.mm:14: #import
"ios/testing/ocmock_complex_type_helper.h"
On 2017/01/31 22:12:26, Eugene But wrote:
> This test tests interaction (that specific methods were called), while it is
> always better to test state.
> This thread contains different discussion which suggest that in Chromium we
> should test state instead of integration:
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-KH_IP0rIWQ...
> 
> a good test for this class could be:
> - assign values to all properties
> - call encodeWithCoder:
> - create a new object with initWithCoder:
> - compare properties
> 
> for legacy structure you can:
> - construct correct NSCoder object in old format
> - create a new object with initWithCoder:
> - verify properties
> 
>  

Done.

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/crw_navi...
ios/web/navigation/crw_navigation_item_storage_unittest.mm:28: // Set up
|item_storage_|.
On 2017/01/31 22:12:26, Eugene But wrote:
> Is it possible to move this code to constructor? It's quite easy to forget
> super::SetUp() call.

Done.

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_item_impl.h (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_item_impl.h:147: friend
NavigationItemStorageBuilder;
On 2017/01/31 22:12:26, Eugene But wrote:
> nit: Do you want to move this to the top (before all ivars)?

Done.

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
File ios/web/public/crw_navigation_item_storage.h (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.h:72: // Exposes serialization helper
functions for tests.
On 2017/01/31 22:12:26, Eugene But wrote:
> From Google Testing Dev Guide: "Test code using real APIs - don’t define back
> doors only for tests."
> goto.google.com/unit-test-practices?cl=head#public-apis
> 
> It's quite easy to avoid using private API if tests test state, not
interaction.

Helpers moved to PageDisplayState.

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
File ios/web/public/crw_navigation_item_storage.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.mm:7: #import
"base/strings/sys_string_conversions.h"
On 2017/01/31 22:12:26, Eugene But wrote:
> nit: s/import/include

Done.

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.mm:117: return [object class] ==
[self class] && [object hash] == [self hash];
On 2017/01/31 22:12:26, Eugene But wrote:
> Equality of hashes does not mean objects equality, so this will not always
work.
> Is this method used outside of tests? If no, can we instead create a helper
test
> only function?

Fixed.

[Eugene But (OOO till 7-30), 2017-02-03 01:52:34]

Thanks for updating the tests!

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
File ios/web/public/crw_navigation_item_storage.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.mm:117: return [object class] ==
[self class] && [object hash] == [self hash];
On 2017/02/03 00:36:09, kkhorimoto_ wrote:
> On 2017/01/31 22:12:26, Eugene But wrote:
> > Equality of hashes does not mean objects equality, so this will not always
> work.
> > Is this method used outside of tests? If no, can we instead create a helper
> test
> > only function?
> 
> Fixed.
So the other question was: "Is this method used outside of tests?". If no then
it is better to have comparison method in test itself and avoid shipping test
code to the users and implementing |hash| (which is quite tricky to do
correctly).

https://codereview.chromium.org/2664113003/diff/60001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_storage_builder.mm (right):

https://codereview.chromium.org/2664113003/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:55: NSMutableArray*
itemStorages = [[NSMutableArray alloc] init];
s/itemStorages/item_storages

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
File ios/web/public/web_state/page_display_state.h (right):

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
ios/web/public/web_state/page_display_state.h:8: #import
<Foundation/Foundation.h>
This changes the header to Objective-C, so please s/include/import where it was
included.

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
ios/web/public/web_state/page_display_state.h:35: static NSString* XOffsetKey();
Should these keys be constants, like all other keys?

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
ios/web/public/web_state/page_display_state.h:134: NSDictionary*
GetSerialization() const;
Could you please add tests for these new functions. I think simple
GetSerialization -> Construct -> Compare is sufficient.

[Eugene But (OOO till 7-30), 2017-02-03 01:57:16]

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
File ios/web/public/crw_navigation_item_storage.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.mm:117: return [object class] ==
[self class] && [object hash] == [self hash];
On 2017/02/03 01:52:34, Eugene But wrote:
> On 2017/02/03 00:36:09, kkhorimoto_ wrote:
> > On 2017/01/31 22:12:26, Eugene But wrote:
> > > Equality of hashes does not mean objects equality, so this will not always
> > work.
> > > Is this method used outside of tests? If no, can we instead create a
helper
> > test
> > > only function?
> > 
> > Fixed.
> So the other question was: "Is this method used outside of tests?". If no then
> it is better to have comparison method in test itself and avoid shipping test
> code to the users and implementing |hash| (which is quite tricky to do
> correctly).
The other reason for not using |hash| and |isEqual:| only for tests is that
those method don't have tests and we can not be sure that they will not break.
And if you write tests for isEqual and hash: then it's much more work and code
to maintain.

[kkhorimoto, 2017-02-03 02:47:23]

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
File ios/web/public/crw_navigation_item_storage.mm (right):

https://codereview.chromium.org/2664113003/diff/1/ios/web/public/crw_navigati...
ios/web/public/crw_navigation_item_storage.mm:117: return [object class] ==
[self class] && [object hash] == [self hash];
On 2017/02/03 01:57:16, Eugene But wrote:
> On 2017/02/03 01:52:34, Eugene But wrote:
> > On 2017/02/03 00:36:09, kkhorimoto_ wrote:
> > > On 2017/01/31 22:12:26, Eugene But wrote:
> > > > Equality of hashes does not mean objects equality, so this will not
always
> > > work.
> > > > Is this method used outside of tests? If no, can we instead create a
> helper
> > > test
> > > > only function?
> > > 
> > > Fixed.
> > So the other question was: "Is this method used outside of tests?". If no
then
> > it is better to have comparison method in test itself and avoid shipping
test
> > code to the users and implementing |hash| (which is quite tricky to do
> > correctly).
> The other reason for not using |hash| and |isEqual:| only for tests is that
> those method don't have tests and we can not be sure that they will not break.
> And if you write tests for isEqual and hash: then it's much more work and code
> to maintain.

Done.

https://codereview.chromium.org/2664113003/diff/60001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_storage_builder.mm (right):

https://codereview.chromium.org/2664113003/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:55: NSMutableArray*
itemStorages = [[NSMutableArray alloc] init];
On 2017/02/03 01:52:34, Eugene But wrote:
> s/itemStorages/item_storages

Done.

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
File ios/web/public/web_state/page_display_state.h (right):

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
ios/web/public/web_state/page_display_state.h:8: #import
<Foundation/Foundation.h>
On 2017/02/03 01:52:34, Eugene But wrote:
> This changes the header to Objective-C, so please s/include/import where it
was
> included.

Done.

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
ios/web/public/web_state/page_display_state.h:35: static NSString* XOffsetKey();
On 2017/02/03 01:52:34, Eugene But wrote:
> Should these keys be constants, like all other keys?

Done.

https://codereview.chromium.org/2664113003/diff/60001/ios/web/public/web_stat...
ios/web/public/web_state/page_display_state.h:134: NSDictionary*
GetSerialization() const;
On 2017/02/03 01:52:34, Eugene But wrote:
> Could you please add tests for these new functions. I think simple
> GetSerialization -> Construct -> Compare is sufficient.

Done.

[Eugene But (OOO till 7-30), 2017-02-03 18:48:13]

lgtm! Thanks for the tests

[kkhorimoto, 2017-02-03 19:20:57]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-03 19:21:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 20:43:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486149657907860,
"parent_rev": "64e7b7c59fe3d4933e2922bbcf1ac72d9ee88713", "commit_rev":
"78f08f6f78417131794d7befbf1ecc97eac7877c"}

[commit-bot: I haz the power, 2017-02-03 20:44:16]

Description was changed from

==========
Moved serialization out of CRWSessionEntry.

This CL creates CRWNavigationItemStorage and its associated builder
class, which handles persisting serialized NavigationItem state.

BUG=454984, 619992
==========

to

==========
Moved serialization out of CRWSessionEntry.

This CL creates CRWNavigationItemStorage and its associated builder
class, which handles persisting serialized NavigationItem state.

BUG=454984, 619992

Review-Url: https://codereview.chromium.org/2664113003
Cr-Commit-Position: refs/heads/master@{#448063}
Committed:
https://chromium.googlesource.com/chromium/src/+/78f08f6f78417131794d7befbf1e...
==========

[commit-bot: I haz the power, 2017-02-03 20:44:18]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/78f08f6f78417131794d7befbf1e...

[justincohen, 2017-02-09 19:08:25]

justincohen@chromium.org changed reviewers:
+ justincohen@chromium.org

[justincohen, 2017-02-09 19:08:26]

This breaks the NTP, see crbug.com/689358