[ios] Add session saving support to clean skeleton.

[ios] Add session saving support to clean skeleton.

BrowserListSessionService is a KeyedService that tracks BrowserList
and automatically save the session when the current WebState of any
WebStateList changes or when a navigation item is committed in an
active WebState.

Use BrowserListSessionService to load the session during the
initialisation of the RootCoordinator.

BUG=710848
Review-Url: https://codereview.chromium.org/2858283002
Cr-Commit-Position: refs/heads/master@{#471802}
Committed: https://chromium.googlesource.com/chromium/src/+/b95d16dd12e99c5f1c3ea2e7316fef8f50066f98

[sdefresne, 2017-05-04 14:49:05]

Patchset #1 (id:1) has been deleted

[sdefresne, 2017-05-04 14:52:34]

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

[sdefresne, 2017-05-04 14:52:34]

sdefresne@chromium.org changed required reviewers:
+ rohitrao@chromium.org

[sdefresne, 2017-05-04 14:52:35]

rohitrao: please take a look

marq: FYI (for the location of session restoration), since you are OOO, I may
land this before you are back and move to proper location as a followup

cmasso: FYI

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:49:
#include
"ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.h"
I'm still unconvinced the BrowserList is part of UI and think it is part of
model.

https://codereview.chromium.org/2858283002/diff/20001/ios/clean/chrome/app/st...
File ios/clean/chrome/app/steps/root_coordinator+application_step.mm (right):

https://codereview.chromium.org/2858283002/diff/20001/ios/clean/chrome/app/st...
ios/clean/chrome/app/steps/root_coordinator+application_step.mm:57: if (!service
|| !service->RestoreSession()) {
marq: I'm not sure whether this is the correct location to load the session; if
not could you recommend a better location

[marq (ping after 24h), 2017-05-09 10:18:59]

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:49:
#include
"ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.h"
On 2017/05/04 14:52:35, sdefresne wrote:
> I'm still unconvinced the BrowserList is part of UI and think it is part of
> model.

To me, it's "model" code in that it doesn't directly implement UI, but it's the
model representation of UI state (which tabs are grouped together). So
conceptually it's the "ui model" (as opposed to "data model"). I'm open to
discussion on this, however.

https://codereview.chromium.org/2858283002/diff/20001/ios/clean/chrome/app/st...
File ios/clean/chrome/app/steps/root_coordinator+application_step.mm (right):

https://codereview.chromium.org/2858283002/diff/20001/ios/clean/chrome/app/st...
ios/clean/chrome/app/steps/root_coordinator+application_step.mm:57: if (!service
|| !service->RestoreSession()) {
On 2017/05/04 14:52:35, sdefresne wrote:
> marq: I'm not sure whether this is the correct location to load the session;
if
> not could you recommend a better location

This seems correct (for now) to me; it's exactly where I had assumed you would
put it. Another option would be to do the service restoration in a separate
ApplicationStep, but I don't think that's necessary.

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
File
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:233:
if (old_count != 1)
Can you add comments to spell out the case that's being handled here and below?

It looks something like:

"If the web state list only contained a single webstate that hadn't been used,
close that webstate.

Is that correct?

(Also, having to handle this case seems clunky, but I'm guessing it's easier
than having to deal with possibly-empty WSLs?)

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:242:
if (navigation_item->GetURL() != kChromeUINewTabURL)
Note that it's possible to have an NTP in the middle of the navigation stack, so
you probably also need to check that the webState has only one nav item.

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:269:
immediately:!!immediately];
Is the !! to avoid bool-to-BOOL weirdness in 32-bit environments?

[sdefresne, 2017-05-11 15:58:00]

PTAL.

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:49:
#include
"ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.h"
On 2017/05/09 10:18:59, marq (ping after 24h) wrote:
> On 2017/05/04 14:52:35, sdefresne wrote:
> > I'm still unconvinced the BrowserList is part of UI and think it is part of
> > model.
> 
> To me, it's "model" code in that it doesn't directly implement UI, but it's
the
> model representation of UI state (which tabs are grouped together). So
> conceptually it's the "ui model" (as opposed to "data model"). I'm open to
> discussion on this, however.

It still seems strange that this is the only file from a /ui/ sub-directory
included in that file.

In addition, desktop make a difference between a Browser and a BrowserWindow
(Browser is model, BrowserWindow is UI and wraps a Browser, with lots of
intermediate classes like BrowserView and BrowserFrame).

https://codereview.chromium.org/2858283002/diff/20001/ios/clean/chrome/app/st...
File ios/clean/chrome/app/steps/root_coordinator+application_step.mm (right):

https://codereview.chromium.org/2858283002/diff/20001/ios/clean/chrome/app/st...
ios/clean/chrome/app/steps/root_coordinator+application_step.mm:57: if (!service
|| !service->RestoreSession()) {
On 2017/05/09 10:18:59, marq (ping after 24h) wrote:
> On 2017/05/04 14:52:35, sdefresne wrote:
> > marq: I'm not sure whether this is the correct location to load the session;
> if
> > not could you recommend a better location
> 
> This seems correct (for now) to me; it's exactly where I had assumed you would
> put it. Another option would be to do the service restoration in a separate
> ApplicationStep, but I don't think that's necessary.

Putting it in a separate ApplicationStep would allow to load the session
asynchronously, and should probably be done at some point. Thank you for
confirming the correct placement.

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
File
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:233:
if (old_count != 1)
On 2017/05/09 10:18:59, marq (ping after 24h) wrote:
> Can you add comments to spell out the case that's being handled here and
below?
> 
> It looks something like:
> 
> "If the web state list only contained a single webstate that hadn't been used,
> close that webstate.
> 
> Is that correct?
> 
> (Also, having to handle this case seems clunky, but I'm guessing it's easier
> than having to deal with possibly-empty WSLs?)

Done.

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:242:
if (navigation_item->GetURL() != kChromeUINewTabURL)
On 2017/05/09 10:18:59, marq (ping after 24h) wrote:
> Note that it's possible to have an NTP in the middle of the navigation stack,
so
> you probably also need to check that the webState has only one nav item.

Currently code in TabModel does not check for any other condition. I've added a
check that the NTP is not in the middle of the navigation stack by checking
whether the navigation manager allows to go back (so it is still possible to
have gone from NTP to another page, then back, and the NTP will still close).
WDYT?

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:269:
immediately:!!immediately];
On 2017/05/09 10:18:59, marq (ping after 24h) wrote:
> Is the !! to avoid bool-to-BOOL weirdness in 32-bit environments?

This is because bool != BOOL (even in 64-bit) but I think it is unnecessary, so
I've removed it (bool should correctly autopromote to BOOL).

[sdefresne, 2017-05-11 15:58:19]

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

[commit-bot: I haz the power, 2017-05-11 15:59:05]

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-05-11 18:27:35]

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

[commit-bot: I haz the power, 2017-05-11 18:27:36]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-05-12 07:48:35]

