Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(855)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2740233002: Remove CRWSessionController usage from BrowserViewController. (Closed)

Created:
3 years, 9 months ago by kkhorimoto
Modified:
3 years, 9 months ago
CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, Eugene But (OOO till 7-30), pkl (ping after 24h if needed), ios-reviews+web_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove CRWSessionController usage from BrowserViewController. BUG=454984

Patch Set 1 #

Total comments: 13

Patch Set 2 : Created separate CLs for NavigationManager work #

Patch Set 3 : Eugene's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -27 lines) Patch
M ios/chrome/browser/ui/DEPS View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M ios/chrome/browser/ui/browser_view_controller.mm View 1 2 4 chunks +14 lines, -20 lines 0 comments Download
M ios/chrome/browser/ui/browser_view_controller_unittest.mm View 1 2 2 chunks +2 lines, -5 lines 0 comments Download

Depends on Patchset:

Issue 2745873002 Patch 1

Messages

Total messages: 5 (2 generated)
kkhorimoto
3 years, 9 months ago (2017-03-10 03:47:51 UTC) #2
Eugene But (OOO till 7-30)
Do you want to split - GetBackward/ForwardItems and InsertStateFromManager additions into 2 separate CLs? GetBackward/ForwardItems ...
3 years, 9 months ago (2017-03-10 16:23:27 UTC) #3
kkhorimoto
3 years, 9 months ago (2017-03-10 23:02:20 UTC) #5 
https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (left):

https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:174: #import
"ios/web/navigation/crw_session_controller.h"
On 2017/03/10 16:23:27, Eugene But wrote:
> Could you please cleanup ios/chrome/browser/ui/DEPS as well

Done.

https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:3403:
DCHECK(navigationManager);
On 2017/03/10 16:23:27, Eugene But wrote:
> nit: There is no need for DCHECK |navigationManager->GetBackwardItems()| will
> crash anyway

Done.

https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:3620:
DCHECK(newNavigationManager);
On 2017/03/10 16:23:27, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller_unittest.mm (right):

https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller_unittest.mm:176:
webStateImpl_->GetNavigationManagerImpl().InitializeSession(false);
On 2017/03/10 16:23:27, Eugene But wrote:
> nit: webStateImpl_->GetNavigationManagerImpl().InitializeSession(NO
> /*opened_by_dom*/);

Done.

https://codereview.chromium.org/2740233002/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller_unittest.mm:176:
webStateImpl_->GetNavigationManagerImpl().InitializeSession(false);
On 2017/03/10 16:23:27, Eugene But wrote:
> nit: webStateImpl_->GetNavigationManagerImpl().InitializeSession(NO
> /*opened_by_dom*/);

Done.

https://codereview.chromium.org/2740233002/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2740233002/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:155: virtual NavigationItemList
GetBackwardItems() const = 0;
On 2017/03/10 16:23:27, Eugene But wrote:
> Could you please add tests for these.

They already exist in crw_session_controller_unittest.mm.  I'll transfer them
over to NavigationManager unittests once the functionality is actually handled
by that class; currently it's just a pass-through to CRWSessionController.

https://codereview.chromium.org/2740233002/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:164: // Inserts copies of
|other_controller|'s NavigationItems to the front of this
On 2017/03/10 16:23:27, Eugene But wrote:
> NavigationController does not have this method, so do we need it in navigation
> manager? This very method is used for preload feature. Do you know how preload
> is implemented on other platforms? CopyStateFromAndPrune could be API we need.

Will do this on the other CL.

Powered by Google App Engine
This is Rietveld 408576698