[ios] Extend WebStateListObserver amd WebStateListDelegate APIs.
In preparation of moving the ownership of all WebStates to WebStateList,
add methods to WebStateListObserver and WebStateListDelegate called when
the WebStates are detached or closed.
Rename the |index| parameters to |atIndex| for WebStateListObserving
protocol for consistency (some where called |index| other |atIndex|).
BUG=546222
Review-Url: https://codereview.chromium.org/2768093003
Cr-Commit-Position: refs/heads/master@{#459417}
Committed: https://chromium.googlesource.com/chromium/src/+/9e42572a191d565d3cdf8650e88c4971a1b6441c
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/176084) ios-simulator-xcode-clang on ...
3 years, 9 months ago
(2017-03-23 14:54:03 UTC)
#4
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/373396)
3 years, 9 months ago
(2017-03-23 17:21:35 UTC)
#10
On 2017/03/24 01:20:52, rohitrao wrote: > lgtm if we need this to ease the transition, ...
3 years, 9 months ago
(2017-03-24 09:26:18 UTC)
#12
On 2017/03/24 01:20:52, rohitrao wrote:
> lgtm if we need this to ease the transition, but I'm hoping we won't need
these
> methods forever, since desktop doesn't have anything like this.
The two methods that do not exists on desktop are:
- WebStateListObserver::WillDetachWebStateAt()
- WebStateListDelegate::DetachedWebStateAt()
They are both needed to respect the current lifecycle of Tab/WebStateList but
can probably be removed in the new architecture.
TabModelWebStateListDelegate::DetachedWebStateAt() disconnects Tab from its
owning TabModel (calls Tab -setParentTabModel:) thus should go away in the new
architecture.
TabModelClosingWebStateListObserver -webStateList:willDetachWebState:atIndex: is
used to record whether the detached WebState was the active WebState to decide
later whether to save the state. We can change this to always save the state and
instead use -webStateList:detachedWebState:atIndex: (called after the active
index has been updated).
https://codereview.chromium.org/2768093003/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model_observers_bridge.mm (right):
https://codereview.chromium.org/2768093003/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model_observers_bridge.mm:36: atIndex:(int)atIndex {
On 2017/03/24 01:20:52, rohitrao wrote:
> Why this change?
Just consistency when naming the parameter (as detailed in the CL description).
I called it |index| in half the methods and |atIndex| in the others. I settled
on |atIndex| because it seemed more common on the old code base, but I can use
|index| if you prefer.
https://codereview.chromium.org/2768093003/diff/20001/ios/shared/chrome/brows...
File ios/shared/chrome/browser/tabs/web_state_list_observer.h (right):
https://codereview.chromium.org/2768093003/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/tabs/web_state_list_observer.h:56: virtual void
WillCloseWebStateAt(WebStateList* web_state_list,
On 2017/03/24 01:20:52, rohitrao wrote:
> I don't see this method being sent from anywhere. Is that coming in a future
> CL?
Yes, it is coming in a future CL
(https://codereview.chromium.org/2775623002/diff/20001/ios/shared/chrome/brows...).
It is different from WebStateObserver::WebStateDestroyed because it is called
before the destructor of WebState is called. The WebState destructor does some
internal cleanup before calling the observers (it close its CRWWebController)
and thus it is too late for some of the clients.
Finally, this method exists in TabStripModelObserver, it is called
TabStripModelObserver::TabClosingAt().
https://codereview.chromium.org/2768093003/diff/20001/ios/shared/chrome/brows...
File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right):
https://codereview.chromium.org/2768093003/diff/20001/ios/shared/chrome/brows...
ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:60: -
(void)webStateList:(WebStateList*)webStateList
On 2017/03/24 01:20:52, rohitrao wrote:
> Inadvertent duplication?
Cause by cherry-picking/rebasing, yes. Thank you. I'm surprised that the
compiler does not complain about duplicated selector on a @protocol.
sdefresne
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-24 09:28:38 UTC)
#13
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-clang/builds/62136) mac_chromium_compile_dbg_ng on ...
3 years, 9 months ago
(2017-03-24 09:36:00 UTC)
#16
3 years, 9 months ago
(2017-03-24 14:18:09 UTC)
#21
lgtm
https://codereview.chromium.org/2768093003/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model_observers_bridge.mm (right):
https://codereview.chromium.org/2768093003/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model_observers_bridge.mm:36: atIndex:(int)atIndex {
On 2017/03/24 09:26:18, sdefresne wrote:
> On 2017/03/24 01:20:52, rohitrao wrote:
> > Why this change?
>
> Just consistency when naming the parameter (as detailed in the CL
description).
> I called it |index| in half the methods and |atIndex| in the others. I settled
> on |atIndex| because it seemed more common on the old code base, but I can use
> |index| if you prefer.
I prefer index, but I don't feel strongly.
sdefresne
The CQ bit was checked by sdefresne@chromium.org
3 years, 9 months ago
(2017-03-24 14:18:55 UTC)
#22
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490365135519930, "parent_rev": "0f7b1afa9784fa186cc18dae4bb43c096dc6616b", "commit_rev": "9e42572a191d565d3cdf8650e88c4971a1b6441c"}
3 years, 9 months ago
(2017-03-24 14:24:43 UTC)
#24
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490365135519930,
"parent_rev": "0f7b1afa9784fa186cc18dae4bb43c096dc6616b", "commit_rev":
"9e42572a191d565d3cdf8650e88c4971a1b6441c"}
commit-bot: I haz the power
Description was changed from ========== [ios] Extend WebStateListObserver amd WebStateListDelegate APIs. In preparation of moving ...
3 years, 9 months ago
(2017-03-24 14:25:32 UTC)
#25
Message was sent while issue was closed.
Description was changed from
==========
[ios] Extend WebStateListObserver amd WebStateListDelegate APIs.
In preparation of moving the ownership of all WebStates to WebStateList,
add methods to WebStateListObserver and WebStateListDelegate called when
the WebStates are detached or closed.
Rename the |index| parameters to |atIndex| for WebStateListObserving
protocol for consistency (some where called |index| other |atIndex|).
BUG=546222
==========
to
==========
[ios] Extend WebStateListObserver amd WebStateListDelegate APIs.
In preparation of moving the ownership of all WebStates to WebStateList,
add methods to WebStateListObserver and WebStateListDelegate called when
the WebStates are detached or closed.
Rename the |index| parameters to |atIndex| for WebStateListObserving
protocol for consistency (some where called |index| other |atIndex|).
BUG=546222
Review-Url: https://codereview.chromium.org/2768093003
Cr-Commit-Position: refs/heads/master@{#459417}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e42572a191d565d3cdf8650e88c...
==========
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9e42572a191d565d3cdf8650e88c4971a1b6441c
3 years, 9 months ago
(2017-03-24 14:25:34 UTC)
#26
Issue 2768093003: [ios] Extend WebStateListObserver amd WebStateListDelegate APIs.
(Closed)
Created 3 years, 9 months ago by sdefresne
Modified 3 years, 9 months ago
Reviewers: rohitrao (ping after 24h)
Base URL:
Comments: 8
Chromium Code Reviews
Issue
2768093003:
[ios] Extend WebStateListObserver amd WebStateListDelegate APIs. (Closed)
