Fix and enable ExtensionApiTest.Bookmarks

chrome.bookmarks.removeTree is asynchronous. The test wasn't waiting
for the result of removeTree before proceeding, hence there was flakiness.
This CL changes the test (getRecentSetup) so that we wait for all steps
to complete before proceeding to next test.

This CL also modifies BookmarksRemoveTreeFunction to not inherit from
BookmarksRemoveFunction. Instead make both of them inherit from
a separate base (BookmarksRemoveFunctionBase). This eliminates the
potential of incorrectly calculating |recursive| variable, because the
current code relies on pointer equality (ExtensionFunction::name() ==
BookmarksRemoveTreeFunction::function_name()) test. The pointer
equality check isn't supposed to be stable and it readily fails
on ASAN bots.

Bug: 383452
Change-Id: Idf3995750b4f7ab647caebe8d1c777a19e708d1e
Reviewed-on: https://chromium-review.googlesource.com/899853
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534669}

chrome/browser/extensions/api/bookmarks/bookmark_apitest.cc

@@ -21,13 +21,7 @@
 
 using bookmarks::BookmarkModel;
 
-// Flaky on Windows and Linux. http://crbug.com/383452
-#if defined(OS_WIN) || defined(OS_LINUX)
-#define MAYBE_Bookmarks DISABLED_Bookmarks
-#else
-#define MAYBE_Bookmarks Bookmarks
-#endif
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, MAYBE_Bookmarks) {
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Bookmarks) {
   // Add test managed bookmarks to verify that the bookmarks API can read them
   // and can't modify them.
   Profile* profile = browser()->profile();

chrome/browser/extensions/api/bookmarks/bookmarks_api.cc

@@ -555,7 +555,7 @@ bool BookmarksSearchFunction::RunOnReady() {
   return true;
 }
 
-bool BookmarksRemoveFunction::RunOnReady() {
+bool BookmarksRemoveFunctionBase::RunOnReady() {
   if (!EditBookmarksEnabled())
     return false;
 
@@ -567,15 +567,21 @@ bool BookmarksRemoveFunction::RunOnReady() {
   if (!GetBookmarkIdAsInt64(params->id, &id))
     return false;
 
-  bool recursive = false;
-  if (name() == BookmarksRemoveTreeFunction::function_name())
-    recursive = true;
-
   BookmarkModel* model = GetBookmarkModel();
   ManagedBookmarkService* managed = GetManagedBookmarkService();
-  if (!bookmark_api_helpers::RemoveNode(model, managed, id, recursive, &error_))
+  if (!bookmark_api_helpers::RemoveNode(model, managed, id, is_recursive(),
+                                        &error_)) {
+    return false;
+  }
+
+  return true;
+}
+
+bool BookmarksRemoveFunction::is_recursive() const {
   return false;
+}
 
+bool BookmarksRemoveTreeFunction::is_recursive() const {
   return true;
 }
 

chrome/browser/extensions/api/bookmarks/bookmarks_api.h

@@ -244,23 +244,36 @@ class BookmarksSearchFunction : public BookmarksFunction {
   bool RunOnReady() override;
 };
 
-class BookmarksRemoveFunction : public BookmarksFunction {
+class BookmarksRemoveFunctionBase : public BookmarksFunction {
+ protected:
+  ~BookmarksRemoveFunctionBase() override {}
+
+  virtual bool is_recursive() const = 0;
+
+  // BookmarksFunction:
+  bool RunOnReady() override;
+};
+
+class BookmarksRemoveFunction : public BookmarksRemoveFunctionBase {
  public:
-  DECLARE_EXTENSION_FUNCTION("bookmarks.remove", BOOKMARKS_REMOVE)
+  DECLARE_EXTENSION_FUNCTION("bookmarks.remove", BOOKMARKS_REMOVE);
 
  protected:
   ~BookmarksRemoveFunction() override {}
 
-  // BookmarksFunction:
-  bool RunOnReady() override;
+  // BookmarksRemoveFunctionBase:
+  bool is_recursive() const override;
 };
 
-class BookmarksRemoveTreeFunction : public BookmarksRemoveFunction {
+class BookmarksRemoveTreeFunction : public BookmarksRemoveFunctionBase {
  public:
   DECLARE_EXTENSION_FUNCTION("bookmarks.removeTree", BOOKMARKS_REMOVETREE)
 
  protected:
   ~BookmarksRemoveTreeFunction() override {}
+
+  // BookmarksRemoveFunctionBase:
+  bool is_recursive() const override;
 };
 
 class BookmarksCreateFunction : public BookmarksFunction {

chrome/test/data/extensions/api_test/bookmarks/test.js

@@ -473,17 +473,24 @@ chrome.test.runTests([
   },
 
   function getRecentSetup() {
+    Promise.all([removeAllChildren('1'), removeAllChildren('2')]).then(
+        pass(afterRemove));
+
+    function removeTreePromise(id) {
+      return new Promise(pass(function(resolve) {
+        chrome.bookmarks.removeTree(id, resolve);
+      }));
+    }
+
     // Clean up tree
-    ["1", "2"].forEach(function(id) {
+    function removeAllChildren(id) {
+      return new Promise(pass(function(resolve) {
         chrome.bookmarks.getChildren(id, pass(function(children) {
-        children.forEach(function(child, i) {
-          chrome.bookmarks.removeTree(child.id, pass(function() {}));
-          // When we have removed the last child we can continue.
-          if (id == "2" && i == children.length - 1)
-            afterRemove();
-        });
+          Promise.all(children.map((child) => removeTreePromise(child.id)))
+              .then(resolve);
         }));
-    });
+      }));
+    }
 
     function afterRemove() {
       // Once done add 3 nodes