Angela Novakovic (Gerrit)

unread,

Feb 27, 2026, 1:03:49 PM (6 days ago) Feb 27

to AyeAye, Alexis Hétu, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Alexis Hétu

Angela Novakovic voted and added 5 comments

Votes added by Angela Novakovic

Commit-Queue

+1

5 comments

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/quick_delete_other_data_view_controller.mm

Line 8, Patchset 6 (Latest):#import "ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/quick_delete_util.h"

Angela Novakovic . unresolved

Typically, we don't want UI to depend on coordinator logic. Should I move the quick_delete_util to a public folder then?

Line 232, Patchset 6 (Latest):- (void)setShouldShowSearchHistoryCell:(BOOL)shouldShowSearchHistoryCell {

Angela Novakovic . unresolved

I've removed the early return in this method as I want it to always reach the snapshot update logic to update both the "Search history" cell and the "My Activity" cell. It results in a a cleaner UI update when both must be updated (eg. removed).

See this link to see how it was behaving previously: https://drive.google.com/file/d/17wD4v791R30aVqWBRR-3Rxp893uxqZXe/view?usp=sharing&resourcekey=0-1nr-nLiVxDdFRLxWOMbMng

Line 243, Patchset 5:

      if ([_otherDataPageTitle
              isEqualToString:l10n_util::GetNSString(
                                  IDS_SETTINGS_OTHER_GOOGLE_DATA_TITLE)]) {

Alexis Hétu . resolved

1) Because of translations, we can't rely on the value of strings. You'll need to pass in a type of some sort.
2) prefer early returns to writing code inside an if statement's scope if possible.

Angela Novakovic

Done

Line 251, Patchset 5: break;

Alexis Hétu . resolved

return? otherwise you would hit the NOTREACHED() below.

Angela Novakovic

Done

Line 259, Patchset 5: NOTREACHED();

Alexis Hétu . resolved

wrong location, this should be one line below, I think.

Angela Novakovic

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

Alexis Hétu

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

Alexis Hétu (Gerrit)

unread,

Feb 27, 2026, 1:37:11 PM (6 days ago) Feb 27

to Angela Novakovic, AyeAye, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Angela Novakovic

Alexis Hétu added 5 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Alexis Hétu . unresolved

Overall question: I just realized the files are in:
`ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/`
why aren't they in:
`ios/chrome/browser/settings/clear_browsing_data/quick_delete_other_data/`

If they use the new ui/public/coordinator/test structure, why are they still located in `ui_bundled`?

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/quick_delete_other_data_view_controller.mm

Line 8, Patchset 6 (Latest):#import "ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/quick_delete_util.h"

Angela Novakovic . unresolved

Typically, we don't want UI to depend on coordinator logic. Should I move the quick_delete_util to a public folder then?

Alexis Hétu

yes

