dom_distiller: Move ArticleEntry away from ArticleSpecifics

ArticleEntry used to be an alias for sync_pb::ArticleSpecifics. However,
the Sync data type for articles doesn't exist anymore, and we'd like to
delete the protos. So this CL replaces the alias with a simple struct.
This lets us remove all build dependencies from dom_distiller to sync.

Bug: 1007942
Change-Id: Ia9eb2cb3256a540ab3b2f63e8ae621f92121ed1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869223
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707408}

components/dom_distiller/DEPS

@@ -4,8 +4,6 @@ include_rules = [
   "+components/pref_registry",
   "+components/prefs",
   "+components/strings/grit/components_strings.h",
-  "+components/sync/model",
-  "+components/sync/protocol",
   "+components/sync_preferences",
   "+components/variations",
   "+crypto", # For sha256

components/dom_distiller/content/browser/BUILD.gn

@@ -32,7 +32,6 @@ static_library("browser") {
     "//components/dom_distiller/content/common/mojom",
     "//components/resources",
     "//components/strings",
-    "//components/sync",
     "//content/public/browser",
     "//content/public/common",
     "//mojo/public/cpp/bindings",

components/dom_distiller/core/BUILD.gn

@@ -63,7 +63,6 @@ static_library("core") {
     "//components/prefs",
     "//components/resources",
     "//components/strings",
-    "//components/sync",
     "//components/variations",
     "//services/network/public/cpp:cpp",
     "//skia",
@@ -95,7 +94,6 @@ static_library("test_support") {
     ":core",
     "//base",
     "//components/leveldb_proto:test_support",
-    "//components/sync",
     "//testing/gmock",
     "//testing/gtest",
     "//url",
@@ -140,7 +138,6 @@ source_set("unit_tests") {
     "//components/leveldb_proto:test_support",
     "//components/resources",
     "//components/strings",
-    "//components/sync",
     "//components/sync_preferences:test_support",
     "//net:test_support",
     "//services/network:test_support",

components/dom_distiller/core/article_entry.cc

@@ -8,24 +8,18 @@
 
 namespace dom_distiller {
 
-bool IsEntryPageValid(const ArticleEntryPage& page) {
-  return page.has_url();
-}
+ArticleEntry::ArticleEntry() = default;
+ArticleEntry::ArticleEntry(const ArticleEntry&) = default;
+ArticleEntry::~ArticleEntry() = default;
 
 bool IsEntryValid(const ArticleEntry& entry) {
-  if (!entry.has_entry_id())
+  if (entry.entry_id.empty())
     return false;
-  for (int i = 0; i < entry.pages_size(); ++i) {
-    if (!IsEntryPageValid(entry.pages(i)))
+  for (const GURL& page : entry.pages) {
+    if (!page.is_valid())
       return false;
   }
   return true;
 }
 
-bool AreEntriesEqual(const ArticleEntry& left, const ArticleEntry& right) {
-  DCHECK(IsEntryValid(left));
-  DCHECK(IsEntryValid(right));
-  return left.SerializeAsString() == right.SerializeAsString();
-}
-
 }  // namespace dom_distiller

components/dom_distiller/core/article_entry.h

@@ -5,17 +5,25 @@
 #ifndef COMPONENTS_DOM_DISTILLER_CORE_ARTICLE_ENTRY_H_
 #define COMPONENTS_DOM_DISTILLER_CORE_ARTICLE_ENTRY_H_
 
-#include "components/sync/protocol/article_specifics.pb.h"
+#include <string>
+#include <vector>
+
+#include "url/gurl.h"
 
 namespace dom_distiller {
 
-typedef sync_pb::ArticleSpecifics ArticleEntry;
-typedef sync_pb::ArticlePage ArticleEntryPage;
+struct ArticleEntry {
+  ArticleEntry();
+  ArticleEntry(const ArticleEntry&);
+  ~ArticleEntry();
 
-// A valid entry has an entry_id and all its pages have a URL.
-bool IsEntryValid(const ArticleEntry& entry);
+  std::string entry_id;
+  std::string title;
+  std::vector<GURL> pages;
+};
 
-bool AreEntriesEqual(const ArticleEntry& left, const ArticleEntry& right);
+// A valid entry has a non-empty entry_id and all its pages have a valid URL.
+bool IsEntryValid(const ArticleEntry& entry);
 
 }  // namespace dom_distiller
 

components/dom_distiller/core/article_entry_unittest.cc

@@ -11,46 +11,12 @@ namespace dom_distiller {
 TEST(DomDistillerArticleEntryTest, TestIsEntryValid) {
   ArticleEntry entry;
   EXPECT_FALSE(IsEntryValid(entry));
-  entry.set_entry_id("entry0");
+  entry.entry_id = "entry0";
   EXPECT_TRUE(IsEntryValid(entry));
-  ArticleEntryPage* page0 = entry.add_pages();
+  entry.pages.push_back(GURL());
   EXPECT_FALSE(IsEntryValid(entry));
-  page0->set_url("example.com/1");
+  entry.pages.back() = GURL("https://example.com/1");
   EXPECT_TRUE(IsEntryValid(entry));
 }
 
-TEST(DomDistillerArticleEntryTest, TestAreEntriesEqual) {
-  ArticleEntry left;
-  ArticleEntry right;
-  left.set_entry_id("entry0");
-  right.set_entry_id("entry1");
-  EXPECT_FALSE(AreEntriesEqual(left, right));
-  right = left;
-  EXPECT_TRUE(AreEntriesEqual(left, right));
-
-  left.set_title("a title");
-  EXPECT_FALSE(AreEntriesEqual(left, right));
-  right.set_title("a different title");
-  EXPECT_FALSE(AreEntriesEqual(left, right));
-  right.set_title("a title");
-  EXPECT_TRUE(AreEntriesEqual(left, right));
-
-  ArticleEntryPage left_page;
-  left_page.set_url("example.com/1");
-  *left.add_pages() = left_page;
-
-  EXPECT_FALSE(AreEntriesEqual(left, right));
-
-  ArticleEntryPage right_page;
-  right_page.set_url("foo.example.com/1");
-  *right.add_pages() = right_page;
-  EXPECT_FALSE(AreEntriesEqual(left, right));
-
-  right = left;
-  EXPECT_TRUE(AreEntriesEqual(left, right));
-
-  *right.add_pages() = right_page;
-  EXPECT_FALSE(AreEntriesEqual(left, right));
-}
-
 }  // namespace dom_distiller

components/dom_distiller/core/distilled_content_store.cc

@@ -36,12 +36,12 @@ void InMemoryContentStore::LoadContent(
   if (callback.is_null())
     return;
 
-  ContentMap::const_iterator it = cache_.Get(entry.entry_id());
+  ContentMap::const_iterator it = cache_.Get(entry.entry_id);
   bool success = it != cache_.end();
   if (!success) {
     // Could not find article by entry ID, so try looking it up by URL.
-    for (int i = 0; i < entry.pages_size(); ++i) {
-      UrlMap::const_iterator url_it = url_to_id_.find(entry.pages(i).url());
+    for (const GURL& page : entry.pages) {
+      UrlMap::const_iterator url_it = url_to_id_.find(page.spec());
       if (url_it != url_to_id_.end()) {
         it = cache_.Get(url_it->second);
         success = it != cache_.end();
@@ -64,7 +64,7 @@ void InMemoryContentStore::LoadContent(
 
 void InMemoryContentStore::InjectContent(const ArticleEntry& entry,
                                          const DistilledArticleProto& proto) {
-  cache_.Put(entry.entry_id(),
+  cache_.Put(entry.entry_id,
              std::unique_ptr<DistilledArticleProto, CacheDeletor>(
                  new DistilledArticleProto(proto), CacheDeletor(this)));
   AddUrlToIdMapping(entry, proto);
@@ -76,7 +76,7 @@ void InMemoryContentStore::AddUrlToIdMapping(
   for (int i = 0; i < proto.pages_size(); i++) {
     const DistilledPageProto& page = proto.pages(i);
     if (page.has_url()) {
-      url_to_id_[page.url()] = entry.entry_id();
+      url_to_id_[page.url()] = entry.entry_id;
     }
   }
 }

components/dom_distiller/core/distilled_content_store.h

@@ -79,7 +79,6 @@ class InMemoryContentStore : public DistilledContentStore {
 
   typedef base::MRUCache<std::string,
                          std::unique_ptr<DistilledArticleProto, CacheDeletor>>
-
       ContentMap;
   typedef std::unordered_map<std::string, std::string> UrlMap;
 

components/dom_distiller/core/distilled_content_store_unittest.cc

@@ -18,22 +18,19 @@ namespace dom_distiller {
 namespace {
 
 ArticleEntry CreateEntry(std::string entry_id,
-                         std::string page_url1,
-                         std::string page_url2,
-                         std::string page_url3) {
+                         GURL page_url1 = GURL(),
+                         GURL page_url2 = GURL(),
+                         GURL page_url3 = GURL()) {
   ArticleEntry entry;
-  entry.set_entry_id(entry_id);
-  if (!page_url1.empty()) {
-    ArticleEntryPage* page = entry.add_pages();
-    page->set_url(page_url1);
+  entry.entry_id = entry_id;
+  if (!page_url1.is_empty()) {
+    entry.pages.push_back(page_url1);
   }
-  if (!page_url2.empty()) {
-    ArticleEntryPage* page = entry.add_pages();
-    page->set_url(page_url2);
+  if (!page_url2.is_empty()) {
+    entry.pages.push_back(page_url2);
   }
-  if (!page_url3.empty()) {
-    ArticleEntryPage* page = entry.add_pages();
-    page->set_url(page_url3);
+  if (!page_url3.is_empty()) {
+    entry.pages.push_back(page_url3);
   }
   return entry;
 }
@@ -41,10 +38,10 @@ ArticleEntry CreateEntry(std::string entry_id,
 DistilledArticleProto CreateDistilledArticleForEntry(
     const ArticleEntry& entry) {
   DistilledArticleProto article;
-  for (int i = 0; i < entry.pages_size(); ++i) {
+  for (const GURL& url : entry.pages) {
     DistilledPageProto* page = article.add_pages();
-    page->set_url(entry.pages(i).url());
-    page->set_html("<div>" + entry.pages(i).url() + "</div>");
+    page->set_url(url.spec());
+    page->set_html("<div>" + url.spec() + "</div>");
   }
   return article;
 }
@@ -79,7 +76,9 @@ class InMemoryContentStoreTest : public testing::Test {
 // Tests whether saving and then loading a single article works as expected.
 TEST_F(InMemoryContentStoreTest, SaveAndLoadSingleArticle) {
   base::test::SingleThreadTaskEnvironment task_environment;
-  const ArticleEntry entry = CreateEntry("test-id", "url1", "url2", "url3");
+  const ArticleEntry entry =
+      CreateEntry("test-id", GURL("https://url1"), GURL("https://url2"),
+                  GURL("https://url3"));
   const DistilledArticleProto stored_proto =
       CreateDistilledArticleForEntry(entry);
   store_->SaveContent(entry, stored_proto,
@@ -102,7 +101,9 @@ TEST_F(InMemoryContentStoreTest, SaveAndLoadSingleArticle) {
 // where success is false.
 TEST_F(InMemoryContentStoreTest, LoadNonExistentArticle) {
   base::test::SingleThreadTaskEnvironment task_environment;
-  const ArticleEntry entry = CreateEntry("bogus-id", "url1", "url2", "url3");
+  const ArticleEntry entry =
+      CreateEntry("bogus-id", GURL("https://url1"), GURL("https://url2"),
+                  GURL("https://url3"));
   store_->LoadContent(entry,
                       base::Bind(&InMemoryContentStoreTest::OnLoadCallback,
                                  base::Unretained(this)));
@@ -116,7 +117,9 @@ TEST_F(InMemoryContentStoreTest, LoadNonExistentArticle) {
 TEST_F(InMemoryContentStoreTest, SaveAndLoadMultipleArticles) {
   base::test::SingleThreadTaskEnvironment task_environment;
   // Store first article.
-  const ArticleEntry first_entry = CreateEntry("first", "url1", "url2", "url3");
+  const ArticleEntry first_entry =
+      CreateEntry("first", GURL("https://url1"), GURL("https://url2"),
+                  GURL("https://url3"));
   const DistilledArticleProto first_stored_proto =
       CreateDistilledArticleForEntry(first_entry);
   store_->SaveContent(first_entry, first_stored_proto,
@@ -128,7 +131,8 @@ TEST_F(InMemoryContentStoreTest, SaveAndLoadMultipleArticles) {
 
   // Store second article.
   const ArticleEntry second_entry =
-      CreateEntry("second", "url4", "url5", "url6");
+      CreateEntry("second", GURL("https://url4"), GURL("https://url5"),
+                  GURL("https://url6"));
   const DistilledArticleProto second_stored_proto =
       CreateDistilledArticleForEntry(second_entry);
   store_->SaveContent(second_entry, second_stored_proto,
@@ -169,7 +173,9 @@ TEST_F(InMemoryContentStoreTest, SaveAndLoadMoreThanMaxArticles) {
   store_.reset(new InMemoryContentStore(kMaxNumArticles));
 
   // Store first article.
-  const ArticleEntry first_entry = CreateEntry("first", "url1", "url2", "url3");
+  const ArticleEntry first_entry =
+      CreateEntry("first", GURL("https://url1"), GURL("https://url2"),
+                  GURL("https://url3"));
   const DistilledArticleProto first_stored_proto =
       CreateDistilledArticleForEntry(first_entry);
   store_->SaveContent(first_entry, first_stored_proto,
@@ -181,7 +187,8 @@ TEST_F(InMemoryContentStoreTest, SaveAndLoadMoreThanMaxArticles) {
 
   // Store second article.
   const ArticleEntry second_entry =
-      CreateEntry("second", "url4", "url5", "url6");
+      CreateEntry("second", GURL("https://url4"), GURL("https://url5"),
+                  GURL("https://url6"));
   const DistilledArticleProto second_stored_proto =
       CreateDistilledArticleForEntry(second_entry);
   store_->SaveContent(second_entry, second_stored_proto,
@@ -192,7 +199,9 @@ TEST_F(InMemoryContentStoreTest, SaveAndLoadMoreThanMaxArticles) {
   save_success_ = false;
 
   // Store third article.
-  const ArticleEntry third_entry = CreateEntry("third", "url7", "url8", "url9");
+  const ArticleEntry third_entry =
+      CreateEntry("third", GURL("https://url7"), GURL("https://url8"),
+                  GURL("https://url9"));
   const DistilledArticleProto third_stored_proto =
       CreateDistilledArticleForEntry(third_entry);
   store_->SaveContent(third_entry, third_stored_proto,
@@ -216,7 +225,8 @@ TEST_F(InMemoryContentStoreTest, SaveAndLoadMoreThanMaxArticles) {
 
   // Store fourth article.
   const ArticleEntry fourth_entry =
-      CreateEntry("fourth", "url10", "url11", "url12");
+      CreateEntry("fourth", GURL("https://url10"), GURL("https://url11"),
+                  GURL("https://url12"));
   const DistilledArticleProto fourth_stored_proto =
       CreateDistilledArticleForEntry(fourth_entry);
   store_->SaveContent(fourth_entry, fourth_stored_proto,
@@ -240,7 +250,9 @@ TEST_F(InMemoryContentStoreTest, SaveAndLoadMoreThanMaxArticles) {
 // Tests whether saving and then loading a single article works as expected.
 TEST_F(InMemoryContentStoreTest, LookupArticleByURL) {
   base::test::SingleThreadTaskEnvironment task_environment;
-  const ArticleEntry entry = CreateEntry("test-id", "url1", "url2", "url3");
+  const ArticleEntry entry =
+      CreateEntry("test-id", GURL("https://url1"), GURL("https://url2"),
+                  GURL("https://url3"));
   const DistilledArticleProto stored_proto =
       CreateDistilledArticleForEntry(entry);
   store_->SaveContent(entry, stored_proto,
@@ -251,7 +263,8 @@ TEST_F(InMemoryContentStoreTest, LookupArticleByURL) {
   save_success_ = false;
 
   // Create an entry where the entry ID does not match, but the first URL does.
-  const ArticleEntry lookup_entry1 = CreateEntry("lookup-id", "url1", "", "");
+  const ArticleEntry lookup_entry1 =
+      CreateEntry("lookup-id", GURL("https://url1"));
   store_->LoadContent(lookup_entry1,
                       base::Bind(&InMemoryContentStoreTest::OnLoadCallback,
                                  base::Unretained(this)));
@@ -262,7 +275,7 @@ TEST_F(InMemoryContentStoreTest, LookupArticleByURL) {
 
   // Create an entry where the entry ID does not match, but the second URL does.
   const ArticleEntry lookup_entry2 =
-      CreateEntry("lookup-id", "bogus", "url2", "");
+      CreateEntry("lookup-id", GURL("bogus"), GURL("https://url2"));
   store_->LoadContent(lookup_entry2,
                       base::Bind(&InMemoryContentStoreTest::OnLoadCallback,
                                  base::Unretained(this)));
@@ -282,7 +295,9 @@ TEST_F(InMemoryContentStoreTest, LoadArticleByURLAfterExpungedFromCache) {
   store_.reset(new InMemoryContentStore(kMaxNumArticles));
 
   // Store an article.
-  const ArticleEntry first_entry = CreateEntry("first", "url1", "url2", "url3");
+  const ArticleEntry first_entry =
+      CreateEntry("first", GURL("https://url1"), GURL("https://url2"),
+                  GURL("https://url3"));
   const DistilledArticleProto first_stored_proto =
       CreateDistilledArticleForEntry(first_entry);
   store_->SaveContent(first_entry, first_stored_proto,
@@ -295,7 +310,7 @@ TEST_F(InMemoryContentStoreTest, LoadArticleByURLAfterExpungedFromCache) {
   // Looking up the first entry by URL should succeed when it is still in the
   // cache.
   const ArticleEntry first_entry_lookup =
-      CreateEntry("lookup-id", "url1", "", "");
+      CreateEntry("lookup-id", GURL("https://url1"));
   store_->LoadContent(first_entry_lookup,
                       base::Bind(&InMemoryContentStoreTest::OnLoadCallback,
                                  base::Unretained(this)));
@@ -306,7 +321,8 @@ TEST_F(InMemoryContentStoreTest, LoadArticleByURLAfterExpungedFromCache) {
 
   // Store second article. This will remove the first article from the cache.
   const ArticleEntry second_entry =
-      CreateEntry("second", "url4", "url5", "url6");
+      CreateEntry("second", GURL("https://url4"), GURL("https://url5"),
+                  GURL("https://url6"));
   const DistilledArticleProto second_stored_proto =
       CreateDistilledArticleForEntry(second_entry);
   store_->SaveContent(second_entry, second_stored_proto,

components/dom_distiller/core/dom_distiller_service.cc

@@ -23,9 +23,8 @@ namespace {
 
 ArticleEntry CreateSkeletonEntryForUrl(const GURL& url) {
   ArticleEntry skeleton;
-  skeleton.set_entry_id(base::GenerateGUID());
-  ArticleEntryPage* page = skeleton.add_pages();
-  page->set_url(url.spec());
+  skeleton.entry_id = base::GenerateGUID();
+  skeleton.pages.push_back(url);
 
   DCHECK(IsEntryValid(skeleton));
   return skeleton;

components/dom_distiller/core/task_tracker.cc

@@ -49,10 +49,10 @@ void TaskTracker::StartDistiller(
   if (distiller_) {
     return;
   }
-  if (entry_.pages_size() == 0) {
+  if (entry_.pages.empty()) {
     return;
   }
-  GURL url(entry_.pages(0).url());
+  GURL url(entry_.pages[0]);
   DCHECK(url.is_valid());
 
   distiller_ = factory->CreateDistillerForUrl(url);
@@ -97,16 +97,16 @@ std::unique_ptr<ViewerHandle> TaskTracker::AddViewer(
 }
 
 const std::string& TaskTracker::GetEntryId() const {
-  return entry_.entry_id();
+  return entry_.entry_id;
 }
 
 bool TaskTracker::HasEntryId(const std::string& entry_id) const {
-  return entry_.entry_id() == entry_id;
+  return entry_.entry_id == entry_id;
 }
 
 bool TaskTracker::HasUrl(const GURL& url) const {
-  for (int i = 0; i < entry_.pages_size(); ++i) {
-    if (entry_.pages(i).url() == url.spec()) {
+  for (const GURL& page : entry_.pages) {
+    if (page == url) {
       return true;
     }
   }
@@ -208,11 +208,10 @@ void TaskTracker::DistilledArticleReady(
   content_ready_ = true;
 
   distilled_article_ = std::move(distilled_article);
-  entry_.set_title(distilled_article_->title());
-  entry_.clear_pages();
+  entry_.title = distilled_article_->title();
+  entry_.pages.clear();
   for (int i = 0; i < distilled_article_->pages_size(); ++i) {
-    sync_pb::ArticlePage* page = entry_.add_pages();
-    page->set_url(distilled_article_->pages(i).url());
+    entry_.pages.push_back(GURL(distilled_article_->pages(i).url()));
   }
 
   NotifyViewersAndCallbacks();

components/dom_distiller/core/task_tracker_unittest.cc

@@ -69,11 +69,9 @@ class DomDistillerTaskTrackerTest : public testing::Test {
 
   ArticleEntry GetDefaultEntry() {
     ArticleEntry entry;
-    entry.set_entry_id(entry_id_);
-    ArticleEntryPage* page0 = entry.add_pages();
-    ArticleEntryPage* page1 = entry.add_pages();
-    page0->set_url(page_0_url_.spec());
-    page1->set_url(page_1_url_.spec());
+    entry.entry_id = entry_id_;
+    entry.pages.push_back(page_0_url_);
+    entry.pages.push_back(page_1_url_);
     return entry;
   }
 
@@ -219,10 +217,10 @@ TEST_F(DomDistillerTaskTrackerTest,
 DistilledArticleProto CreateDistilledArticleForEntry(
     const ArticleEntry& entry) {
   DistilledArticleProto article;
-  for (int i = 0; i < entry.pages_size(); ++i) {
+  for (const GURL& url : entry.pages) {
     DistilledPageProto* page = article.add_pages();
-    page->set_url(entry.pages(i).url());
-    page->set_html("<div>" + entry.pages(i).url() + "</div>");
+    page->set_url(url.spec());
+    page->set_html("<div>" + url.spec() + "</div>");
   }
   return article;
 }