Moved NavigationManagerImpl serialization out of CRWSessionController.

Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerStorage, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.  A factory method for these objects
  as added to WebState's public interface.
- Created NavigationManagerStorageBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerStorage.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

BUG=454984
Review-Url: https://codereview.chromium.org/1360993002
Cr-Commit-Position: refs/heads/master@{#446279}
Committed: https://chromium.googlesource.com/chromium/src/+/be48752128d7ec4a5bd6d980157deb60dbb29548

[kkhorimoto, 2015-09-23 01:17:01]

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

[kkhorimoto, 2015-09-23 01:17:01]

Addresses changes discussed with Eugene last quarter.

[Eugene But (OOO till 7-30), 2015-09-23 16:33:44]

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
File ios/web/navigation/crw_serialized_navigation_manager.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:17: @interface
CRWSerializedNavigationManager : NSObject<NSCoding>
class needs comments

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:17: @interface
CRWSerializedNavigationManager : NSObject<NSCoding>
This is not actually a manager. Maybe CRWNavigationStorage ?

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:20: @property(nonatomic,
copy) NSString* tabId;
s/tabId/tabID

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:21: @property(nonatomic,
copy) NSString* openerId;
openerID

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:35: @property(nonatomic,
retain) XCallbackParameters* xCallbackParameters;
s/xCallbackParameters/XCallbackParameters

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
File ios/web/navigation/crw_serialized_navigation_manager.mm (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:16: NSString* const
kOpenerIdKey = @"openerId";
kOpenerIDKey

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:20: NSString* const
kTabIdKey = @"tabId";
kTabIDKey

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:54: -
(instancetype)initWithCoder:(nonnull NSCoder*)aDecoder {
s/aDecoder/decoder

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:57: self.tabId =
[aDecoder decodeObjectForKey:kTabIdKey];
Please don't access properties in init

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:78:
[[CRWSessionCertificatePolicyManager alloc] init];
This is a memory leak

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:81: [[aDecoder
decodeObjectForKey:kXCallbackParametersKey] retain];
ditto

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:86: -
(void)encodeWithCoder:(NSCoder*)aCoder {
s/aCoder/coder

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

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_impl.h:65: // Returns a copy of the
NavigationManager for use in serialization.  Does not
Deep copy or shallow copy?

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
File ios/web/navigation/serialized_navigation_manager_builder.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
ios/web/navigation/serialized_navigation_manager_builder.h:11: #import
"base/mac/scoped_nsobject.h"
This include is unused

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
File ios/web/navigation/serialized_navigation_manager_builder.mm (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
ios/web/navigation/serialized_navigation_manager_builder.mm:36:
[[[CRWSerializedNavigationManager alloc] init] autorelease];
Please use scoped_nsobject instead of autorelease pool

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
ios/web/navigation/serialized_navigation_manager_builder.mm:65:
[[[CRWSessionController alloc] init] autorelease];
ditto

https://codereview.chromium.org/1360993002/diff/1/ios/web/web_state/web_state...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/web_state/web_state...
ios/web/web_state/web_state_impl.h:62: WebStateImpl(BrowserState* browser_State,
s/browser_State/browser_state/

[kkhorimoto, 2015-09-24 18:13:00]

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
File ios/web/navigation/crw_serialized_navigation_manager.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:17: @interface
CRWSerializedNavigationManager : NSObject<NSCoding>
On 2015/09/23 16:33:44, eugenebut wrote:
> This is not actually a manager. Maybe CRWNavigationStorage ?

I added the comment, but left the name the same.  The name is meant as "a
serialized NavigationManager", whereas you're reading it as "the manager of
serialized navigations".  This matches the naming convention upstream for
SerializedNavigationEntry.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:20: @property(nonatomic,
copy) NSString* tabId;
On 2015/09/23 16:33:44, eugenebut wrote:
> s/tabId/tabID

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:21: @property(nonatomic,
copy) NSString* openerId;
On 2015/09/23 16:33:43, eugenebut wrote:
> openerID

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:35: @property(nonatomic,
retain) XCallbackParameters* xCallbackParameters;
On 2015/09/23 16:33:44, eugenebut wrote:
> s/xCallbackParameters/XCallbackParameters

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
File ios/web/navigation/crw_serialized_navigation_manager.mm (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:16: NSString* const
kOpenerIdKey = @"openerId";
On 2015/09/23 16:33:44, eugenebut wrote:
> kOpenerIDKey

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:20: NSString* const
kTabIdKey = @"tabId";
On 2015/09/23 16:33:44, eugenebut wrote:
> kTabIDKey

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:54: -
(instancetype)initWithCoder:(nonnull NSCoder*)aDecoder {
On 2015/09/23 16:33:44, eugenebut wrote:
> s/aDecoder/decoder

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:57: self.tabId =
[aDecoder decodeObjectForKey:kTabIdKey];
On 2015/09/23 16:33:44, eugenebut wrote:
> Please don't access properties in init

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:78:
[[CRWSessionCertificatePolicyManager alloc] init];
On 2015/09/23 16:33:44, eugenebut wrote:
> This is a memory leak

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:81: [[aDecoder
decodeObjectForKey:kXCallbackParametersKey] retain];
On 2015/09/23 16:33:44, eugenebut wrote:
> ditto

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.mm:86: -
(void)encodeWithCoder:(NSCoder*)aCoder {
On 2015/09/23 16:33:44, eugenebut wrote:
> s/aCoder/coder

Done.

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

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_impl.h:65: // Returns a copy of the
NavigationManager for use in serialization.  Does not
On 2015/09/23 16:33:44, eugenebut wrote:
> Deep copy or shallow copy?

Deep. I've added a comment.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
File ios/web/navigation/serialized_navigation_manager_builder.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
ios/web/navigation/serialized_navigation_manager_builder.h:11: #import
"base/mac/scoped_nsobject.h"
On 2015/09/23 16:33:44, eugenebut wrote:
> This include is unused

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
File ios/web/navigation/serialized_navigation_manager_builder.mm (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
ios/web/navigation/serialized_navigation_manager_builder.mm:36:
[[[CRWSerializedNavigationManager alloc] init] autorelease];
On 2015/09/23 16:33:44, eugenebut wrote:
> Please use scoped_nsobject instead of autorelease pool

Why?  I've created functions that returned scoped_nsobjects before that you've
made me change to return autoreleased objects.  I like that scoped nsobjects
explicitly transfer ownership of the objects, but it seems less idiomatic.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
ios/web/navigation/serialized_navigation_manager_builder.mm:65:
[[[CRWSessionController alloc] init] autorelease];
On 2015/09/23 16:33:44, eugenebut wrote:
> ditto

Done.

https://codereview.chromium.org/1360993002/diff/1/ios/web/web_state/web_state...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/web_state/web_state...
ios/web/web_state/web_state_impl.h:62: WebStateImpl(BrowserState* browser_State,
On 2015/09/23 16:33:44, eugenebut wrote:
> s/browser_State/browser_state/

Done.

[Eugene But (OOO till 7-30), 2015-09-24 18:44:29]

lgtm I will defer naming question to Stuart

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
File ios/web/navigation/crw_serialized_navigation_manager.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:17: @interface
CRWSerializedNavigationManager : NSObject<NSCoding>
On 2015/09/24 18:12:59, kkhorimoto_ wrote:
> On 2015/09/23 16:33:44, eugenebut wrote:
> > This is not actually a manager. Maybe CRWNavigationStorage ?
> 
> I added the comment, but left the name the same.  The name is meant as "a
> serialized NavigationManager", whereas you're reading it as "the manager of
> serialized navigations".  This matches the naming convention upstream for
> SerializedNavigationEntry.
SerializedNavigationEntry is unambiguous name. But many people will still read
CRWSerializedNavigationManager as "the manager of serialized navigations". I'm
strongly in favor for not using Manager suffix for storage classes, and can
suggest something like CRWNavigationManagerData. I think that strategy to match
content naming pattern *sometimes* leads to misleading naming. Stuart will be in
a position to make a decision on this.

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
File ios/web/navigation/serialized_navigation_manager_builder.mm (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/serializ...
ios/web/navigation/serialized_navigation_manager_builder.mm:36:
[[[CRWSerializedNavigationManager alloc] init] autorelease];
On 2015/09/24 18:12:59, kkhorimoto_ wrote:
> On 2015/09/23 16:33:44, eugenebut wrote:
> > Please use scoped_nsobject instead of autorelease pool
> 
> Why?  I've created functions that returned scoped_nsobjects before that you've
> made me change to return autoreleased objects.  I like that scoped nsobjects
> explicitly transfer ownership of the objects, but it seems less idiomatic.
I apologies, I missed that fact that you return serialized_navigation_manager.
For some reason I thought it's a temp object.

https://codereview.chromium.org/1360993002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_serialized_navigation_manager.h (right):

https://codereview.chromium.org/1360993002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_serialized_navigation_manager.h:21: // Properties taken
from NavigationManagerImpl and CRWSessionController.
Do you want to mention CRWSessionController here?

https://codereview.chromium.org/1360993002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_serialized_navigation_manager.h:25: @property(nonatomic,
assign) NSInteger openerNavigationIndex;
This property is encoded/decoded as int. NavigationManager uses int as well. So
maybe this property should also be int afterwards?

https://codereview.chromium.org/1360993002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_serialized_navigation_manager.h:27: @property(nonatomic,
assign) NSInteger currentNavigationIndex;
ditto

https://codereview.chromium.org/1360993002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_serialized_navigation_manager.h:28: @property(nonatomic,
assign) NSInteger previousNavigationIndex;
ditto

[stuartmorgan, 2015-09-28 22:27:33]

lgtm

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
File ios/web/navigation/crw_serialized_navigation_manager.h (right):

https://codereview.chromium.org/1360993002/diff/1/ios/web/navigation/crw_seri...
ios/web/navigation/crw_serialized_navigation_manager.h:17: @interface
CRWSerializedNavigationManager : NSObject<NSCoding>
On 2015/09/24 18:44:29, eugenebut wrote:
> On 2015/09/24 18:12:59, kkhorimoto_ wrote:
> > On 2015/09/23 16:33:44, eugenebut wrote:
> > > This is not actually a manager. Maybe CRWNavigationStorage ?
> > 
> > I added the comment, but left the name the same.  The name is meant as "a
> > serialized NavigationManager", whereas you're reading it as "the manager of
> > serialized navigations".  This matches the naming convention upstream for
> > SerializedNavigationEntry.
> SerializedNavigationEntry is unambiguous name. But many people will still read
> CRWSerializedNavigationManager as "the manager of serialized navigations". I'm
> strongly in favor for not using Manager suffix for storage classes, and can
> suggest something like CRWNavigationManagerData. I think that strategy to
match
> content naming pattern *sometimes* leads to misleading naming. Stuart will be
in
> a position to make a decision on this.

I think Eugene's point is a good one. For instance, there's a
SerializedNavigationBuilder elsewhere in the codebase which is in fact a builder
of serialized navigations, rather than a serialization of navigation builders. I
think just Data loses some of the clarity of its purpose though; how about
CRWNavigationManagerSerialization?

[kkhorimoto, 2017-01-25 06:55:18]

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

[commit-bot: I haz the power, 2017-01-25 06:55:31]

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-25 07:01:00]

Description was changed from

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWSerializedNavigationManager, which holds all the data that
  is persisted for a single NavigationManagerImpl.  It conforms to
  NSCoding and is responsible for encoding sessions.
- Created SerializedNavigationManagerBuilder, which contains functions
  to convert between NavigationManagerImpls and
  CRWSerializedNavigationManagers.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed NavigationManagerImpls upon
  construction when restoring state.

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

to

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerSerialization, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.
- Created NavigationManagerSerializationBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerSerializations.  It is exposed in the public WebState
  interface and can be used by chrome// to create serializable sessions.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

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

[kkhorimoto, 2017-01-25 07:01:00]

kkhorimoto@chromium.org changed reviewers:
- stuartmorgan@chromium.org

[kkhorimoto, 2017-01-25 07:01:54]

PTAL, I've updated this CL to include changes that were previously downstream. 
I've changed the implementation a bit, but the original downstream CL can be
found here for reference: https://chromereviews.googleplex.com/211487013/

[commit-bot: I haz the power, 2017-01-25 07:03:09]

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

[commit-bot: I haz the power, 2017-01-25 07:03:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kkhorimoto, 2017-01-25 07:59:51]

test fixes, move builder to implementation detail

[kkhorimoto, 2017-01-25 08:00:05]

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

[commit-bot: I haz the power, 2017-01-25 08:00:35]

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-25 08:02:22]

Description was changed from

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerSerialization, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.
- Created NavigationManagerSerializationBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerSerializations.  It is exposed in the public WebState
  interface and can be used by chrome// to create serializable sessions.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

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

to

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerSerialization, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.  A factory method for these objects
  as added to WebState's public interface.
- Created NavigationManagerSerializationBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerSerializations.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

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

[commit-bot: I haz the power, 2017-01-25 08:08:56]

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

[commit-bot: I haz the power, 2017-01-25 08:08:57]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[kkhorimoto, 2017-01-25 08:12:33]

cleanup

[kkhorimoto, 2017-01-25 08:17:14]

test fix

[kkhorimoto, 2017-01-25 08:17:36]

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

[commit-bot: I haz the power, 2017-01-25 08:17:51]

Dry run: 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-01-25 09:26:11]

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

[commit-bot: I haz the power, 2017-01-25 09:26:12]

Dry run: This issue passed the CQ dry run.

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

Is this a temporary incremental change or the final design? If latter then I
think we should understand how serialization works in content and see if their
approach should be followed on iOS.

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/crw_nav...
File ios/web/public/crw_navigation_manager_serialization.h (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/crw_nav...
ios/web/public/crw_navigation_manager_serialization.h:14: @interface
CRWNavigationManagerSerialization : NSObject<NSCoding>
Is there //content analogue for this public class?

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/web_sta...
File ios/web/public/web_state/web_state.h (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/web_sta...
ios/web/public/web_state/web_state.h:63: CRWNavigationManagerSerialization*
serialization);
Content does not have serialized state in CreateParams. Do we have a good reason
to diverge from content? I noticed that they have
WebContents::CreateWithSessionStorage. Do you think CreateWithSessionStorage is
something that we should use on iOS?

Serialization seems like an area where diverging from content is unnecessary.

[Eugene But (OOO till 7-30), 2017-01-25 19:24:30]

Thanks for cleanup! lgtm

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/session_window.mm (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/session_window.mm:13: #include
"base/mac/scoped_nsobject.h"
nit: s/include/import

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/session_window_unittest.mm (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/session_window_unittest.mm:111: EXPECT_EQ(2U,
sessions.count);
nit: ASSERT_EQ? So sessions[0] does not crash if empty.

https://codereview.chromium.org/1360993002/diff/100001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:208: CRWNavigationManagerSerialization*
BuildSerializedNavigationManager()
Is this analogue of WebContentsImpl::GetSessionStorageNamespaceMap?

https://codereview.chromium.org/1360993002/diff/100001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:357: NavigationManagerSerializationBuilder
serialization_builder_;
Do we need this to be a member? Should this be a local variable instead?

[kkhorimoto, 2017-01-26 07:19:10]

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/session_window.mm (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/session_window.mm:13: #include
"base/mac/scoped_nsobject.h"
On 2017/01/25 19:24:29, Eugene But wrote:
> nit: s/include/import

Done.

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
File ios/chrome/browser/sessions/session_window_unittest.mm (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/chrome/browser/ses...
ios/chrome/browser/sessions/session_window_unittest.mm:111: EXPECT_EQ(2U,
sessions.count);
On 2017/01/25 19:24:29, Eugene But wrote:
> nit: ASSERT_EQ? So sessions[0] does not crash if empty.

Done.

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/crw_nav...
File ios/web/public/crw_navigation_manager_serialization.h (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/crw_nav...
ios/web/public/crw_navigation_manager_serialization.h:14: @interface
CRWNavigationManagerSerialization : NSObject<NSCoding>
On 2017/01/25 16:03:38, Eugene But wrote:
> Is there //content analogue for this public class?

It looks like content uses SessionTab as an analogous class.  I've added a TODO
to investigate folding our iOS sessions code into the upstream sessions
component so we can reuse some code.

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/web_sta...
File ios/web/public/web_state/web_state.h (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/web/public/web_sta...
ios/web/public/web_state/web_state.h:63: CRWNavigationManagerSerialization*
serialization);
On 2017/01/25 16:03:38, Eugene But wrote:
> Content does not have serialized state in CreateParams. Do we have a good
reason
> to diverge from content? I noticed that they have
> WebContents::CreateWithSessionStorage. Do you think CreateWithSessionStorage
is
> something that we should use on iOS?
> 
> Serialization seems like an area where diverging from content is unnecessary.

Done.

https://codereview.chromium.org/1360993002/diff/100001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1360993002/diff/100001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:208: CRWNavigationManagerSerialization*
BuildSerializedNavigationManager()
On 2017/01/25 19:24:30, Eugene But wrote:
> Is this analogue of WebContentsImpl::GetSessionStorageNamespaceMap?

Yes

https://codereview.chromium.org/1360993002/diff/100001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.h:357: NavigationManagerSerializationBuilder
serialization_builder_;
On 2017/01/25 19:24:30, Eugene But wrote:
> Do we need this to be a member? Should this be a local variable instead?

Done.

[kkhorimoto, 2017-01-26 07:19:49]

Description was changed from

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerSerialization, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.  A factory method for these objects
  as added to WebState's public interface.
- Created NavigationManagerSerializationBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerSerializations.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

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

to

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerStorage, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.  A factory method for these objects
  as added to WebState's public interface.
- Created NavigationManagerStorageBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerStorage.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

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

[kkhorimoto, 2017-01-26 07:20:01]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-01-26 07:20:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
stuartmorgan@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/1360993002/#ps120001
(title: "eugene's comments")

[commit-bot: I haz the power, 2017-01-26 07:20:23]

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-01-26 07:22:07]

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

[commit-bot: I haz the power, 2017-01-26 07:22:09]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kkhorimoto, 2017-01-26 07:27:11]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-01-26 07:27:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
stuartmorgan@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/1360993002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-26 07:27:30]

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-01-26 07:34:50]

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

[commit-bot: I haz the power, 2017-01-26 07:34:51]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kkhorimoto, 2017-01-26 07:43:24]

compilation fix after rebase

[kkhorimoto, 2017-01-26 07:44:29]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-01-26 07:44:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
stuartmorgan@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/1360993002/#ps160001
(title: "compilation fix after rebase")

[commit-bot: I haz the power, 2017-01-26 07:44:39]

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-01-26 08:50:53]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485416669947810,
"parent_rev": "6f956feb2af23b688f12dd8e4a8d656be803b437", "commit_rev":
"be48752128d7ec4a5bd6d980157deb60dbb29548"}

[commit-bot: I haz the power, 2017-01-26 08:51:30]

Description was changed from

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerStorage, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.  A factory method for these objects
  as added to WebState's public interface.
- Created NavigationManagerStorageBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerStorage.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

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

to

==========
Moved NavigationManagerImpl serialization out of CRWSessionController.

- Created CRWNavigationManagerStorage, which holds all the data that
  is persisted for a single NavigationManager.  It conforms to NSCoding and
  is responsible for encoding sessions.  A factory method for these objects
  as added to WebState's public interface.
- Created NavigationManagerStorageBuilder, which contains functions
  to convert between NavigationManagers and
  CRWNavigationManagerStorage.
- Changed the relationship between WebStateImpls and
  NavigationManagerImpls.  Previously, WebStateImpls created empty
  NavigationManagerImpls and injected deserialized CRWSessionControllers
  into them.  Now, WebStateImpls are passed serialized NavigationManagers
  upon construction when restoring state.
- SessionWindowIOS's interface is updated to use NavigationManager
  serializations rather than WebStateImpls, and SessionWindowIOS clients in
  chrome// can use these to create WebStates using CreateParams.

BUG=454984

Review-Url: https://codereview.chromium.org/1360993002
Cr-Commit-Position: refs/heads/master@{#446279}
Committed:
https://chromium.googlesource.com/chromium/src/+/be48752128d7ec4a5bd6d980157d...
==========

[commit-bot: I haz the power, 2017-01-26 08:51:31]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/be48752128d7ec4a5bd6d980157d...

[gambard, 2017-01-26 10:22:51]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2655253002/ by gambard@chromium.org.

The reason for reverting is: Lot of tests are failing. It seems that
sequence_checker_.CalledOnValidSequence() returns false in those cases.
Example:
https://build.chromium.org/p/chromium.fyi/builders/EarlGreyiOS/builds/23584/s....