Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months ago by sdefresne
Modified:
5 months, 1 week ago
CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), 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.
7 months ago (2017-02-16 12:40:27 UTC) #2
marq (ping after 24h)
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 ...
7 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 ...
7 months ago (2017-02-16 15:42:01 UTC) #5
marq (ping after 24h)
lgtm
7 months ago (2017-02-16 15:54:48 UTC) #6
sdefresne
rohitrao: ping
7 months ago (2017-02-20 15:17:52 UTC) #17
rohitrao (ping after 24h)
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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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
6 months, 4 weeks 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, ...
6 months, 4 weeks 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
6 months, 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
6 months, 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 ...
5 months, 1 week ago (2017-04-10 21:30:59 UTC) #42
Eugene But
5 months, 1 week 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 b40b6558b