Fixed title updating for back forward navigation.

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}
Committed: https://chromium.googlesource.com/chromium/src/+/487e4cfd53bac5944dad49cbce3b939a0c7bf035

[Eugene But (OOO till 7-30), 2017-02-07 23:21:36]

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

[commit-bot: I haz the power, 2017-02-07 23:22:28]

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-02-07 23:27:03]

Description was changed from

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

BUG=688047
==========

to

==========
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
==========

[Eugene But (OOO till 7-30), 2017-02-07 23:27:51]

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

[Eugene But (OOO till 7-30), 2017-02-07 23:27:51]

Kurt, PTAL
CCing Rohit for Tab changes

[commit-bot: I haz the power, 2017-02-07 23:46:54]

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

[commit-bot: I haz the power, 2017-02-07 23:46:55]

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

[kkhorimoto, 2017-02-07 23:59:44]

In addition to my other comments, can you also add a reference to
crbug.com/677356 in the CL description?

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1390: base::string16 newTitle =
base::SysNSStringToUTF16([_webView title]);
How about a separate variable for the NSString equal to [_webView title], since
it's used multiple places in this function?

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1401: DCHECK([_webView title]);
Should we be DCHECKing this earlier?  Whats the consequence of passing a nil
title here?

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:5022: if
(web::WKNavigationState::COMMITTED <=
It's not obvious without looking up the enum definition ordering what this
means.  How about instead of <=, we check if it's == to COMMITTED, FINISHED, or
FAILED?  It's a little more verbose, but helps readability.

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

Description was changed from

==========
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
==========

to

==========
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
==========

[Eugene But (OOO till 7-30), 2017-02-08 01:50:28]

Thanks! Updated CL description and disabled EG test back (so I can cherry pick
this CL). PTAL

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1390: base::string16 newTitle =
base::SysNSStringToUTF16([_webView title]);
On 2017/02/07 23:59:44, kkhorimoto_ wrote:
> How about a separate variable for the NSString equal to [_webView title],
since
> it's used multiple places in this function?

Done.

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1401: DCHECK([_webView title]);
On 2017/02/07 23:59:44, kkhorimoto_ wrote:
> Should we be DCHECKing this earlier?  Whats the consequence of passing a nil
> title here?
Done. I don't think anything will break, but this method should not be called
when title is nil (f.e. when web process crashes).

https://codereview.chromium.org/2685803002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:5022: if
(web::WKNavigationState::COMMITTED <=
On 2017/02/07 23:59:44, kkhorimoto_ wrote:
> It's not obvious without looking up the enum definition ordering what this
> means.  How about instead of <=, we check if it's == to COMMITTED, FINISHED,
or
> FAILED?  It's a little more verbose, but helps readability.
I think checking multiple states is more error prone (one state can be missed by
accident). Added explanatory variable to improve readability.

[Eugene But (OOO till 7-30), 2017-02-08 01:50:32]

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

[commit-bot: I haz the power, 2017-02-08 01:50:59]

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

[kkhorimoto, 2017-02-08 01:55:09]

lgtm

[commit-bot: I haz the power, 2017-02-08 02:19:49]

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

[commit-bot: I haz the power, 2017-02-08 02:19:49]

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-02-10 02:16:37]

The CQ bit was checked by eugenebut@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2685803002/#ps40001
(title: "Stubbed title call")

[commit-bot: I haz the power, 2017-02-10 02:17:14]

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 02:34:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486692997078510,
"parent_rev": "7370f20058e27a62dc19eddd398962a2d24b583f", "commit_rev":
"487e4cfd53bac5944dad49cbce3b939a0c7bf035"}

[commit-bot: I haz the power, 2017-02-10 02:35:01]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/487e4cfd53bac5944dad49cbce3b...
==========

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

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

[stkhapugin, 2017-02-10 12:09:00]

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

The reason for reverting is: Breaks -[TabHistoryPopupControllerTestCase
testTabHistoryMenu] tab_history_popup_controller_egtest.mm downstream, because
New Tab in history popup (when long pressed on back) displays "chrome://newtab"
instead of "New Tab".

I actually don't know why this test is not ran upstream, but I have to revert,
sorry!.

[rohitrao (ping after 24h), 2017-02-10 12:56:40]

On 2017/02/10 12:09:00, stkhapugin wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/2684343003/ by mailto:stkhapugin@chromium.org.
> 
> The reason for reverting is: Breaks -[TabHistoryPopupControllerTestCase
> testTabHistoryMenu] tab_history_popup_controller_egtest.mm downstream, because
> New Tab in history popup (when long pressed on back) displays
"chrome://newtab"
> instead of "New Tab".
> 
> I actually don't know why this test is not ran upstream, but I have to revert,
> sorry!.

EG tests are run on chromium.fyi upstream, but we haven't gotten them onto the
main waterfall yet.  We're working on getting upstream trybots, but they will
need to be manually triggered (won't be part of the default set).