[iOS][mvt-customization] Check historical visits condition [chromium/src : main]

[Ginny Huang (Gerrit)]

Attention needed from Benjamin Williams

Ginny Huang voted and added 1 comment

Votes added by Ginny Huang

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Ginny Huang . resolved

Ben can you give me a first pass review? Thanks!

Open in Gerrit

Related details

Attention is currently required from:

• Benjamin Williams

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7e4593d5f89df11be219ea6a5996197fdd4910aa

Gerrit-Change-Number: 7487790

Gerrit-PatchSet: 6

Gerrit-Owner: Ginny Huang <ginny...@chromium.org>

Gerrit-Reviewer: Benjamin Williams <bwwil...@google.com>

Gerrit-Reviewer: Ginny Huang <ginny...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Benjamin Williams <bwwil...@google.com>

Gerrit-Comment-Date: Fri, 16 Jan 2026 06:54:21 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Ginny Huang (Gerrit)]

Attention needed from Benjamin Williams

Ginny Huang added 1 comment

File ios/chrome/browser/content_suggestions/most_visited_tiles/coordinator/most_visited_tiles_mediator_unittest.mm

Line 214, Patchset 6 (Latest):TEST_F(MostVisitedTilesMediatorTest, TestPinSiteInProductHelpCondition) {

Ginny Huang . unresolved

Just realized that it's hard to read as is. I'll add more empty lines to this later...

Open in Gerrit

Related details

Attention is currently required from:

• Benjamin Williams

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7e4593d5f89df11be219ea6a5996197fdd4910aa

Gerrit-Change-Number: 7487790

Gerrit-PatchSet: 6

Gerrit-Owner: Ginny Huang <ginny...@chromium.org>

Gerrit-Reviewer: Benjamin Williams <bwwil...@google.com>

Gerrit-Reviewer: Ginny Huang <ginny...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Benjamin Williams <bwwil...@google.com>

Gerrit-Comment-Date: Fri, 16 Jan 2026 06:55:47 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Ginny Huang (Gerrit)]

Attention needed from Benjamin Williams

Ginny Huang added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Ginny Huang . unresolved

I think the failed test case is flaky

Gerrit-Comment-Date: Fri, 16 Jan 2026 15:41:57 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Benjamin Williams (Gerrit)]

Attention needed from Ginny Huang

Benjamin Williams voted and added 5 comments

Votes added by Benjamin Williams

Code-Review

+1

5 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Benjamin Williams . resolved

LGTM with comments

Commit Message

Line 10, Patchset 6 (Latest):any site in the most visited tiles for more than 3 times in the past

Benjamin Williams . unresolved

Is it strictly "more than 3" or "3 or more"? Bcuz the current logic is "3 or more"

File ios/chrome/browser/content_suggestions/most_visited_tiles/coordinator/most_visited_tiles_mediator.mm

Line 75, Patchset 6 (Latest):const int kRepeatingVisitsToTriggerIPH = 3;

Benjamin Williams . unresolved

Optional + Out of scope for this CL, but you didn't want this to be Finchable?

Line 616, Patchset 6 (Latest): id<HelpCommands> helpHandler = self.helpHandler;

Benjamin Williams . unresolved

Should you weakify `helpHandler`?

Line 626, Patchset 6 (Latest): _historyService->GetMostRecentVisitsForGurl(

Benjamin Williams . unresolved

Should you check `_historyService` is valid to avoid `nullptr` dereference here?

Open in Gerrit

Related details

Attention is currently required from:

• Ginny Huang

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7e4593d5f89df11be219ea6a5996197fdd4910aa

Gerrit-Change-Number: 7487790

Gerrit-PatchSet: 6

Gerrit-Owner: Ginny Huang <ginny...@chromium.org>

Gerrit-Reviewer: Benjamin Williams <bwwil...@google.com>

Gerrit-Reviewer: Ginny Huang <ginny...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Ginny Huang <ginny...@chromium.org>

Gerrit-Comment-Date: Fri, 16 Jan 2026 17:03:42 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Ginny Huang (Gerrit)]

Attention needed from Hailey Wang and Tibor Goldschwendt

Ginny Huang voted and added 5 comments

Votes added by Ginny Huang

Commit-Queue

+1

