Instead of marking all default apps as bookmark apps, allow it to be specified per-app.

Controlled via the "is_bookmark_app" property in the external extensions JSON
file.

Removes the bookmark app option from GMail, since it uses the "notifications"
permission, thus isn't just a bookmark.

Also removes workaround for bug http://crbug.com/109791 (where we would reset
the FROM_BOOKMARK flag even if it wasn't present anymore), since it's been
out for a few releases, so no more users should need it.

BUG=126042
R=asargent@chromium.org

Review URL: http://codereview.chromium.org/10376004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@135451 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 53 insertions, 80 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 8aeb3a8..9b61cec 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -678,21 +678,6 @@ bool ExtensionService::UpdateExtension(
   if (extension && extension->from_bookmark())
     creation_flags |= Extension::FROM_BOOKMARK;
 
-  if (extension) {
-    // Additionally, if the extension is an external extension, we preserve the
-    // creation flags (usually from_bookmark), even if the current pref values
-    // don't reflect them. This is to fix http://crbug.com/109791 for users that
-    // had default apps updated and lost the from_bookmark bit.
-    ProviderCollection::const_iterator i;
-    for (i = external_extension_providers_.begin();
-        i != external_extension_providers_.end(); ++i) {
-      ExternalExtensionProviderInterface* provider = i->get();
-      if (provider->HasExtension(extension->id())) {
-        creation_flags |= provider->GetCreationFlags();
-        break;
-      }
-    }
-  }
   installer->set_creation_flags(creation_flags);
 
   installer->set_delete_source(true);
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index eb206ba..4366791 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -152,20 +152,7 @@ class MockExtensionProvider : public ExternalExtensionProviderInterface {
   MockExtensionProvider(
       VisitorInterface* visitor,
       Extension::Location location)
-    : location_(location),
-      visitor_(visitor),
-      visit_count_(0),
-      creation_flags_(Extension::NO_FLAGS) {
-  }
-
-  MockExtensionProvider(
-      VisitorInterface* visitor,
-      Extension::Location location,
-      int creation_flags)
-  : location_(location),
-    visitor_(visitor),
-    visit_count_(0),
-    creation_flags_(creation_flags) {
+    : location_(location), visitor_(visitor), visit_count_(0) {
   }
 
   virtual ~MockExtensionProvider() {}
@@ -190,7 +177,7 @@ class MockExtensionProvider : public ExternalExtensionProviderInterface {
 
       visitor_->OnExternalExtensionFileFound(
           i->first, version.get(), i->second.second, location_,
-          creation_flags_, false);
+          Extension::NO_FLAGS, false);
     }
     visitor_->OnExternalProviderReady(this);
   }
@@ -220,10 +207,6 @@ class MockExtensionProvider : public ExternalExtensionProviderInterface {
     return true;
   }
 
-  virtual int GetCreationFlags() const OVERRIDE {
-    return creation_flags_;
-  }
-
   virtual void ServiceShutdown() OVERRIDE {
   }
 
@@ -244,8 +227,6 @@ class MockExtensionProvider : public ExternalExtensionProviderInterface {
   // from the class being mocked.
   mutable int visit_count_;
 
-  int creation_flags_;
-
   DISALLOW_COPY_AND_ASSIGN(MockExtensionProvider);
 };
 
@@ -258,7 +239,14 @@ class MockProviderVisitor
   // and without an empty path using this parameter.
   explicit MockProviderVisitor(FilePath fake_base_path)
       : ids_found_(0),
-        fake_base_path_(fake_base_path) {
+        fake_base_path_(fake_base_path),
+        expected_creation_flags_(Extension::NO_FLAGS) {
+  }
+
+  MockProviderVisitor(FilePath fake_base_path, int expected_creation_flags)
+      : ids_found_(0),
+        fake_base_path_(fake_base_path),
+        expected_creation_flags_(expected_creation_flags) {
   }
 
   int Visit(const std::string& json_data) {
@@ -298,7 +286,7 @@ class MockProviderVisitor
                                             Extension::Location unused,
                                             int creation_flags,
                                             bool mark_acknowledged) {
-    EXPECT_EQ(Extension::NO_FLAGS, creation_flags);
+    EXPECT_EQ(expected_creation_flags_, creation_flags);
 
     ++ids_found_;
     DictionaryValue* pref;
@@ -372,6 +360,7 @@ class MockProviderVisitor
  private:
   int ids_found_;
   FilePath fake_base_path_;
+  int expected_creation_flags_;
   scoped_ptr<ExternalExtensionProviderImpl> provider_;
   scoped_ptr<DictionaryValue> prefs_;
 
@@ -1262,14 +1251,14 @@ TEST_F(ExtensionServiceTest, InstallingExternalExtensionWithFlags) {
   set_extensions_enabled(true);
 
   // Register and install an external extension.
-  MockExtensionProvider* provider =
-      new MockExtensionProvider(service_,
-                                Extension::EXTERNAL_POLICY_DOWNLOAD,
-                                Extension::FROM_BOOKMARK);
-  AddMockExternalProvider(provider);
-  provider->UpdateOrAddExtension(good_crx, "1.0.0.0",
-                                 data_dir_.AppendASCII("good.crx"));
-  service_->CheckForExternalUpdates();
+  scoped_ptr<Version> version(Version::GetVersionFromString("1.0.0.0"));
+  service_->OnExternalExtensionFileFound(
+      good_crx,
+      version.get(),
+      path,
+      Extension::EXTERNAL_PREF,
+      Extension::FROM_BOOKMARK,
+      false /* mark_acknowledged */);
   loop_.RunAllPending();
 
   const Extension* extension = service_->GetExtensionById(good_crx, false);
@@ -1284,23 +1273,6 @@ TEST_F(ExtensionServiceTest, InstallingExternalExtensionWithFlags) {
   extension = service_->GetExtensionById(good_crx, false);
   ASSERT_TRUE(extension);
   ASSERT_TRUE(extension->from_bookmark());
-
-  // Somehow, the "from bookmark pref" gets reset (simulating
-  // http://crbug.com/109791).
-  SetPrefBool(extension->id(), kPrefFromBookmark, false);
-  service_->ReloadExtensions();
-  extension = service_->GetExtensionById(good_crx, false);
-  ASSERT_TRUE(extension);
-  ASSERT_FALSE(extension->from_bookmark());
-  ValidateBooleanPref(good_crx, kPrefFromBookmark, false);
-
-  // If the app gets updated again, we'll reset the "from bookmark" pref if
-  // the external extension provider is still serving that extension.
-  UpdateExtension(good_crx, path, ENABLED);
-  ValidateBooleanPref(good_crx, kPrefFromBookmark, true);
-  extension = service_->GetExtensionById(good_crx, false);
-  ASSERT_TRUE(extension);
-  ASSERT_TRUE(extension->from_bookmark());
 }
 
 // Test the handling of Extension::EXTERNAL_EXTENSION_UNINSTALLED
@@ -2903,7 +2875,7 @@ TEST_F(ExtensionServiceTest, ExternalExtensionAutoAcknowledgement) {
   {
     // Register and install an external extension.
     MockExtensionProvider* provider =
-        new MockExtensionProvider(service_, Extension::EXTERNAL_PREF, 0);
+        new MockExtensionProvider(service_, Extension::EXTERNAL_PREF);
     AddMockExternalProvider(provider);
     provider->UpdateOrAddExtension(good_crx, "1.0.0.0",
                                    data_dir_.AppendASCII("good.crx"));
@@ -3785,6 +3757,19 @@ TEST_F(ExtensionServiceTest, ExternalPrefProvider) {
     ScopedBrowserLocale guard("en-US");
     EXPECT_EQ(2, visitor.Visit(json_data));
   }
+
+  // Test is_bookmark_app.
+  MockProviderVisitor from_bookmark_visitor(
+      base_path, Extension::FROM_BOOKMARK);
+  json_data =
+      "{"
+      "  \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\": {"
+      "    \"external_crx\": \"RandomExtension.crx\","
+      "    \"external_version\": \"1.0\","
+      "    \"is_bookmark_app\": true"
+      "  }"
+      "}";
+  EXPECT_EQ(1, from_bookmark_visitor.Visit(json_data));
 }
 
 // Test loading good extensions from the profile directory.
@@ -4922,9 +4907,7 @@ TEST_F(ExtensionServiceTest, AlertableExtensionHappyPath) {
   scoped_ptr<ExtensionGlobalError> extension_global_error(
       new ExtensionGlobalError(service_));
   MockExtensionProvider* provider =
-      new MockExtensionProvider(service_,
-                                Extension::EXTERNAL_PREF,
-                                0);
+      new MockExtensionProvider(service_, Extension::EXTERNAL_PREF);
   AddMockExternalProvider(provider);
 
   // Should return false, meaning there aren't any extensions that the user
diff --git a/chrome/browser/extensions/external_extension_provider_impl.cc b/chrome/browser/extensions/external_extension_provider_impl.cc
index be832ac..46c3385 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.cc
+++ b/chrome/browser/extensions/external_extension_provider_impl.cc
@@ -49,6 +49,8 @@ const char ExternalExtensionProviderImpl::kExternalUpdateUrl[] =
     "external_update_url";
 const char ExternalExtensionProviderImpl::kSupportedLocales[] =
     "supported_locales";
+const char ExternalExtensionProviderImpl::kIsBookmarkApp[] =
+    "is_bookmark_app";
 
 ExternalExtensionProviderImpl::ExternalExtensionProviderImpl(
     VisitorInterface* service,
@@ -168,6 +170,13 @@ void ExternalExtensionProviderImpl::SetPrefs(DictionaryValue* prefs) {
       }
     }
 
+    int creation_flags = creation_flags_;
+    bool is_bookmark_app;
+    if (extension->GetBoolean(kIsBookmarkApp, &is_bookmark_app) &&
+        is_bookmark_app) {
+      creation_flags |= Extension::FROM_BOOKMARK;
+    }
+
     if (has_external_crx) {
       if (crx_location_ == Extension::INVALID) {
         LOG(WARNING) << "This provider does not support installing external "
@@ -203,7 +212,7 @@ void ExternalExtensionProviderImpl::SetPrefs(DictionaryValue* prefs) {
         continue;
       }
       service_->OnExternalExtensionFileFound(extension_id, version.get(), path,
-                                             crx_location_, creation_flags_,
+                                             crx_location_, creation_flags,
                                              auto_acknowledge_);
     } else { // if (has_external_update_url)
       CHECK(has_external_update_url);  // Checking of keys above ensures this.
@@ -243,10 +252,6 @@ bool ExternalExtensionProviderImpl::IsReady() const {
   return ready_;
 }
 
-int ExternalExtensionProviderImpl::GetCreationFlags() const {
-  return creation_flags_;
-}
-
 bool ExternalExtensionProviderImpl::HasExtension(
     const std::string& id) const {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -388,7 +393,7 @@ void ExternalExtensionProviderImpl::CreateExternalProviders(
                   ExternalPrefExtensionLoader::NONE),
               Extension::EXTERNAL_PREF,
               Extension::INVALID,
-              Extension::FROM_BOOKMARK)));
+              Extension::FROM_WEBSTORE)));
 #endif
 
 #if defined(OS_CHROMEOS)
diff --git a/chrome/browser/extensions/external_extension_provider_impl.h b/chrome/browser/extensions/external_extension_provider_impl.h
index b65edd2..1436fdf 100644
--- a/chrome/browser/extensions/external_extension_provider_impl.h
+++ b/chrome/browser/extensions/external_extension_provider_impl.h
@@ -63,12 +63,12 @@ class ExternalExtensionProviderImpl
                                    scoped_ptr<Version>* version) const OVERRIDE;
 
   virtual bool IsReady() const OVERRIDE;
-  virtual int GetCreationFlags() const OVERRIDE;
 
   static const char kExternalCrx[];
   static const char kExternalVersion[];
   static const char kExternalUpdateUrl[];
   static const char kSupportedLocales[];
+  static const char kIsBookmarkApp[];
 
   void set_auto_acknowledge(bool auto_acknowledge) {
     auto_acknowledge_ = auto_acknowledge;
diff --git a/chrome/browser/extensions/external_extension_provider_interface.h b/chrome/browser/extensions/external_extension_provider_interface.h
index ce51e0e..03951ec 100644
--- a/chrome/browser/extensions/external_extension_provider_interface.h
+++ b/chrome/browser/extensions/external_extension_provider_interface.h
@@ -80,10 +80,6 @@ class ExternalExtensionProviderInterface {
   // Determines if this provider had loaded the list of external extensions
   // from its source.
   virtual bool IsReady() const = 0;
-
-  // The creation flags that the provider passes to
-  // OnExternalExtensionFileFound.
-  virtual int GetCreationFlags() const = 0;
 };
 
 typedef std::vector<linked_ptr<ExternalExtensionProviderInterface> >
diff --git a/chrome/browser/resources/default_apps/OWNERS b/chrome/browser/resources/default_apps/OWNERS
new file mode 100644
index 0000000..ca168aa
--- /dev/null
+++ b/chrome/browser/resources/default_apps/OWNERS
@@ -0,0 +1,2 @@
+asargent@chromium.org
+mihaip@chromium.org
diff --git a/chrome/browser/resources/default_apps/external_extensions.json b/chrome/browser/resources/default_apps/external_extensions.json
index ba08f21..bdc29f9 100644
--- a/chrome/browser/resources/default_apps/external_extensions.json
+++ b/chrome/browser/resources/default_apps/external_extensions.json
@@ -7,11 +7,13 @@
 {
   "blpcfgokakmgnkcojhhkbfbldkacnbeo" : {
     "external_crx": "youtube.crx",
-    "external_version": "4.2"
+    "external_version": "4.2",
+    "is_bookmark_app": true
   },
   "coobgpohoikkiipiblmjeljniedjpjpf" : {
     "external_crx": "search.crx",
-    "external_version": "0.0.0.14"
+    "external_version": "0.0.0.14",
+    "is_bookmark_app": true
   },
   "pjkljhegncpnkpknbcohdijeoejaedia" : {
     "external_crx": "gmail.crx",