Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

Issue 2697193004: Add opener-opened relationship between WebState in WebStateList. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by sdefresne
Modified:
1 week, 6 days ago
Reviewers:
Eugene But, marq, rohitrao
CC:
chromium-reviews, marq+watch_chromium.org, pklpkl, noyau+watch_chromium.org, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+482 lines, -111 lines) Patch
M ios/chrome/browser/tabs/tab_model.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M ios/chrome/browser/tabs/tab_model.mm View 1 2 3 4 9 chunks +89 lines, -62 lines 0 comments Download
M ios/chrome/browser/tabs/tab_model_unittest.mm View 1 2 3 4 2 chunks +23 lines, -4 lines 0 comments Download
M ios/shared/chrome/browser/tabs/web_state_list.h View 1 2 3 4 2 chunks +47 lines, -4 lines 0 comments Download
M ios/shared/chrome/browser/tabs/web_state_list.mm View 1 2 3 4 5 7 chunks +127 lines, -6 lines 0 comments Download
M ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper_unittest.mm View 1 chunk +1 line, -1 line 0 comments Download
M ios/shared/chrome/browser/tabs/web_state_list_unittest.mm View 1 2 3 4 18 chunks +194 lines, -34 lines 2 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 43 (30 generated)
sdefresne
Please take a look.
2 months ago (2017-02-16 12:40:27 UTC) #2
marq
LGTM modulo comment and naming nits. https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/browser/tabs/web_state_list.h File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/browser/tabs/web_state_list.h#newcode112 ios/shared/chrome/browser/tabs/web_state_list.h:112: int skip_up_to, Can ...
2 months ago (2017-02-16 15:19:17 UTC) #4
sdefresne
Thank you for the review. https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/browser/tabs/web_state_list.h File ios/shared/chrome/browser/tabs/web_state_list.h (right): https://codereview.chromium.org/2697193004/diff/20001/ios/shared/chrome/browser/tabs/web_state_list.h#newcode112 ios/shared/chrome/browser/tabs/web_state_list.h:112: int skip_up_to, On 2017/02/16 ...
2 months ago (2017-02-16 15:42:01 UTC) #5
marq
lgtm
2 months ago (2017-02-16 15:54:48 UTC) #6
sdefresne
rohitrao: ping
2 months ago (2017-02-20 15:17:52 UTC) #17
rohitrao
lgtm https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tabs/tab_model.mm File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tabs/tab_model.mm#newcode996 ios/chrome/browser/tabs/tab_model.mm:996: // Recreate all the restored tabs but do ...
2 months ago (2017-02-22 14:57:53 UTC) #27
sdefresne
Thank you for the review. https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tabs/tab_model.mm File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2697193004/diff/140001/ios/chrome/browser/tabs/tab_model.mm#newcode996 ios/chrome/browser/tabs/tab_model.mm:996: // Recreate all the ...
2 months ago (2017-02-22 18:23:47 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697193004/180001
2 months ago (2017-02-22 18:24:52 UTC) #32
commit-bot: I haz the power
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/158046) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
2 months ago (2017-02-22 18:34:16 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697193004/200001
1 month, 4 weeks ago (2017-02-23 09:01:49 UTC) #37
commit-bot: I haz the power
Committed patchset #6 (id:200001) as https://chromium.googlesource.com/chromium/src/+/e0a9ba6076a8522976835cafac5e389aab66a43e
1 month, 4 weeks ago (2017-02-23 09:14:52 UTC) #40
Eugene But
https://codereview.chromium.org/2697193004/diff/200001/ios/shared/chrome/browser/tabs/web_state_list_unittest.mm File ios/shared/chrome/browser/tabs/web_state_list_unittest.mm (right): https://codereview.chromium.org/2697193004/diff/200001/ios/shared/chrome/browser/tabs/web_state_list_unittest.mm#newcode104 ios/shared/chrome/browser/tabs/web_state_list_unittest.mm:104: class FakeNavigationManer : public web::TestNavigationManager { Do you want ...
1 week, 6 days ago (2017-04-10 21:30:59 UTC) #42
Eugene But
1 week, 6 days ago (2017-04-10 21:48:05 UTC) #43
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?".
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46