Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697193004:
Add opener-opened relationship between WebState in WebStateList. (Closed)
DescriptionAdd opener-opened relationship between WebState in WebStateList.
In preparation of refactoring TabModelOrderController to be shared
with the new architecture, move the opener-opened relationship to
WebStateList.
The relationship can be implemented without any reference to tabId,
openerId or openerNavigationIndex. Those notions will be needed for
serialisation (and will be possible to implement once serialisation
refactoring is complete, until then leave the serialisation in the
TabModel class).
BUG=687207
Review-Url: https://codereview.chromium.org/2697193004
Cr-Commit-Position: refs/heads/master@{#452439}
Committed: https://chromium.googlesource.com/chromium/src/+/e0a9ba6076a8522976835cafac5e389aab66a43e
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments. #Patch Set 3 : Rebase. #Patch Set 4 : Fix opener relationship when restoring a session. #
Total comments: 8
Patch Set 5 : Address comments. #Patch Set 6 : Fix copy-n-paste error. #
Total comments: 2
Messages
Total messages: 43 (30 generated)
sdefresne@chromium.org changed reviewers: + marq@chromium.org, rohitrao@chromium.org
Please take a look.
Patchset #1 (id:1) has been deleted
LGTM modulo comment and naming nits. https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.h:112: int skip_up_to, Can you document how |skip_up_to| relates to the 'n' alluded to in the method name? https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.mm (right): https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:35: // Returns whether opener spawned the wrapped WebState. If |use_group| is |opener| https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:38: bool WasOpenBy(const web::WebState* opener, WasOpenedBy(). yay, English! https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:217: DCHECK_GT(skip_up_to, 0); Document this precondition in the header comments.
Thank you for the review. https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.h:112: int skip_up_to, On 2017/02/16 15:19:17, marq wrote: > Can you document how |skip_up_to| relates to the 'n' alluded to in the method > name? Renamed the parameter to n as discussed offline, and updated the comment to mention it is > 0. https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... File ios/shared/chrome/browser/tabs/web_state_list.mm (right): https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:35: // Returns whether opener spawned the wrapped WebState. If |use_group| is On 2017/02/16 15:19:17, marq wrote: > |opener| Done. https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:38: bool WasOpenBy(const web::WebState* opener, On 2017/02/16 15:19:17, marq wrote: > WasOpenedBy(). yay, English! Done. https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/brows... ios/shared/chrome/browser/tabs/web_state_list.mm:217: DCHECK_GT(skip_up_to, 0); On 2017/02/16 15:19:17, marq wrote: > Document this precondition in the header comments. Done.
lgtm
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 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...
rohitrao: ping
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:60001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
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.
lgtm https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab_model.mm:996: // Recreate all the restored tabs but do not insert them into WebStateList Doesn't this insert the tabs into the WebStateList at line 1010? Does the comment need to be updated? https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab_model.mm:998: // of the n-th restored tab may be at an index larger than n). Is there a test for this "index larger than n" case? Can there be one? https://codereview.chromium.org/2697193004/diff/140001/ios/shared/chrome/brow... File ios/shared/chrome/browser/tabs/web_state_list.mm (right): https://codereview.chromium.org/2697193004/diff/140001/ios/shared/chrome/brow... ios/shared/chrome/browser/tabs/web_state_list.mm:70: using std::swap; Why using? Could you just call std::swap(web_state, web_state_) instead? https://codereview.chromium.org/2697193004/diff/140001/ios/shared/chrome/brow... ios/shared/chrome/browser/tabs/web_state_list.mm:212: void WebStateList::FixOpenersReferencing(int index) { Is this really ClearOpenersReferencing()?
Patchset #5 (id:160001) has been deleted
Thank you for the review. https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab_model.mm:996: // Recreate all the restored tabs but do not insert them into WebStateList On 2017/02/22 14:57:52, rohitrao wrote: > Doesn't this insert the tabs into the WebStateList at line 1010? Does the > comment need to be updated? Yes, the comment is incorrect and need to be updated. It should read: // Recreate all the restored Tabs and add them to the WebStateList without // any opener-opened relationship (as the n-th restored Tab opener may be // at an index larger than n). Then in a second pass fix the openers. https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab_model.mm:998: // of the n-th restored tab may be at an index larger than n). On 2017/02/22 14:57:52, rohitrao wrote: > Is there a test for this "index larger than n" case? Can there be one? I've updated TabModelTest.PersistSelectionChange to test thhis. https://codereview.chromium.org/2697193004/diff/140001/ios/shared/chrome/brow... File ios/shared/chrome/browser/tabs/web_state_list.mm (right): https://codereview.chromium.org/2697193004/diff/140001/ios/shared/chrome/brow... ios/shared/chrome/browser/tabs/web_state_list.mm:70: using std::swap; On 2017/02/22 14:57:53, rohitrao wrote: > Why using? Could you just call std::swap(web_state, web_state_) instead? This is an habit that I grew that allows to force argument depend lookup to select a specialised version of swap() algorithm. If you have a custom type Foo that implements a custom swap method Foo::swap, you generally also want to expose a public free function swap() that takes two Foo like this: class Foo { public: Foo& swap(Foo& other) { // Custom implementation... return *this; } }; void swap(Foo& lhs, Foo& rhs) { lhs.swap(rhs); } Then if client code does "Foo foo1, foo2; std::swap(foo1, foo2);" they won't use your custom swap function and will default to using an temporary and two copies. This may be bad if copying your Foos is expensive. They can have the optimised version by doing "Foo foo1, foo2; swap(foo1, foo2);". However, if I'm writing a templated code then I don't know if I should write "swap(t1, t2);" or "std::swap(t1, t2);". If there is no custom swap, then first will fail, and if there is one the second will fail. Adding "using std::swap;" before the first means that the correct implementation will be used. template <typename T> void DoSomething(T& lhs, T& rhs) { using std::swap; swap(lhs, rhs); // Will use custom "swap" if available or "std::swap". }; Here it is unnecessary, so I've switched back to just using "std::swap(web_state, web_state_);". https://codereview.chromium.org/2697193004/diff/140001/ios/shared/chrome/brow... ios/shared/chrome/browser/tabs/web_state_list.mm:212: void WebStateList::FixOpenersReferencing(int index) { On 2017/02/22 14:57:52, rohitrao wrote: > Is this really ClearOpenersReferencing()? Yes. I've named it this following TabStripModel (but TabStripModel also clears group() property of the WebContents wrapper). Renamed.
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, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2697193004/#ps180001 (title: "Address comments.")
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: 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
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/2697193004/#ps200001 (title: "Fix copy-n-paste error.")
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": 200001, "attempt_start_ts": 1487840488066730,
"parent_rev": "1f77544d5c62ed481e81524b02e34d23439ad80b", "commit_rev":
"e0a9ba6076a8522976835cafac5e389aab66a43e"}
Message was sent while issue was closed.
Description was changed from ========== Add opener-opened relationship between WebState in WebStateList. In preparation of refactoring TabModelOrderController to be shared with the new architecture, move the opener-opened relationship to WebStateList. The relationship can be implemented without any reference to tabId, openerId or openerNavigationIndex. Those notions will be needed for serialisation (and will be possible to implement once serialisation refactoring is complete, until then leave the serialisation in the TabModel class). BUG=687207 ========== to ========== Add opener-opened relationship between WebState in WebStateList. In preparation of refactoring TabModelOrderController to be shared with the new architecture, move the opener-opened relationship to WebStateList. The relationship can be implemented without any reference to tabId, openerId or openerNavigationIndex. Those notions will be needed for serialisation (and will be possible to implement once serialisation refactoring is complete, until then leave the serialisation in the TabModel class). BUG=687207 Review-Url: https://codereview.chromium.org/2697193004 Cr-Commit-Position: refs/heads/master@{#452439} Committed: https://chromium.googlesource.com/chromium/src/+/e0a9ba6076a8522976835cafac5e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as https://chromium.googlesource.com/chromium/src/+/e0a9ba6076a8522976835cafac5e...
Message was sent while issue was closed.
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2697193004/diff/200001/ios/shared/chrome/brow... File ios/shared/chrome/browser/tabs/web_state_list_unittest.mm (right): https://codereview.chromium.org/2697193004/diff/200001/ios/shared/chrome/brow... ios/shared/chrome/browser/tabs/web_state_list_unittest.mm:104: class FakeNavigationManer : public web::TestNavigationManager { Do you want to use existing fake from ios/web/public/test/fakes instead of creating a new one? Each fake object is a maintenance burden and sharing the code could lower that burden.
Message was sent while issue was closed.
https://codereview.chromium.org/2697193004/diff/200001/ios/shared/chrome/brow... File ios/shared/chrome/browser/tabs/web_state_list_unittest.mm (right): https://codereview.chromium.org/2697193004/diff/200001/ios/shared/chrome/brow... ios/shared/chrome/browser/tabs/web_state_list_unittest.mm:104: class FakeNavigationManer : public web::TestNavigationManager { On 2017/04/10 21:30:59, Eugene But wrote: > Do you want to use existing fake from ios/web/public/test/fakes instead of > creating a new one? Each fake object is a maintenance burden and sharing the > code could lower that burden. Sorry, what I was trying to ask here is: "Do you want to change existing fake?". |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
