Skip to content

Commit

Permalink
[NTP] Separate internal custom link updates from user changes
Browse files Browse the repository at this point in the history 
When a default URL scheme is updated after URL validation, it is counted
as a user action. This causes "Undo" to revert the URL scheme update
instead of the user's add/update action.

Add an additional parameter to CustomLinksManager::UpdateLink in order
to distinguish between internal and user changes. Internal changes will
not update the previous state that is restored when
CustomLinksManager::UndoAction is called.

Bug: 896143
Change-Id: I643679fab5e546fac7e41214af6ff8a4ba9c8b53
Reviewed-on: https://chromium-review.googlesource.com/c/1287259
Commit-Queue: Kristi Park <kristipark@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600988}
  • Loading branch information
Kristi Park authored and Commit Bot committed Oct 19, 2018
1 parent 1436aab commit adf0565
Show file tree
Hide file tree
Showing 8 changed files with 102 additions and 31 deletions.
14 changes: 9 additions & 5 deletions chrome/browser/search/instant_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,8 @@ bool InstantService::UpdateCustomLink(const GURL& url,
const std::string& new_title) {
if (most_visited_sites_) {
return most_visited_sites_->UpdateCustomLink(url, new_url,
base::UTF8ToUTF16(new_title));
base::UTF8ToUTF16(new_title),
/*is_user_action=*/true);
}
return false;
}
Expand Down Expand Up @@ -489,11 +490,14 @@ void InstantService::OnDoesUrlResolveComplete(
// already timed out.
if (duration >
base::TimeDelta::FromSeconds(kCustomLinkDialogTimeoutSeconds)) {
GURL::Replacements replacements;
replacements.SetSchemeStr(url::kHttpScheme);
GURL new_url = url.ReplaceComponents(replacements);
UpdateCustomLink(url, new_url, /*new_title=*/std::string());
timeout = true;
if (most_visited_sites_) {
GURL::Replacements replacements;
replacements.SetSchemeStr(url::kHttpScheme);
GURL new_url = url.ReplaceComponents(replacements);
most_visited_sites_->UpdateCustomLink(url, new_url, base::string16(),
/*is_user_action=*/false);
}
}
}
std::move(callback).Run(resolves, timeout);
Expand Down
7 changes: 5 additions & 2 deletions components/ntp_tiles/custom_links_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,13 @@ class CustomLinksManager {
// no longer be considered Most Visited. Returns false and does nothing if
// custom links is not initialized, either URL is invalid, |url| does not
// exist in the list, |new_url| already exists in the list, or both parameters
// are empty.
// are empty. |is_user_action| is true if this was executed by the user (i.e.
// by editing a custom link). Only user actions will update the previous state
// that is restored when CustomLinksManager::UndoAction is called.
virtual bool UpdateLink(const GURL& url,
const GURL& new_url,
const base::string16& new_title) = 0;
const base::string16& new_title,
bool is_user_action) = 0;
// Deletes the link with the specified |url|. Returns false and does nothing
// if custom links is not initialized, |url| is invalid, or |url| does not
// exist in the list.
Expand Down
8 changes: 6 additions & 2 deletions components/ntp_tiles/custom_links_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ bool CustomLinksManagerImpl::AddLink(const GURL& url,

bool CustomLinksManagerImpl::UpdateLink(const GURL& url,
const GURL& new_url,
const base::string16& new_title) {
const base::string16& new_title,
bool is_user_action) {
if (!IsInitialized() || !url.is_valid() ||
(new_url.is_empty() && new_title.empty())) {
return false;
Expand All @@ -112,7 +113,10 @@ bool CustomLinksManagerImpl::UpdateLink(const GURL& url,
return false;

// At this point, we will be modifying at least one of the values.
previous_links_ = current_links_;
if (is_user_action) {
// Save the previous state since this was a user update.
previous_links_ = current_links_;
}

if (!new_url.is_empty())
it->url = new_url;
Expand Down
3 changes: 2 additions & 1 deletion components/ntp_tiles/custom_links_manager_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ class CustomLinksManagerImpl : public CustomLinksManager,
bool AddLink(const GURL& url, const base::string16& title) override;
bool UpdateLink(const GURL& url,
const GURL& new_url,
const base::string16& new_title) override;
const base::string16& new_title,
bool is_user_action) override;
bool DeleteLink(const GURL& url) override;
bool UndoAction() override;

Expand Down
75 changes: 63 additions & 12 deletions components/ntp_tiles/custom_links_manager_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,18 +219,20 @@ TEST_F(CustomLinksManagerImplTest, UpdateLink) {

// Update the link's URL.
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(kTestUrl),
base::string16()));
base::string16(),
/*is_user_action=*/true));
EXPECT_EQ(links_after_update_url, custom_links_->GetLinks());

// Update the link's title.
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestUrl), GURL(),
base::UTF8ToUTF16(kTestTitle)));
base::UTF8ToUTF16(kTestTitle),
/*is_user_action=*/true));
EXPECT_EQ(links_after_update_title, custom_links_->GetLinks());

// Update the link's URL and title.
EXPECT_TRUE(
custom_links_->UpdateLink(GURL(kTestUrl), GURL(kTestCase1[0].url),
base::UTF8ToUTF16(kTestCase1[0].title)));
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestUrl), GURL(kTestCase1[0].url),
base::UTF8ToUTF16(kTestCase1[0].title),
/*is_user_action=*/true));
EXPECT_EQ(links_after_update_both, custom_links_->GetLinks());
}

Expand All @@ -246,20 +248,24 @@ TEST_F(CustomLinksManagerImplTest, UpdateLinkWithInvalidParams) {
// Try to update a link that does not exist. This should fail and not modify
// the list.
EXPECT_FALSE(custom_links_->UpdateLink(GURL(kTestUrl), GURL(),
base::UTF8ToUTF16(kTestTitle)));
base::UTF8ToUTF16(kTestTitle),
/*is_user_action=*/true));
EXPECT_EQ(initial_links, custom_links_->GetLinks());

// Try to pass empty params. This should fail and not modify the list.
EXPECT_FALSE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(),
base::string16()));
base::string16(),
/*is_user_action=*/true));
EXPECT_EQ(initial_links, custom_links_->GetLinks());

// Try to pass an invalid URL. This should fail and not modify the list.
EXPECT_FALSE(custom_links_->UpdateLink(kInvalidUrl, GURL(),
base::UTF8ToUTF16(kTestTitle)));
base::UTF8ToUTF16(kTestTitle),
/*is_user_action=*/true));
EXPECT_EQ(initial_links, custom_links_->GetLinks());
EXPECT_FALSE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), kInvalidUrl,
base::string16()));
base::string16(),
/*is_user_action=*/true));
EXPECT_EQ(initial_links, custom_links_->GetLinks());
}

Expand All @@ -274,7 +280,8 @@ TEST_F(CustomLinksManagerImplTest, UpdateLinkWhenUrlAlreadyExists) {
// Try to update a link with a URL that exists in the list. This should fail
// and not modify the list.
EXPECT_FALSE(custom_links_->UpdateLink(
GURL(kTestCase2[0].url), GURL(kTestCase2[1].url), base::string16()));
GURL(kTestCase2[0].url), GURL(kTestCase2[1].url), base::string16(),
/*is_user_action=*/true));
EXPECT_EQ(initial_links, custom_links_->GetLinks());
}

Expand Down Expand Up @@ -350,7 +357,8 @@ TEST_F(CustomLinksManagerImplTest, UndoUpdateLink) {

// Update the link's URL.
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(kTestUrl),
base::string16()));
base::string16(),
/*is_user_action=*/true));
EXPECT_EQ(links_after_update_url, custom_links_->GetLinks());

// Undo update link.
Expand All @@ -359,7 +367,8 @@ TEST_F(CustomLinksManagerImplTest, UndoUpdateLink) {

// Update the link's title.
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(),
base::UTF8ToUTF16(kTestTitle)));
base::UTF8ToUTF16(kTestTitle),
/*is_user_action=*/true));
EXPECT_EQ(links_after_update_title, custom_links_->GetLinks());

// Undo update link.
Expand Down Expand Up @@ -413,6 +422,48 @@ TEST_F(CustomLinksManagerImplTest, UndoDeleteLinkAfterAdd) {
EXPECT_EQ(expected_links, custom_links_->GetLinks());
}

TEST_F(CustomLinksManagerImplTest, ShouldNotUndoActionIfInternal) {
NTPTilesVector initial_tiles = FillTestTiles(kTestCase1);
std::vector<Link> initial_links = FillTestLinks(kTestCase1);
std::vector<Link> links_after_update_twice;
links_after_update_twice.emplace_back(
Link{GURL(kTestUrl), base::UTF8ToUTF16(kTestTitle), false});
std::vector<Link> links_after_add_and_update(initial_links);
links_after_add_and_update.emplace_back(Link{
GURL(kTestCase2[1].url), base::UTF8ToUTF16(kTestCase2[1].title), false});
links_after_add_and_update[0].url = GURL(kTestUrl);
links_after_add_and_update[0].is_most_visited = false;

// Initialize.
ASSERT_TRUE(custom_links_->Initialize(initial_tiles));
ASSERT_EQ(initial_links, custom_links_->GetLinks());

// Update twice. Specify that the second update was internal.
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(),
base::UTF8ToUTF16(kTestTitle),
/*is_user_action=*/true));
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(kTestUrl),
base::string16(),
/*is_user_action=*/false));
EXPECT_EQ(links_after_update_twice, custom_links_->GetLinks());

