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

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

BUG=

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

[aboxhall, 2014-07-18 17:23:16]

Ready for a first look.

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

as discussed it would be great if to test this we could remove all of those
implementations (BluetoothManifestPermission etc) and they should all still
work.

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
File extensions/common/permissions/manifest_permission.cc (right):

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.cc:18:
DCHECK(clone->FromValue(ToValue().get()));
make this a CHECK if anything, otherwise FromValue() will be compiled out.

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.cc:49:
IPC::LogParam(singleton, log);
see comment in previous CL about using JSONWriter for this rather than
ListValue/IPC.

see
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
File extensions/common/permissions/manifest_permission.h (right):

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.h:52: // Clones this.
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.

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.h:63: virtual
ManifestPermission* Intersect(const ManifestPermission* rhs)
you should be able to implement Intersect as Union(rhs)->Diff(Diff(rhs)) ... I
think.

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.h:70: virtual bool Equal(const
ManifestPermission* rhs) const = 0;
you should be able to implement Equal as well?

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

PTAL

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
File extensions/common/permissions/manifest_permission.cc (right):

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.cc:18:
DCHECK(clone->FromValue(ToValue().get()));
On 2014/07/18 17:41:17, kalman wrote:
> make this a CHECK if anything, otherwise FromValue() will be compiled out.

Done.

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.cc:49:
IPC::LogParam(singleton, log);
On 2014/07/18 17:41:17, kalman wrote:
> see comment in previous CL about using JSONWriter for this rather than
> ListValue/IPC.
> 
> see
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...

Done. (Not using OPTIONS_PRETTY_PRINT though, is that ok?)

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
File extensions/common/permissions/manifest_permission.h (right):

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.h:52: // Clones this.
On 2014/07/18 17:41:18, kalman wrote:
> 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.

Done (made them non-virtual).

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.h:63: virtual
ManifestPermission* Intersect(const ManifestPermission* rhs)
On 2014/07/18 17:41:17, kalman wrote:
> you should be able to implement Intersect as Union(rhs)->Diff(Diff(rhs)) ... I
> think.

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

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.h:70: virtual bool Equal(const
ManifestPermission* rhs) const = 0;
On 2014/07/18 17:41:17, kalman wrote:
> you should be able to implement Equal as well?

Done.

[not at google - send to devlin, 2014-07-18 22:02:18]

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
File extensions/common/permissions/manifest_permission.cc (right):

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.cc:49:
IPC::LogParam(singleton, log);
On 2014/07/18 21:08:52, aboxhall wrote:
> On 2014/07/18 17:41:17, kalman wrote:
> > see comment in previous CL about using JSONWriter for this rather than
> > ListValue/IPC.
> > 
> > see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...
> 
> Done. (Not using OPTIONS_PRETTY_PRINT though, is that ok?)

so like I mentioned in the other CL I don't really know the purpose for this
method anyway, since I can't find it called from anywhere. but presuming that
the log is for human consumption then having OPTIONS_PRETTY_PRINT sounds like
the right thing.

https://codereview.chromium.org/408493002/diff/120001/extensions/common/manif...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/408493002/diff/120001/extensions/common/manif...
extensions/common/manifest_handler.h:141: virtual ManifestPermission*
CreatePermission(const std::string& key);
see comment in manifest_permission.cc. it's a shame to be making this virtual
just for testing (and FYI the destructor needs to be virtual too).

https://codereview.chromium.org/408493002/diff/120001/extensions/common/permi...
File extensions/common/permissions/manifest_permission.cc (right):

https://codereview.chromium.org/408493002/diff/120001/extensions/common/permi...
extensions/common/permissions/manifest_permission.cc:19: ManifestPermission*
clone = ManifestHandler::CreatePermission(name());
you could implement Clone() via Union with this, right? even though it's kind of
odd, it avoids you needing to make CreatePermission virtual and jump through
those testing hoops.

https://codereview.chromium.org/408493002/diff/120001/extensions/common/permi...
extensions/common/permissions/manifest_permission.cc:26: return
Intersect(rhs)->Equal(rhs);
I think this is a leak, you need to wrap the result of Intersect in a
scoepd_ptr.

[aboxhall, 2014-07-18 22:38:16]

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
File extensions/common/permissions/manifest_permission.cc (right):

https://codereview.chromium.org/408493002/diff/60001/extensions/common/permis...
extensions/common/permissions/manifest_permission.cc:49:
IPC::LogParam(singleton, log);
On 2014/07/18 22:02:18, kalman wrote:
> On 2014/07/18 21:08:52, aboxhall wrote:
> > On 2014/07/18 17:41:17, kalman wrote:
> > > see comment in previous CL about using JSONWriter for this rather than
> > > ListValue/IPC.
> > > 
> > > see
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&sq=...
> > 
> > Done. (Not using OPTIONS_PRETTY_PRINT though, is that ok?)
> 
> so like I mentioned in the other CL I don't really know the purpose for this
> method anyway, since I can't find it called from anywhere. but presuming that
> the log is for human consumption then having OPTIONS_PRETTY_PRINT sounds like
> the right thing.

Done.

https://codereview.chromium.org/408493002/diff/120001/extensions/common/manif...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/408493002/diff/120001/extensions/common/manif...
extensions/common/manifest_handler.h:141: virtual ManifestPermission*
CreatePermission(const std::string& key);
On 2014/07/18 22:02:18, kalman wrote:
> see comment in manifest_permission.cc. it's a shame to be making this virtual
> just for testing (and FYI the destructor needs to be virtual too).

Acknowledged.

https://codereview.chromium.org/408493002/diff/120001/extensions/common/permi...
File extensions/common/permissions/manifest_permission.cc (right):

https://codereview.chromium.org/408493002/diff/120001/extensions/common/permi...
extensions/common/permissions/manifest_permission.cc:19: ManifestPermission*
clone = ManifestHandler::CreatePermission(name());
On 2014/07/18 22:02:18, kalman wrote:
> you could implement Clone() via Union with this, right? even though it's kind
of
> odd, it avoids you needing to make CreatePermission virtual and jump through
> those testing hoops.

SGTM, done.

https://codereview.chromium.org/408493002/diff/120001/extensions/common/permi...
extensions/common/permissions/manifest_permission.cc:26: return
Intersect(rhs)->Equal(rhs);
On 2014/07/18 22:02:18, kalman wrote:
> I think this is a leak, you need to wrap the result of Intersect in a
> scoepd_ptr.

Done.

[not at google - send to devlin, 2014-07-18 22:42:07]

lgtm

[aboxhall, 2014-07-19 01:31:44]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-07-19 01:33:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aboxhall@chromium.org/408493002/140001

[commit-bot: I haz the power, 2014-07-19 02:14:32]

Change committed as 284279