Move permission warning message handling from PermissionSet to PermissionMessageProvider.

Move permission warning message handling from PermissionSet to PermissionMessageProvider.

This refactors PermissionSet to be closer to just a set of permissions and moves the understanding of permission message strings to a utility class.

BUG=162530
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229565

[Yoyo Zhou, 2013-10-16 01:31:32]

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set.h (right):

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set.h:5: #ifndef
CHROME_COMMON_EXTENSIONS_PERMISSIONS_PERMISSION_SET_H_
We could move PermissionSet to extensions/common/permissions at this point, but
this CL is big enough already.

[Matt Perry, 2013-10-16 20:07:04]

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/app_permission_message_provider.cc
(right):

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/app_permission_message_provider.cc:19:
AppPermissionMessageProvider::GetHostPermissionMessages(
It seems like these 2 special cases are the only reason you have multiple
message providers. Wouldn't it be simpler to just have 1 provider, and pass in
the manifest_type to these 2 methods?

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
File
chrome/common/extensions/permissions/extension_permission_message_provider.h
(right):

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/extension_permission_message_provider.h:17:
class ExtensionPermissionMessageProvider : public PermissionMessageProvider {
Since this is used for apps as well, maybe BasePermissionMessageProvider is more
fitting.

https://codereview.chromium.org/27446002/diff/3001/extensions/common/permissi...
File extensions/common/permissions/permission_message_provider.cc (right):

https://codereview.chromium.org/27446002/diff/3001/extensions/common/permissi...
extensions/common/permissions/permission_message_provider.cc:20:
std::vector<std::pair<Manifest::Type, PermissionMessageProvider* > >
how about a map?

https://codereview.chromium.org/27446002/diff/3001/extensions/common/permissi...
File extensions/common/permissions/permission_message_provider.h (right):

https://codereview.chromium.org/27446002/diff/3001/extensions/common/permissi...
extensions/common/permissions/permission_message_provider.h:31: static void
SetProviders(
This interface feels a bit clunky. How about splitting it up into:
  SetDefaultProvider(x);
  SetProviderForType(type, x);

Also, use scoped_ptr to advertise the ownership transfer.

[Yoyo Zhou, 2013-10-17 19:45:51]

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/app_permission_message_provider.cc
(right):

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/app_permission_message_provider.cc:19:
AppPermissionMessageProvider::GetHostPermissionMessages(
On 2013/10/16 20:07:04, Matt Perry wrote:
> It seems like these 2 special cases are the only reason you have multiple
> message providers. Wouldn't it be simpler to just have 1 provider, and pass in
> the manifest_type to these 2 methods?

Ok. I thought this was going to be more useful than it turned out to be. Changed
back to a single message provider.

(This makes a bunch of the other comments obsolete.)

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
File
chrome/common/extensions/permissions/extension_permission_message_provider.h
(right):

https://codereview.chromium.org/27446002/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/extension_permission_message_provider.h:17:
class ExtensionPermissionMessageProvider : public PermissionMessageProvider {
On 2013/10/16 20:07:04, Matt Perry wrote:
> Since this is used for apps as well, maybe BasePermissionMessageProvider is
more
> fitting.

Done.

https://codereview.chromium.org/27446002/diff/3001/extensions/common/permissi...
File extensions/common/permissions/permission_message_provider.h (right):

https://codereview.chromium.org/27446002/diff/3001/extensions/common/permissi...
extensions/common/permissions/permission_message_provider.h:31: static void
SetProviders(
On 2013/10/16 20:07:04, Matt Perry wrote:
> This interface feels a bit clunky. How about splitting it up into:
>   SetDefaultProvider(x);
>   SetProviderForType(type, x);
> 
> Also, use scoped_ptr to advertise the ownership transfer.

I moved the ownership to ChromeExtensionsClient.

[Matt Perry, 2013-10-17 21:47:29]

LGTM except for the name

https://codereview.chromium.org/27446002/diff/13001/chrome/common/extensions/...
File chrome/common/extensions/permissions/base_permission_message_provider.h
(right):

https://codereview.chromium.org/27446002/diff/13001/chrome/common/extensions/...
chrome/common/extensions/permissions/base_permission_message_provider.h:17:
class BasePermissionMessageProvider : public PermissionMessageProvider {
Well, now it's not a base class anymore :)

How about ChromePermissionMessageProvider, in the spirit of the other chrome
delegate implementations? (sorry to be a pain)

[Yoyo Zhou, 2013-10-18 19:08:00]

https://codereview.chromium.org/27446002/diff/13001/chrome/common/extensions/...
File chrome/common/extensions/permissions/base_permission_message_provider.h
(right):

https://codereview.chromium.org/27446002/diff/13001/chrome/common/extensions/...
chrome/common/extensions/permissions/base_permission_message_provider.h:17:
class BasePermissionMessageProvider : public PermissionMessageProvider {
On 2013/10/17 21:47:30, Matt Perry wrote:
> Well, now it's not a base class anymore :)
> 
> How about ChromePermissionMessageProvider, in the spirit of the other chrome
> delegate implementations? (sorry to be a pain)

Sure.

[commit-bot: I haz the power, 2013-10-18 19:45:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/27446002/19001

[commit-bot: I haz the power, 2013-10-18 19:45:18]

Failed to apply patch for
chrome/common/extensions/permissions/permission_set.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/permissions/permission_set.cc
  Hunk #2 FAILED at 141.
  Hunk #3 succeeded at 293 (offset -18 lines).
  Hunk #4 succeeded at 321 (offset -18 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/common/extensions/permissions/permission_set.cc.rej

Patch:       chrome/common/extensions/permissions/permission_set.cc
Index: chrome/common/extensions/permissions/permission_set.cc
diff --git a/chrome/common/extensions/permissions/permission_set.cc
b/chrome/common/extensions/permissions/permission_set.cc
index
d027605f281369a3562aea726b88d388870d2997..9c9701463d516900e2f512bdfa4dbfe3eb34f699
100644
--- a/chrome/common/extensions/permissions/permission_set.cc
+++ b/chrome/common/extensions/permissions/permission_set.cc
@@ -8,35 +8,15 @@
 #include <iterator>
 #include <string>
 
-#include "base/stl_util.h"
-#include "chrome/common/extensions/permissions/permission_message_util.h"
-#include "content/public/common/url_constants.h"
-#include "extensions/common/extensions_client.h"
 #include "extensions/common/permissions/permissions_info.h"
 #include "extensions/common/url_pattern.h"
 #include "extensions/common/url_pattern_set.h"
-#include "grit/generated_resources.h"
-#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
-#include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
 using extensions::URLPatternSet;
 
 namespace {
 
-// Helper for GetDistinctHosts(): com > net > org > everything else.
-bool RcdBetterThan(const std::string& a, const std::string& b) {
-  if (a == b)
-    return false;
-  if (a == "com")
-    return true;
-  if (a == "net")
-    return b != "com";
-  if (a == "org")
-    return b != "com" && b != "net";
-  return false;
-}
-
 void AddPatternsAndRemovePaths(const URLPatternSet& set, URLPatternSet* out) {
   DCHECK(out);
   for (URLPatternSet::const_iterator i = set.begin(); i != set.end(); ++i) {
@@ -161,97 +141,6 @@ std::set<std::string> PermissionSet::GetAPIsAsStrings()
const {
   return apis_str;
 }
 
-PermissionMessages PermissionSet::GetPermissionMessages(
-    Manifest::Type extension_type) const {
-  PermissionMessages messages;
-
-  if (HasEffectiveFullAccess()) {
-    messages.push_back(PermissionMessage(
-        PermissionMessage::kFullAccess,
-        l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_FULL_ACCESS)));
-    return messages;
-  }
-
-  std::set<PermissionMessage> host_msgs =
-      GetHostPermissionMessages(extension_type);
-  std::set<PermissionMessage> api_msgs = GetAPIPermissionMessages();
-  messages.insert(messages.end(), host_msgs.begin(), host_msgs.end());
-  messages.insert(messages.end(), api_msgs.begin(), api_msgs.end());
-
-  return messages;
-}
-
-std::vector<string16> PermissionSet::GetWarningMessages(
-    Manifest::Type extension_type) const {
-  std::vector<string16> messages;
-  PermissionMessages permissions = GetPermissionMessages(extension_type);
-
-  bool audio_capture = false;
-  bool video_capture = false;
-  bool media_galleries_read = false;
-  bool media_galleries_copy_to = false;
-  for (PermissionMessages::const_iterator i = permissions.begin();
-       i != permissions.end(); ++i) {
-    switch (i->id()) {
-      case PermissionMessage::kAudioCapture:
-        audio_capture = true;
-        break;
-      case PermissionMessage::kVideoCapture:
-        video_capture = true;
-        break;
-      case PermissionMessage::kMediaGalleriesAllGalleriesRead:
-        media_galleries_read = true;
-        break;
-      case PermissionMessage::kMediaGalleriesAllGalleriesCopyTo:
-        media_galleries_copy_to = true;
-        break;
-      default:
-        break;
-    }
-  }
-
-  for (PermissionMessages::const_iterator i = permissions.begin();
-       i != permissions.end(); ++i) {
-    int id = i->id();
-    if (audio_capture && video_capture) {
-      if (id == PermissionMessage::kAudioCapture) {
-        messages.push_back(l10n_util::GetStringUTF16(
-            IDS_EXTENSION_PROMPT_WARNING_AUDIO_AND_VIDEO_CAPTURE));
-        continue;
-      } else if (id == PermissionMessage::kVideoCapture) {
-        // The combined message will be pushed above.
-        continue;
-      }
-    }
-    if (media_galleries_read && media_galleries_copy_to) {
-      if (id == PermissionMessage::kMediaGalleriesAllGalleriesRead) {
-        messages.push_back(l10n_util::GetStringUTF16(
-            IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE));
-        continue;
-      } else if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo) {
-        // The combined message will be pushed above.
-        continue;
-      }
-    }
-
-    messages.push_back(i->message());
-  }
-
-  return messages;
-}
-
-std::vector<string16> PermissionSet::GetWarningMessagesDetails(
-    Manifest::Type extension_type) const {
-  std::vector<string16> messages;
-  PermissionMessages permissions = GetPermissionMessages(extension_type);
-
-  for (PermissionMessages::const_iterator i = permissions.begin();
-       i != permissions.end(); ++i)
-    messages.push_back(i->details());
-
-  return messages;
-}
-
 bool PermissionSet::IsEmpty() const {
   // Not default if any host permissions are present.
   if (!(explicit_hosts().is_empty() && scriptable_hosts().is_empty()))
@@ -331,84 +220,8 @@ bool PermissionSet::HasEffectiveFullAccess() const {
   return false;
 }
 
-bool PermissionSet::HasLessPrivilegesThan(
-    const PermissionSet* permissions,
-    Manifest::Type extension_type) const {
-  // Things can't get worse than native code access.
-  if (HasEffectiveFullAccess())
-    return false;
-
-  // Otherwise, it's a privilege increase if the new one has full access.
-  if (permissions->HasEffectiveFullAccess())
-    return true;
-
-  if (HasLessHostPrivilegesThan(permissions, extension_type))
-    return true;
-
-  if (HasLessAPIPrivilegesThan(permissions))
-    return true;
-
-  return false;
-}
-
 PermissionSet::~PermissionSet() {}
 
-// static
-std::set<std::string> PermissionSet::GetDistinctHosts(
-    const URLPatternSet& host_patterns,
-    bool include_rcd,
-    bool exclude_file_scheme) {
-  // Use a vector to preserve order (also faster than a map on small sets).
-  // Each item is a host split into two parts: host without RCDs and
-  // current best RCD.
-  typedef std::vector<std::pair<std::string, std::string> > HostVector;
-  HostVector hosts_best_rcd;
-  for (URLPatternSet::const_iterator i = host_patterns.begin();
-       i != host_patterns.end(); ++i) {
-    if (exclude_file_scheme && i->scheme() == chrome::kFileScheme)
-      continue;
-
-    std::string host = i->host();
-
-    // Add the subdomain wildcard back to the host, if necessary.
-    if (i->match_subdomains())
-      host = "*." + host;
-
-    // If the host has an RCD, split it off so we can detect duplicates.
-    std::string rcd;
-    size_t reg_len = net::registry_controlled_domains::GetRegistryLength(
-        host,
-        net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
-        net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
-    if (reg_len && reg_len != std::string::npos) {
-      if (include_rcd)  // else leave rcd empty
-        rcd = host.substr(host.size() - reg_len);
-      host = host.substr(0, host.size() - reg_len);
-    }
-
-    // Check if we've already seen this host.
-    HostVector::iterator it = hosts_best_rcd.begin();
-    for (; it != hosts_best_rcd.end(); ++it) {
-      if (it->first == host)
-        break;
-    }
-    // If this host was found, replace the RCD if this one is better.
-    if (it != hosts_best_rcd.end()) {
-      if (include_rcd && RcdBetterThan(rcd, it->second))
-        it->second = rcd;
-    } else {  // Previously unseen host, append it.
-      hosts_best_rcd.push_back(std::make_pair(host, rcd));
-    }
-  }
-
-  // Build up the final vector by concatenating hosts and RCDs.
-  std::set<std::string> distinct_hosts;
-  for (HostVector::iterator it = hosts_best_rcd.begin();
-       it != hosts_best_rcd.end(); ++it)
-    distinct_hosts.insert(it->first + it->second);
-  return distinct_hosts;
-}
-
 void PermissionSet::InitImplicitPermissions() {
   // The downloads permission implies the internal version as well.
   if (apis_.find(APIPermission::kDownloads) != apis_.end())
@@ -435,127 +248,4 @@ void PermissionSet::InitEffectiveHosts() {
       explicit_hosts(), scriptable_hosts(), &effective_hosts_);
 }
 
-std::set<PermissionMessage> PermissionSet::GetAPIPermissionMessages() const {
-  std::set<PermissionMessage> messages;
-  for (APIPermissionSet::const_iterator permission_it = apis_.begin();
-       permission_it != apis_.end(); ++permission_it) {
-    if (permission_it->HasMessages()) {
-      PermissionMessages new_messages = permission_it->GetMessages();
-      messages.insert(new_messages.begin(), new_messages.end());
-    }
-  }
-
-  // A special hack: If kFileSystemWriteDirectory would be displayed, hide
-  // kFileSystemDirectory and and kFileSystemWrite as the write directory
-  // message implies the other two.
-  // TODO(sammc): Remove this. See http://crbug.com/284849.
-  std::set<PermissionMessage>::iterator write_directory_message =
-      messages.find(PermissionMessage(
-          PermissionMessage::kFileSystemWriteDirectory, string16()));
-  if (write_directory_message != messages.end()) {
-    messages.erase(
-        PermissionMessage(PermissionMessage::kFileSystemWrite, string16()));
-    messages.erase(
-        PermissionMessage(PermissionMessage::kFileSystemDirectory,
string16()));
-  }
-
-  // A special hack: The warning message for declarativeWebRequest
-  // permissions speaks about blocking parts of pages, which is a
-  // subset of what the "<all_urls>" access allows. Therefore we
-  // display only the "<all_urls>" warning message if both perm…
(message too large)

[commit-bot: I haz the power, 2013-10-18 21:37:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/27446002/99001

[commit-bot: I haz the power, 2013-10-19 15:37:33]

Change committed as 229565