|
|
DescriptionIntroduce WebStateList to manage a list of web::WebState.
Introduce WebStateList class that contains a list of web::WebState.
Followup CLs will refactor TabModel to use it and share code with
the new clean architecture.
WebStateList can either owns the web::WebState objects (should be
used by the new clean architecture or eventually after refactoring
the Tab ownership) or just borrow them.
BUG=687207
Review-Url: https://codereview.chromium.org/2680403005
Cr-Commit-Position: refs/heads/master@{#451286}
Committed: https://chromium.googlesource.com/chromium/src/+/d7519ff3a7e074f03a8d481c1f63fe3f54431682
Patch Set 1 #Patch Set 2 : Fix "gn check". #
Total comments: 24
Patch Set 3 : Rebase && address comments. #
Total comments: 8
Patch Set 4 : Convert WebStateList API to use int (following style guide) & add NSFastEnumeration tests. #Patch Set 5 : WebStateListFastEnumerationHelper owns id<WebStateProxyFactory>. #Patch Set 6 : Rebase. #Patch Set 7 : Rebase on https://codereview.chromium.org/2690893003/. #Patch Set 8 : Rebase. #Dependent Patchsets: Messages
Total messages: 68 (42 generated)
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdefresne@chromium.org changed reviewers: + marq@chromium.org, rohitrao@chromium.org
Please take a look. This just introduce WebStateList. You can get a sense of how it will be used by looking at the combined CL other there: https://codereview.chromium.org/2669463005/. Expect dependent CLs that will start updating TabModel to use this.
The CQ bit was unchecked by commit-bot@chromium.org
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...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rohitrao & marq: ping
LGTM with comment nits. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.h:43: // with an index such that |ContainsIndex(index)| returns fals.e s/.e/e./, or f.xp in vi. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.mm (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:16: // Wrapper around a WebState stored in a WebStateList. May owns the WebState s/owns/own/ https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper.h:24: // Helper class allowing to perform implement NSFastEnumeration over a s/to perform implement//
https://codereview.chromium.org/2680403005/diff/20001/ios/shared/DEPS File ios/shared/DEPS (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/DEPS#newcode5 ios/shared/DEPS:5: # forbids circular dependencies. Typo: forbid. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.mm (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:21: WebStateWrapper(web::WebState* web_state, bool own_web_state); "bool assume_ownership" maybe? "Own" doesn't always parse as a verb, which can be confusing. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:28: // Invoked when the WebState at the specified index is moved to another index. Is this called before or after the move? Is that an implementation detail or does it belong in the API contract? https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:34: // Invoked when the WebState at the specified index is replaced by another Is this called before or after the replacement? Is that an implementation detail or does it belong in the API contract? https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:43: virtual void WebStateRemovedAt(WebStateList* web_state_list, Is there value in calling this "Detached" instead of "Removed", for symmetry with desktop? https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:22: webStateInserted:(web::WebState*)webState To match cocoa touch conventions, consider naming this method "didInsertWebState" instead of "webStateInserted". (Or willInsert, if that's when the method is called.) Also for all of the observer methods below. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:34: webStateReplaced:(web::WebState*)oldWebState didReplaceWebState:withWebState:
https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:15: @protocol WebStateListObserver<NSObject> How does this compile? You can have a protocol with the same name as a C++ class? Can we change this to WebStateListObserving, to avoid confusion?
On 2017/02/14 13:01:26, rohitrao wrote: > https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... > File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right): > > https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... > ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:15: @protocol > WebStateListObserver<NSObject> > How does this compile? You can have a protocol with the same name as a C++ > class? > > Can we change this to WebStateListObserving, to avoid confusion? Protocols live in a separate namespace as classes. There is no risk of confusion (for the compiler at least) as a protocol can only be used to annotate an Objective-C class (like id<WebStateListObserver> or NSObject<WebStateListObserver>*) while the class can only be used somewhere a type is expected (i.e. WebStateListObserver*, scoped_ptr<WebStateListObserver>). I personally think it is simpler for developers too as the Objective-C protocol has the same name as the C++ abstract base class. So something observing a WebStateList will always be a WebStateListObserver. There's some examples of protocol and class with the same name in the Foundation (NSObject class and NSObject protocol for example).
https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:15: @protocol WebStateListObserver<NSObject> On 2017/02/14 13:01:26, rohitrao wrote: > How does this compile? You can have a protocol with the same name as a C++ > class? > > Can we change this to WebStateListObserving, to avoid confusion? Protocols aren't types, so there's no compilation issue. I said this was OK, but maybe WebStateListObserving is better. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:22: webStateInserted:(web::WebState*)webState On 2017/02/13 20:54:29, rohitrao wrote: > To match cocoa touch conventions, consider naming this method > "didInsertWebState" instead of "webStateInserted". (Or willInsert, if that's > when the method is called.) > > Also for all of the observer methods below. Yeah, I should have caught this. +1.
For me, protocols occupy the same mental namespace as classes, so I find it confusing when they overlap.
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for the review. I renamed the protocol, the methods and the parameter. PTAL. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/DEPS File ios/shared/DEPS (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/DEPS#newcode5 ios/shared/DEPS:5: # forbids circular dependencies. On 2017/02/13 20:54:29, rohitrao wrote: > Typo: forbid. Done. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.h:43: // with an index such that |ContainsIndex(index)| returns fals.e On 2017/02/13 16:32:53, marq wrote: > s/.e/e./, or f.xp in vi. Done (note: that's Ctrl+t when the cursor is after the e in both emacs and Xcode). https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.mm (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:16: // Wrapper around a WebState stored in a WebStateList. May owns the WebState On 2017/02/13 16:32:53, marq wrote: > s/owns/own/ Done. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:21: WebStateWrapper(web::WebState* web_state, bool own_web_state); On 2017/02/13 20:54:29, rohitrao wrote: > "bool assume_ownership" maybe? "Own" doesn't always parse as a verb, which can > be confusing. Done. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper.h:24: // Helper class allowing to perform implement NSFastEnumeration over a On 2017/02/13 16:32:53, marq wrote: > s/to perform implement// Done. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:28: // Invoked when the WebState at the specified index is moved to another index. On 2017/02/13 20:54:29, rohitrao wrote: > Is this called before or after the move? Is that an implementation detail or > does it belong in the API contract? After, I've updated the documentation. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:34: // Invoked when the WebState at the specified index is replaced by another On 2017/02/13 20:54:29, rohitrao wrote: > Is this called before or after the replacement? Is that an implementation > detail or does it belong in the API contract? After, I've updated the documentation. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:43: virtual void WebStateRemovedAt(WebStateList* web_state_list, On 2017/02/13 20:54:29, rohitrao wrote: > Is there value in calling this "Detached" instead of "Removed", for symmetry > with desktop? Done. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:15: @protocol WebStateListObserver<NSObject> On 2017/02/14 13:11:48, marq wrote: > On 2017/02/14 13:01:26, rohitrao wrote: > > How does this compile? You can have a protocol with the same name as a C++ > > class? > > > > Can we change this to WebStateListObserving, to avoid confusion? > > Protocols aren't types, so there's no compilation issue. I said this was OK, but > maybe WebStateListObserving is better. Done. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:22: webStateInserted:(web::WebState*)webState On 2017/02/14 13:11:48, marq wrote: > On 2017/02/13 20:54:29, rohitrao wrote: > > To match cocoa touch conventions, consider naming this method > > "didInsertWebState" instead of "webStateInserted". (Or willInsert, if that's > > when the method is called.) > > > > Also for all of the observer methods below. > > Yeah, I should have caught this. +1. Done. https://codereview.chromium.org/2680403005/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:34: webStateReplaced:(web::WebState*)oldWebState On 2017/02/13 20:54:29, rohitrao wrote: > didReplaceWebState:withWebState: Done.
lgtm https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.h:61: // Detachs the WebState at the specified index. Detaches. https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer.h (right): https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:42: // Invoked after the WebState at the specified index is being detached. The "has been detached" https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:35: byWebState:(web::WebState*)newWebState Consider changing this to "withWebState". https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:38: // Invoked after the WebState at the specified index is being removed. The "has been removed" or "is removed".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take another (last) look. https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.h:61: // Detachs the WebState at the specified index. On 2017/02/14 13:49:31, rohitrao wrote: > Detaches. Done. https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer.h (right): https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer.h:42: // Invoked after the WebState at the specified index is being detached. The On 2017/02/14 13:49:31, rohitrao wrote: > "has been detached" Done. https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:35: byWebState:(web::WebState*)newWebState On 2017/02/14 13:49:31, rohitrao wrote: > Consider changing this to "withWebState". Done. https://codereview.chromium.org/2680403005/diff/40001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h:38: // Invoked after the WebState at the specified index is being removed. The On 2017/02/14 13:49:31, rohitrao wrote: > "has been removed" or "is removed". Done.
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by sdefresne@chromium.org
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org Link to the patchset: https://codereview.chromium.org/2680403005/#ps80001 (title: "WebStateListFastEnumerationHelper owns id<WebStateProxyFactory>.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sdefresne@chromium.org
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdefresne@chromium.org changed reviewers: + mef@chromium.org
mef: can you review the new DEPS on //net. Note that we are refactoring src/ios/chrome/browser code and cleaned up code is put in src/ios/shared/chrome/browser (or src/ios/clean/chrome/browser) so this is just moving a dependency that already exists.
If this looks good to you could you send to CQ?
The CQ bit was unchecked by sdefresne@chromium.org
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mef@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2680403005/#ps120001 (title: "Rebase on https://codereview.chromium.org/2690893003/.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, marq@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2680403005/#ps140001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487332363698050, "parent_rev": "8156e7b0d6415027be848a6778140febc13ee334", "commit_rev": "d7519ff3a7e074f03a8d481c1f63fe3f54431682"}
Message was sent while issue was closed.
Description was changed from ========== Introduce WebStateList to manage a list of web::WebState. Introduce WebStateList class that contains a list of web::WebState. Followup CLs will refactor TabModel to use it and share code with the new clean architecture. WebStateList can either owns the web::WebState objects (should be used by the new clean architecture or eventually after refactoring the Tab ownership) or just borrow them. BUG=687207 ========== to ========== Introduce WebStateList to manage a list of web::WebState. Introduce WebStateList class that contains a list of web::WebState. Followup CLs will refactor TabModel to use it and share code with the new clean architecture. WebStateList can either owns the web::WebState objects (should be used by the new clean architecture or eventually after refactoring the Tab ownership) or just borrow them. BUG=687207 Review-Url: https://codereview.chromium.org/2680403005 Cr-Commit-Position: refs/heads/master@{#451286} Committed: https://chromium.googlesource.com/chromium/src/+/d7519ff3a7e074f03a8d481c1f63... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d7519ff3a7e074f03a8d481c1f63... |