[iOS]Add a delegate to sync encryption view [chromium/src : main]
1 view
Skip to first unread message
Arthur Milchior (Gerrit)
Jul 3, 2025, 7:09:24 AM7/3/25
to Jérôme Lebel, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Jérôme Lebel
Arthur Milchior voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
Attention is currently required from:
- Jérôme Lebel
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jérôme Lebel (Gerrit)
Jul 4, 2025, 5:15:09 AM7/4/25
to Arthur Milchior, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Arthur Milchior
Jérôme Lebel voted and added 6 comments![]( "Open in Gerrit")
Votes added by Jérôme Lebel
| Code-Review | +1 |
6 comments
Patchset-level comments
File ios/chrome/browser/settings/ui_bundled/settings_navigation_controller.mm
Line 310, Patchset 4 (Latest):
navigationController->_syncEncryptionPassphraseTableViewController =Jérôme Lebel . unresolved
We should avoid doing that. I guess it should be a property.
Line 1171, Patchset 4 (Latest):
SyncEncryptionPassphraseTableViewController* controller =Jérôme Lebel . unresolved
Why do you use `_syncEncryptionPassphraseTableViewController`? I would imagine the CHECK in line 1316 should fail?
File ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller.h
Line 44, Patchset 4 (Latest):
@property(weak, nonatomic)Jérôme Lebel . unresolved
We usually put nonatomic before weak.
Line 17, Patchset 4 (Latest):
@protocol SyncEncryptionPassphraseTableViewControllerPresentationDelegateJérôme Lebel . unresolved
This should inherit from `NSObject`.
File ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller.mm
Line 592, Patchset 4 (Latest):
if (_settingsAreDismissed) {Jérôme Lebel . unresolved
So I guess with your changes this method will often be called twice. I think this is not really a problem. But we can remove the comment now?
Related details
Attention is currently required from:
- Arthur Milchior
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jérôme Lebel (Gerrit)
Jul 4, 2025, 7:26:59 AM7/4/25
to Arthur Milchior, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Arthur Milchior
Jérôme Lebel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jérôme Lebel
| Code-Review | +0 |
1 comment
File ios/chrome/browser/settings/ui_bundled/google_services/manage_sync_settings_coordinator.mm
Line 556, Patchset 4 (Latest):
controllerToPush = _syncEncryptionTableViewController =Jérôme Lebel . unresolved
There is an issue here. You set `_syncEncryptionTableViewController` but we never set it to nil. Therefore, with your patch, it is not possible to open twice `SyncEncryptionTableViewController`.
Related details
Attention is currently required from:
- Arthur Milchior
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Milchior (Gerrit)
Jul 7, 2025, 10:36:53 AM7/7/25
to Jérôme Lebel, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Jérôme Lebel
Arthur Milchior voted and added 6 comments![]( "Open in Gerrit")
Votes added by Arthur Milchior
| Auto-Submit | +1 |
6 comments
File ios/chrome/browser/settings/ui_bundled/google_services/manage_sync_settings_coordinator.mm
Line 556, Patchset 4 (Latest):
controllerToPush = _syncEncryptionTableViewController =Jérôme Lebel . resolved
There is an issue here. You set `_syncEncryptionTableViewController` but we never set it to nil. Therefore, with your patch, it is not possible to open twice `SyncEncryptionTableViewController`.
Arthur Milchior
Oh, you are right. I remove the early return for `_syncEncryptionTableViewController` then. It’ll be in a different CL, this one is already big enough I believe
File ios/chrome/browser/settings/ui_bundled/settings_navigation_controller.mm
Line 310, Patchset 4 (Latest):
navigationController->_syncEncryptionPassphraseTableViewController =Jérôme Lebel . resolved
We should avoid doing that. I guess it should be a property.
Arthur Milchior
Done
Line 1171, Patchset 4 (Latest):
SyncEncryptionPassphraseTableViewController* controller =Jérôme Lebel . unresolved
Arthur MilchiorWhy do you use `_syncEncryptionPassphraseTableViewController`? I would imagine the CHECK in line 1316 should fail?
I assume your question was expected to be "Why don’t you use". Appart from that, you’re quite right, thanks.
File ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller.h
Line 44, Patchset 4 (Latest):
@property(weak, nonatomic)Jérôme Lebel . resolved
We usually put nonatomic before weak.
Arthur Milchior
Done
Line 17, Patchset 4 (Latest):
@protocol SyncEncryptionPassphraseTableViewControllerPresentationDelegateJérôme Lebel . resolved
This should inherit from `NSObject`.
Arthur Milchior
Done
File ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller.mm
Line 592, Patchset 4 (Latest):
if (_settingsAreDismissed) {Jérôme Lebel . resolved
So I guess with your changes this method will often be called twice. I think this is not really a problem. But we can remove the comment now?
Attention is currently required from:
- Jérôme Lebel
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jérôme Lebel (Gerrit)
Jul 8, 2025, 5:21:40 AM7/8/25
to Arthur Milchior, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Arthur Milchior
Jérôme Lebel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jérôme Lebel
| Code-Review | +1 |
1 comment
File ios/chrome/browser/settings/ui_bundled/settings_navigation_controller.mm
Line 1171, Patchset 4:
SyncEncryptionPassphraseTableViewController* controller =Jérôme Lebel . resolved
Arthur MilchiorWhy do you use `_syncEncryptionPassphraseTableViewController`? I would imagine the CHECK in line 1316 should fail?
I assume your question was expected to be "Why don’t you use". Appart from that, you’re quite right, thanks.
Attention is currently required from:
- Arthur Milchior
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Milchior (Gerrit)
Jul 8, 2025, 3:44:34 PM7/8/25
to Jérôme Lebel, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Arthur Milchior voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jul 8, 2025, 4:28:18 PM7/8/25
to Arthur Milchior, Jérôme Lebel, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[iOS]Add a delegate to sync encryption view
If the sync encryption view is opened twice quickly (with a double tap),
and `settingsWillBeDismissed` is called on the first ViewController,
`viewDidLoad` will be executed after the `settingsWillBeDismissed`,
causing for a crash due to access to `self.browser` which became null.
By ensuring that the view is only opened once, this crash is avoided.
There is still the issue that if we simultaneously dismiss this view and
tap on the account menu, the account menu get closed.
However, it’s not clear it’s related, and at least, now, we have a
closed menu instead of an access to a null pointer.
Bug: 428895047
Change-Id: Iffeb09658f0d4f86abdac83532409dd0da8e34f8
Auto-Submit: Arthur Milchior <arthurm...@chromium.org>
Reviewed-by: Jérôme Lebel <jle...@chromium.org>
Commit-Queue: Arthur Milchior <arthurm...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1483964}
Files:
- M ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_coordinator.mm
- M ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_coordinator_unittests.mm
- M ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_mediator.h
- M ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_mediator.mm
- M ios/chrome/browser/authentication/ui_bundled/account_menu/account_menu_mediator_unittests.mm
- M ios/chrome/browser/settings/ui_bundled/google_services/manage_sync_settings_coordinator.mm
- M ios/chrome/browser/settings/ui_bundled/settings_navigation_controller.h
- M ios/chrome/browser/settings/ui_bundled/settings_navigation_controller.mm
- M ios/chrome/browser/settings/ui_bundled/sync/BUILD.gn
- M ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller.h
- M ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller.mm
- A ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller_presentation_delegate.h
- M ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_passphrase_table_view_controller_unittest.mm
- M ios/chrome/browser/settings/ui_bundled/sync/sync_encryption_table_view_controller.mm
Change size: M
Delta: 14 files changed, 142 insertions(+), 24 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Jérôme Lebel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages