New issue
Advanced search Search tips

Issue 911350 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Task

Blocking:
issue 783777



Sign in to add a comment

Remove TabModelObserver

Project Member Reported by rohitrao@chromium.org, Dec 4

Issue description

Callers should observe the corresponding WebStateList instead.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 4

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

commit 8495f54c9330bfa4824eecb1d2e02c88281c10b1
Author: Rohit Rao <rohitrao@chromium.org>
Date: Tue Dec 04 16:48:40 2018

[ios] Removes the unused tabModel:didStartLoading: observer method.

BUG=911350

Change-Id: Id9870f3ee82dddd59d698c50b0d14251287a8484
Reviewed-on: https://chromium-review.googlesource.com/c/1359952
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613574}
[modify] https://crrev.com/8495f54c9330bfa4824eecb1d2e02c88281c10b1/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/8495f54c9330bfa4824eecb1d2e02c88281c10b1/ios/chrome/browser/tabs/tab_model_observer.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 3

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

commit 2e73be2f0f43453a68961e5ed1a5dd4ac8c7d36c
Author: Rohit Rao <rohitrao@chromium.org>
Date: Thu Jan 03 21:11:27 2019

[ios] Removes the unused tabModelClosedAllTabs: method.

BUG=911350

Change-Id: Iec791dc2bf41b10705686bf01bbadc39ccee9e9d
Reviewed-on: https://chromium-review.googlesource.com/c/1394013
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619752}
[modify] https://crrev.com/2e73be2f0f43453a68961e5ed1a5dd4ac8c7d36c/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/2e73be2f0f43453a68961e5ed1a5dd4ac8c7d36c/ios/chrome/browser/tabs/tab_model_observer.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 4

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

commit eea69f7286e6a58bf3ff84edfd60c3d57ab0799b
Author: Rohit Rao <rohitrao@chromium.org>
Date: Fri Jan 04 01:30:57 2019

[ios] Removes the tabModel:willStartLoadingTab: observer method.

The single implementation of this method, in BrowserViewController, is modified
to use WebStateObserver methods instead.

BUG=911350

Change-Id: I8543fc6e76b2586ae7cf341f8f7800e2410ec3fd
Reviewed-on: https://chromium-review.googlesource.com/c/1394401
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619849}
[modify] https://crrev.com/eea69f7286e6a58bf3ff84edfd60c3d57ab0799b/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/eea69f7286e6a58bf3ff84edfd60c3d57ab0799b/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/eea69f7286e6a58bf3ff84edfd60c3d57ab0799b/ios/chrome/browser/tabs/tab_model_observer.h
[modify] https://crrev.com/eea69f7286e6a58bf3ff84edfd60c3d57ab0799b/ios/chrome/browser/ui/browser_view_controller.mm

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 8

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

commit cad41c0ed52647c1fefc9625f015d316b7be4779
Author: Rohit Rao <rohitrao@chromium.org>
Date: Tue Jan 08 13:01:24 2019

[ios] Removes the tabModel:didDeselectTab: observer method.

BrowserViewController was the only object listened for this
notification, and its implementation was moved into
tabModel:didChangeActiveTab:previousTab:atIndex: instead.

The BVC unittest covering this functionality was converted to an egtest
instead, as bringing up a second live Tab would have required many lines
of mocks -- a shorter, simpler egtest seemed like the better option.

BUG=911350

Change-Id: I7e17b3bafa3ede1c53f340adfe98b2d044fd4f75
Reviewed-on: https://chromium-review.googlesource.com/c/1399048
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620697}
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/tabs/tab_model_observer.h
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/ui/browser_view_controller_egtest.mm
[modify] https://crrev.com/cad41c0ed52647c1fefc9625f015d316b7be4779/ios/chrome/browser/ui/browser_view_controller_unittest.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 8

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

commit d0f6c27f85d00bfea742efee3424b7f2dd5c725e
Author: Rohit Rao <rohitrao@chromium.org>
Date: Tue Jan 08 18:34:20 2019

[ios] Removes the tabModelDidChangeTabCount observer method.

Callers are converted to listen for tabModel:didRemoveTab:atIndex:.
Previously, didRemoveTab was sent to all observers, and afterwards
didChangeTabCount was sent to all observers.  Now, code that was moved
into didRemoveTab will execute earlier, potentially reordered before
code that was in other observers' implementations of didRemoveTab.

The most serious instance of this is in MainController, which used
didChangeTabCount to destroy the incognito TabModel and
BrowserState. Destroying these objects earlier is risky, as the
BrowserState could potentially be destroyed before any other observers
get to run their implementations of didRemoveTab.  The current
implementation of -[MainController tabModel:didRemoveTab:atIndex:]
happens to schedule TabModel/BrowserState destruction on a later
iteration of the runloop, however, which guarantees a window for other
observers to run.

This still has the potential to create issues after a future
refactoring, but there are no guarantees on the ordering of observer
methods, so it would be incorrect to change the code in the future to
have MainController destroy the BrowserState synchronously.

BUG=911350

Change-Id: I3c5f6dd75205a4d8275cf257949f6443821a0656
Reviewed-on: https://chromium-review.googlesource.com/c/1396800
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620808}
[modify] https://crrev.com/d0f6c27f85d00bfea742efee3424b7f2dd5c725e/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/d0f6c27f85d00bfea742efee3424b7f2dd5c725e/ios/chrome/browser/tabs/tab_model_observer.h
[modify] https://crrev.com/d0f6c27f85d00bfea742efee3424b7f2dd5c725e/ios/chrome/browser/tabs/tab_model_observers_bridge.mm
[modify] https://crrev.com/d0f6c27f85d00bfea742efee3424b7f2dd5c725e/ios/chrome/browser/ui/tabs/tab_strip_controller.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 14

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

commit f3dde2b89edf0d69e803816bcb951d85d3993ef2
Author: Rohit Rao <rohitrao@chromium.org>
Date: Mon Jan 14 14:23:15 2019

[ios] Removes the tabModel:didFinishLoadingTab: observer method.

The only observer was BrowserViewController, which now listens for
webState:didLoadPageWithSuccess: instead.

BUG=911350

Change-Id: Iddd8ed6d851b57188aa0cd61c8edeae0f16b5b40
Reviewed-on: https://chromium-review.googlesource.com/c/1400763
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622445}
[modify] https://crrev.com/f3dde2b89edf0d69e803816bcb951d85d3993ef2/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/f3dde2b89edf0d69e803816bcb951d85d3993ef2/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/f3dde2b89edf0d69e803816bcb951d85d3993ef2/ios/chrome/browser/tabs/tab_model_observer.h
[modify] https://crrev.com/f3dde2b89edf0d69e803816bcb951d85d3993ef2/ios/chrome/browser/ui/browser_view_controller.mm

Sign in to add a comment