New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 754642 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task



Sign in to add a comment

Remove use of TopPresentedViewController()

Project Member Reported by marq@chromium.org, Aug 11 2017

Issue description

TopPresetedViewController() and TopPresetedViewControllerFrom() allow arbitrary code to add arbitrary view controllers to the UI. This is adds to the general problem of managing presented state, and their use makes refactoring into a new UI architecture more difficult.

Use of these functions, especially when they are followed by presenting a new view controller on top of the view controller returned by them, needs to be replaced with a more structured placement of the desired view controller.

Once other uses of these functions are removed, MainController, which is currently where dismissing all "presented state" is handled, can continue to use these functions privately for the purpose of unwinding the view controller stack -- but not for adding to it.
 
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/1ab47f2c9a7f7778d5e9ae76fa71206f5ea0c4e8

commit 1ab47f2c9a7f7778d5e9ae76fa71206f5ea0c4e8
Author: marq <marq@google.com>
Date: Fri Aug 11 11:41:52 2017

Components: UI>Browser>Core
Components: -UI>Browser>Core Internals

Comment 5 by marq@chromium.org, Nov 7 2017

Cc: marq@chromium.org
Owner: edchin@chromium.org
Handing this off to Ed, who's actually working on it.
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 9 2017

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

commit f7b41e982ae99b0804bbf2c6d4fd9ec00118d812
Author: Rohit Rao <rohitrao@chromium.org>
Date: Thu Nov 09 00:17:45 2017

[ios] Fixes AlertCoordinatorTest to present on top of the correct VC.

This CL introduces new calls to TopPresentedViewController() that will need to
be fixed in the future, but it is better to make this dependency explicit for
now.

BUG=754642

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I62f2e6d1d11f7c77f78d10eefd347ff78f49069e
Reviewed-on: https://chromium-review.googlesource.com/758566
Commit-Queue: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515026}
[modify] https://crrev.com/f7b41e982ae99b0804bbf2c6d4fd9ec00118d812/ios/chrome/browser/ui/alert_coordinator/BUILD.gn
[modify] https://crrev.com/f7b41e982ae99b0804bbf2c6d4fd9ec00118d812/ios/chrome/browser/ui/alert_coordinator/alert_coordinator_egtest.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 9 2017

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

commit e7eeafd9a2eb8c73c7bb7563e690b2c59b3c9dd7
Author: edchin <edchin@chromium.org>
Date: Thu Nov 09 09:52:33 2017

[ios] Remove unused -showPasswordSettings

-showPasswordSettings was left unused after password_generation_agent
was removed.

Bug: 754642
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I21b391b5fd45fcef117c0f5fc1b6fc14af9966fe
Reviewed-on: https://chromium-review.googlesource.com/759218
Reviewed-by: edchin <edchin@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515127}
[modify] https://crrev.com/e7eeafd9a2eb8c73c7bb7563e690b2c59b3c9dd7/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/e7eeafd9a2eb8c73c7bb7563e690b2c59b3c9dd7/ios/chrome/browser/ui/commands/application_commands.h
[modify] https://crrev.com/e7eeafd9a2eb8c73c7bb7563e690b2c59b3c9dd7/ios/clean/chrome/browser/ui/adaptor/application_commands_adaptor.mm

Comment 8 by marq@chromium.org, Nov 9 2017

Summary: Remove use of TopPresentedViewController() (was: Remove use of TopPresetedViewController())
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 11 2017

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

commit 57eb740bffca754829243d93fb781fec0993080f
Author: Rohit Rao <rohitrao@chromium.org>
Date: Sat Nov 11 20:05:51 2017

[ios] Fixes the translate infobar to present on top of the correct VC.

This CL introduces new calls to TopPresentedViewController() that will need to
be fixed in the future, but it is better to make this dependency explicit for
now.

BUG=754642

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: If67343f6424d3bd043825524d32d2a4f693cc1f6
Reviewed-on: https://chromium-review.googlesource.com/764650
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515852}
[modify] https://crrev.com/57eb740bffca754829243d93fb781fec0993080f/ios/chrome/browser/translate/BUILD.gn
[modify] https://crrev.com/57eb740bffca754829243d93fb781fec0993080f/ios/chrome/browser/translate/before_translate_infobar_controller.mm

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 14 2017

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

commit c9a4b056eaece89e4023051d611578928f6224ec
Author: edchin <edchin@chromium.org>
Date: Tue Nov 14 19:46:05 2017

[ios] Remove -topPresentedViewController from signin_interaction

-topPresentedViewController was used to present signin_interaction
above identity_interaction. Instead of doing the "VC walk", we
now keep track of the top view controller whenever identity_interaction
calls its delegate method to present a view controller.

Bug: 754642
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Idc9281c76953307741253bdf515f7fd7ed9d8b35
Reviewed-on: https://chromium-review.googlesource.com/760237
Commit-Queue: edchin <edchin@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516395}
[modify] https://crrev.com/c9a4b056eaece89e4023051d611578928f6224ec/ios/chrome/browser/ui/signin_interaction/BUILD.gn
[modify] https://crrev.com/c9a4b056eaece89e4023051d611578928f6224ec/ios/chrome/browser/ui/signin_interaction/signin_interaction_controller.mm
[modify] https://crrev.com/c9a4b056eaece89e4023051d611578928f6224ec/ios/chrome/browser/ui/signin_interaction/signin_interaction_coordinator.mm
[modify] https://crrev.com/c9a4b056eaece89e4023051d611578928f6224ec/ios/chrome/browser/ui/signin_interaction/signin_interaction_presenting.h

Status: Assigned (was: Started)
This is no longer in active development so I'm setting to assigned, rather than started. Many instances of TopViewController() have been removed thus far. There are still instances that cannot be removed at this time. 

Sign in to add a comment