Skip to content

Commit ebbf2a9

Browse files
[cr139] MostVisitedSites::OnURLsAvailable changed
This function has an extra argument and this affects our overrides for it, and tests, but it doesn't seem to be relevant to our usecase. Chromium changes: https://chromium.googlesource.com/chromium/src/+/9d636d2c0f86e448fef4679c8b05457b80d2ac74 commit 9d636d2c0f86e448fef4679c8b05457b80d2ac74 Author: Samuel Huang <huangs@chromium.org> Date: Thu Jun 12 21:39:03 2025 -0700 [MVT Customization] Fix split-screen MVT update mismatch on user action. On Android Chrome, consider using "Split screen" feature to show 2 NTPs. A problem is that user-initiated MVT change to Top Sites Tiles (TSTs) or Custom Tiles (CTs) in "NTP 1" is not shown in "NTP 2", which maintains stale data. Cause (using example of adding a CT in NTP 1): * NTP 1: TileGroup updates `mPendingChanges`, then calls C++ backend to add CT. * The backend performs the work, then recomputes site suggestions. Then TileGroup.onSiteSuggestionsAvailable() for both {NTP 1, NTP 2} get called, with data passed. * To render tile loadTiles() needs to be called. The condition is: !mHasReceivedData || !mUiDelegate.isVisible() || expectedChangeCompleted * NTP 1 satisfies this, with `expectedChangeCompleted` enabled by earlier `mPendingChanges`. * NTP 2 fails, and so no update. Note that !mUiDelegate.isVisible() was added in https://codereview.chromium.org/2742433002 . * A second problem: loadTiles() heuristically compare "old" and "new" site suggestions, but this ignores CT reordering. This CL fixes the issue. Basic ideas: * Add `isUserTriggered` param to onSiteSuggestionsAvailable(). This is passed from (and plumbed in) backend for user-triggered tile changes, including TST {pin, remove (+undo)} and CT {add, edit, unpin (+undo), reorder}. * Remove `expectedChangeCompleted` logic, which leads to removing many `PendingChanges` fields and assignments thereof. * Update loadTiles() to perform exact comparison between "old" and "new" to elide update. Details: * ntp_tiles::MostVisitedSites(): * Observer::OnURLsAvailable(): Add `is_user_triggered` param. * This is used to decide whether to make tile update "eager" (for responsiveness if true) or "deferred" (for tile stability if false, among other factors). * Update 7 functions to take `is_user_triggered` param. * Where `is_user_triggered` get set: * OnCustomLinksChanged(): Pass `is_user_triggered` = true. * TopSitesChanged(): Pass `change_reason == BLOCKED_URLS`. * TileGroup.Delegate and TileGroupDelegateImpl: Remove PendingChanges usage, now unneeded. * TileGroup: * Add tileListAreEqual() for robust List<Tile> equivalence check. * Requires making findTileByUrl() static. * Move `mPendingChanges.taskToRunAfterTileReload` usage from onSiteSuggestionsAvailable() to loadTiles(). * onSiteSuggestionsAvailable(): Take `isUserTriggered`, which replaces `expectedChangeCompleted` usage for "eager" update, and enabling simplification re. PendingChanges. * PendingChanges: Remove {`removalUrl`, `insertionUrl`, `customTilesIndicator`}; rename `tiles` -> `siteSuggestions`. * Also remove usages. * loadTiles(): Remove param `forcedUpdate`; replace bizarre naming `oldPersonali{s,z}edTilesCount` with `old` vs. `new`; replace change detection heuristic with tileListAreEqual() (more precise). * Add createTileData() to absorb some logic in loadTiles(). * CustomTileModificationDelegateImpl: Remove `customTilesIndicator` set / reset-on-error. Test changes: * Update 5 tests to accommodate `OnURLsAvailable()` change. * FakeMostVisitedSites: * notifyTileSuggestionsAvailable(): Take `isUserTriggered` param and pass to onSiteSuggestionsAvailable(). * setTileSuggestions() (3 overloads): Pass `isUserTriggered` = true, since this is the usual expected test behavior. * Add setTileSuggestionsPassive() (3 overloads) to pass false, and use this in TileGroupUnitTest. Bug: 423590359, 388782412 Change-Id: Ibbf2caa712e0eb6b94024935bb7dd7e92e62518c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6625370 Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org> Commit-Queue: Samuel Huang <huangs@chromium.org> Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org> Cr-Commit-Position: refs/heads/main@{#1473368}
1 parent 508fe9d commit ebbf2a9

