Merge 288365 "Fix permission regression where warnings aren't sh..."

> Fix permission regression where warnings aren't shown in the dialog
> 
> A regression was introduced in r275142 causing the permissions dialog to not
> show permissions. Obviously very bad. Fixed, and added a regression test.
> 
> BUG=397900
> 
> Review URL: https://codereview.chromium.org/448883002

TBR=rdevlin.cronin@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/branches/2062/src@289119 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 84 insertions, 22 deletions

diff --git a/chrome/browser/extensions/extension_install_prompt.cc b/chrome/browser/extensions/extension_install_prompt.cc
index 074b046..97af6cb 100644
--- a/chrome/browser/extensions/extension_install_prompt.cc
+++ b/chrome/browser/extensions/extension_install_prompt.cc
@@ -712,22 +712,18 @@ void ExtensionInstallPrompt::ShowConfirmation() {
   else
     prompt_->set_experiment(ExtensionInstallPromptExperiment::ControlGroup());
 
-  if (permissions_.get()) {
-    if (extension_) {
-      const extensions::PermissionsData* permissions_data =
-          extension_->permissions_data();
-      prompt_->SetPermissions(permissions_data->GetPermissionMessageStrings());
-      prompt_->SetPermissionsDetails(
-          permissions_data->GetPermissionMessageDetailsStrings());
-    } else {
-      const extensions::PermissionMessageProvider* message_provider =
-          extensions::PermissionMessageProvider::Get();
-      prompt_->SetPermissions(message_provider->GetWarningMessages(
-          permissions_, Manifest::TYPE_UNKNOWN));
-      prompt_->SetPermissionsDetails(
-          message_provider->GetWarningMessagesDetails(permissions_,
-                                                      Manifest::TYPE_UNKNOWN));
-    }
+  if (permissions_.get() &&
+      (!extension_ ||
+       !extensions::PermissionsData::ShouldSkipPermissionWarnings(
+           extension_->id()))) {
+    Manifest::Type type =
+        extension_ ? extension_->GetType() : Manifest::TYPE_UNKNOWN;
+    const extensions::PermissionMessageProvider* message_provider =
+        extensions::PermissionMessageProvider::Get();
+    prompt_->SetPermissions(
+        message_provider->GetWarningMessages(permissions_, type));
+    prompt_->SetPermissionsDetails(
+        message_provider->GetWarningMessagesDetails(permissions_, type));
   }
 
   switch (prompt_->type()) {
diff --git a/chrome/browser/extensions/extension_install_prompt.h b/chrome/browser/extensions/extension_install_prompt.h
index 57ed542..59dcd6c 100644
--- a/chrome/browser/extensions/extension_install_prompt.h
+++ b/chrome/browser/extensions/extension_install_prompt.h
@@ -353,6 +353,10 @@ class ExtensionInstallPrompt
   // Installation failed. This is declared virtual for testing.
   virtual void OnInstallFailure(const extensions::CrxInstallerError& error);
 
+  void set_callback_for_test(const ShowDialogCallback& show_dialog_callback) {
+    show_dialog_callback_ = show_dialog_callback;
+  }
+
  protected:
   friend class extensions::ExtensionWebstorePrivateApiTest;
   friend class WebstoreStartupInstallUnpackFailureTest;
diff --git a/chrome/browser/extensions/extension_install_prompt_unittest.cc b/chrome/browser/extensions/extension_install_prompt_unittest.cc
new file mode 100644
index 0000000..347db0f
--- /dev/null
+++ b/chrome/browser/extensions/extension_install_prompt_unittest.cc
@@ -0,0 +1,57 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "chrome/browser/extensions/extension_install_prompt.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/permissions/api_permission.h"
+#include "extensions/common/permissions/api_permission_set.h"
+#include "extensions/common/permissions/manifest_permission_set.h"
+#include "extensions/common/permissions/permission_set.h"
+#include "extensions/common/url_pattern_set.h"
+#include "extensions/common/value_builder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+void VerifyPromptPermissionsCallback(
+    const base::Closure& quit_closure,
+    const ExtensionInstallPrompt::ShowParams& params,
+    ExtensionInstallPrompt::Delegate* delegate,
+    scoped_refptr<ExtensionInstallPrompt::Prompt> install_prompt) {
+  ASSERT_TRUE(install_prompt.get());
+  EXPECT_EQ(1u, install_prompt->GetPermissionCount());
+  quit_closure.Run();
+}
+
+TEST(ExtensionInstallPromptUnittest, PromptShowsPermissionWarnings) {
+  content::TestBrowserThreadBundle thread_bundle;
+  APIPermissionSet api_permissions;
+  api_permissions.insert(APIPermission::kTab);
+  scoped_refptr<PermissionSet> permission_set =
+      new PermissionSet(api_permissions,
+                        ManifestPermissionSet(),
+                        URLPatternSet(),
+                        URLPatternSet());
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder().SetManifest(
+          DictionaryBuilder().Set("name", "foo")
+                             .Set("version", "1.0")
+                             .Set("manifest_version", 2)
+                             .Set("description", "Random Ext")).Build();
+  ExtensionInstallPrompt prompt(NULL /* no web contents in this test */);
+  base::RunLoop run_loop;
+  prompt.set_callback_for_test(base::Bind(&VerifyPromptPermissionsCallback,
+                                          run_loop.QuitClosure()));
+  prompt.ConfirmPermissions(NULL,  // no delegate
+                            extension,
+                            permission_set);
+  run_loop.Run();
+}
+
+}  // namespace extensions
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index 0e3721c..bdfcec7 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -966,6 +966,7 @@
         'browser/extensions/extension_garbage_collector_chromeos_unittest.cc',
         'browser/extensions/extension_gcm_app_handler_unittest.cc',
         'browser/extensions/extension_icon_manager_unittest.cc',
+        'browser/extensions/extension_install_prompt_unittest.cc',
         'browser/extensions/extension_message_bubble_controller_unittest.cc',
         'browser/extensions/extension_prefs_unittest.cc',
         'browser/extensions/extension_prefs_unittest.h',
diff --git a/extensions/common/permissions/permissions_data.cc b/extensions/common/permissions/permissions_data.cc
index 1b160ca..b6c87d4 100644
--- a/extensions/common/permissions/permissions_data.cc
+++ b/extensions/common/permissions/permissions_data.cc
@@ -25,12 +25,6 @@ namespace {
 
 PermissionsData::PolicyDelegate* g_policy_delegate = NULL;
 
-// Returns true if this extension id is from a trusted provider.
-bool ShouldSkipPermissionWarnings(const std::string& extension_id) {
-  // See http://b/4946060 for more details.
-  return extension_id == std::string("nckgahadagoaajjgafhacjanaoiihapd");
-}
-
 }  // namespace
 
 PermissionsData::PermissionsData(const Extension* extension)
@@ -71,6 +65,12 @@ bool PermissionsData::CanExecuteScriptEverywhere(const Extension* extension) {
          whitelist.end();
 }
 
+bool PermissionsData::ShouldSkipPermissionWarnings(
+    const std::string& extension_id) {
+  // See http://b/4946060 for more details.
+  return extension_id == std::string("nckgahadagoaajjgafhacjanaoiihapd");
+}
+
 // static
 bool PermissionsData::IsRestrictedUrl(const GURL& document_url,
                                       const GURL& top_frame_url,
diff --git a/extensions/common/permissions/permissions_data.h b/extensions/common/permissions/permissions_data.h
index 9470d58..bcdc7c3 100644
--- a/extensions/common/permissions/permissions_data.h
+++ b/extensions/common/permissions/permissions_data.h
@@ -68,6 +68,10 @@ class PermissionsData {
   // whitelist of extensions that can script all pages.
   static bool CanExecuteScriptEverywhere(const Extension* extension);
 
+  // Returns true if we should skip the permisisons warning for the extension
+  // with the given |extension_id|.
+  static bool ShouldSkipPermissionWarnings(const std::string& extension_id);
+
   // Returns true if the given |url| is restricted for the given |extension|,
   // as is commonly the case for chrome:// urls.
   // NOTE: You probably want to use CanAccessPage().