New issue
Advanced search Search tips

Issue 677356 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Feature



Sign in to add a comment

Manage page title updates in the web// layer without relying on the delegate.

Project Member Reported by kkhorimoto@chromium.org, Dec 29 2016

Issue description

Currently, when a page's title is updated on Chrome for iOS, we receive a KVO callback from the WKWebView's |title| property, and the web delegate is notified.  Tab is currently responsible for updating the NavigationItem, and if there is no delegate, then the current NavigationItem's title is not reset to the new value.  Updating the NavigationManager model should occur internally for title changes, and the delegate should be notified of the new title after the model is updated.
 
Cc: -eugene...@chromium.org kkhorimoto@chromium.org
Owner: eugene...@chromium.org
Reassigning to Eugene  since https://codereview.chromium.org/2685803002/ fixes this bug.
Labels: -Type-Bug Type-Feature
Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69378e0417fa4dbcae4dc863e5b46b68d71ac0be

commit 69378e0417fa4dbcae4dc863e5b46b68d71ac0be
Author: stkhapugin <stkhapugin@chromium.org>
Date: Fri Feb 10 12:10:09 2017

Revert of Fixed title updating for back forward navigation. (patchset #3 id:40001 of https://codereview.chromium.org/2685803002/ )

Reason for revert:
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!

Original issue's description:
> 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

TBR=kkhorimoto@chromium.org,eugenebut@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688047 ,  677356 

Review-Url: https://codereview.chromium.org/2684343003
Cr-Commit-Position: refs/heads/master@{#449595}

[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/adb03a4e58c33d0adc812a2ddae2180abe8ac148

commit adb03a4e58c33d0adc812a2ddae2180abe8ac148
Author: eugenebut <eugenebut@chromium.org>
Date: Sat Feb 11 02:03:52 2017

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}

[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d

commit 6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d
Author: eugenebut <eugenebut@chromium.org>
Date: Sun Feb 12 02:04:08 2017

Revert of Relanding "Fixed title updating for back forward navigation." (patchset #3 id:40001 of https://codereview.chromium.org/2691693002/ )

Reason for revert:
Breaks some other tests this time:
https://uberchromegw.corp.google.com/i/internal.bling.main/builders/iphone9-simulator-x64/builds/9939

Original issue's description:
> 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

TBR=kkhorimoto@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688047 ,  677356 

Review-Url: https://codereview.chromium.org/2689083002
Cr-Commit-Position: refs/heads/master@{#449881}

[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/176138f21cb2df89eb6157bb748b60207038eff4

commit 176138f21cb2df89eb6157bb748b60207038eff4
Author: eugenebut <eugenebut@chromium.org>
Date: Sun Feb 12 04:14:48 2017

Reland of landing "Fixed title updating for back forward navigation." (patchset #1 id:1 of https://codereview.chromium.org/2689083002/ )

Reason for revert:
This revert did not help. Reverting the revert.

Original issue's description:
> Revert of Relanding "Fixed title updating for back forward navigation." (patchset #3 id:40001 of https://codereview.chromium.org/2691693002/ )
>
> Reason for revert:
> Breaks some other tests this time:
> https://uberchromegw.corp.google.com/i/internal.bling.main/builders/iphone9-simulator-x64/builds/9939
>
> Original issue's description:
> > 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
>
> TBR=kkhorimoto@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 688047 ,  677356 
>
> Review-Url: https://codereview.chromium.org/2689083002
> Cr-Commit-Position: refs/heads/master@{#449881}
> Committed: https://chromium.googlesource.com/chromium/src/+/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d

TBR=kkhorimoto@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688047 ,  677356 

Review-Url: https://codereview.chromium.org/2694583002
Cr-Commit-Position: refs/heads/master@{#449886}

[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 15 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55217cf9efff8866160932c1614077d4ed43a2d3

commit 55217cf9efff8866160932c1614077d4ed43a2d3
Author: Eugene But <eugenebut@google.com>
Date: Wed Feb 15 16:30:32 2017

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/2701463002 .
Cr-Commit-Position: refs/branch-heads/2987@{#521}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77569373deed80a50d4787cd594f40953d1596a0

commit 77569373deed80a50d4787cd594f40953d1596a0
Author: eugenebut <eugenebut@chromium.org>
Date: Thu Feb 23 19:54:34 2017

Cherry-picked remaining parts of 'Fixed title updating for back forward navigation.'

Cherry-picked patchset #1 and #2 from cl/2691693002

BUG= 688047 ,  677356 
Review-Url: https://codereview.chromium.org/2685803002
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2711123004
Cr-Commit-Position: refs/branch-heads/2987@{#666}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/77569373deed80a50d4787cd594f40953d1596a0/ios/web/web_state/ui/crw_web_controller.mm

Sign in to add a comment