New issue
Advanced search Search tips

Issue 781534 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task

Blocking:
issue 734150



Sign in to add a comment

Remove WebStateObserver::NavigationItemCommitted callback

Project Member Reported by eugene...@chromium.org, Nov 4 2017

Issue description

NavigationItemCommitted is called in subset of cases when WebStateObserver::DidFinishNavigation is called. All clients of NavigationItemCommitted should be converted to use DidFinishNavigation.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 6 2017

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

commit 65c914c69bbc2856df6fefd6afa41091fc3e30e1
Author: Eugene But <eugenebut@chromium.org>
Date: Mon Nov 06 18:05:14 2017

Deprecated NavigationItemCommitted.

Clients should switch to DidFinishNavigation.

Bug:  781534 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ie9b20635827a36f4c4bf03fdf7644ff2176bb713
Reviewed-on: https://chromium-review.googlesource.com/753866
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514174}
[modify] https://crrev.com/65c914c69bbc2856df6fefd6afa41091fc3e30e1/ios/web/public/web_state/web_state_observer.h

Components: Mobile>iOSWeb
Components: -Mobile>WebView>Glue
Components: -Mobile>iOSWeb Mobile>iOSWeb>PublicAPI
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 4

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

commit d1f6f1bbf815be6878f6bee64be3947b60f91dbd
Author: Eugene But <eugenebut@chromium.org>
Date: Tue Dec 04 17:22:57 2018

Reference correct bug number in NavigationItemCommitted comment.

Old TODO was incorrectly referencing  crbug.com/720786 , which was filed
for NavigationItemChanged removal. Correct bug for
NavigationItemCommitted() removal is  crbug.com/781534 .

Bug:  781534 
Change-Id: Ibb25be4d5a7eca2d5cbaf5a4b96f35263cf81a58
Reviewed-on: https://chromium-review.googlesource.com/c/1360981
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613583}
[modify] https://crrev.com/d1f6f1bbf815be6878f6bee64be3947b60f91dbd/ios/web/public/web_state/web_state_observer.h

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 10

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

commit f9fb6596d0d197dcca19d0ce9fdfa675bdbf8895
Author: Eugene But <eugenebut@google.com>
Date: Mon Dec 10 18:55:23 2018

s/NavigationItemCommitted/DidFinishNavigation in IOSChromeLocalSessionEventRouter.

NavigationItemCommitted is deprecated in favour of DidStartNavigation.

Bug:  781534 
Change-Id: I72d0cc47606b9cb388adcc05cd62c8f0e34ec839
Reviewed-on: https://chromium-review.googlesource.com/c/1368834
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615189}
[modify] https://crrev.com/f9fb6596d0d197dcca19d0ce9fdfa675bdbf8895/ios/chrome/browser/sync/sessions/ios_chrome_local_session_event_router.h
[modify] https://crrev.com/f9fb6596d0d197dcca19d0ce9fdfa675bdbf8895/ios/chrome/browser/sync/sessions/ios_chrome_local_session_event_router.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 7

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

commit 925c9579bd0928f0412df757c3b1cf535654488f
Author: Eugene But <eugenebut@google.com>
Date: Mon Jan 07 20:30:26 2019

Replace didCommitNavigationWithDetails with didFinishNavigation.

didCommitNavigationWithDetails is deprecated in favour of
didFinishNavigation.

Bug:  781534 
Change-Id: I226dd88cb4e571b28b7b3282d2cb7c423d62e737
Reviewed-on: https://chromium-review.googlesource.com/c/1396778
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620451}
[modify] https://crrev.com/925c9579bd0928f0412df757c3b1cf535654488f/ios/web/shell/view_controller.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 7

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

commit 59904d9020ff127161c3897c473a7c816d3ba36e
Author: Eugene But <eugenebut@google.com>
Date: Mon Jan 07 20:54:10 2019

Use DidFinishNavigation in InfoBarManagerImpl.

Deprecated NavigationItemCommitted was repaced with DidFinishNavigation.
This slightly changes the behavior because LoadCommittedDetails.is_in_page
is not equivalent to NavigationContext::IsSameDocument(), but these 2
are pretty close and the feature should work as expected.

Bug:  781534 
Change-Id: I0e2994f656f0d3d8b61b25826c3a131e558ac413
Reviewed-on: https://chromium-review.googlesource.com/c/1396400
Commit-Queue: Eugene But <eugenebut@chromium.org>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620462}
[modify] https://crrev.com/59904d9020ff127161c3897c473a7c816d3ba36e/ios/chrome/browser/infobars/infobar_manager_impl.h
[modify] https://crrev.com/59904d9020ff127161c3897c473a7c816d3ba36e/ios/chrome/browser/infobars/infobar_manager_impl.mm

Blocking: 734150
In many cases, this callback is not called for slim-navigation-manager, so this bug should block the launch.
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 8

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

commit 5b3a5cdcbafcbd441675b78a7517e0251e015254
Author: Eugene But <eugenebut@google.com>
Date: Tue Jan 08 17:21:44 2019