File tree

5 files changed

+13
-8
lines changed

5 files changed

+13
-8
lines changed

browser/ui/webui/brave_new_tab_page_refresh/top_sites_facade.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ void TopSitesFacade::SetSitesUpdatedCallback(
149149
}
150150

151151
void TopSitesFacade::OnURLsAvailable(
152+
bool is_user_triggered,
152153
const std::map<ntp_tiles::SectionType, ntp_tiles::NTPTilesVector>&
153154
sections) {
154155
current_sites_ = TopSitesFromSections(sections);

browser/ui/webui/brave_new_tab_page_refresh/top_sites_facade.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ class TopSitesFacade : public ntp_tiles::MostVisitedSites::Observer {
6666

6767
// MostVisitedSites::Observer:
6868
void OnURLsAvailable(
69+
bool is_user_triggered,
6970
const std::map<ntp_tiles::SectionType, ntp_tiles::NTPTilesVector>&
7071
sections) override;
7172
void OnIconMadeAvailable(const GURL& site_url) override;

browser/ui/webui/new_tab_page/top_sites_message_handler.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ void TopSitesMessageHandler::RegisterMessages() {
8383

8484
// ntp_tiles::MostVisitedSites::Observer:
8585
void TopSitesMessageHandler::OnURLsAvailable(
86+
bool is_user_triggered,
8687
const std::map<ntp_tiles::SectionType, ntp_tiles::NTPTilesVector>&
8788
sections) {
8889
if (!most_visited_sites_)

browser/ui/webui/new_tab_page/top_sites_message_handler.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ class TopSitesMessageHandler : public content::WebUIMessageHandler,
4242

4343
// ntp_tiles::MostVisitedSites::Observer:
4444
void OnURLsAvailable(
45+
bool is_user_triggered,
4546
const std::map<ntp_tiles::SectionType, ntp_tiles::NTPTilesVector>&
4647
sections) override;
4748
void OnIconMadeAvailable(const GURL& site_url) override;

chromium_src/components/ntp_tiles/most_visited_sites_unittest.cc

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ TEST_F(MostVisitedSitesTest,
1616
MostVisitedURLList{MakeMostVisitedURL(u"Site 1", "http://site1/")}));
1717

1818
InSequence seq;
19-
EXPECT_CALL(mock_observer_,
20-
OnURLsAvailable(Contains(
21-
Pair(SectionType::PERSONALIZED,
22-
ElementsAre(MatchesTile(u"Site 1", "http://site1/",
23-
TileSource::TOP_SITES))))));
19+
EXPECT_CALL(
20+
mock_observer_,
21+
OnURLsAvailable(
22+
_, Contains(Pair(SectionType::PERSONALIZED,
23+
ElementsAre(MatchesTile(u"Site 1", "http://site1/",
24+
TileSource::TOP_SITES))))));
2425
EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
2526

2627
most_visited_sites_->AddMostVisitedURLsObserver(&mock_observer_,
@@ -32,7 +33,7 @@ TEST_F(MostVisitedSitesTest,
3233
EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_))
3334
.WillOnce(base::test::RunOnceCallback<0>(
3435
MostVisitedURLList{MakeMostVisitedURL(u"Site 2", "http://site2/")}));
35-
EXPECT_CALL(mock_observer_, OnURLsAvailable(_));
36+
EXPECT_CALL(mock_observer_, OnURLsAvailable(_, _));
3637
mock_top_sites_->NotifyTopSitesChanged(
3738
history::TopSitesObserver::ChangeReason::MOST_VISITED);
3839
base::RunLoop().RunUntilIdle();
@@ -50,8 +51,8 @@ TEST_F(MostVisitedSitesTest,
5051
MakeMostVisitedURL(u"Google", "http://www.google.com/")}));
5152
EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
5253
std::map<SectionType, NTPTilesVector> sections;
53-
EXPECT_CALL(mock_observer_, OnURLsAvailable(_))
54-
.WillOnce(SaveArg<0>(&sections));
54+
EXPECT_CALL(mock_observer_, OnURLsAvailable(_, _))
55+
.WillOnce(SaveArg<1>(&sections));
5556

5657
most_visited_sites_->AddMostVisitedURLsObserver(&mock_observer_,
5758
/*max_num_sites=*/6);

0 commit comments

Comments
 (0)