// Undo should revert to the state before the first action.
EXPECT_TRUE(custom_links_->UndoAction());
EXPECT_EQ(initial_links, custom_links_->GetLinks());

// Add then update. Specify that the update was internal.
EXPECT_TRUE(custom_links_->AddLink(GURL(kTestCase2[1].url),
base::UTF8ToUTF16(kTestCase2[1].title)));
EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(kTestUrl),
base::string16(),
/*is_user_action=*/false));
EXPECT_EQ(links_after_add_and_update, custom_links_->GetLinks());

// Undo should revert to the state before the first action.
EXPECT_TRUE(custom_links_->UndoAction());
EXPECT_EQ(initial_links, custom_links_->GetLinks());
}

TEST_F(CustomLinksManagerImplTest, ShouldDeleteMostVisitedOnHistoryDeletion) {
NTPTilesVector initial_tiles = FillTestTiles(kTestCase2);
std::vector<Link> initial_links = FillTestLinks(kTestCase2);
Expand Down
9 changes: 6 additions & 3 deletions components/ntp_tiles/most_visited_sites.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,16 +301,19 @@ bool MostVisitedSites::AddCustomLink(const GURL& url,

bool MostVisitedSites::UpdateCustomLink(const GURL& url,
const GURL& new_url,
const base::string16& new_title) {
const base::string16& new_title,
bool is_user_action) {
if (!custom_links_ || !custom_links_enabled_)
return false;

// Initialize custom links if they have not been initialized yet.
InitializeCustomLinks();

bool success = custom_links_->UpdateLink(url, new_url, new_title);
bool success =
custom_links_->UpdateLink(url, new_url, new_title, is_user_action);
if (success) {
if (custom_links_action_count_ != -1)
// Only update the action count if this was executed by the user.
if (is_user_action && custom_links_action_count_ != -1)
custom_links_action_count_++;
BuildCurrentTiles();
}
Expand Down
6 changes: 4 additions & 2 deletions components/ntp_tiles/most_visited_sites.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,12 @@ class MostVisitedSites : public history::TopSitesObserver,
// Updates the URL and/or title of the custom link specified by |url|. If
// |url| does not exist or |new_url| already exists in the custom link list,
// returns false and does nothing. Will initialize custom links if they have
// not been initialized yet. Custom links must be enabled.
// not been initialized yet. |is_user_action| is true if this was executed by
// the user (i.e. editing a custom link). Custom links must be enabled.
bool UpdateCustomLink(const GURL& url,
const GURL& new_url,
const base::string16& new_title);
const base::string16& new_title,
bool is_user_action);
// Deletes the custom link with the specified |url|. If |url| does not exist
// in the custom link list, returns false and does nothing. Will initialize
// custom links if they have not been initialized yet. Custom links must be
Expand Down
11 changes: 7 additions & 4 deletions components/ntp_tiles/most_visited_sites_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,10 +272,11 @@ class MockCustomLinksManager : public CustomLinksManager {
MOCK_CONST_METHOD0(IsInitialized, bool());
MOCK_CONST_METHOD0(GetLinks, const std::vector<CustomLinksManager::Link>&());
MOCK_METHOD2(AddLink, bool(const GURL& url, const base::string16& title));
MOCK_METHOD3(UpdateLink,
MOCK_METHOD4(UpdateLink,
bool(const GURL& url,
const GURL& new_url,
const base::string16& new_title));
const base::string16& new_title,
bool is_user_action));
MOCK_METHOD1(DeleteLink, bool(const GURL& url));
MOCK_METHOD0(UndoAction, bool());
MOCK_METHOD1(RegisterCallbackForOnChanged,
Expand Down Expand Up @@ -1501,15 +1502,17 @@ TEST_P(MostVisitedSitesWithCustomLinksTest,

// Initialize custom links and complete a custom link action.
EXPECT_CALL(*mock_custom_links_, Initialize(_)).WillOnce(Return(true));
EXPECT_CALL(*mock_custom_links_, UpdateLink(_, _, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_custom_links_, UpdateLink(_, _, _, _))
.WillOnce(Return(true));
EXPECT_CALL(*mock_custom_links_, IsInitialized())
.WillRepeatedly(Return(true));
EXPECT_CALL(*mock_custom_links_, GetLinks())
.WillRepeatedly(ReturnRef(expected_links));
EXPECT_CALL(mock_observer_, OnURLsAvailable(_))
.WillOnce(SaveArg<0>(&sections));
most_visited_sites_->UpdateCustomLink(GURL("test.com"), GURL("test.com"),
base::UTF8ToUTF16("test"));
base::UTF8ToUTF16("test"),
/*is_user_action=*/true);
base::RunLoop().RunUntilIdle();
ASSERT_THAT(
sections.at(SectionType::PERSONALIZED),
Expand Down

0 comments on commit adf0565

Please sign in to comment.