Use DidFinishNavigation in WebStateTopSitesObserver.

Deprecated NavigationItemCommitted was replaced with DidFinishNavigation.
This CL should not lead to any functional changes.

Bug:  781534 
Change-Id: I437462f069daa67e0c73035242f415e1d3204181
Reviewed-on: https://chromium-review.googlesource.com/c/1398442
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620773}
[modify] https://crrev.com/5b3a5cdcbafcbd441675b78a7517e0251e015254/components/history/ios/browser/web_state_top_sites_observer.h
[modify] https://crrev.com/5b3a5cdcbafcbd441675b78a7517e0251e015254/components/history/ios/browser/web_state_top_sites_observer.mm

Labels: Proj-WKBackForwardListBlocker
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 8

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

commit eed10f07153f2aaf0cce41645e1258753819af1e
Author: Eugene But <eugenebut@google.com>
Date: Tue Jan 08 20:28:47 2019

Use DidFinishNavigation in TextToSpeechPlaybackController.

Deprecated NavigationItemCommitted was repaced with DidFinishNavigation.

DidFinishNavigation is called when navigation is committed, replaced or
aborted. NavigationItemCommitted is called when navigation is committed.
For TextToSpeechPlaybackController it's ok to stop playback if the
navigation was replaced or aborted. So this CL should not lead to any
functional changes.

Bug:  781534 
Change-Id: I7e288c6adf9ffb8c635c9d85d9c5bc1d51138a80
Reviewed-on: https://chromium-review.googlesource.com/c/1398502
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620859}
[modify] https://crrev.com/eed10f07153f2aaf0cce41645e1258753819af1e/ios/chrome/browser/ui/voice/text_to_speech_playback_controller.h
[modify] https://crrev.com/eed10f07153f2aaf0cce41645e1258753819af1e/ios/chrome/browser/ui/voice/text_to_speech_playback_controller.mm

Owner: eugene...@chromium.org
Status: Started (was: Available)
Project Member

Comment 15 by bugdroid1@chromium.org, Jan 8

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

commit 6766ad66994e3298a19e1df10ffc8b750fcd254e
Author: Eugene But <eugenebut@google.com>
Date: Tue Jan 08 22:06:55 2019

Use DidFinishNavigation in VoiceSearchNavigationTabHelper.

Deprecated NavigationItemCommitted was replaced with DidFinishNavigation.

DidFinishNavigation is called when navigation is committed, replaced or
aborted. NavigationItemCommitted is called only when navigation is committed.
New code checks NavigationContext::HasCommitted() flag to preserve original
behavior.

Also remove VoiceSearchNavigationMarker as unused.

Bug:  781534 
Change-Id: I7140e454947bd6a7e39a518402168256204de912
Reviewed-on: https://chromium-review.googlesource.com/c/1398501
Commit-Queue: Eugene But <eugenebut@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620898}
[modify] https://crrev.com/6766ad66994e3298a19e1df10ffc8b750fcd254e/ios/chrome/browser/voice/voice_search_navigations_tab_helper.h
[modify] https://crrev.com/6766ad66994e3298a19e1df10ffc8b750fcd254e/ios/chrome/browser/voice/voice_search_navigations_tab_helper.mm

Project Member

Comment 16 by bugdroid1@chromium.org, Jan 8

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

commit 438f9be2230005f12b7355a5affc1ec368c8fe98
Author: Eugene But <eugenebut@google.com>
Date: Tue Jan 08 22:43:02 2019

Use DidFinishNavigation in FindTabHelper.

Deprecated NavigationItemCommitted was replaced with DidFinishNavigation.

DidFinishNavigation is called when navigation is committed, replaced or
aborted. NavigationItemCommitted is called when navigation is committed.
It's fine to dismiss FIP UI when navigation is aborted or replaced.

Bug:  781534 , 919977
Change-Id: I1d86fa6483d55c5bf71e49d5ad7f2164e2834862
Reviewed-on: https://chromium-review.googlesource.com/c/1398288
Commit-Queue: Eugene But <eugenebut@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620915}
[modify] https://crrev.com/438f9be2230005f12b7355a5affc1ec368c8fe98/ios/chrome/browser/find_in_page/find_tab_helper.h
[modify] https://crrev.com/438f9be2230005f12b7355a5affc1ec368c8fe98/ios/chrome/browser/find_in_page/find_tab_helper.mm
[modify] https://crrev.com/438f9be2230005f12b7355a5affc1ec368c8fe98/ios/chrome/browser/ui/browser_view_controller.mm

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 10

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

commit 5c6a82d4a00b7e9691576e7d9e836fcd371822f1
Author: Eugene But <eugenebut@google.com>
Date: Thu Jan 10 17:15:32 2019

Replace didCommitNavigationWithDetails with didFinishNavigation in TabModel.

Deprecated NavigationItemCommitted was repaced with DidFinishNavigation.
DidFinishNavigation is called when navigation is committed, replaced or
aborted. NavigationItemCommitted is called when navigation is committed.