Line 232, Patchset 6 (Latest):- (void)setShouldShowSearchHistoryCell:(BOOL)shouldShowSearchHistoryCell {

Angela Novakovic . resolved

I've removed the early return in this method as I want it to always reach the snapshot update logic to update both the "Search history" cell and the "My Activity" cell. It results in a a cleaner UI update when both must be updated (eg. removed).
See this link to see how it was behaving previously: https://drive.google.com/file/d/17wD4v791R30aVqWBRR-3Rxp893uxqZXe/view?usp=sharing&resourcekey=0-1nr-nLiVxDdFRLxWOMbMng

Alexis Hétu

Acknowledged

Line 337, Patchset 6 (Latest):

      cell.accessoryView = [[UIImageView alloc]
          initWithImage:DefaultAccessorySymbolConfigurationWithRegularWeight(
                            kExternalLinkSymbol)];
      cell.accessoryView.tintColor = [UIColor colorNamed:kGrey500Color];
      return;
    case ItemIdentifier::kMyActivityIdentifier: {

Alexis Hétu . unresolved

[[fallthrough]];

Line 349, Patchset 6 (Latest): NOTREACHED();

Alexis Hétu . unresolved

still in the wrong location, this should be AFTER the `}` on the next line

Open in Gerrit

Related details

Attention is currently required from:

Angela Novakovic

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

Angela Novakovic (Gerrit)

unread,

Mar 2, 2026, 1:34:59 PM (3 days ago) Mar 2

to Tommy Martino, AyeAye, Alexis Hétu, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Tommy Martino

Angela Novakovic added 4 comments

Patchset-level comments

File-level comment, Patchset 6:

Alexis Hétu . unresolved

Overall question: I just realized the files are in:
`ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/`
why aren't they in:
`ios/chrome/browser/settings/clear_browsing_data/quick_delete_other_data/`
If they use the new ui/public/coordinator/test structure, why are they still located in `ui_bundled`?

Angela Novakovic

I think that at the time, we were just reorganizing the `clear_browsing_data` folder, but I can definitely move the `clear_browsing_data` folder to be directly at the `settings/` level in another CL.

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/quick_delete_other_data_view_controller.mm

Line 8, Patchset 6:#import "ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/quick_delete_util.h"

Angela Novakovic . resolved

Typically, we don't want UI to depend on coordinator logic. Should I move the quick_delete_util to a public folder then?

Alexis Hétu

yes

Angela Novakovic

Done

Line 337, Patchset 6: cell.accessoryView = [[UIImageView alloc]


          initWithImage:DefaultAccessorySymbolConfigurationWithRegularWeight(
                            kExternalLinkSymbol)];
      cell.accessoryView.tintColor = [UIColor colorNamed:kGrey500Color];
      return;
    case ItemIdentifier::kMyActivityIdentifier: {

Alexis Hétu . resolved

[[fallthrough]];

Angela Novakovic

Done

Line 349, Patchset 6: NOTREACHED();

Alexis Hétu . resolved

still in the wrong location, this should be AFTER the `}` on the next line

Angela Novakovic

Done

Open in Gerrit

Related details

Attention is currently required from:

Tommy Martino

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

Tommy Martino (Gerrit)

unread,

Mar 4, 2026, 10:50:49 AM (yesterday) Mar 4

to Angela Novakovic, AyeAye, Alexis Hétu, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Alexis Hétu and Angela Novakovic

Tommy Martino voted and added 2 comments

Votes added by Tommy Martino

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 6:

Alexis Hétu . resolved

Overall question: I just realized the files are in:
`ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/`
why aren't they in:
`ios/chrome/browser/settings/clear_browsing_data/quick_delete_other_data/`
If they use the new ui/public/coordinator/test structure, why are they still located in `ui_bundled`?

Angela Novakovic

I think that at the time, we were just reorganizing the `clear_browsing_data` folder, but I can definitely move the `clear_browsing_data` folder to be directly at the `settings/` level in another CL.

Tommy Martino

+1, please do this (in a follow-up CL)

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/quick_delete_other_data_view_controller.mm

Line 296, Patchset 9 (Latest): (BOOL)animated {

Tommy Martino . unresolved

Do we ever expect to pass `@NO` for this parameter? If not, it's somewhat simpler to make this a no-arg method and just always animate.

Open in Gerrit

Related details

Attention is currently required from:

Alexis Hétu
Angela Novakovic

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

Angela Novakovic (Gerrit)

unread,

Mar 4, 2026, 2:19:46 PM (24 hours ago) Mar 4

to Filipa Senra, Tommy Martino, AyeAye, Alexis Hétu, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Filipa Senra and Leo Zhao

Angela Novakovic added 2 comments

Patchset-level comments

File-level comment, Patchset 6:

Alexis Hétu . resolved

Overall question: I just realized the files are in:
`ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/`
why aren't they in:
`ios/chrome/browser/settings/clear_browsing_data/quick_delete_other_data/`
If they use the new ui/public/coordinator/test structure, why are they still located in `ui_bundled`?

Angela Novakovic

I think that at the time, we were just reorganizing the `clear_browsing_data` folder, but I can definitely move the `clear_browsing_data` folder to be directly at the `settings/` level in another CL.

Tommy Martino

+1, please do this (in a follow-up CL)

Angela Novakovic

CL is in the review stage. 😊

File ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/quick_delete_other_data_view_controller.mm

Line 296, Patchset 9: (BOOL)animated {

Tommy Martino . resolved

Do we ever expect to pass `@NO` for this parameter? If not, it's somewhat simpler to make this a no-arg method and just always animate.

Angela Novakovic

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

Filipa Senra
Leo Zhao

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

unsatisfied_requirement

open

diffy

Filipa Senra (Gerrit)

unread,

10:39 AM (4 hours ago) 10:39 AM

to Angela Novakovic, Tommy Martino, AyeAye, Alexis Hétu, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Angela Novakovic and Leo Zhao

Filipa Senra voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Angela Novakovic
Leo Zhao

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

unsatisfied_requirement

open

diffy

Tommy Martino (Gerrit)

unread,

11:29 AM (3 hours ago) 11:29 AM

to Angela Novakovic, Filipa Senra, AyeAye, Alexis Hétu, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Angela Novakovic and Leo Zhao

Tommy Martino voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Angela Novakovic
Leo Zhao

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

Angela Novakovic (Gerrit)

unread,

11:30 AM (3 hours ago) 11:30 AM

to Tommy Martino, Filipa Senra, AyeAye, Alexis Hétu, Leo Zhao, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Attention needed from Leo Zhao

Angela Novakovic voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Leo Zhao

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

Chromium LUCI CQ (Gerrit)

unread,

11:33 AM (3 hours ago) 11:33 AM

to Angela Novakovic, Tommy Martino, Filipa Senra, AyeAye, Alexis Hétu, Leo Zhao, chromium...@chromium.org, feature-me...@chromium.org, dullweb...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, msrame...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[iOS][PRDBD] Connect consumer to the "Other data" page view controller

This CL connects the QuickDeleteOtherDataConsumer to the
QuickDeleteOtherDataViewController and updates the view controller to
display the "Search history" cell and the "My Activity" cell based on
the consumer.

Design doc: go/bling-delete-browsing-data

Fixed: 471025894

Change-Id: I9f020046a474929549022581d138eae5615f1325

Bypass-Check-License: Moved existing files with original copyright years.

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7608145

Reviewed-by: Tommy Martino <tmar...@chromium.org>

Commit-Queue: Angela Novakovic <novak...@google.com>

Reviewed-by: Filipa Senra <fse...@google.com>

Cr-Commit-Position: refs/heads/main@{#1594716}

Files:

M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/BUILD.gn
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/quick_delete_mediator.mm
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/coordinator/quick_delete_mediator_unittest.mm
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/BUILD.gn
R ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/quick_delete_util.h
R ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/quick_delete_util.mm
R ios/chrome/browser/settings/ui_bundled/clear_browsing_data/public/quick_delete_util_unittest.mm
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/coordinator/BUILD.gn
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/coordinator/quick_delete_other_data_mediator.mm
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/coordinator/quick_delete_other_data_mediator_unittest.mm
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/test/BUILD.gn
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/test/quick_delete_other_data_egtest.mm
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/BUILD.gn
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/quick_delete_other_data_consumer.h
M ios/chrome/browser/settings/ui_bundled/clear_browsing_data/quick_delete_other_data/ui/quick_delete_other_data_view_controller.mm

Change size: L

Delta: 15 files changed, 338 insertions(+), 136 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Tommy Martino, +1 by Filipa Senra

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[iOS][PRDBD] Connect consumer to the "Other data" page view controller [chromium/src : main]

Angela Novakovic (Gerrit)

Angela Novakovic voted and added 5 comments

Votes added by Angela Novakovic

5 comments

Related details

Alexis Hétu (Gerrit)

Alexis Hétu added 5 comments

Related details

Angela Novakovic (Gerrit)

Angela Novakovic added 4 comments

Related details

Tommy Martino (Gerrit)

Tommy Martino voted and added 2 comments

Votes added by Tommy Martino

2 comments

Related details

Angela Novakovic (Gerrit)

Angela Novakovic added 2 comments

Related details

Filipa Senra (Gerrit)

Filipa Senra voted Code-Review+1

Related details

Tommy Martino (Gerrit)

Tommy Martino voted Code-Review+1

Related details

Angela Novakovic (Gerrit)

Angela Novakovic voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information