Kay Lin (Gerrit)

unread,

Mar 10, 2026, 6:37:31 AM (2 days ago) Mar 10

to chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Kay Lin has uploaded the change for review

Commit message

Site Search: Extract common base classes for mediators

This CL refactors the site search settings mediators to eliminate
significant code duplication across multiple components. Note that this
is a pure refactoring change without new logic introduced.

Changes introduced:
1. Created BaseSiteSearchMediator: Centralizes TemplateUrlService, icons
   fetching and standard PropertyModel creation.
2. Created ExpandableSiteSearchMediator: Encapsulates the logic for
   "More" button state and expand/collapse actions.

Bug: 489633787

Change-Id: I8f3382e0be2655d79153a9d71007268e08d99995

Change diff

Change information

Files:

M chrome/browser/search_engines/android/BUILD.gn
A chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/BaseSiteSearchMediator.java
A chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediator.java
M chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/custom_search_engine/CustomSearchEngineListMediator.java
M chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/custom_site_search/CustomSiteSearchMediator.java
M chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/extensions/ExtensionSearchEngineMediator.java
M chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/inactive_shortcut/InactiveShortcutMediator.java
A chrome/browser/search_engines/android/junit/src/org/chromium/chrome/browser/search_engines/settings/common/BaseSiteSearchMediatorUnitTest.java
A chrome/browser/search_engines/android/junit/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediatorUnitTest.java

Change size: XL

Delta: 9 files changed, 639 insertions(+), 380 deletions(-)

Open in Gerrit

Related details

Attention set is empty

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

Kay Lin (Gerrit)

unread,

Mar 11, 2026, 2:46:09 AM (yesterday) Mar 11

to Tomasz Wiszkowski, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Tomasz Wiszkowski

Kay Lin added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Kay Lin . resolved

Hi Tomasz! I was originally hoping to do this in a single CL, but the code diff was getting too large, so I split the logic into two different CLs. Please let me know if you have any questions~

Open in Gerrit

Related details

Attention is currently required from:

Tomasz Wiszkowski

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

Tomasz Wiszkowski (Gerrit)

unread,

Mar 11, 2026, 4:30:44 PM (20 hours ago) Mar 11

to Kay Lin, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Kay Lin

Tomasz Wiszkowski voted and added 4 comments

Votes added by Tomasz Wiszkowski

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Tomasz Wiszkowski . unresolved

stamping to unblock.

the test should def be reconsidered. i don't think it tests what matters
i do believe the member variables should be `private` too, as is this is risky.