This CL changes the frequency of Tabs.TabCountPerLoad histogram logging.
Before the change the histogram was logged for pushState/replaceState
navigation. After the change iOS behavior is on par with other platforms
and histograms is not logged for same-document navigations. The change is
not important because it does not really matter how often Tabs.TabCountPerLoad
is logged, only the number of logged tabs is important.

With this change kTabUrlStartedLoadingNotificationForCrashReporting will be
posted more often, which is not an issue, because lastCommittedURL will be
the same anyways.

Bug:  781534 
Change-Id: I0b63f5f874f52c59385c3a74e9363bb95816f2bc
Reviewed-on: https://chromium-review.googlesource.com/c/1400837
Commit-Queue: Eugene But <eugenebut@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621619}
[modify] https://crrev.com/5c6a82d4a00b7e9691576e7d9e836fcd371822f1/ios/chrome/browser/tabs/tab_model.mm

Project Member

Comment 18 by bugdroid1@chromium.org, Jan 10

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

commit 7f7b3b7be7e15366fc89043c8eb6a564a2ebcf7e
Author: Eugene But <eugenebut@google.com>
Date: Thu Jan 10 18:39:16 2019

Remove the usage of deprecated webState:didCommitNavigationWithDetails:.

didCommitNavigationWithDetails is deprecated in favour of
didFinishNavigation which is called when navigation is committed,
aborted or replaced. webViewDidCommitNavigation: is now called from
webState:didFinishNavigation: behind |if (navigation->HasCommitted())|
check to preserve original behavior.

Bug:  781534 
Change-Id: I7785a74100077e921474e0b729c65e9f663d6f8a
Reviewed-on: https://chromium-review.googlesource.com/c/1401562
Reviewed-by: John Wu <jzw@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621660}
[modify] https://crrev.com/7f7b3b7be7e15366fc89043c8eb6a564a2ebcf7e/ios/web_view/internal/cwv_web_view.mm

Project Member

Comment 19 by bugdroid1@chromium.org, Jan 11

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

commit 5736a9c135b6b4927f3bbe2033e41dc7d634c5b9
Author: Eugene But <eugenebut@google.com>
Date: Fri Jan 11 18:52:19 2019

Replace NavigationItemCommitted with DidFinishNavigation in WebTestWithWebState.

NavigationItemCommitted is deprecated and should be removed.
DidFinishNavigation is more general purpose method, which is called when
navigation is committed, replaced or aborted.

Bug:  781534 
Change-Id: I77d79970548e4d5291b9953c09490fddb50c6d72
Reviewed-on: https://chromium-review.googlesource.com/c/1406076
Reviewed-by: Danyao Wang <danyao@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622074}
[modify] https://crrev.com/5736a9c135b6b4927f3bbe2033e41dc7d634c5b9/ios/web/public/test/web_test_with_web_state.mm

Project Member

Comment 21 by bugdroid1@chromium.org, Jan 11

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

commit b3c6980e6798e31899fb214f4be75f45b907733f
Author: Eugene But <eugenebut@google.com>
Date: Fri Jan 11 23:12:47 2019

Remove WebStateObserver::NavigationItemCommitted.

This method is deprecated, and not used anymore. Hence can be safely
removed.

Bug:  749542 ,  781534 
Change-Id: Ia6b9e4cc263e30122ed26bc1df59a788ba18ac91
Reviewed-on: https://chromium-review.googlesource.com/c/1406084
Reviewed-by: Danyao Wang <danyao@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622201}
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/BUILD.gn
[delete] https://crrev.com/341089f274d224f5355e466aa0304a4b4a8aefe0/ios/web/load_committed_details.cc
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/navigation/legacy_navigation_manager_impl.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/navigation/navigation_manager_delegate.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/navigation/navigation_manager_impl.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/navigation/navigation_manager_impl.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/navigation/navigation_manager_impl_unittest.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/navigation/wk_based_navigation_manager_impl.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/BUILD.gn
[delete] https://crrev.com/341089f274d224f5355e466aa0304a4b4a8aefe0/ios/web/public/load_committed_details.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/test/fakes/crw_test_web_state_observer.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/test/fakes/crw_test_web_state_observer.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/test/fakes/test_web_state_observer.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/test/fakes/test_web_state_observer.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/test/fakes/test_web_state_observer_util.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/web_state/web_state_observer.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/public/web_state/web_state_observer_bridge.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/test/fakes/fake_navigation_manager_delegate.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/test/fakes/fake_navigation_manager_delegate.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/web_state/web_state_impl.h
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/web_state/web_state_impl_unittest.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/web_state/web_state_observer.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/web_state/web_state_observer_bridge.mm
[modify] https://crrev.com/b3c6980e6798e31899fb214f4be75f45b907733f/ios/web/web_state/web_state_observer_bridge_unittest.mm

Status: Fixed (was: Started)

Sign in to add a comment