5 comments

Patchset-level comments

File-level comment, Patchset 8 (Latest):

Ginny Huang . resolved

Thanks!

Commit Message

Line 10, Patchset 6:any site in the most visited tiles for more than 3 times in the past

Benjamin Williams . resolved

Is it strictly "more than 3" or "3 or more"? Bcuz the current logic is "3 or more"

Ginny Huang

Done

File ios/chrome/browser/content_suggestions/most_visited_tiles/coordinator/most_visited_tiles_mediator.mm

Line 75, Patchset 6:const int kRepeatingVisitsToTriggerIPH = 3;

Benjamin Williams . resolved

Optional + Out of scope for this CL, but you didn't want this to be Finchable?

Ginny Huang

I'm not too worried about the discoverability of this feature (the "Add Site" button is visible in the tiles) so I wouldn't invest time and resources on experimenting with different IPH configurations. In fact the IPH is enabled by default, so if we do an experiment on this value we don't even have a control group 🤔

Line 616, Patchset 6: id<HelpCommands> helpHandler = self.helpHandler;

Benjamin Williams . resolved

Should you weakify `helpHandler`?

Ginny Huang

Explained in the last CL.

Line 626, Patchset 6: _historyService->GetMostRecentVisitsForGurl(

Benjamin Williams . resolved

Should you check `_historyService` is valid to avoid `nullptr` dereference here?

Ginny Huang

I'd rather do a `CHECK` in the initializer, which is now done

Open in Gerrit

Related details

Attention is currently required from:

• Hailey Wang
• Tibor Goldschwendt

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7e4593d5f89df11be219ea6a5996197fdd4910aa

Gerrit-Change-Number: 7487790

Gerrit-PatchSet: 8

Gerrit-Owner: Ginny Huang <ginny...@chromium.org>

Gerrit-Reviewer: Benjamin Williams <bwwil...@google.com>

Gerrit-Reviewer: Ginny Huang <ginny...@chromium.org>

Gerrit-Reviewer: Hailey Wang <haile...@google.com>

Gerrit-Reviewer: Tibor Goldschwendt <tib...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Hailey Wang <haile...@google.com>

Gerrit-Attention: Tibor Goldschwendt <tib...@chromium.org>

Gerrit-Comment-Date: Fri, 16 Jan 2026 21:35:52 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Benjamin Williams <bwwil...@google.com>

[Ginny Huang (Gerrit)]

Attention needed from Hailey Wang and Tibor Goldschwendt

Ginny Huang voted Auto-Submit+1

Auto-Submit

+1

Gerrit-Comment-Date: Fri, 16 Jan 2026 21:36:08 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Ginny Huang (Gerrit)]

Attention needed from Hailey Wang and Tibor Goldschwendt

Ginny Huang added 1 comment

File ios/chrome/browser/content_suggestions/most_visited_tiles/coordinator/most_visited_tiles_mediator_unittest.mm

Line 214, Patchset 6:TEST_F(MostVisitedTilesMediatorTest, TestPinSiteInProductHelpCondition) {

Ginny Huang . resolved

Just realized that it's hard to read as is. I'll add more empty lines to this later...

Ginny Huang

Done

Gerrit-Comment-Date: Fri, 16 Jan 2026 21:36:16 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Ginny Huang <ginny...@chromium.org>

[Tibor Goldschwendt (Gerrit)]

Attention needed from Ginny Huang and Hailey Wang

Tibor Goldschwendt voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

• Ginny Huang
• Hailey Wang

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7e4593d5f89df11be219ea6a5996197fdd4910aa

Gerrit-Change-Number: 7487790

Gerrit-PatchSet: 9

Gerrit-Owner: Ginny Huang <ginny...@chromium.org>

Gerrit-Reviewer: Benjamin Williams <bwwil...@google.com>

Gerrit-Reviewer: Ginny Huang <ginny...@chromium.org>

Gerrit-Reviewer: Hailey Wang <haile...@google.com>

Gerrit-Reviewer: Tibor Goldschwendt <tib...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Hailey Wang <haile...@google.com>

Gerrit-Attention: Ginny Huang <ginny...@chromium.org>

Gerrit-Comment-Date: Fri, 16 Jan 2026 22:00:04 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes