[ios] Add free function to (de)serialise WebStateList.

[ios] Add free function to (de)serialise WebStateList.

BUG=546222
Review-Url: https://codereview.chromium.org/2771863002
Cr-Commit-Position: refs/heads/master@{#462043}
Committed: https://chromium.googlesource.com/chromium/src/+/08c0ab4e786593dd8d91407165da85d923d0179b

[sdefresne, 2017-03-23 17:01:35]

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

[commit-bot: I haz the power, 2017-03-23 17:02:21]

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

[sdefresne, 2017-03-23 17:02:31]

sdefresne@chromium.org changed reviewers:
+ marq@chromium.org

[sdefresne, 2017-03-23 17:02:32]

Please take a look.

[commit-bot: I haz the power, 2017-03-23 18:11:34]

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

[commit-bot: I haz the power, 2017-03-23 18:11:35]

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...)

[marq (ping after 24h), 2017-03-24 10:37:18]

https://codereview.chromium.org/2771863002/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2771863002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:708: DCHECK_GT(_webStateList->count(),
oldCount);
restoredCount must be >= 0, so can be 0.
Presumably _webStateList->count() is expected to be oldCount + restoredCount? So
this could be 0, which isn't > oldCount?

(maybe line 706 should be GT and not GE?)

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list_serialisation.h (right):

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:10: #import
"ios/web/public/web_state/web_state.h"
I'm sure you've explained this already, but assume I have no useful attention
span. Why can't these be WebStateList methods? I don't see any problematic
dependencies.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:21: // number of
WebStates restored.
Prefer comments that explicitly name parameters:

// Restores |sessions| into |web_state_list| ...