lgtm

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
File
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:242:
if (navigation_item->GetURL() != kChromeUINewTabURL)
On 2017/05/11 15:57:59, sdefresne wrote:
> On 2017/05/09 10:18:59, marq (ping after 24h) wrote:
> > Note that it's possible to have an NTP in the middle of the navigation
stack,
> so
> > you probably also need to check that the webState has only one nav item.
> 
> Currently code in TabModel does not check for any other condition. I've added
a
> check that the NTP is not in the middle of the navigation stack by checking
> whether the navigation manager allows to go back (so it is still possible to
> have gone from NTP to another page, then back, and the NTP will still close).
> WDYT?

Look good, thanks.

[sdefresne, 2017-05-15 08:59:26]

rohitrao: ping

[rohitrao (ping after 24h), 2017-05-15 11:56:03]

lgtm

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:49:
#include
"ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.h"
On 2017/05/04 14:52:35, sdefresne wrote:
> I'm still unconvinced the BrowserList is part of UI and think it is part of
> model.

It owns Browser, which in turn owns a dispatcher and a broadcaster.  I think
both of those are solidly part of the UI, so we're in a weird state.  I do agree
that Browser and BrowserList are more model than ui, but I don't know how to
resolve this contradiction.

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
File ios/shared/chrome/browser/ui/browser_list/browser_list_session_service.h
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service.h:26: //
session recording is scheduled with a delay, it is cancelled.
Would it be accurate to switch to this wording? "In all cases, if another
session recording was previously scheduled with a delay, that will be
cancelled."

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
File
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.mm
(right):

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.mm:36:
web_state->GetSessionCertificatePolicyCache()->UpdateCertificatePolicyCache(
I was not expecting to see SessionCertificatePolicyCache-related code in here. 
Do you know why this is needed?  If so, please add a comment.

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
File
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm
(right):

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:107:
web_state_list_->AddObserver(this);
If web_state_list_ already has an active WebState, should we register as an
observer for it?  Or should we DCHECK that there is no active webstate?

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:202:
observer_ = base::MakeUnique<BrowserListSessionServiceBrowserListObserver>(
What happens if the browser_list is non-empty when we construct this
ServiceImpl?  In that case, would we fail to register as an observer for that
WebStateList?

Should we DCHECK that browser_list is empty here?  Or register as an observer
for any WebStateList that is already in the browser_list?

[sdefresne, 2017-05-15 13:57:34]

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

[sdefresne, 2017-05-15 13:57:36]

I've addressed comments.

rohitrao: PTAL

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
File ios/shared/chrome/browser/ui/browser_list/browser_list_session_service.h
(right):

https://codereview.chromium.org/2858283002/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service.h:26: //
session recording is scheduled with a delay, it is cancelled.
On 2017/05/15 11:56:03, rohitrao (ping after 24h) wrote:
> Would it be accurate to switch to this wording? "In all cases, if another
> session recording was previously scheduled with a delay, that will be
> cancelled."

Done.

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
File
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.mm
(right):

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_factory.mm:36:
web_state->GetSessionCertificatePolicyCache()->UpdateCertificatePolicyCache(
On 2017/05/15 11:56:03, rohitrao (ping after 24h) wrote:
> I was not expecting to see SessionCertificatePolicyCache-related code in here.

> Do you know why this is needed?  If so, please add a comment.

I don't know why this is not yet done in WebState factory function. In the old
architecture, the call is done from TabModel after restoring all the WebState.
However, this can be done for each WebState in sequence, so I've put it in the
factory function wrapper. I've added a comment to explain this.

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
File
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm
(right):

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:107:
web_state_list_->AddObserver(this);
On 2017/05/15 11:56:03, rohitrao (ping after 24h) wrote:
> If web_state_list_ already has an active WebState, should we register as an
> observer for it?  Or should we DCHECK that there is no active webstate?

I think it should be observed. Fixed. Thank you.

https://codereview.chromium.org/2858283002/diff/40001/ios/shared/chrome/brows...
ios/shared/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm:202:
observer_ = base::MakeUnique<BrowserListSessionServiceBrowserListObserver>(
On 2017/05/15 11:56:03, rohitrao (ping after 24h) wrote:
> What happens if the browser_list is non-empty when we construct this
> ServiceImpl?  In that case, would we fail to register as an observer for that
> WebStateList?

BrowserListSessionServiceBrowserListObserver constructor supports non-empty
BrowserList, and BrowserListSessionServiceWebStateListObserver supports
non-empty WebStateList, so it should be fine to create this with a non-empty
Browser.

> Should we DCHECK that browser_list is empty here?  Or register as an observer
> for any WebStateList that is already in the browser_list?

We do the later in BrowserListSessionServiceBrowserListObserver constructor.

[commit-bot: I haz the power, 2017-05-15 13:57:57]

Dry run: CQ is trying da patch.

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

[rohitrao (ping after 24h), 2017-05-15 14:04:03]

lgtm

[sdefresne, 2017-05-15 15:41:07]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2017-05-15 15:41:11]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-05-15 15:41:12]

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/2858283002/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-05-15 15:41: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-05-15 16:51:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494862871621070,
"parent_rev": "f30da9cab0d5c860219ac2c1863bdd0202690bc4", "commit_rev":
"6f1812dc737b7d9f68f4cb60ecdc046bd93fc312"}

[commit-bot: I haz the power, 2017-05-15 16:51:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494862871621070,
"parent_rev": "8169ed379429eceac9b8520dfd6b6512ba35289c", "commit_rev":
"b95d16dd12e99c5f1c3ea2e7316fef8f50066f98"}

[commit-bot: I haz the power, 2017-05-15 16:51:41]

Description was changed from

==========
[ios] Add session saving support to clean skeleton.

BrowserListSessionService is a KeyedService that tracks BrowserList
and automatically save the session when the current WebState of any
WebStateList changes or when a navigation item is committed in an
active WebState.

Use BrowserListSessionService to load the session during the
initialisation of the RootCoordinator.

BUG=710848
==========

to

==========
[ios] Add session saving support to clean skeleton.

BrowserListSessionService is a KeyedService that tracks BrowserList
and automatically save the session when the current WebState of any
WebStateList changes or when a navigation item is committed in an
active WebState.

Use BrowserListSessionService to load the session during the
initialisation of the RootCoordinator.

BUG=710848

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

[commit-bot: I haz the power, 2017-05-15 16:51:45]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b95d16dd12e99c5f1c3ea2e7316f...