New issue
Advanced search Search tips

Issue 687207 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Refactor TabModel for reuse in clean architecture.

Project Member Reported by sdefresne@chromium.org, Jan 31 2017

Issue description

Tracking bug.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62487280fe60b83fd4e66730c1e0478c55528faf

commit 62487280fe60b83fd4e66730c1e0478c55528faf
Author: sdefresne <sdefresne@chromium.org>
Date: Mon Feb 06 15:02:23 2017

Invokes TabParentingGlobalObserver for every Tab parented.

TabParentingGlobalObserver documentation states that it "[a]llows
clients to observe every tab (i.e., WebState) that is parented" so
fix TabModel to invokes it for every tab that is parented not just
the ones that are restored from a session.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2678823002
Cr-Commit-Position: refs/heads/master@{#448250}

[modify] https://crrev.com/62487280fe60b83fd4e66730c1e0478c55528faf/ios/chrome/browser/tabs/tab_model.mm

Project Member

Comment 2 by bugdroid1@chromium.org, Feb 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6bb97fbb52d03df492bc510ac8e9014d8c16ffae

commit 6bb97fbb52d03df492bc510ac8e9014d8c16ffae
Author: sdefresne <sdefresne@chromium.org>
Date: Mon Feb 06 18:06:38 2017

Add a ScopedNSAutoreleasePool to TabModelTest.PersistSelectionChange.

Refactoring TabModel introduces some autoreleased NSArray* that
increase the lifetime of Tab and thus of the registered observers.
Introduce a ScopedNSAutoreleasePool to ensure they are cleaned
before the ChromeBrowserState is cleaned.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2678883002
Cr-Commit-Position: refs/heads/master@{#448303}

[modify] https://crrev.com/6bb97fbb52d03df492bc510ac8e9014d8c16ffae/ios/chrome/browser/tabs/tab_model_unittest.mm

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44c09dc47b0df3de230ad862c98ffeccf1997699

commit 44c09dc47b0df3de230ad862c98ffeccf1997699
Author: sdefresne <sdefresne@chromium.org>
Date: Tue Feb 07 11:13:08 2017

Refactor TabModel initialisation from a saved session.

There was two code path to restore Tabs from a saved sessions, one
in the TabModel initialiser and one in -restoreSessionWindow:. They
were almost the same with the few differences (used helper methods
vs manipulating _tabs directly, thus not sending notifications).

When considering the fact that there are no observer registered at
the time the initialiser is called, nor are there any Tabs in the
_tabs array, the only significant difference left was whether the
state was persisted after updating _currentTab.

Refactor the initialiser to call -restoreSessionWindow: method by
adding an helper method allowing to choose whether the state should
be persisted or not.

While restoring a session in the initialiser via the shared code path,
the selection of the selected tab will be recorded to be reported
as a metric. To not change the metric, the count is reset after
restoring the session in the initialiser.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2674893002
Cr-Commit-Position: refs/heads/master@{#448598}

[modify] https://crrev.com/44c09dc47b0df3de230ad862c98ffeccf1997699/ios/chrome/browser/tabs/tab_model.mm

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c92bada1c0623a5355817bfac78d7dc082119a8d

commit c92bada1c0623a5355817bfac78d7dc082119a8d
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Feb 08 14:43:49 2017

Make certificate policy restoration thread-safe.

Use CancellableTaskTracker to make CleanCertificatePolicyCache
thread-safe (i.e. nothing happens if the TabModel is deallocated
while call to RestoreCertificatePolicyCacheFromModel is pending).

BUG= 687207 

Review-Url: https://codereview.chromium.org/2674293002
Cr-Commit-Position: refs/heads/master@{#448990}

[modify] https://crrev.com/c92bada1c0623a5355817bfac78d7dc082119a8d/ios/chrome/browser/tabs/tab_model.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e398e7fd334c08a2cfb805b25b0792fb15e674d3

commit e398e7fd334c08a2cfb805b25b0792fb15e674d3
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Feb 08 14:51:59 2017

Remove unused parameter for -replaceTab:withTab:keepOldTabOpen:.

The sole caller of TabModel -replaceTab:withTab:keepOldTabOpen: is
always passing NO for keepOldTabOpen value so remove the parameter.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2677263002
Cr-Commit-Position: refs/heads/master@{#448993}

[modify] https://crrev.com/e398e7fd334c08a2cfb805b25b0792fb15e674d3/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/e398e7fd334c08a2cfb805b25b0792fb15e674d3/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/e398e7fd334c08a2cfb805b25b0792fb15e674d3/ios/chrome/browser/ui/browser_view_controller.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2176e15afc8a82813c303e3001a02aad930246a2

commit 2176e15afc8a82813c303e3001a02aad930246a2
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Feb 08 14:58:21 2017

Remove TabModel -replaceWebState: method.

In order to allow refactoring Tab and WebState ownership relationship,
remove the method allowing a test to replace the WebState owned by a
Tab.

The method was only used from two tests suites to allow using a mock
CRWWebController. As the mock is only implementing part of the class
API, registering the tab helpers with the WebState fails.

Instead introduce an additional parameter to the Tab initialiser to
control whether the tab helpers are registered or not. Call this new
initialiser from the two test suites to allow them to control which
tab helpers are initialised.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2685653002
Cr-Commit-Position: refs/heads/master@{#448995}

[modify] https://crrev.com/2176e15afc8a82813c303e3001a02aad930246a2/ios/chrome/browser/tabs/tab.h
[modify] https://crrev.com/2176e15afc8a82813c303e3001a02aad930246a2/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/2176e15afc8a82813c303e3001a02aad930246a2/ios/chrome/browser/tabs/tab_model_unittest.mm
[modify] https://crrev.com/2176e15afc8a82813c303e3001a02aad930246a2/ios/chrome/browser/tabs/tab_private.h
[modify] https://crrev.com/2176e15afc8a82813c303e3001a02aad930246a2/ios/chrome/browser/tabs/tab_unittest.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a762e43411be0bd7426da0d749b7e49d4de47b3

commit 0a762e43411be0bd7426da0d749b7e49d4de47b3
Author: sdefresne <sdefresne@chromium.org>
Date: Tue Feb 14 11:11:25 2017

Extract TabModelObservers from tab_model.mm to its own file.

TabModelObservers is a helper class that allow for forwarding
TabModelObserver events to multiple TabModelObserver instances.
Extract it to a separate file in preparation of refactoring the
TabModel.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2684073002
Cr-Commit-Position: refs/heads/master@{#450320}

[modify] https://crrev.com/0a762e43411be0bd7426da0d749b7e49d4de47b3/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/0a762e43411be0bd7426da0d749b7e49d4de47b3/ios/chrome/browser/tabs/tab_model.mm
[add] https://crrev.com/0a762e43411be0bd7426da0d749b7e49d4de47b3/ios/chrome/browser/tabs/tab_model_observers.h
[add] https://crrev.com/0a762e43411be0bd7426da0d749b7e49d4de47b3/ios/chrome/browser/tabs/tab_model_observers.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf571dff95b062353eacd029bbf7cb545549601a

commit bf571dff95b062353eacd029bbf7cb545549601a
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Feb 15 15:44:08 2017

Remove obsolete code in TabModelOrderController.

The insertion direction in TabModelOrderController could be
controlled by insertionPolicy property however it was never
set to another value after initialisation, thus becoming a
constant.

Remove the enumeration, the property and inline the value in
the different location it was used, removing the alternatives
that are now unreachable.

Refactor -insertionIndexForTab:transition:opener:adjacency
to use early exit instead of using nested if blocks. Remove
obsolete method -firstTabWithOpener: from TabModel.

Modernize the TabModelOrderController API (move the ivar to
the implementation file, init returns instancetype, convert
the file to ARC).

BUG= 687207 

Review-Url: https://codereview.chromium.org/2690893003
Cr-Commit-Position: refs/heads/master@{#450709}

[modify] https://crrev.com/bf571dff95b062353eacd029bbf7cb545549601a/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/bf571dff95b062353eacd029bbf7cb545549601a/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/bf571dff95b062353eacd029bbf7cb545549601a/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/bf571dff95b062353eacd029bbf7cb545549601a/ios/chrome/browser/tabs/tab_model_order_controller.h
[modify] https://crrev.com/bf571dff95b062353eacd029bbf7cb545549601a/ios/chrome/browser/tabs/tab_model_order_controller.mm
[modify] https://crrev.com/bf571dff95b062353eacd029bbf7cb545549601a/ios/chrome/browser/tabs/tab_model_unittest.mm

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7519ff3a7e074f03a8d481c1f63fe3f54431682

commit d7519ff3a7e074f03a8d481c1f63fe3f54431682
Author: sdefresne <sdefresne@chromium.org>
Date: Fri Feb 17 12:32:51 2017

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}

[modify] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/chrome/test/BUILD.gn
[modify] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/DEPS
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/BUILD.gn
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list.h
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list.mm
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper.h
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper.mm
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper_unittest.mm
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list_observer.h
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.mm
[add] https://crrev.com/d7519ff3a7e074f03a8d481c1f63fe3f54431682/ios/shared/chrome/browser/tabs/web_state_list_unittest.mm

The end goal is to have WebStateList API be as close as possible to TabStripModel API. To reduce the size of the CLs and make the transition in incremental steps, the APIs may diverge temporarily

One example, TabStripModel uses the active tab as opener, but correct handling of active tab requires opener-opened relationship, so WebStateList will temporarily require the opener to be passed to the public methods to implement opener-opened relationship without knowledge of active tab, then once active tab is moved to WebStateList, the opener will be checked to be the active tab and eventually removed.
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32c0c38623575d0441969a8a11b23ee540bf28cb

commit 32c0c38623575d0441969a8a11b23ee540bf28cb
Author: sdefresne <sdefresne@chromium.org>
Date: Fri Feb 17 16:37:26 2017

Refactor TabModel to use WebStateList to store WebStates.

Refactor TabModel to use WebStateList to store WebStates to share
code with the new architecture. Keep a NSSet<Tab*> to retain the
Tabs (as TabModel owns them and WebStateList cannot).

BUG= 687207 

Review-Url: https://codereview.chromium.org/2683393003
Cr-Commit-Position: refs/heads/master@{#451319}

[modify] https://crrev.com/32c0c38623575d0441969a8a11b23ee540bf28cb/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/32c0c38623575d0441969a8a11b23ee540bf28cb/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/32c0c38623575d0441969a8a11b23ee540bf28cb/ios/chrome/browser/tabs/tab_model_order_controller_unittest.mm

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 20 2017

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0a9ba6076a8522976835cafac5e389aab66a43e

commit e0a9ba6076a8522976835cafac5e389aab66a43e
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Feb 23 09:14:11 2017

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}

[modify] https://crrev.com/e0a9ba6076a8522976835cafac5e389aab66a43e/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/e0a9ba6076a8522976835cafac5e389aab66a43e/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/e0a9ba6076a8522976835cafac5e389aab66a43e/ios/chrome/browser/tabs/tab_model_unittest.mm
[modify] https://crrev.com/e0a9ba6076a8522976835cafac5e389aab66a43e/ios/shared/chrome/browser/tabs/web_state_list.h
[modify] https://crrev.com/e0a9ba6076a8522976835cafac5e389aab66a43e/ios/shared/chrome/browser/tabs/web_state_list.mm
[modify] https://crrev.com/e0a9ba6076a8522976835cafac5e389aab66a43e/ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper_unittest.mm
[modify] https://crrev.com/e0a9ba6076a8522976835cafac5e389aab66a43e/ios/shared/chrome/browser/tabs/web_state_list_unittest.mm

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef097a7cd0861251cd05ee6adfe0293eeb56197e

commit ef097a7cd0861251cd05ee6adfe0293eeb56197e
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Feb 23 15:41:04 2017

Add WebStateListOrderController to control WebState insertion.

Introduce WebStateListOrderController to control the position of
insertion of WebState and refactor TabModel to use (using new API
of WebStateList).

BUG= 687207 

Review-Url: https://codereview.chromium.org/2699833004
Cr-Commit-Position: refs/heads/master@{#452496}

[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/chrome/browser/tabs/tab_model_order_controller.h
[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/chrome/browser/tabs/tab_model_order_controller.mm
[delete] https://crrev.com/ef704d88025002793b942e1df5b57e312ada65a0/ios/chrome/browser/tabs/tab_model_order_controller_unittest.mm
[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/shared/DEPS
[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/shared/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/shared/chrome/browser/tabs/web_state_list.h
[modify] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/shared/chrome/browser/tabs/web_state_list.mm
[add] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/shared/chrome/browser/tabs/web_state_list_order_controller.h
[add] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/shared/chrome/browser/tabs/web_state_list_order_controller.mm
[add] https://crrev.com/ef097a7cd0861251cd05ee6adfe0293eeb56197e/ios/shared/chrome/browser/tabs/web_state_list_order_controller_unittest.mm

Project Member

Comment 16 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/073e0546fc624d92d47dcf4778fdce5a2391f835

commit 073e0546fc624d92d47dcf4778fdce5a2391f835
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Feb 23 16:34:32 2017

WebStateListObserver methods have default implementation.

As most of the observer are only interested in a sub-set of the
events and to mirror the Objective-C API (WebStateListObserving)
that marks all methods as optional, add empty implementation to
WebStateList methods.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2708733003
Cr-Commit-Position: refs/heads/master@{#452516}

[modify] https://crrev.com/073e0546fc624d92d47dcf4778fdce5a2391f835/ios/chrome/browser/tabs/tab_parenting_observer.h
[modify] https://crrev.com/073e0546fc624d92d47dcf4778fdce5a2391f835/ios/chrome/browser/tabs/tab_parenting_observer.mm
[modify] https://crrev.com/073e0546fc624d92d47dcf4778fdce5a2391f835/ios/shared/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/073e0546fc624d92d47dcf4778fdce5a2391f835/ios/shared/chrome/browser/tabs/web_state_list_metrics_observer.h
[modify] https://crrev.com/073e0546fc624d92d47dcf4778fdce5a2391f835/ios/shared/chrome/browser/tabs/web_state_list_metrics_observer.mm
[modify] https://crrev.com/073e0546fc624d92d47dcf4778fdce5a2391f835/ios/shared/chrome/browser/tabs/web_state_list_observer.h
[add] https://crrev.com/073e0546fc624d92d47dcf4778fdce5a2391f835/ios/shared/chrome/browser/tabs/web_state_list_observer.mm

Project Member

Comment 17 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/419718939c4af924bcc43dea66656ec03419cb7d

commit 419718939c4af924bcc43dea66656ec03419cb7d
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Feb 23 17:24:14 2017

Change creation order of TabModel observers.

In preparation of future refactoring, create the TabModel observers
after setting the other ivar and creating the other internal objects
(in particular the TabUsageRecorder).

BUG= 687207 

Review-Url: https://codereview.chromium.org/2706253002
Cr-Commit-Position: refs/heads/master@{#452533}

[modify] https://crrev.com/419718939c4af924bcc43dea66656ec03419cb7d/ios/chrome/browser/tabs/tab_model.mm

Project Member

Comment 18 by bugdroid1@chromium.org, Feb 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c97b05e65f15da1e23689dceb3f9c4c01e5f4212

commit c97b05e65f15da1e23689dceb3f9c4c01e5f4212
Author: sdefresne <sdefresne@chromium.org>
Date: Fri Feb 24 13:47:16 2017

Remove stub for TabModel method never called in the test.

The -addTabWithURL:referrer:windowName: method from TabModel is never
invoked from any of the BrowserViewControllerTest tests, so remove the
stub (in preparation of removal of the stubbed method).

Convert C-style casts to static_cast<> as C-style cast are forbidden
by the style guide.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2715733002
Cr-Commit-Position: refs/heads/master@{#452809}

[modify] https://crrev.com/c97b05e65f15da1e23689dceb3f9c4c01e5f4212/ios/chrome/browser/ui/browser_view_controller_unittest.mm

Project Member

Comment 19 by bugdroid1@chromium.org, Feb 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a504b117059bfa27ecbf7e061c380e1f91c54018

commit a504b117059bfa27ecbf7e061c380e1f91c54018
Author: sdefresne <sdefresne@chromium.org>
Date: Tue Feb 28 21:58:37 2017

Refactor serialisation of openerId & openerNavigationIndex.

The two properties openerId and openerNavigationIndex of the class
CRWSessionController were never used in ios/web/ (as they are used
to serialize ios/chrome/ notion of opener of a Tab) so refactor
them to use serializable user data.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2720613005
Cr-Commit-Position: refs/heads/master@{#453715}

[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/chrome/browser/native_app_launcher/native_app_navigation_util_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/chrome/browser/sessions/session_window_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/chrome/browser/tabs/tab.h
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/chrome/browser/tabs/tab_model_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/crw_session_controller+private_constructors.h
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/crw_session_controller.h
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/crw_session_controller.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/crw_session_controller_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/crw_session_storage_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/navigation_manager_impl.h
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/navigation_manager_impl.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/navigation_manager_impl_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/serializable_user_data_manager_impl.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/navigation/session_storage_builder.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/public/crw_session_storage.h
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/public/crw_session_storage.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/public/test/web_test_with_web_state.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/web_state/web_state_impl_unittest.mm
[modify] https://crrev.com/a504b117059bfa27ecbf7e061c380e1f91c54018/ios/web/webui/web_ui_mojo_inttest.mm

Project Member

Comment 20 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/977ff883dfd769b8b79bdb73ed3544b5fa971abb

commit 977ff883dfd769b8b79bdb73ed3544b5fa971abb
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Mar 01 01:35:51 2017

Fix WebStateListObserver::WebStateDetachedAt() parameter lifetime.

The WebState pointer passed to WebStateListObserver::WebStateDetachedAt()
was a pointer to deallocated memory if WebStateList managed the ownership
of the WebState because destroying the WebStateListWrapper deallocated it.

Change WebStateList::DetachWebStateAt() to release the ownership of the
WebState and mark the method (and WebStateList::ReplaceWebStateAt) with
WARN_UNUSED_RESULT so that compiler ensure client code uses the returned
value (by passing it to a std::unique_ptr<> for example).

Change how the lifetime of the WebState is managed in WebStateList to
avoid making the same class of errors in the future (still not optimal
as the best would be to use std::unique_ptr<> but it is not possible
until Tab* and WebState* ownership is reversed).

BUG= 687207 

Review-Url: https://codereview.chromium.org/2710183003
Cr-Commit-Position: refs/heads/master@{#453790}

[modify] https://crrev.com/977ff883dfd769b8b79bdb73ed3544b5fa971abb/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/977ff883dfd769b8b79bdb73ed3544b5fa971abb/ios/shared/chrome/browser/tabs/web_state_list.h
[modify] https://crrev.com/977ff883dfd769b8b79bdb73ed3544b5fa971abb/ios/shared/chrome/browser/tabs/web_state_list.mm
[modify] https://crrev.com/977ff883dfd769b8b79bdb73ed3544b5fa971abb/ios/shared/chrome/browser/tabs/web_state_list_unittest.mm

Project Member

Comment 21 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f7781ce46951904a2615fbda44d2a9f4ced23ca

commit 2f7781ce46951904a2615fbda44d2a9f4ced23ca
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Mar 02 19:12:46 2017

Move the notion of current Tab from TabModel to WebStateList.

Add methods to track and set the currently active WebState in the
WebStateList (referenced by index) and corresponding events when
the active WebState is changed (explicitly or automatically after
detaching a WebState).

Add new WebStateListObserver (and WebStateListObserving when the
observer need to keep a __weak pointer to an Objective-C class)
that react to the event send when the active WebState changes.

Add a -loadFinished property to Tab to avoid accessing the private
//ios/web private API in new code.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2703333006
Cr-Commit-Position: refs/heads/master@{#454329}

[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/metrics/BUILD.gn
[add] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/metrics/tab_usage_recorder_web_state_list_observer.h
[add] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/metrics/tab_usage_recorder_web_state_list_observer.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/snapshots/BUILD.gn
[add] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/snapshots/snapshot_cache_web_state_list_observer.h
[add] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/snapshots/snapshot_cache_web_state_list_observer.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/tabs/tab.h
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/tabs/tab_model_observers_bridge.mm
[delete] https://crrev.com/e30455327cf0108d83b431127908c465e71d98b1/ios/chrome/browser/tabs/tab_model_order_controller.h
[delete] https://crrev.com/e30455327cf0108d83b431127908c465e71d98b1/ios/chrome/browser/tabs/tab_model_order_controller.mm
[add] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/tabs/tab_model_selected_tab_observer.h
[add] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list.h
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_metrics_observer.h
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_metrics_observer.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_observer.h
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_observer.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.h
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_observer_bridge.mm
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_order_controller.h
[modify] https://crrev.com/2f7781ce46951904a2615fbda44d2a9f4ced23ca/ios/shared/chrome/browser/tabs/web_state_list_order_controller.mm

Project Member

Comment 22 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8dd0cb3cdf573d6ea0021f2962ef8116c9e80857

commit 8dd0cb3cdf573d6ea0021f2962ef8116c9e80857
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Mar 02 19:44:01 2017

Convert TabModelTest to use the public API of TabModel.

Give a unique window name to all the inserted Tabs and disable web
usage on the TabModel. With those changes it is possible to restrict
TabModelTest to use the public API of TabModel.

Using the public API makes it easier to refactor the code (as there
is no dependency on the implementation or hole into it from the test).

BUG= 687207 

Review-Url: https://codereview.chromium.org/2715753002
Cr-Commit-Position: refs/heads/master@{#454345}

[modify] https://crrev.com/8dd0cb3cdf573d6ea0021f2962ef8116c9e80857/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/8dd0cb3cdf573d6ea0021f2962ef8116c9e80857/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/8dd0cb3cdf573d6ea0021f2962ef8116c9e80857/ios/chrome/browser/tabs/tab_model_unittest.mm

Components: UI>Browser>WebUI
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/365a73ab9da546dc6898bef703b688c82c292527

commit 365a73ab9da546dc6898bef703b688c82c292527
Author: sdefresne <sdefresne@chromium.org>
Date: Tue Mar 14 15:07:13 2017

[ios] Add a delegate to WebStateList class.

In order to attach the tab helpers to WebState when they are inserted
in the WebStateList, without having the list of tab helpers known to
the WebStateList.

BUG= 687207 

Review-Url: https://codereview.chromium.org/2748793002
Cr-Commit-Position: refs/heads/master@{#456705}

[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/chrome/browser/tabs/BUILD.gn
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/chrome/browser/tabs/tab_model.mm
[add] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/chrome/browser/tabs/tab_model_web_state_list_delegate.h
[add] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/chrome/browser/tabs/tab_model_web_state_list_delegate.mm
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/clean/chrome/browser/model/BUILD.gn
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/clean/chrome/browser/model/browser.h
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/clean/chrome/browser/model/browser.mm
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/clean/chrome/browser/model/browser_list.h
[add] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/clean/chrome/browser/model/browser_web_state_list_delegate.h
[add] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/clean/chrome/browser/model/browser_web_state_list_delegate.mm
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/BUILD.gn
[add] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/fake_web_state_list_delegate.h
[add] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/fake_web_state_list_delegate.mm
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/web_state_list.h
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/web_state_list.mm
[add] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/web_state_list_delegate.h
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper_unittest.mm
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/web_state_list_order_controller_unittest.mm
[modify] https://crrev.com/365a73ab9da546dc6898bef703b688c82c292527/ios/shared/chrome/browser/tabs/web_state_list_unittest.mm

Project Member

Comment 25 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0c3c780c8297209abc2a7e8f9b2efb63d064436

commit f0c3c780c8297209abc2a7e8f9b2efb63d064436
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Thu Aug 10 13:47:35 2017

Remove unused methods from TabModel.

Those method are now part of WebStateList and can be removed from
TabModel (the only callsite where in the unit test). WebStateList
unit tests already have the same tests, so no need to add any new
test there.

Bug:  687207 
Change-Id: I78d1cd9c363b71bf58201debfd889f5bca86ae42
Reviewed-on: https://chromium-review.googlesource.com/610142
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493373}
[modify] https://crrev.com/f0c3c780c8297209abc2a7e8f9b2efb63d064436/ios/chrome/browser/tabs/tab_model.h
[modify] https://crrev.com/f0c3c780c8297209abc2a7e8f9b2efb63d064436/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/f0c3c780c8297209abc2a7e8f9b2efb63d064436/ios/chrome/browser/tabs/tab_model_unittest.mm

Status: WontFix (was: Started)

Sign in to add a comment