Fix for broken "Load unpacked extension" popup

chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc

Index: chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
diff --git a/chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc b/chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
index 4065c34e767debeba2be3093ea56473dd5a53245..3923f796ba8ca96b9113b328b6db628e97b88ad4 100644
--- a/chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
+++ b/chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc
@@ -13,7 +13,13 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/common/permissions/api_permission.h"
+#include "extensions/common/permissions/api_permission_set.h"
+#include "extensions/common/permissions/manifest_permission.h"
+#include "extensions/common/permissions/manifest_permission_set.h"
 #include "extensions/common/permissions/permission_set.h"
+#include "extensions/common/url_pattern.h"
+#include "extensions/common/url_pattern_set.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
@@ -23,6 +29,49 @@ namespace {
 const char kWhitelistedId[] = "cbkkbcmdlboombapidmoeolnmdacpkch";
 const char kBogusId[] = "bogus";
 
+class MockManifestPermission : public ManifestPermission {

[Ivan Šandrk, 2017/04/26 13:25:02]

I've c/p-ed this from manifest_permission_set_unittest. Maybe I should extract
it from there into mock_manifest_permission.cc/h so it's easier to reuse?

[Devlin, 2017/04/26 14:58:26]

That would work.  Another option would be to just use a "real" manifest
permission, e.g. SocketsManifestPermission.  I'm fine with either.

(And if you go with the extraction route, feel free to defer that to a separate
CL if you add a TODO)

[Ivan Šandrk, 2017/04/26 17:04:43]

TODO it is.

+ public:
+  MockManifestPermission(const std::string& name)
+      : name_(name) {
+  }
+
+  std::string name() const override { return name_; }
+
+  std::string id() const override { return name(); }
+
+  PermissionIDSet GetPermissions() const override { return PermissionIDSet(); }
+
+  bool FromValue(const base::Value* value) override { return true; }
+
+  std::unique_ptr<base::Value> ToValue() const override {
+    return base::MakeUnique<base::Value>();
+  }
+
+  ManifestPermission* Diff(const ManifestPermission* rhs) const override {
+    const MockManifestPermission* other =
+        static_cast<const MockManifestPermission*>(rhs);
+    EXPECT_EQ(name_, other->name_);
+    return NULL;
+  }
+
+  ManifestPermission* Union(const ManifestPermission* rhs) const override {
+    const MockManifestPermission* other =
+        static_cast<const MockManifestPermission*>(rhs);
+    EXPECT_EQ(name_, other->name_);
+    return new MockManifestPermission(name_);
+  }
+
+  ManifestPermission* Intersect(const ManifestPermission* rhs) const override {
+    const MockManifestPermission* other =
+        static_cast<const MockManifestPermission*>(rhs);
+    EXPECT_EQ(name_, other->name_);
+    return new MockManifestPermission(name_);
+  }
+
+ private:
+  std::string name_;
+};
+
 scoped_refptr<Extension> CreateExtension(const std::string& id) {
   std::string error;
   base::DictionaryValue manifest;
@@ -43,9 +92,16 @@ std::unique_ptr<const PermissionSet> CreatePermissions() {
   apis.insert(APIPermission::kAudio);
   apis.insert(APIPermission::kClipboardRead);
   apis.insert(APIPermission::kFullscreen);
+  ManifestPermissionSet manifest;
+  manifest.insert(new MockManifestPermission("author"));
+  manifest.insert(new MockManifestPermission("background"));
+  URLPatternSet explicit_hosts({
+      URLPattern(URLPattern::SCHEME_ALL, "http://www.google.com/*"),
+      URLPattern(URLPattern::SCHEME_ALL, "<all_urls>")});
+  URLPatternSet scriptable_hosts({
+    URLPattern(URLPattern::SCHEME_ALL, "http://www.wikipedia.com/*")});
   auto permissions = base::MakeUnique<const PermissionSet>(
-      apis, ManifestPermissionSet(),
-      URLPatternSet(), URLPatternSet());
+      apis, manifest, explicit_hosts, scriptable_hosts);
   return permissions;
 }
 
@@ -83,13 +139,18 @@ TEST(PermissionsUpdaterDelegateChromeOSTest,
   delegate.InitializePermissions(extension.get(), &granted_permissions);
   EXPECT_EQ(*CreatePermissions(), *granted_permissions);
 
-  // Bogus extension ID (never whitelisted), ClipboardRead filtered out.
+  // Bogus extension ID (never whitelisted), ClipboardRead filtered out,
+  // everything else stays.
   extension = CreateExtension(kBogusId);
   granted_permissions = CreatePermissions();
   delegate.InitializePermissions(extension.get(), &granted_permissions);
-  EXPECT_FALSE(granted_permissions->HasAPIPermission(
-                   APIPermission::kClipboardRead));
-  EXPECT_EQ(2u, granted_permissions->apis().size());
+  APIPermissionSet apis;
+  apis.insert(APIPermission::kClipboardRead);
+  auto expected_permissions = PermissionSet::CreateDifference(

[Devlin, 2017/04/26 14:58:26]

nitty nit: This mirrors the code a bit too much. :)  I think this would be
cleaner when we construct each set individually and omit clipboard in one of
them, e.g. (pseudo-code):

PermissionSet original = {clipboard, other1, other2};
PermissionSet expected = {other1, other2};

so that it's clear what the difference is.

Given creating the set is more lines of code than we'd want to dupe, I'd suggest
having CreatePermissions take a bool include_clipboard, so that then we do:
PermissionSet original = CreatePermissions(true);
...
EXPECT_EQ(CreatePermissions(false), *granted);

WDYT?

(I realize that fundamentally it's the same, but IMHO this makes it a bit more
clear what exactly the expected difference is)

[Ivan Šandrk, 2017/04/26 17:04:44]

Good point about the mirroring, I actually had a weird feeling about it.

Good suggestion, done.

I'm using a default parameter on CreatePermissions, do you think that's ok or
maybe bad for some reason?

[Devlin, 2017/04/26 18:34:52]

We *used* to have strong rules about not allowing default values, but they've
since been lifted and these are now allowed, but with scrutiny.  Given this
function is local to this file, this would be fine.

My personal preference would be to require the parameter, since it's used in
relatively few places, but that's entirely subjective, so feel free to do
whichever you prefer. :)

+      *CreatePermissions(),
+      PermissionSet(apis, ManifestPermissionSet(),
+                    URLPatternSet(), URLPatternSet()));
+  EXPECT_EQ(*expected_permissions, *granted_permissions);
 
   // Reset state at the end of test.
   chromeos::LoginState::Shutdown();