Commit ebb18035 authored by pneubeck's avatar pneubeck Committed by Commit bot

Revert of Don't create a sync node when updating an URL that wasn't typed....

Revert of Don't create a sync node when updating an URL that wasn't typed. (patchset #1 id:1 of https://codereview.chromium.org/1126633005/)

Reason for revert:
Suspected to cause failure of
MigrationSingleClientTest.AllTypesIndividually
on Linux GN:

https://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/28301

Original issue's description:
> Don't create a sync node when updating an URL that wasn't typed.
>
> TypedUrlChangeProcessor is a HistoryService observer and gets notified
> about changes to all addresses, not just typed ones. This means that
> when HistoryService::SetPageTitle() is called for a non-typed URL,
> TypedUrlChangeProcessor::OnURLsModified() will be triggered and,
> consequently, TypedUrlChangeProcessor::CreateOrUpdateSyncNode().
> As the name suggests, if the modified node was not previously present
> (because it was filtered out by ShouldSyncVisit() in OnURLVisited()),
> it would be created.
> End effect was that *all* URLs that had their title updated (which means
> every page that has a <title> tag) were synced, even if they were not typed.
>
> The solution is to return early in CreateOrUpdateSyncNode() if we detect
> that the URL in question has no typed visits.
>
> (Caused by https://codereview.chromium.org/1087773002/)
>
> BUG=
>
> Committed: https://crrev.com/3be1b71d63be98a42c0e98673ff35fcd4fc738ab
> Cr-Commit-Position: refs/heads/master@{#329370}

TBR=maniscalco@chromium.org,pvalenzuela@chromium.org,mpawlowski@opera.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review URL: https://codereview.chromium.org/1127353004

Cr-Commit-Position: refs/heads/master@{#329396}
parent 3d20a9d0
......@@ -157,14 +157,6 @@ bool TypedUrlChangeProcessor::CreateOrUpdateSyncNode(
return false;
}
if (std::find_if(visit_vector.begin(), visit_vector.end(),
[](const history::VisitRow& visit) {
return ui::PageTransitionCoreTypeIs(
visit.transition, ui::PAGE_TRANSITION_TYPED);
}) == visit_vector.end())
// This URL has no TYPED visits, don't sync it.
return false;
syncer::ReadNode typed_url_root(trans);
if (typed_url_root.InitTypeRoot(syncer::TYPED_URLS) !=
syncer::BaseNode::INIT_OK) {
......
......@@ -377,44 +377,6 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, UpdateToNonTypedURL) {
ASSERT_EQ(2, GetVisitCountForFirstURL(0));
}
IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest,
DontSyncUpdatedNonTypedURLs) {
// Checks if a non-typed URL that has been updated (modified) doesn't get
// synced. This is a regression test after fixing a bug where adding a
// non-typed URL was guarded against but later modifying it was not. Since
// "update" is "update or create if missing", non-typed URLs were being
// created.
const GURL kNonTypedURL("http://link.google.com/");
const GURL kTypedURL("http://typed.google.com/");
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
AddUrlToHistoryWithTransition(0, kNonTypedURL, ui::PAGE_TRANSITION_LINK,
history::SOURCE_BROWSED);
AddUrlToHistoryWithTransition(0, kTypedURL, ui::PAGE_TRANSITION_TYPED,
history::SOURCE_BROWSED);
// Modify the non-typed URL. It should not get synced.
typed_urls_helper::SetPageTitle(0, kNonTypedURL, "Welcome to Non-Typed URL");
ASSERT_TRUE(AwaitCheckAllProfilesHaveSameURLsAsVerifier());
history::VisitVector visits;
// First client has both visits.
visits = typed_urls_helper::GetVisitsForURLFromClient(0, kNonTypedURL);
ASSERT_EQ(1U, visits.size());
EXPECT_TRUE(ui::PageTransitionCoreTypeIs(visits[0].transition,
ui::PAGE_TRANSITION_LINK));
visits = typed_urls_helper::GetVisitsForURLFromClient(0, kTypedURL);
ASSERT_EQ(1U, visits.size());
EXPECT_TRUE(ui::PageTransitionCoreTypeIs(visits[0].transition,
ui::PAGE_TRANSITION_TYPED));
// Second client has only the typed visit.
visits = typed_urls_helper::GetVisitsForURLFromClient(1, kNonTypedURL);
ASSERT_EQ(0U, visits.size());
visits = typed_urls_helper::GetVisitsForURLFromClient(1, kTypedURL);
ASSERT_EQ(1U, visits.size());
EXPECT_TRUE(ui::PageTransitionCoreTypeIs(visits[0].transition,
ui::PAGE_TRANSITION_TYPED));
}
IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, SyncTypedRedirects) {
const base::string16 kHistoryUrl(ASCIIToUTF16("http://typed.google.com/"));
const base::string16 kRedirectedHistoryUrl(
......
......@@ -169,6 +169,7 @@ void AddToHistory(history::HistoryService* service,
transition,
source,
false);
service->SetPageTitle(url, base::ASCIIToUTF16(url.spec() + " - title"));
}
history::URLRows GetTypedUrlsFromHistoryService(
......@@ -331,16 +332,6 @@ void DeleteUrlsFromHistory(int index, const std::vector<GURL>& urls) {
WaitForHistoryDBThread(index);
}
void SetPageTitle(int index, const GURL& url, const std::string& title) {
HistoryServiceFactory::GetForProfileWithoutCreating(test()->GetProfile(index))
->SetPageTitle(url, base::UTF8ToUTF16(title));
if (test()->use_verifier())
HistoryServiceFactory::GetForProfile(test()->verifier(),
ServiceAccessType::IMPLICIT_ACCESS)
->SetPageTitle(url, base::UTF8ToUTF16(title));
WaitForHistoryDBThread(index);
}
bool CheckURLRowVectorsAreEqual(const history::URLRows& left,
const history::URLRows& right) {
if (left.size() != right.size())
......
......@@ -61,9 +61,6 @@ void DeleteUrlFromHistory(int index, const GURL& url);
// profile.
void DeleteUrlsFromHistory(int index, const std::vector<GURL>& urls);
// Modifies an URL stored in history by setting a new title.
void SetPageTitle(int index, const GURL& url, const std::string& title);
// Returns true if all clients match the verifier profile.
bool CheckAllProfilesHaveSameURLsAsVerifier();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment