[iOS] Split LayoutState in SceneLayoutState and BrowserLayoutState [chromium/src : main]

0 views

Skip to first unread message

Federica Germinario (Gerrit)

unread,

5:41 AM (2 hours ago) 5:41 AM

to Gauthier Ambard, Mark Cogan, Chromium LUCI CQ, chromium...@chromium.org, christia...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Gauthier Ambard and Mark Cogan

Federica Germinario voted and added 4 comments

Votes added by Federica Germinario

Code-Review

4 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Federica Germinario . resolved

Overall change LGTM

File ios/chrome/browser/app_bar/coordinator/app_bar_coordinator.mm

Line 181, Patchset 7 (Latest): _containerViewController.browserLayoutState =

Federica Germinario . unresolved

Should this coordinator update the container's `browserLayoutState` when the active browser changes? (in case incognito browser has a different toolbar position compared to regular)

File ios/chrome/browser/shared/coordinator/scene/state/scene_layout_state.h

Line 32, Patchset 7 (Latest):- (void)layoutState:(SceneLayoutState*)layoutState

Federica Germinario . unresolved

nit: Since the class is now `SceneLayoutState`, consider renaming these to `sceneLayoutState:...` (to be also consistent with `BrowserLayoutStateObserver`).

File ios/chrome/browser/toolbar/coordinator/main_toolbar_coordinator.mm

Line 1190, Patchset 7 (Latest): // LegacyToolbarMediator). Update the SceneLayoutState to keep it in sync.

Federica Germinario . unresolved

Nit: This comment mentions `SceneLayoutState`, but the code below now updates the `BrowserLayoutState`.

Open in Gerrit

Related details

Attention is currently required from:

Gauthier Ambard
Mark Cogan

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

7:15 AM (8 minutes ago) 7:15 AM

to Federica Germinario, Mark Cogan, Chromium LUCI CQ, chromium...@chromium.org, christia...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Mark Cogan

Gauthier Ambard voted and added 3 comments

Votes added by Gauthier Ambard

Commit-Queue

3 comments

File ios/chrome/browser/app_bar/coordinator/app_bar_coordinator.mm

Line 181, Patchset 7: _containerViewController.browserLayoutState =

Federica Germinario . resolved

Should this coordinator update the container's `browserLayoutState` when the active browser changes? (in case incognito browser has a different toolbar position compared to regular)

Gauthier Ambard

Done

File ios/chrome/browser/shared/coordinator/scene/state/scene_layout_state.h

Line 32, Patchset 7:- (void)layoutState:(SceneLayoutState*)layoutState

Federica Germinario . resolved

nit: Since the class is now `SceneLayoutState`, consider renaming these to `sceneLayoutState:...` (to be also consistent with `BrowserLayoutStateObserver`).

Gauthier Ambard

This CL is already big, I will do it in a future CL.

File ios/chrome/browser/toolbar/coordinator/main_toolbar_coordinator.mm

Line 1190, Patchset 7: // LegacyToolbarMediator). Update the SceneLayoutState to keep it in sync.

Federica Germinario . resolved

Nit: This comment mentions `SceneLayoutState`, but the code below now updates the `BrowserLayoutState`.

Gauthier Ambard

Done

Open in Gerrit

Related details

Attention is currently required from:

Mark Cogan

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages