[ios] Reduce access to private //ios/web API.

[ios] Reduce access to private //ios/web API.

Call CRWWebController -recordStateInHistory when building the
serialised session storage CRWSessionStorage from WebStateImpl
to ensure the scroll position is correctly saved.

Remove direct call CRWWebController -recordStateInHistory from
//ios/chrome/browser as it is a private //ios/web API.

BUG=620465, 620480, 710848
Review-Url: https://codereview.chromium.org/2807063006
Cr-Commit-Position: refs/heads/master@{#466618}
Committed: https://chromium.googlesource.com/chromium/src/+/c8f7b8538f35a1d800bcd9e70ec1bd74a863e916

[sdefresne, 2017-04-11 11:13:52]

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

[commit-bot: I haz the power, 2017-04-11 11:14:07]

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

[sdefresne, 2017-04-11 11:14:33]

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

[sdefresne, 2017-04-11 11:14:34]

As discussed via email, I've removed uses of CRWWebController
-recordStateInHistory private method from ios/chrome/browser and added the call
just before WebStateImpl serialisation.

Please take a look.

[commit-bot: I haz the power, 2017-04-11 13:19:21]

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

[commit-bot: I haz the power, 2017-04-11 13:19:22]

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

[Eugene But (OOO till 7-30), 2017-04-11 14:50:48]

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

[Eugene But (OOO till 7-30), 2017-04-11 14:50:48]

lgtm! Kurt, does it look correct to you?

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

https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:38: DCHECK(oldTab);
nit: I don't think you need to DCHECK here. |@{kTabModelTabKey : oldTab}| will
crash anyways.

[kkhorimoto, 2017-04-11 18:20:00]

https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (left):

https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:41: [oldTab
recordStateInHistory];
From what I can tell, it looks like we're no longer recording state when we
switch Tabs.  It looks like WebStates record their display state when
loads/navigations occur and when the session storage is built.  If we were to
enter the Tab switcher and a WKWebView is evicted, do we have a mechanism to
restore the scroll offset when the page is reloaded?  Are we building a session
storage every time we enter the Tab Switcher?

[Eugene But (OOO till 7-30), 2017-04-11 18:39:39]

https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (left):

https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:41: [oldTab
recordStateInHistory];
On 2017/04/11 18:20:00, kkhorimoto_ wrote:
> From what I can tell, it looks like we're no longer recording state when we
> switch Tabs.  It looks like WebStates record their display state when
> loads/navigations occur and when the session storage is built.  If we were to
> enter the Tab switcher and a WKWebView is evicted, do we have a mechanism to
> restore the scroll offset when the page is reloaded?  Are we building a
session
> storage every time we enter the Tab Switcher?
Would it be useful to commit "safe" part in one CL and then work on case by case
basis to remove |recordStateInHistory| usage from places which can potentially
break the app?

[sdefresne, 2017-04-12 09:10:56]

On 2017/04/11 18:39:39, Eugene But wrote:
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (left):
> 
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:41: [oldTab
> recordStateInHistory];
> On 2017/04/11 18:20:00, kkhorimoto_ wrote:
> > From what I can tell, it looks like we're no longer recording state when we
> > switch Tabs.  It looks like WebStates record their display state when
> > loads/navigations occur and when the session storage is built.  If we were
to
> > enter the Tab switcher and a WKWebView is evicted, do we have a mechanism to
> > restore the scroll offset when the page is reloaded?  Are we building a
> session
> > storage every time we enter the Tab Switcher?

We currently record session every time the TabModel -currentTab changes (in
tab_model_selected_tab_observer.mm). The session is not saved immediately
(there's a delay of 2.5s) but the construction of the CRWSessionStorage happens
immediately (before the delay) as SessionServiceIOS takes a SessionWindowIOS as
parameter for -saveSessionWindow:sessionPath:immediately:. This is only done if
the new Tab has finished loading though.

However, I'm not sure that Tab/TabModel should be responsible for saving the
position so that it is correct in case of eviction. Isn't the WebState involved
in the eviction process? Can't it record the scroll position at that time?

> Would it be useful to commit "safe" part in one CL and then work on case by
case
> basis to remove |recordStateInHistory| usage from places which can potentially
> break the app?

[sdefresne, 2017-04-12 13:59:56]

Description was changed from

==========
[ios] Reduce access to private //ios/web API.

Call CRWWebController -recordStateInHistory when building the
serialised session storage CRWSessionStorage from WebStateImpl
to ensure the scroll position is correctly saved.

Remove direct call CRWWebController -recordStateInHistory from
//ios/chrome/browser as it is a private //ios/web API.

BUG=620465,620480
==========

to

==========
[ios] Reduce access to private //ios/web API.

Call CRWWebController -recordStateInHistory when building the
serialised session storage CRWSessionStorage from WebStateImpl
to ensure the scroll position is correctly saved.

Remove direct call CRWWebController -recordStateInHistory from
//ios/chrome/browser as it is a private //ios/web API.

BUG=620465,620480,710848
==========

[sdefresne, 2017-04-12 15:34:44]

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

[commit-bot: I haz the power, 2017-04-12 15:35:16]

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

[Eugene But (OOO till 7-30), 2017-04-12 16:47:00]

On 2017/04/12 09:10:56, sdefresne wrote:
> On 2017/04/11 18:39:39, Eugene But wrote:
> >
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> > File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (left):
> > 
> >
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> > ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:41: [oldTab
> > recordStateInHistory];
> > On 2017/04/11 18:20:00, kkhorimoto_ wrote:
> > > From what I can tell, it looks like we're no longer recording state when
we
> > > switch Tabs.  It looks like WebStates record their display state when
> > > loads/navigations occur and when the session storage is built.  If we were
> to
> > > enter the Tab switcher and a WKWebView is evicted, do we have a mechanism
to
> > > restore the scroll offset when the page is reloaded?  Are we building a
> > session
> > > storage every time we enter the Tab Switcher?
> 
> We currently record session every time the TabModel -currentTab changes (in
> tab_model_selected_tab_observer.mm). The session is not saved immediately
> (there's a delay of 2.5s) but the construction of the CRWSessionStorage
happens
> immediately (before the delay) as SessionServiceIOS takes a SessionWindowIOS
as
> parameter for -saveSessionWindow:sessionPath:immediately:. This is only done
if
> the new Tab has finished loading though.
> 
> However, I'm not sure that Tab/TabModel should be responsible for saving the
> position so that it is correct in case of eviction. Isn't the WebState
involved
> in the eviction process? Can't it record the scroll position at that time?
I agree that Tab/TabModel should not be responsible for saving the scroll
position.
-[CRWWebController wasHidden] is called when WebState's view is removed from the
screen. Is it sufficient to call |recordStateInHistory| inside |wasHidden|?

[commit-bot: I haz the power, 2017-04-12 16:59:11]

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

[commit-bot: I haz the power, 2017-04-12 16:59:12]

Dry run: This issue passed the CQ dry run.

[kkhorimoto, 2017-04-12 17:21:21]

On 2017/04/12 16:47:00, Eugene But wrote:
> On 2017/04/12 09:10:56, sdefresne wrote:
> > On 2017/04/11 18:39:39, Eugene But wrote:
> > >
> >
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> > > File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> > > ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:41: [oldTab
> > > recordStateInHistory];
> > > On 2017/04/11 18:20:00, kkhorimoto_ wrote:
> > > > From what I can tell, it looks like we're no longer recording state when
> we
> > > > switch Tabs.  It looks like WebStates record their display state when
> > > > loads/navigations occur and when the session storage is built.  If we
were
> > to
> > > > enter the Tab switcher and a WKWebView is evicted, do we have a
mechanism
> to
> > > > restore the scroll offset when the page is reloaded?  Are we building a
> > > session
> > > > storage every time we enter the Tab Switcher?
> > 
> > We currently record session every time the TabModel -currentTab changes (in
> > tab_model_selected_tab_observer.mm). The session is not saved immediately
> > (there's a delay of 2.5s) but the construction of the CRWSessionStorage
> happens
> > immediately (before the delay) as SessionServiceIOS takes a SessionWindowIOS
> as
> > parameter for -saveSessionWindow:sessionPath:immediately:. This is only done
> if
> > the new Tab has finished loading though.
> > 
> > However, I'm not sure that Tab/TabModel should be responsible for saving the
> > position so that it is correct in case of eviction. Isn't the WebState
> involved
> > in the eviction process? Can't it record the scroll position at that time?
> I agree that Tab/TabModel should not be responsible for saving the scroll
> position.
> -[CRWWebController wasHidden] is called when WebState's view is removed from
the
> screen. Is it sufficient to call |recordStateInHistory| inside |wasHidden|?

I agree that recording state in |-wasHidden| would be a better approach; I'll lg
this CL once we move the TabModelSelectedTabObserver code to CRWWebController

[sdefresne, 2017-04-20 12:21:29]

On 2017/04/12 17:21:21, kkhorimoto_ wrote:
> On 2017/04/12 16:47:00, Eugene But wrote:
> > On 2017/04/12 09:10:56, sdefresne wrote:
> > > On 2017/04/11 18:39:39, Eugene But wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> > > > File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2807063006/diff/1/ios/chrome/browser/tabs/tab...
> > > > ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:41: [oldTab
> > > > recordStateInHistory];
> > > > On 2017/04/11 18:20:00, kkhorimoto_ wrote:
> > > > > From what I can tell, it looks like we're no longer recording state
when
> > we
> > > > > switch Tabs.  It looks like WebStates record their display state when
> > > > > loads/navigations occur and when the session storage is built.  If we
> were
> > > to
> > > > > enter the Tab switcher and a WKWebView is evicted, do we have a
> mechanism
> > to
> > > > > restore the scroll offset when the page is reloaded?  Are we building
a
> > > > session
> > > > > storage every time we enter the Tab Switcher?
> > > 
> > > We currently record session every time the TabModel -currentTab changes
(in
> > > tab_model_selected_tab_observer.mm). The session is not saved immediately
> > > (there's a delay of 2.5s) but the construction of the CRWSessionStorage
> > happens
> > > immediately (before the delay) as SessionServiceIOS takes a
SessionWindowIOS
> > as
> > > parameter for -saveSessionWindow:sessionPath:immediately:. This is only
done
> > if
> > > the new Tab has finished loading though.
> > > 
> > > However, I'm not sure that Tab/TabModel should be responsible for saving
the
> > > position so that it is correct in case of eviction. Isn't the WebState
> > involved
> > > in the eviction process? Can't it record the scroll position at that time?
> > I agree that Tab/TabModel should not be responsible for saving the scroll
> > position.
> > -[CRWWebController wasHidden] is called when WebState's view is removed from
> the
> > screen. Is it sufficient to call |recordStateInHistory| inside |wasHidden|?
> 
> I agree that recording state in |-wasHidden| would be a better approach; I'll
lg
> this CL once we move the TabModelSelectedTabObserver code to CRWWebController

I've added the call in CRWWebController -wasHidden. Could you please take
another look?

[sdefresne, 2017-04-20 12:21:45]

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

[commit-bot: I haz the power, 2017-04-20 12:21:55]

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-04-20 12:34:32]

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

[commit-bot: I haz the power, 2017-04-20 12:34:33]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-04-20 15:49:14]

still lgtm

[sdefresne, 2017-04-21 10:09:44]

kkhorimoto: ping

[kkhorimoto, 2017-04-21 17:53:02]

lgtm!

[sdefresne, 2017-04-24 11:32:07]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2017-04-24 11:32: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-04-24 11:44:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493033527443820,
"parent_rev": "4e6d135f38ce0f12b6b06fd8515597f7bc7ed1c3", "commit_rev":
"c8f7b8538f35a1d800bcd9e70ec1bd74a863e916"}

[commit-bot: I haz the power, 2017-04-24 11:44:35]

Description was changed from

==========
[ios] Reduce access to private //ios/web API.

Call CRWWebController -recordStateInHistory when building the
serialised session storage CRWSessionStorage from WebStateImpl
to ensure the scroll position is correctly saved.

Remove direct call CRWWebController -recordStateInHistory from
//ios/chrome/browser as it is a private //ios/web API.

BUG=620465,620480,710848
==========

to

==========
[ios] Reduce access to private //ios/web API.

Call CRWWebController -recordStateInHistory when building the
serialised session storage CRWSessionStorage from WebStateImpl
to ensure the scroll position is correctly saved.

Remove direct call CRWWebController -recordStateInHistory from
//ios/chrome/browser as it is a private //ios/web API.

BUG=620465,620480,710848

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

[commit-bot: I haz the power, 2017-04-24 11:44:38]

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