Remove WebStateObserver::NavigationItemChanged |
||||||||
Issue descriptionThis method is called in 2 cases: 1.) Navigation Item title was changed 2.) Push/Replace state JS API was used First case is covered by WebStateObserver::TitleWasSet Second case is covered by WebStateObserver::DidFinishNavigation As it stands NavigationItemChanged is misleading.
,
Nov 4 2017
Danyao, we should either preserve the current behavior of NavigationItemChanged in WKBackForwardList navigation, or (even better) remove this method, which is currently used only by a single client.
,
Nov 7 2017
I would prefer removing it. :) GlobalWebStateEventTracker is the single remaining use case. It doesn't do anything except to pass on the NavigationItemChanged event to its observers via GlobalWebStateObserver::NavigationItemChanged API. The only client of this API is IOSChromeLocalSessionEventRouter. It listens to every WebState change and runs tab sync. We have a few opitons: 1) Add TitleWasSet and DidFinishNavigation to GlobalWebStateObserver. Pro: matches current behavior exactly Con: grows GlobalWebStateObserver API where the only use case (IOSChromeLocalSessionEventRouter) likely doesn't care about TitleWasSet. 2) Only add DidFinishNavigation Pro: does not grow GlobalWebStateObserver API unnecessarily In either case, GlobalWebStateObserver::NavigationItemCommitted should probably also be merged into DidFinishNavigation. What do you think about the options?
,
Nov 7 2017
Option #1 lgtm, because we going to remove GlobalWebStateObserver anyways, but we may have more options here. Sylvain, do you think we can switch IOSChromeLocalSessionEventRouter to observe multiple WebState objects and avoid changing GlobalWebStateObserver?
,
Nov 7 2017
I think GlobalWebStateEventTracker should be removed. My plan would be to: 1. change GlobalWebStateObserver to be informed of WebState creation 2. change all observers to directly observe the WebStates 3. remove all other method from GlobalWebStateObserver. I can probably take care of this as a followup of https://bugs.chromium.org/p/chromium/issues/detail?id=775684 (created https://bugs.chromium.org/p/chromium/issues/detail?id=782269 to track this). So I would recommend option #1 but with comment marking those methods as deprecated and a TODO(crbug.com/782269) to remove uses of the method (bonus point for adding the same TODO to the other method of GlobalWebStateObserver).
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f178c9f93a105eba880f285a54ae2dde1b9f552 commit 3f178c9f93a105eba880f285a54ae2dde1b9f552 Author: Danyao Wang <danyao@google.com> Date: Thu Nov 09 17:43:25 2017 Add deprecation notes to GlobalWebStateObserver. Bug: 720786 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I80e08d02f49e452596a21e4a91f0d0d7f3810236 Reviewed-on: https://chromium-review.googlesource.com/758484 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#515200} [modify] https://crrev.com/3f178c9f93a105eba880f285a54ae2dde1b9f552/ios/web/public/web_state/global_web_state_observer.h [modify] https://crrev.com/3f178c9f93a105eba880f285a54ae2dde1b9f552/ios/web/public/web_state/web_state_observer.h
,
Nov 9 2017
,
Nov 9 2017
,
Dec 8 2017
Danyao, does this method work correctly for WK based navigation manager or we need to remove it?
,
Dec 8 2017
This method should work the same in WK based navigation manager as legacy one now because we added back push/replace JS override. Longer term, I'd like us to remove it for the reasons you mentioned in the original post so we have one fewer WebStateObserver event. But it's not a blocker now.
,
Oct 26
,
Oct 26
,
Oct 26
,
Dec 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2002f254fc201c1260de86bc5e9e9dae0a4a793a commit 2002f254fc201c1260de86bc5e9e9dae0a4a793a Author: Eugene But <eugenebut@google.com> Date: Mon Dec 03 18:58:48 2018 Do not use LoadCommittedDetails.item in InfoBarManagerImpl. NavigationItemCommitted is deprecated in favour of DidFinishNavigation. This change will simplify transition from NavigationItemCommitted to DidFinishNavigation, because DidFinishNavigation does not pass NavigationItem as argument. Bug: 720786 Change-Id: Ic71f49c687aa73957afc0033910dd028a2f942e9 Reviewed-on: https://chromium-review.googlesource.com/c/1356772 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/heads/master@{#613176} [modify] https://crrev.com/2002f254fc201c1260de86bc5e9e9dae0a4a793a/ios/chrome/browser/infobars/infobar_manager_impl.mm
,
Dec 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da0c6d87d49bb75a6fceeba65fb9ddf902a6a8f9 commit da0c6d87d49bb75a6fceeba65fb9ddf902a6a8f9 Author: Eugene But <eugenebut@google.com> Date: Mon Dec 03 23:50:09 2018 Do not use LoadCommittedDetails.item in VoiceSearchNavigationTabHelper. NavigationItemCommitted is deprecated in favour of DidFinishNavigation. This change will simplify transition from NavigationItemCommitted to DidFinishNavigation, because DidFinishNavigation does not pass NavigationItem as argument. Bug: 720786 Change-Id: I758d7220c2d600bc663d8c55eeb49b5c54e619ff Reviewed-on: https://chromium-review.googlesource.com/c/1356770 Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#613330} [modify] https://crrev.com/da0c6d87d49bb75a6fceeba65fb9ddf902a6a8f9/ios/chrome/browser/voice/voice_search_navigations_tab_helper.mm
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0efebbe850b96f020cfacbe478b2b0c0bb43dbe commit b0efebbe850b96f020cfacbe478b2b0c0bb43dbe Author: Eugene But <eugenebut@google.com> Date: Tue Dec 04 14:08:21 2018 Do not use LoadCommittedDetails.item in WebStateTopSitesObserver. NavigationItemCommitted is deprecated in favour of DidFinishNavigation. This change will simplify transition from NavigationItemCommitted to DidFinishNavigation, because DidFinishNavigation does not pass NavigationItem as argument. Bug: 720786 Change-Id: Ic9819904b7aa875180a5f540e3a6ca15b81917f5 Reviewed-on: https://chromium-review.googlesource.com/c/1356771 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#613536} [modify] https://crrev.com/b0efebbe850b96f020cfacbe478b2b0c0bb43dbe/components/history/ios/browser/web_state_top_sites_observer.mm
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c82006414aa7d03f2b8034b4e0de2db3094a7233 commit c82006414aa7d03f2b8034b4e0de2db3094a7233 Author: Eugene But <eugenebut@google.com> Date: Fri Dec 07 19:25:53 2018 IOSChromeLocalSessionEventRouter does not subclass from GlobalWebStateObserver. IOSChromeLocalSessionEventRouter will observe multiple WebState objects without using deprecated GlobalWebStateObserver. Also removed browser state check in OnWebStateChange. This check was needed because each WSO callback was called for both BrowserState objects. So the router had to check if the call belongs to correct browser state. After this CL there are no extra calls, hence no need for check. Bug: 910894, 782269, 720786 Change-Id: Ie25ae62efea9edd4a981800d1a43b6ef28b35b14 Reviewed-on: https://chromium-review.googlesource.com/c/1359863 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Cr-Commit-Position: refs/heads/master@{#614779} [modify] https://crrev.com/c82006414aa7d03f2b8034b4e0de2db3094a7233/ios/chrome/browser/sync/BUILD.gn [rename] https://crrev.com/c82006414aa7d03f2b8034b4e0de2db3094a7233/ios/chrome/browser/sync/session_sync_service_factory.mm [modify] https://crrev.com/c82006414aa7d03f2b8034b4e0de2db3094a7233/ios/chrome/browser/sync/sessions/BUILD.gn [modify] https://crrev.com/c82006414aa7d03f2b8034b4e0de2db3094a7233/ios/chrome/browser/sync/sessions/ios_chrome_local_session_event_router.h [modify] https://crrev.com/c82006414aa7d03f2b8034b4e0de2db3094a7233/ios/chrome/browser/sync/sessions/ios_chrome_local_session_event_router.mm
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8e01908e4ac54c6e88e1c476edcad982a954922 commit b8e01908e4ac54c6e88e1c476edcad982a954922 Author: Eugene But <eugenebut@google.com> Date: Tue Dec 11 22:33:12 2018 Do not override deprecated NavigationItemChanged in IOSChromeLocalSessionEventRouter. NavigationItemChanged is called in 2 cases: 1.) title was changed for navigation item 2.) window.location.replace API was called Case #1 can be tracked with TitleWasSet and case #2 can be tarcked with DidFinishNavigation. IOSChromeLocalSessionEventRouter already overrides DidFinishNavigation, so this CL replaces NavigationItemChanged with TitleWasSet. Bug: 720786 Change-Id: I15f839467cc2541308823038eae86ec9a6a9a9e0 Reviewed-on: https://chromium-review.googlesource.com/c/1369053 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#615696} [modify] https://crrev.com/b8e01908e4ac54c6e88e1c476edcad982a954922/ios/chrome/browser/sync/sessions/ios_chrome_local_session_event_router.h [modify] https://crrev.com/b8e01908e4ac54c6e88e1c476edcad982a954922/ios/chrome/browser/sync/sessions/ios_chrome_local_session_event_router.mm
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85 commit 76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85 Author: Eugene But <eugenebut@chromium.org> Date: Wed Dec 12 18:11:12 2018 Remove WebStateObserver::NavigationItemChanged callback. This callback is deprecated. Clients should use |TitleWasSet| to listen for title changes and |DidFinishNavigation| for |window.location.replace|. Bug: 720786 , 546218 Change-Id: Ic8e0dbbb773b8943ddca3961440af553901aa799 Reviewed-on: https://chromium-review.googlesource.com/c/1372850 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#615960} [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/legacy_navigation_manager_impl.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/legacy_navigation_manager_impl.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/navigation_manager_delegate.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/navigation_manager_impl.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/navigation_manager_impl.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/navigation_manager_impl_unittest.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/wk_based_navigation_manager_impl.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/wk_based_navigation_manager_impl.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/public/test/fakes/test_web_state_observer.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/public/test/fakes/test_web_state_observer.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/public/test/fakes/test_web_state_observer_util.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/public/web_state/global_web_state_observer.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/public/web_state/web_state_observer.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/test/fakes/fake_navigation_manager_delegate.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/test/fakes/fake_navigation_manager_delegate.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/web_state/global_web_state_event_tracker.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/web_state/global_web_state_event_tracker.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/web_state/ui/crw_web_controller.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/web_state/web_state_impl.h [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/web_state/web_state_impl.mm [modify] https://crrev.com/76c6ec2d4dfc999fb6c7f7dfce4d9f0536ab5a85/ios/web/web_state/web_state_impl_unittest.mm
,
Dec 12
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, May 10 2017