refreshList() being called twice is a nice-to-have correction (it's a little wasteful).

File chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/BaseSiteSearchMediator.java

Line 64, Patchset 4 (Latest):

        mTemplateUrlService.addObserver(this);
        mTemplateUrlService.runWhenLoaded(this::refreshList);

Tomasz Wiszkowski . unresolved

will this `refreshList()` twice?

File chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediator.java

Line 27, Patchset 4 (Latest):

    protected boolean mIsExpanded;

    /**
     * The items to display when the list is expanded. Subclasses are responsible for populating
     * this list during {@link #refreshList()} or lazy-loading them via {@link
     * #prepareHiddenItemsIfNeeded()}.
     */
    protected final List<ListItem> mHiddenItems = new ArrayList<>();

Tomasz Wiszkowski . unresolved

i think both of these should be `private`.

`protected` members here permit subclasses to override these values which is risky.

File chrome/browser/search_engines/android/junit/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediatorUnitTest.java

Line 70, Patchset 4 (Latest):


            for (int i = 0; i < mUrlsToSimulate; i++) {
                ListItem item = createListItem(mTemplateUrl);
                if (i < DEFAULT_MAX_ROWS) {
                    mModelList.add(item);
                } else {
                    mHiddenItems.add(item);
                }
            }

Tomasz Wiszkowski . unresolved

this is risky. any tests checking mModelList and mHiddenItems will be testing the testing support code, not the actual mediator.

why is it necessary to track that at all?

the `Expandable.*Mediator` is an abstract class. it should be tested with

```
mock(ExpandableSiteSearchMediator.class,
     Mockito.CALLS_REAL_METHODS);
```

or

```
@Mock(answer = Answers.CALLS_REAL_METHODS) ExpandableMediator mMediator'
```

and all the logic below should `verify()` that appropriate methods are called, rather than checking what the test support code does with that.

Open in Gerrit

Related details

Attention is currently required from:

Kay Lin

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

Kay Lin (Gerrit)

unread,

6:00 AM (6 hours ago) 6:00 AM

to Tomasz Wiszkowski, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Kay Lin added 5 comments

Patchset-level comments

File-level comment, Patchset 4:

Tomasz Wiszkowski . resolved

stamping to unblock.
the test should def be reconsidered. i don't think it tests what matters
i do believe the member variables should be `private` too, as is this is risky.
refreshList() being called twice is a nice-to-have correction (it's a little wasteful).

Kay Lin

the test should def be reconsidered. i don't think it tests what matters

todo (crbug.com/492061324) added.

i do believe the member variables should be private too, as is this is risky.

todo (crbug.com/492059926) added.

File-level comment, Patchset 5 (Latest):

Kay Lin . resolved

Thanks for the review Tomasz! these are very valuable comments

File chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/BaseSiteSearchMediator.java

Line 64, Patchset 4:

        mTemplateUrlService.addObserver(this);
        mTemplateUrlService.runWhenLoaded(this::refreshList);

Tomasz Wiszkowski . resolved

will this `refreshList()` twice?

Kay Lin

No, IMHO when the observer is added, it doesn't trigger onTemplateUrlChanged; instead, runWhenLoaded runs the first time for refreshList(). I also conducted some experiments here and confirmed that refreshList is called only once.

File chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediator.java

Line 27, Patchset 4: protected boolean mIsExpanded;



    /**
     * The items to display when the list is expanded. Subclasses are responsible for populating
     * this list during {@link #refreshList()} or lazy-loading them via {@link
     * #prepareHiddenItemsIfNeeded()}.
     */
    protected final List<ListItem> mHiddenItems = new ArrayList<>();

Tomasz Wiszkowski . resolved

i think both of these should be `private`.
`protected` members here permit subclasses to override these values which is risky.

Kay Lin

Agreed. I'll add a TODO here to be addressed in a subsequent CL.

File chrome/browser/search_engines/android/junit/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediatorUnitTest.java

Line 70, Patchset 4:


            for (int i = 0; i < mUrlsToSimulate; i++) {
                ListItem item = createListItem(mTemplateUrl);
                if (i < DEFAULT_MAX_ROWS) {
                    mModelList.add(item);
                } else {
                    mHiddenItems.add(item);
                }
            }

Tomasz Wiszkowski . resolved

this is risky. any tests checking mModelList and mHiddenItems will be testing the testing support code, not the actual mediator.
why is it necessary to track that at all?
the `Expandable.*Mediator` is an abstract class. it should be tested with
```
mock(ExpandableSiteSearchMediator.class,
     Mockito.CALLS_REAL_METHODS);
```
or
```
@Mock(answer = Answers.CALLS_REAL_METHODS) ExpandableMediator mMediator'
```
and all the logic below should `verify()` that appropriate methods are called, rather than checking what the test support code does with that.

Kay Lin

After a second look, I totally agree with you. I was so caught up in implementing the tests for the full feature—especially the refactoring from other components. I've added a todo here and will look into it later. Thanks for the pointer!

Open in Gerrit

Related details

Attention set is empty

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,

6:55 AM (5 hours ago) 6:55 AM

to Kay Lin, Tomasz Wiszkowski, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/browser/search_engines/android/junit/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediatorUnitTest.java
Insertions: 2, Deletions: 0.

@@ -38,6 +38,8 @@
 /** Unit tests for {@link ExpandableSiteSearchMediator}. */
 @RunWith(BaseRobolectricTestRunner.class)
 public class ExpandableSiteSearchMediatorUnitTest {
+    // TODO(crbug.com/492059926): This abstract class should be tested with mock and verify the
+    // methods called instead.
     @Rule public MockitoRule mMockitoRule = MockitoJUnit.rule();
 
     @Mock private Profile mProfile;
```
```
The name of the file: chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediator.java
Insertions: 1, Deletions: 0.

@@ -24,6 +24,7 @@
     /** The default maximum number of items to show before displaying a "More" button. */
     protected static final int DEFAULT_MAX_ROWS = 5;
 
+    // TODO(crbug.com/492061324): Make mIsExpanded and mHiddenItems private.
     protected boolean mIsExpanded;
 
     /**
```

Change information

Commit message:

Site Search: Extract common base classes for mediators

Part 1/2: This CL introduces the base class for site search to eliminate


significant code duplication across multiple components. Note that this
is a pure refactoring change without new logic introduced.

Changes introduced:
1. Created BaseSiteSearchMediator: Centralizes TemplateUrlService, icons
   fetching and standard PropertyModel creation.
2. Created ExpandableSiteSearchMediator: Encapsulates the logic for
   "More" button state and expand/collapse actions.

Bug: 489633787

Change-Id: I8f3382e0be2655d79153a9d71007268e08d99995

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

Commit-Queue: Kay Lin <kaiy...@google.com>

Reviewed-by: Tomasz Wiszkowski <en...@google.com>

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

Files:

M chrome/browser/search_engines/android/BUILD.gn
A chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/BaseSiteSearchMediator.java
A chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediator.java

A chrome/browser/search_engines/android/junit/src/org/chromium/chrome/browser/search_engines/settings/common/BaseSiteSearchMediatorUnitTest.java
A chrome/browser/search_engines/android/junit/src/org/chromium/chrome/browser/search_engines/settings/common/ExpandableSiteSearchMediatorUnitTest.java

Change size: L

Delta: 5 files changed, 561 insertions(+), 0 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Tomasz Wiszkowski

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

Site Search: Extract common base classes for mediators [chromium/src : main]

Kay Lin (Gerrit)

Kay Lin has uploaded the change for review

Commit message

Change diff

Change information

Related details

Kay Lin (Gerrit)

Kay Lin added 1 comment

Related details

Tomasz Wiszkowski (Gerrit)

Tomasz Wiszkowski voted and added 4 comments

Votes added by Tomasz Wiszkowski

4 comments

Related details

Kay Lin (Gerrit)

Kay Lin added 5 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information