Make Clone, Contains, Equal and IPC function non-virtual and implement in manifest_permission.cc

extensions/common/permissions/manifest_permission.h

 // Copyright 2013 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.
 
 #ifndef EXTENSIONS_COMMON_PERMISSIONS_MANIFEST_PERMISSION_H_
 #define EXTENSIONS_COMMON_PERMISSIONS_MANIFEST_PERMISSION_H_
 
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
@@ skipping 31 matching lines @@
 
   // Returns the localized permission messages of this permission.
   virtual PermissionMessages GetMessages() const = 0;
 
   // Parses the ManifestPermission from |value|. Returns false if error happens.
   virtual bool FromValue(const base::Value* value) = 0;
 
   // Stores this into a new created Value.
   virtual scoped_ptr<base::Value> ToValue() const = 0;
 
   // Clones this.

[not at google - send to devlin, 2014/07/18 17:41:18]

you should add comments for all of these functions which are implemented by
default, or even better, move them into a bunch with a generic comment. I mean
like:

// These functions have already been implemented for you due to yada yada,
// but maybe you insist on implementing them yourselves.

or even better, just don't make them virtual as discussed.

[aboxhall, 2014/07/18 21:08:52]

Done (made them non-virtual).

-  virtual ManifestPermission* Clone() const = 0;
+  virtual ManifestPermission* Clone() const;
 
   // Returns a new manifest permission which equals this - |rhs|.
   virtual ManifestPermission* Diff(const ManifestPermission* rhs) const = 0;
 
   // Returns a new manifest permission which equals the union of this and |rhs|.
   virtual ManifestPermission* Union(const ManifestPermission* rhs) const = 0;
 
   // Returns a new manifest permission which equals the intersect of this and
   // |rhs|.
   virtual ManifestPermission* Intersect(const ManifestPermission* rhs)

[not at google - send to devlin, 2014/07/18 17:41:17]

you should be able to implement Intersect as Union(rhs)->Diff(Diff(rhs)) ... I
think.

[aboxhall, 2014/07/18 21:08:52]

Not done. Too weird/magical - I can see the logic, but it's not immediately
obvious the way Contains() is.

       const = 0;
 
   // Returns true if |rhs| is a subset of this.
-  virtual bool Contains(const ManifestPermission* rhs) const = 0;
+  virtual bool Contains(const ManifestPermission* rhs) const;
 
   // Returns true if |rhs| is equal to this.
   virtual bool Equal(const ManifestPermission* rhs) const = 0;

[not at google - send to devlin, 2014/07/18 17:41:17]

you should be able to implement Equal as well?

[aboxhall, 2014/07/18 21:08:52]

Done.

 
   // IPC functions
   // Writes this into the given IPC message |m|.
-  virtual void Write(IPC::Message* m) const = 0;
+  virtual void Write(IPC::Message* m) const;
 
   // Reads from the given IPC message |m|.
-  virtual bool Read(const IPC::Message* m, PickleIterator* iter) = 0;
+  virtual bool Read(const IPC::Message* m, PickleIterator* iter);
 
   // Logs this permission.
-  virtual void Log(std::string* log) const = 0;
+  virtual void Log(std::string* log) const;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ManifestPermission);
 };
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_COMMON_PERMISSIONS_MANIFEST_PERMISSION_H_