Revert "[NTP] Separate internal custom link updates from user changes"

This reverts commit adf05659.

Reason for revert: Removing the HEAD request. See https://crbug/874194.

Original change's description:
> [NTP] Separate internal custom link updates from user changes
> 
> 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}

TBR=treib@chromium.org,kristipark@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 896143
Change-Id: I26fd1f3a92161fe06009681ff90b5ea65c2db2b7
Reviewed-on: https://chromium-review.googlesource.com/c/1298553Reviewed-by: Kristi Park <kristipark@chromium.org>
Commit-Queue: Kristi Park <kristipark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602536}

chrome/browser/search/instant_service.cc

@@ -322,8 +322,7 @@ 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),
-                                                 /*is_user_action=*/true);
+                                                 base::UTF8ToUTF16(new_title));
   }
   return false;
 }
@@ -490,14 +489,11 @@ 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);

components/ntp_tiles/custom_links_manager.h

@@ -65,13 +65,10 @@ 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. |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.
+  // are empty.
   virtual bool UpdateLink(const GURL& url,
                           const GURL& new_url,
-                          const base::string16& new_title,
-                          bool is_user_action) = 0;
+                          const base::string16& new_title) = 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.

components/ntp_tiles/custom_links_manager_impl.cc

@@ -94,8 +94,7 @@ bool CustomLinksManagerImpl::AddLink(const GURL& url,
 
 bool CustomLinksManagerImpl::UpdateLink(const GURL& url,
                                         const GURL& new_url,
-                                        const base::string16& new_title,
-                                        bool is_user_action) {
+                                        const base::string16& new_title) {
   if (!IsInitialized() || !url.is_valid() ||
       (new_url.is_empty() && new_title.empty())) {
     return false;
@@ -113,10 +112,7 @@ bool CustomLinksManagerImpl::UpdateLink(const GURL& url,
     return false;
 
   // At this point, we will be modifying at least one of the values.
-  if (is_user_action) {
-    // Save the previous state since this was a user update.
-    previous_links_ = current_links_;
-  }
+  previous_links_ = current_links_;
 
   if (!new_url.is_empty())
     it->url = new_url;

components/ntp_tiles/custom_links_manager_impl.h

@@ -48,8 +48,7 @@ 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,
-                  bool is_user_action) override;
+                  const base::string16& new_title) override;
   bool DeleteLink(const GURL& url) override;
   bool UndoAction() override;
 

components/ntp_tiles/custom_links_manager_impl_unittest.cc

@@ -219,20 +219,18 @@ TEST_F(CustomLinksManagerImplTest, UpdateLink) {
 
   // Update the link's URL.
   EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(kTestUrl),
-                                        base::string16(),
-                                        /*is_user_action=*/true));
+                                        base::string16()));
   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),
-                                        /*is_user_action=*/true));
+                                        base::UTF8ToUTF16(kTestTitle)));
   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),
-                                        /*is_user_action=*/true));
+  EXPECT_TRUE(
+      custom_links_->UpdateLink(GURL(kTestUrl), GURL(kTestCase1[0].url),
+                                base::UTF8ToUTF16(kTestCase1[0].title)));
   EXPECT_EQ(links_after_update_both, custom_links_->GetLinks());
 }
 
@@ -248,24 +246,20 @@ 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),
-                                         /*is_user_action=*/true));
+                                         base::UTF8ToUTF16(kTestTitle)));
   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(),
-                                         /*is_user_action=*/true));
+                                         base::string16()));
   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),
-                                         /*is_user_action=*/true));
+                                         base::UTF8ToUTF16(kTestTitle)));
   EXPECT_EQ(initial_links, custom_links_->GetLinks());
   EXPECT_FALSE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), kInvalidUrl,
-                                         base::string16(),
-                                         /*is_user_action=*/true));
+                                         base::string16()));
   EXPECT_EQ(initial_links, custom_links_->GetLinks());
 }
 
@@ -280,8 +274,7 @@ 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(),
-      /*is_user_action=*/true));
+      GURL(kTestCase2[0].url), GURL(kTestCase2[1].url), base::string16()));
   EXPECT_EQ(initial_links, custom_links_->GetLinks());
 }
 
@@ -357,8 +350,7 @@ TEST_F(CustomLinksManagerImplTest, UndoUpdateLink) {
 
   // Update the link's URL.
   EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(kTestUrl),
-                                        base::string16(),
-                                        /*is_user_action=*/true));
+                                        base::string16()));
   EXPECT_EQ(links_after_update_url, custom_links_->GetLinks());
 
   // Undo update link.
@@ -367,8 +359,7 @@ TEST_F(CustomLinksManagerImplTest, UndoUpdateLink) {
 
   // Update the link's title.
   EXPECT_TRUE(custom_links_->UpdateLink(GURL(kTestCase1[0].url), GURL(),
-                                        base::UTF8ToUTF16(kTestTitle),
-                                        /*is_user_action=*/true));
+                                        base::UTF8ToUTF16(kTestTitle)));
   EXPECT_EQ(links_after_update_title, custom_links_->GetLinks());
 
   // Undo update link.
@@ -422,48 +413,6 @@ 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);

components/ntp_tiles/most_visited_sites.cc

@@ -299,19 +299,16 @@ bool MostVisitedSites::AddCustomLink(const GURL& url,
 
 bool MostVisitedSites::UpdateCustomLink(const GURL& url,
                                         const GURL& new_url,
-                                        const base::string16& new_title,
-                                        bool is_user_action) {
+                                        const base::string16& new_title) {
   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, is_user_action);
+  bool success = custom_links_->UpdateLink(url, new_url, new_title);
   if (success) {
-    // Only update the action count if this was executed by the user.
-    if (is_user_action && custom_links_action_count_ != -1)
+    if (custom_links_action_count_ != -1)
       custom_links_action_count_++;
     BuildCurrentTiles();
   }

components/ntp_tiles/most_visited_sites.h

@@ -180,12 +180,10 @@ 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. |is_user_action| is true if this was executed by
-  // the user (i.e. editing a custom link). Custom links must be enabled.
+  // not been initialized yet. Custom links must be enabled.
   bool UpdateCustomLink(const GURL& url,
                         const GURL& new_url,
-                        const base::string16& new_title,
-                        bool is_user_action);
+                        const base::string16& new_title);
   // 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

components/ntp_tiles/most_visited_sites_unittest.cc

@@ -272,11 +272,10 @@ 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_METHOD4(UpdateLink,
+  MOCK_METHOD3(UpdateLink,
                bool(const GURL& url,
                     const GURL& new_url,
-                    const base::string16& new_title,
-                    bool is_user_action));
+                    const base::string16& new_title));
   MOCK_METHOD1(DeleteLink, bool(const GURL& url));
   MOCK_METHOD0(UndoAction, bool());
   MOCK_METHOD1(RegisterCallbackForOnChanged,
@@ -1505,8 +1504,7 @@ 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())
@@ -1514,8 +1512,7 @@ TEST_P(MostVisitedSitesWithCustomLinksTest,
   EXPECT_CALL(mock_observer_, OnURLsAvailable(_))
       .WillOnce(SaveArg<0>(&sections));
   most_visited_sites_->UpdateCustomLink(GURL("test.com"), GURL("test.com"),
-                                        base::UTF8ToUTF16("test"),
-                                        /*is_user_action=*/true);
+                                        base::UTF8ToUTF16("test"));
   base::RunLoop().RunUntilIdle();
   ASSERT_THAT(
       sections.at(SectionType::PERSONALIZED),