Relanding "Fixed title updating for back forward navigation."

Relanding "Fixed title updating for back forward navigation."

Chrome relies on KVO compliant "title" property to subscribe for
title updates. If this KVO change happens during then navigation
it is unclear if title was changes for the previous page, or the
navigation was committed and title was changed for the new page.

So if there is a navigation in progress WebController should ignore
KVO change, but it should always attempt to update title when
navigation is committed.

BUG=688047, 677356
Review-Url: https://codereview.chromium.org/2685803002
Cr-Commit-Position: refs/heads/master@{#449527}
(cherry picked from commit 487e4cfd53bac5944dad49cbce3b939a0c7bf035)

Review-Url: https://codereview.chromium.org/2691693002
Cr-Commit-Position: refs/heads/master@{#449828}
Committed: https://chromium.googlesource.com/chromium/src/+/adb03a4e58c33d0adc812a2ddae2180abe8ac148

[Eugene But (OOO till 7-30), 2017-02-10 23:02:24]

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

[commit-bot: I haz the power, 2017-02-10 23:02:59]

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-02-10 23:18:27]

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

[commit-bot: I haz the power, 2017-02-10 23:18:28]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-02-11 01:20:04]

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

[kkhorimoto, 2017-02-11 01:29:02]

https://codereview.chromium.org/2691693002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2691693002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:705: // Sets |title| to the last
committed navigation item. |title| can not be nil.
|title| with the bars refers to a specific variable name, meaning the input
parameter here.  Can you replace it with something more descriptive (i.e. the
current NavigationItem's title)?

https://codereview.chromium.org/2691693002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:1826: if ([self.nativeController
respondsToSelector:@selector(setDelegate:)]) {
Is the outer condition here still necessary? It seems that we should always be
setting the native content's delegate regardless of |_delegate|'s implemented
selectors.  It makes more sense to perform the outer check in the
CRWNativeContentDelegate callbacks.

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

Thanks for quick review. PTAL.

https://codereview.chromium.org/2691693002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2691693002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:705: // Sets |title| to the last
committed navigation item. |title| can not be nil.
On 2017/02/11 01:29:02, kkhorimoto_ wrote:
> |title| with the bars refers to a specific variable name, meaning the input
> parameter here.  Can you replace it with something more descriptive (i.e. the
> current NavigationItem's title)?

Done.

https://codereview.chromium.org/2691693002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:1826: if ([self.nativeController
respondsToSelector:@selector(setDelegate:)]) {
On 2017/02/11 01:29:02, kkhorimoto_ wrote:
> Is the outer condition here still necessary? It seems that we should always be
> setting the native content's delegate regardless of |_delegate|'s implemented
> selectors.  It makes more sense to perform the outer check in the
> CRWNativeContentDelegate callbacks.
I don't know why outer condition even exist, and I don't want to remove it in
this CL to minimize changes on the branch. webController:titleDidChange: is
CRWWebDleegate method, which will be removed soon, so I'd prefer to leave this
code as it is.

[kkhorimoto, 2017-02-11 01:41:29]

lgtm

[Eugene But (OOO till 7-30), 2017-02-11 01:47:58]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-02-11 01:48: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-02-11 02:03:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486777678565270,
"parent_rev": "4815ea656c5fde045cb6808d70cd84f87ad84bc1", "commit_rev":
"adb03a4e58c33d0adc812a2ddae2180abe8ac148"}

[commit-bot: I haz the power, 2017-02-11 02:04:34]

Description was changed from

==========
Relanding "Fixed title updating for back forward navigation."

Chrome relies on KVO compliant "title" property to subscribe for
title updates. If this KVO change happens during then navigation
it is unclear if title was changes for the previous page, or the
navigation was committed and title was changed for the new page.

So if there is a navigation in progress WebController should ignore
KVO change, but it should always attempt to update title when
navigation is committed.

BUG=688047, 677356

Review-Url: https://codereview.chromium.org/2685803002
Cr-Commit-Position: refs/heads/master@{#449527}
(cherry picked from commit 487e4cfd53bac5944dad49cbce3b939a0c7bf035)
==========

to

==========
Relanding "Fixed title updating for back forward navigation."

Chrome relies on KVO compliant "title" property to subscribe for
title updates. If this KVO change happens during then navigation
it is unclear if title was changes for the previous page, or the
navigation was committed and title was changed for the new page.

So if there is a navigation in progress WebController should ignore
KVO change, but it should always attempt to update title when
navigation is committed.

BUG=688047, 677356

Review-Url: https://codereview.chromium.org/2685803002
Cr-Commit-Position: refs/heads/master@{#449527}
(cherry picked from commit 487e4cfd53bac5944dad49cbce3b939a0c7bf035)

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

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

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

[Eugene But (OOO till 7-30), 2017-02-12 02:03:20]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2689083002/ by eugenebut@chromium.org.

The reason for reverting is: Breaks some other tests this time:
https://uberchromegw.corp.google.com/i/internal.bling.main/builders/iphone9-s....