Please add a comment explaining what |param| does.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:23: int
DeserializeWebStateListFrom(WebStateList* web_state_list,
Is the return value just a convenience, or is it actually needed? That is, can a
caller assume that the return value should always equal the different between
|web_state_list|'s count() before and after calling this function?

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:23: int
DeserializeWebStateListFrom(WebStateList* web_state_list,
Naming nit: If you're going to use a preposition at the end of the function name
(such as "From"), then the referent of that preposition should be the first
parameter. Right now this reads to me as "Deserialize a WebStateList from
|web_state_list_, also using |param| and |sessions|". 

So either drop the "From", or move |sessions| to be the first parameter.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm (right):

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:28: // before the
M-60 release. Remove once M-70 has shipped.
I'd prefer a bug to track this rather than milestones that are pretty much
guaranteed to be wrong.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:33: struct
NSStringHash {
You're using ~15 lines of helpers here to enable using NSStrings as keys to an
unordered_map with webState pointers as values.

Why not invert the solution and use an NSMutableDictionary, storing the webState
pointers using [NSValue valueWithPointer:]? That seems less intrusive.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:49: // Returns
the value bound to |key| casted to type T. If the value is missing
casted -> cast. Yay, English!

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:52: T*
GetValueForSerialisationKey(web::WebState* web_state, NSString* key) {
Is using the same name as a web::SerializableUserDataManager intentional? Is it
potentially confusing?

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:59: //
compatibility code to load sessions saved with M-59 or older; remove this
Same comment wrt milestones.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:64: bool
pre_m60_saved_session = false;
legacy_saved_session?

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:68:
web::SerializableUserDataManager::FromWebState(web_state);
Doesn't your helper method on line 52 do this for you?

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:77: if
(pre_m60_saved_session) {
This is a 90-line method, so maybe extract the legacy deserialization into its
own function.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:89: // The value
may be missing during tests, skip object.
Is this conditional *only* to support tests? If so, we need better tests.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:94:
id_to_web_state.insert(std::make_pair(object_id, web_state));
Any reason why the (to me) more readable

 id_to_web_state[object_id] = web_state;

isn't preferred? (this is more of a C++ question than a review comment)

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:112: continue;
Also continue if the opener nav index < 0?

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:186: return
serialized_session;
[serialized_session copy] since the return value is immutable.

[sdefresne, 2017-03-27 16:14:45]

PTAL

https://codereview.chromium.org/2771863002/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2771863002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:708: DCHECK_GT(_webStateList->count(),
oldCount);
On 2017/03/24 10:37:17, marq wrote:
> restoredCount must be >= 0, so can be 0.
> Presumably _webStateList->count() is expected to be oldCount + restoredCount?
So
> this could be 0, which isn't > oldCount?
> 
> (maybe line 706 should be GT and not GE?)

The method early return (see line 696) if sessions.count == 0, so the
restoredCount must always be > 0, changed the DCHECK_GE to DCHECK_GT instead.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list_serialisation.h (right):

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:10: #import
"ios/web/public/web_state/web_state.h"
On 2017/03/24 10:37:17, marq wrote:
> I'm sure you've explained this already, but assume I have no useful attention
> span. Why can't these be WebStateList methods? I don't see any problematic
> dependencies.

Two reasons:
1. on desktop, those are not methods of TabStripModel,
2. they can be implemented as separate functions.

Creating this as separate function also allow me to test them with test
implementation of WebStateList.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:21: // number of
WebStates restored.
On 2017/03/24 10:37:17, marq wrote:
> Prefer comments that explicitly name parameters:
> 
> // Restores |sessions| into |web_state_list| ...
> 
> Please add a comment explaining what |param| does.

Done.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:23: int
DeserializeWebStateListFrom(WebStateList* web_state_list,
On 2017/03/24 10:37:17, marq wrote:
> Is the return value just a convenience, or is it actually needed? That is, can
a
> caller assume that the return value should always equal the different between
> |web_state_list|'s count() before and after calling this function?

For convenience. Removed.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:23: int
DeserializeWebStateListFrom(WebStateList* web_state_list,
On 2017/03/24 10:37:17, marq wrote:
> Naming nit: If you're going to use a preposition at the end of the function
name
> (such as "From"), then the referent of that preposition should be the first
> parameter. Right now this reads to me as "Deserialize a WebStateList from
> |web_state_list_, also using |param| and |sessions|". 
> 
> So either drop the "From", or move |sessions| to be the first parameter.

Dropped "From".

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm (right):

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:28: // before the
M-60 release. Remove once M-70 has shipped.
On 2017/03/24 10:37:18, marq wrote:
> I'd prefer a bug to track this rather than milestones that are pretty much
> guaranteed to be wrong.

Done.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:33: struct
NSStringHash {
On 2017/03/24 10:37:17, marq wrote:
> You're using ~15 lines of helpers here to enable using NSStrings as keys to an
> unordered_map with webState pointers as values.
> 
> Why not invert the solution and use an NSMutableDictionary, storing the
webState
> pointers using [NSValue valueWithPointer:]? That seems less intrusive.

Done.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:49: // Returns
the value bound to |key| casted to type T. If the value is missing
On 2017/03/24 10:37:17, marq wrote:
> casted -> cast. Yay, English!

Function was never used, removed.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:52: T*
GetValueForSerialisationKey(web::WebState* web_state, NSString* key) {
On 2017/03/24 10:37:18, marq wrote:
> Is using the same name as a web::SerializableUserDataManager intentional? Is
it
> potentially confusing?

It was intentional. I've removed as it is not used.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:59: //
compatibility code to load sessions saved with M-59 or older; remove this
On 2017/03/24 10:37:18, marq wrote:
> Same comment wrt milestones.

Done.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:64: bool
pre_m60_saved_session = false;
On 2017/03/24 10:37:17, marq wrote:
> legacy_saved_session?

Ack.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:68:
web::SerializableUserDataManager::FromWebState(web_state);
On 2017/03/24 10:37:17, marq wrote:
> Doesn't your helper method on line 52 do this for you?

I've removed the method.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:77: if
(pre_m60_saved_session) {
On 2017/03/24 10:37:17, marq wrote:
> This is a 90-line method, so maybe extract the legacy deserialization into its
> own function.

Done.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:89: // The value
may be missing during tests, skip object.
On 2017/03/24 10:37:18, marq wrote:
> Is this conditional *only* to support tests? If so, we need better tests.

Removed the comment as this is also to support "corrupted" saved state.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:94:
id_to_web_state.insert(std::make_pair(object_id, web_state));
On 2017/03/24 10:37:17, marq wrote:
> Any reason why the (to me) more readable
> 
>  id_to_web_state[object_id] = web_state;
> 
> isn't preferred? (this is more of a C++ question than a review comment)

operator [] requires the value to be copyable, default constructible, and, if
used on a const map,  copy constructible. This mean that using
"id_to_web_state[object_id] = web_state;" would cause an object to be default
constructed (to return a reference to it) then the assignment operator would be
called. The map value were pointers, so this is okay (default construction of a
pointer set it to nullptr), but I just got used to using "insert" method instead
to avoid those unnecessary method calls.

I've changed the code to use NSMutableDictionary, so this is no longer relevant.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:112: continue;
On 2017/03/24 10:37:17, marq wrote:
> Also continue if the opener nav index < 0?

No, negative value is used during the tests.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.mm:186: return
serialized_session;
On 2017/03/24 10:37:17, marq wrote:
> [serialized_session copy] since the return value is immutable.

Done.

[marq (ping after 24h), 2017-03-28 15:47:24]

lgtm

https://codereview.chromium.org/2771863002/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2771863002/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:708: DCHECK_GT(_webStateList->count(),
oldCount);
On 2017/03/27 16:14:44, sdefresne wrote:
> On 2017/03/24 10:37:17, marq wrote:
> > restoredCount must be >= 0, so can be 0.
> > Presumably _webStateList->count() is expected to be oldCount +
restoredCount?
> So
> > this could be 0, which isn't > oldCount?
> > 
> > (maybe line 706 should be GT and not GE?)
> 
> The method early return (see line 696) if sessions.count == 0, so the
> restoredCount must always be > 0, changed the DCHECK_GE to DCHECK_GT instead.

Thanks -- a comment saying that because of line 696 we know its > 0 would also
have been fine.

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list_serialisation.h (right):

https://codereview.chromium.org/2771863002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_serialisation.h:10: #import
"ios/web/public/web_state/web_state.h"
On 2017/03/27 16:14:44, sdefresne wrote:
> On 2017/03/24 10:37:17, marq wrote:
> > I'm sure you've explained this already, but assume I have no useful
attention
> > span. Why can't these be WebStateList methods? I don't see any problematic
> > dependencies.
> 
> Two reasons:
> 1. on desktop, those are not methods of TabStripModel,
> 2. they can be implemented as separate functions.
> 
> Creating this as separate function also allow me to test them with test
> implementation of WebStateList.

I'm not sure I find the first two reasons compelling, although the testing is
nice. It's OK for now, but I'll probably want to talk about general patterns for
free functions vs methods at some point.

[sdefresne, 2017-04-05 11:44:58]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-04-05 11:44:58]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2771863002/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-05 11:45:16]

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-04-05 12:49:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491392698000590,
"parent_rev": "65a731dee06622f7ed04bf0335ec98ed2189570a", "commit_rev":
"08c0ab4e786593dd8d91407165da85d923d0179b"}

[commit-bot: I haz the power, 2017-04-05 12:50:37]

Description was changed from

==========
[ios] Add free function to (de)serialise WebStateList.

BUG=546222
==========

to

==========
[ios] Add free function to (de)serialise WebStateList.

BUG=546222

Review-Url: https://codereview.chromium.org/2771863002
Cr-Commit-Position: refs/heads/master@{#462043}
Committed:
https://chromium.googlesource.com/chromium/src/+/08c0ab4e786593dd8d91407165da...
==========

[commit-bot: I haz the power, 2017-04-05 12:50:38]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/08c0ab4e786593dd8d91407165da...