Create a ManifestPermission implementation for Automation.

Create a ManifestPermission implementation for Automation.

BUG=

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

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

[David Tseng, 2014-07-08 18:37:39]

One small comment.

https://codereview.chromium.org/377553003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/automation.cc:40: class
AutomationManifestPermission : public ManifestPermission {
I personally would prefer if this class lived in its own file/header.

[aboxhall, 2014-07-18 15:51:02]

Ok, this is ready for an actual look now.

[aboxhall, 2014-07-18 15:55:15]

https://codereview.chromium.org/377553003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/automation.cc:40: class
AutomationManifestPermission : public ManifestPermission {
On 2014/07/08 18:37:39, David Tseng wrote:
> I personally would prefer if this class lived in its own file/header.

Yeah, I'm in two minds about this, and there are examples of both styles (with
and without header) in the codebase:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
(with)
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
(without)

I've left it as-is for now, but happy to change it if we decide that's better.

For keeping it as it is, I guess the argument is that it's really an
implementation detail - the only really interesting bit is GetMessages().

For pulling it out into its own type, I guess the argument would be that it's
fairly complex?

[not at google - send to devlin, 2014-07-18 16:07:23]

sorry devlin, just a couple of drive-bys since alice and I were discussing the
IPC implementation earlier, and I've been thinking about the verbosity of
ManifestPermission in the past.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:91: virtual
ManifestPermission* Clone() const OVERRIDE {
(comment made even later)

and you could implement Clone() as "return Union(this)".

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:162: virtual bool
Equal(const ManifestPermission* rhs) const OVERRIDE {
(writing this comment before below)

and you could write this as Value::Equals(ToValue(), rhs->ToValue())

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:183: virtual bool
Read(const IPC::Message* m, PickleIterator* iter) OVERRIDE {
It makes a lot of sense to implement Read/Write in terms of To/FromValue just in
general. Could you do that? Make this the default ManifestPermission Read/Write
implementation? Smaller code size (once we update the other ManifestPermission
subclasses to use it), less code added here...

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:195: virtual void
Log(std::string* log) const OVERRIDE {
Likewise.

and it may make sense to implement Log using JSONWriter rather than IPC, since
it's a bit weird that you'd get a list for logging, and who knows what IPC
logging actually ends up doing.

(and note that I can't find ManifestPermission::Log actually being called
anywhere, but I suppose codesearch might just be behaving funny).

[not at google - send to devlin, 2014-07-18 16:08:15]

Also +rpaquay as an FYI, maybe he has ideas too.

[Devlin, 2014-07-18 22:38:51]

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:53: virtual
PermissionMessages GetMessages() const OVERRIDE {
We should probably not have these all in-lined...

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:91: virtual
ManifestPermission* Clone() const OVERRIDE {
On 2014/07/18 16:07:22, kalman wrote:
> (comment made even later)
> 
> and you could implement Clone() as "return Union(this)".

Still gonna need Clone() for line 243, so I'm not sure I'd worry about it (since
Union() will also be slower).

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:111: return new
AutomationManifestPermission(info.Pass());
nit: inline |info| (here and below)

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:162: virtual bool
Equal(const ManifestPermission* rhs) const OVERRIDE {
On 2014/07/18 16:07:22, kalman wrote:
> (writing this comment before below)
> 
> and you could write this as Value::Equals(ToValue(), rhs->ToValue())

I don't think that's a great solution - it's a ton of extra work for what it
does.

If you're just worried about line count, what about
return *automation_info_ ==
       *(static_cast<const
AutomationManifestPermission*>(rhs))->automation_info_;

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:177: base::ListValue
singleton;
not sure "singleton" is the right name for this...

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:179:
singleton.Append(value);
nit: inline value.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:183: virtual bool
Read(const IPC::Message* m, PickleIterator* iter) OVERRIDE {
On 2014/07/18 16:07:22, kalman wrote:
> It makes a lot of sense to implement Read/Write in terms of To/FromValue just
in
> general. Could you do that? Make this the default ManifestPermission
Read/Write
> implementation? Smaller code size (once we update the other ManifestPermission
> subclasses to use it), less code added here...

+1

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:339: Automation*
automation = new Automation;
Why not make this a scoped_ptr from the start?  It's being returned as one
everywhere...

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.h (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.h:47: static
scoped_ptr<base::Value> ToValue(const AutomationInfo& info);
Could most or all of these be moved to the anonymous namespace in the .cc file?

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation_unittest.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation_unittest.cc:39:
ASSERT_EQ(0u, warnings.size());
nit: EXPECT_EQ(), and can inline:
EXPECT_EQ(
    0u, extension->permissions_data()->GetPermissionMessageStrings().size());

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation_unittest.cc:175:
ASSERT_EQ(0u, warnings.size());
nit: EXPECT_EQ and inline

https://codereview.chromium.org/377553003/diff/180001/chrome/test/data/extens...
File
chrome/test/data/extensions/manifest_tests/automation_matches_and_permissions.json
(right):

https://codereview.chromium.org/377553003/diff/180001/chrome/test/data/extens...
chrome/test/data/extensions/manifest_tests/automation_matches_and_permissions.json:6:
"http://www.google.com/"
nit: indentation off.

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
extensions/common/url_pattern_set.cc:248: std::unique(value->begin(),
value->end());
I don't think we need this, since |patterns_| is a set.

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
File extensions/common/url_pattern_set.h (right):

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
extensions/common/url_pattern_set.h:82: bool MatchesAllURLs() const;
nit: comment

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
extensions/common/url_pattern_set.h:97: scoped_ptr<std::vector<std::string> >
ToStringVector() const;
Put a new line between methods.  And it's probably best to not put a new method
between the two "Populate()" methods. ;)

[aboxhall, 2014-07-19 22:08:21]

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:53: virtual
PermissionMessages GetMessages() const OVERRIDE {
On 2014/07/18 22:38:51, Devlin wrote:
> We should probably not have these all in-lined...

This is the .cc file, so they're not inlined, unless I've misunderstood your
comment.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:91: virtual
ManifestPermission* Clone() const OVERRIDE {
On 2014/07/18 22:38:51, Devlin wrote:
> On 2014/07/18 16:07:22, kalman wrote:
> > (comment made even later)
> > 
> > and you could implement Clone() as "return Union(this)".
> 
> Still gonna need Clone() for line 243, so I'm not sure I'd worry about it
(since
> Union() will also be slower).

This is moot now, since Clone() is implemented in the superclass, but there's
really not that much in it performance-wise actually: an extra cast and some
boolean operations. For a very infrequently used method, I think that's totally
fine.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:111: return new
AutomationManifestPermission(info.Pass());
On 2014/07/18 22:38:51, Devlin wrote:
> nit: inline |info| (here and below)

Done.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:162: virtual bool
Equal(const ManifestPermission* rhs) const OVERRIDE {
On 2014/07/18 22:38:50, Devlin wrote:
> On 2014/07/18 16:07:22, kalman wrote:
> > (writing this comment before below)
> > 
> > and you could write this as Value::Equals(ToValue(), rhs->ToValue())
> 
> I don't think that's a great solution - it's a ton of extra work for what it
> does.
> 
> If you're just worried about line count, what about
> return *automation_info_ ==
>        *(static_cast<const
> AutomationManifestPermission*>(rhs))->automation_info_;

Moot: this is now in the superclass.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:177: base::ListValue
singleton;
On 2014/07/18 22:38:50, Devlin wrote:
> not sure "singleton" is the right name for this...

Moot, now in superclass.

FWIW, this was intended to echo things like
http://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#singleton...,
rather than the GoF Singleton pattern.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:179:
singleton.Append(value);
On 2014/07/18 22:38:50, Devlin wrote:
> nit: inline value.

Moot :)

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:183: virtual bool
Read(const IPC::Message* m, PickleIterator* iter) OVERRIDE {
On 2014/07/18 22:38:51, Devlin wrote:
> On 2014/07/18 16:07:22, kalman wrote:
> > It makes a lot of sense to implement Read/Write in terms of To/FromValue
just
> in
> > general. Could you do that? Make this the default ManifestPermission
> Read/Write
> > implementation? Smaller code size (once we update the other
ManifestPermission
> > subclasses to use it), less code added here...
> 
> +1

Done.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:195: virtual void
Log(std::string* log) const OVERRIDE {
On 2014/07/18 16:07:22, kalman wrote:
> Likewise.
> 
> and it may make sense to implement Log using JSONWriter rather than IPC, since
> it's a bit weird that you'd get a list for logging, and who knows what IPC
> logging actually ends up doing.
> 
> (and note that I can't find ManifestPermission::Log actually being called
> anywhere, but I suppose codesearch might just be behaving funny).

Done.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:339: Automation*
automation = new Automation;
On 2014/07/18 22:38:50, Devlin wrote:
> Why not make this a scoped_ptr from the start?  It's being returned as one
> everywhere...

Good point, done.

[aboxhall, 2014-07-21 16:32:57]

[bump - just bubbling this up in your inbox]

[aboxhall, 2014-07-21 19:27:27]

On 2014/07/21 16:32:57, aboxhall wrote:
> [bump - just bubbling this up in your inbox]

@rdevlin.cronin ping?

[Devlin, 2014-07-21 19:45:36]

Sorry for the delay.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:53: virtual
PermissionMessages GetMessages() const OVERRIDE {
On 2014/07/19 22:08:21, aboxhall wrote:
> On 2014/07/18 22:38:51, Devlin wrote:
> > We should probably not have these all in-lined...
> 
> This is the .cc file, so they're not inlined, unless I've misunderstood your
> comment.

Inlining has nothing to do with .h vs .cc file - it's when the method is
declared and defined in the same piece of code.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation_unittest.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation_unittest.cc:39:
ASSERT_EQ(0u, warnings.size());
On 2014/07/18 22:38:51, Devlin wrote:
> nit: EXPECT_EQ(), and can inline:
> EXPECT_EQ(
>     0u, extension->permissions_data()->GetPermissionMessageStrings().size());

Didn't like these ones? ;)

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:172: scoped_ptr<const
AutomationInfo> info(new AutomationInfo);
inline |info|

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:191:
scoped_ptr<AutomationInfo> AutomationInfo::FromValue(const base::Value& value) {
Is this used?

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:270: return
AsManifestType(info)->ToValue().Pass();
Are you anticipating using AsManifestType more later, or can we just combine it
and ToValue()?

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:293: scoped_ptr<const
AutomationInfo> AutomationInfo::Clone(
If you don't plan on using Clone() elsewhere, can we just inline this at line
180?

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:297: return
make_scoped_ptr(result);
inline?

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.h (right):

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.h:78: virtual bool
Parse(Extension* extensions, base::string16* error) OVERRIDE;
nit: If we don't need these exposed, let's go ahead and move them all into
private. Also, please add a "ManifestHandler implementation." comment.

[aboxhall, 2014-07-21 22:44:11]

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:53: virtual
PermissionMessages GetMessages() const OVERRIDE {
On 2014/07/21 19:45:35, Devlin wrote:
> On 2014/07/19 22:08:21, aboxhall wrote:
> > On 2014/07/18 22:38:51, Devlin wrote:
> > > We should probably not have these all in-lined...
> > 
> > This is the .cc file, so they're not inlined, unless I've misunderstood your
> > comment.
> 
> Inlining has nothing to do with .h vs .cc file - it's when the method is
> declared and defined in the same piece of code.

I see. But given these are virtual methods and never called directly on this
type, what's the issue with defining them here?

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation_unittest.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation_unittest.cc:39:
ASSERT_EQ(0u, warnings.size());
On 2014/07/21 19:45:35, Devlin wrote:
> On 2014/07/18 22:38:51, Devlin wrote:
> > nit: EXPECT_EQ(), and can inline:
> > EXPECT_EQ(
> >     0u,
extension->permissions_data()->GetPermissionMessageStrings().size());
> 
> Didn't like these ones? ;)

Don't know what happened there!

Made it an EXPECT, but left warnings as a variable for consistency (and thus
ease of reading) with other tests.

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation_unittest.cc:175:
ASSERT_EQ(0u, warnings.size());
On 2014/07/18 22:38:51, Devlin wrote:
> nit: EXPECT_EQ and inline

Partially done, as above.

https://codereview.chromium.org/377553003/diff/180001/chrome/test/data/extens...
File
chrome/test/data/extensions/manifest_tests/automation_matches_and_permissions.json
(right):

https://codereview.chromium.org/377553003/diff/180001/chrome/test/data/extens...
chrome/test/data/extensions/manifest_tests/automation_matches_and_permissions.json:6:
"http://www.google.com/"
On 2014/07/18 22:38:51, Devlin wrote:
> nit: indentation off.

Done.

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
File extensions/common/url_pattern_set.cc (right):

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
extensions/common/url_pattern_set.cc:248: std::unique(value->begin(),
value->end());
On 2014/07/18 22:38:51, Devlin wrote:
> I don't think we need this, since |patterns_| is a set.

Makes sense, done.

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
File extensions/common/url_pattern_set.h (right):

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
extensions/common/url_pattern_set.h:82: bool MatchesAllURLs() const;
On 2014/07/18 22:38:51, Devlin wrote:
> nit: comment

Done.

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
extensions/common/url_pattern_set.h:97: scoped_ptr<std::vector<std::string> >
ToStringVector() const;
On 2014/07/18 22:38:51, Devlin wrote:
> Put a new line between methods.  And it's probably best to not put a new
method
> between the two "Populate()" methods. ;)

Actually I did it like this deliberately, to pair the two to/from methods with
their comments - see the pre-existing pair above.

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:172: scoped_ptr<const
AutomationInfo> info(new AutomationInfo);
On 2014/07/21 19:45:35, Devlin wrote:
> inline |info|

Done.

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:191:
scoped_ptr<AutomationInfo> AutomationInfo::FromValue(const base::Value& value) {
On 2014/07/21 19:45:35, Devlin wrote:
> Is this used?

Not any more! Removed.

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:270: return
AsManifestType(info)->ToValue().Pass();
On 2014/07/21 19:45:35, Devlin wrote:
> Are you anticipating using AsManifestType more later, or can we just combine
it
> and ToValue()?

I'm unlikely to use it again, but I think it's more readable to pull it out into
a separate method and make it clear that ToValue is just delegating to the
ToValue method on the manifest type. Since this method is going to be called
O(1) times in each browser session, I think we can live with the very tiny
performance hit?

I made it private in the header file, though, since there's no point exposing
it.

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:293: scoped_ptr<const
AutomationInfo> AutomationInfo::Clone(
On 2014/07/21 19:45:35, Devlin wrote:
> If you don't plan on using Clone() elsewhere, can we just inline this at line
> 180?

Done.

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:297: return
make_scoped_ptr(result);
On 2014/07/21 19:45:35, Devlin wrote:
> inline?

Acknowledged.

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.h (right):

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.h:78: virtual bool
Parse(Extension* extensions, base::string16* error) OVERRIDE;
On 2014/07/21 19:45:36, Devlin wrote:
> nit: If we don't need these exposed, let's go ahead and move them all into
> private. Also, please add a "ManifestHandler implementation." comment.

Done.

[Devlin, 2014-07-21 23:43:53]

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:53: virtual
PermissionMessages GetMessages() const OVERRIDE {
On 2014/07/21 22:44:10, aboxhall wrote:
> On 2014/07/21 19:45:35, Devlin wrote:
> > On 2014/07/19 22:08:21, aboxhall wrote:
> > > On 2014/07/18 22:38:51, Devlin wrote:
> > > > We should probably not have these all in-lined...
> > > 
> > > This is the .cc file, so they're not inlined, unless I've misunderstood
your
> > > comment.
> > 
> > Inlining has nothing to do with .h vs .cc file - it's when the method is
> > declared and defined in the same piece of code.
> 
> I see. But given these are virtual methods and never called directly on this
> type, what's the issue with defining them here?

More than anything else... style.  In theory, inlining methods actually doesn't
matter much, because the compiler should be smart enough to say "no".  But we
want to be consistent, and we (shouldn't) be inlining large methods like this
(though, obviously, we aren't always consistent - particularly with testing).

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
File extensions/common/url_pattern_set.h (right):

https://codereview.chromium.org/377553003/diff/180001/extensions/common/url_p...
extensions/common/url_pattern_set.h:97: scoped_ptr<std::vector<std::string> >
ToStringVector() const;
On 2014/07/21 22:44:10, aboxhall wrote:
> On 2014/07/18 22:38:51, Devlin wrote:
> > Put a new line between methods.  And it's probably best to not put a new
> method
> > between the two "Populate()" methods. ;)
> 
> Actually I did it like this deliberately, to pair the two to/from methods with
> their comments - see the pre-existing pair above.

Ah, I see.  Okay, that makes sense, I guess.

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/220001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:270: return
AsManifestType(info)->ToValue().Pass();
On 2014/07/21 22:44:11, aboxhall wrote:
> On 2014/07/21 19:45:35, Devlin wrote:
> > Are you anticipating using AsManifestType more later, or can we just combine
> it
> > and ToValue()?
> 
> I'm unlikely to use it again, but I think it's more readable to pull it out
into
> a separate method and make it clear that ToValue is just delegating to the
> ToValue method on the manifest type. Since this method is going to be called
> O(1) times in each browser session, I think we can live with the very tiny
> performance hit?
> 
> I made it private in the header file, though, since there's no point exposing
> it.

Not worried about performance; just like to keep the header clean.  But private
is good enough for me.

https://codereview.chromium.org/377553003/diff/240001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.h (right):

https://codereview.chromium.org/377553003/diff/240001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.h:78: // ManifestHandler
implementation
nit: either a ':' or a '.' at the end.

https://codereview.chromium.org/377553003/diff/240001/extensions/common/url_p...
File extensions/common/url_pattern_set.h (right):

https://codereview.chromium.org/377553003/diff/240001/extensions/common/url_p...
extensions/common/url_pattern_set.h:82: // Test if the extent matches all URLs
(for example, <all_urls>)
nit: .

[aboxhall, 2014-07-22 17:00:09]

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:53: virtual
PermissionMessages GetMessages() const OVERRIDE {
On 2014/07/21 23:43:52, Devlin wrote:
> On 2014/07/21 22:44:10, aboxhall wrote:
> > On 2014/07/21 19:45:35, Devlin wrote:
> > > On 2014/07/19 22:08:21, aboxhall wrote:
> > > > On 2014/07/18 22:38:51, Devlin wrote:
> > > > > We should probably not have these all in-lined...
> > > > 
> > > > This is the .cc file, so they're not inlined, unless I've misunderstood
> your
> > > > comment.
> > > 
> > > Inlining has nothing to do with .h vs .cc file - it's when the method is
> > > declared and defined in the same piece of code.
> > 
> > I see. But given these are virtual methods and never called directly on this
> > type, what's the issue with defining them here?
> 
> More than anything else... style.  In theory, inlining methods actually
doesn't
> matter much, because the compiler should be smart enough to say "no".  But we
> want to be consistent, and we (shouldn't) be inlining large methods like this
> (though, obviously, we aren't always consistent - particularly with testing).

Done, although please note that this style is used very frequently throughout
the codebase, not just in tests.

https://codereview.chromium.org/377553003/diff/240001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.h (right):

https://codereview.chromium.org/377553003/diff/240001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.h:78: // ManifestHandler
implementation
On 2014/07/21 23:43:53, Devlin wrote:
> nit: either a ':' or a '.' at the end.

Done.

https://codereview.chromium.org/377553003/diff/240001/extensions/common/url_p...
File extensions/common/url_pattern_set.h (right):

https://codereview.chromium.org/377553003/diff/240001/extensions/common/url_p...
extensions/common/url_pattern_set.h:82: // Test if the extent matches all URLs
(for example, <all_urls>)
On 2014/07/21 23:43:53, Devlin wrote:
> nit: .

Done.

[Devlin, 2014-07-22 17:36:02]

LGTM

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/180001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:53: virtual
PermissionMessages GetMessages() const OVERRIDE {
On 2014/07/22 17:00:09, aboxhall wrote:
> On 2014/07/21 23:43:52, Devlin wrote:
> > On 2014/07/21 22:44:10, aboxhall wrote:
> > > On 2014/07/21 19:45:35, Devlin wrote:
> > > > On 2014/07/19 22:08:21, aboxhall wrote:
> > > > > On 2014/07/18 22:38:51, Devlin wrote:
> > > > > > We should probably not have these all in-lined...
> > > > > 
> > > > > This is the .cc file, so they're not inlined, unless I've
misunderstood
> > your
> > > > > comment.
> > > > 
> > > > Inlining has nothing to do with .h vs .cc file - it's when the method is
> > > > declared and defined in the same piece of code.
> > > 
> > > I see. But given these are virtual methods and never called directly on
this
> > > type, what's the issue with defining them here?
> > 
> > More than anything else... style.  In theory, inlining methods actually
> doesn't
> > matter much, because the compiler should be smart enough to say "no".  But
we
> > want to be consistent, and we (shouldn't) be inlining large methods like
this
> > (though, obviously, we aren't always consistent - particularly with
testing).
> 
> Done, although please note that this style is used very frequently throughout
> the codebase, not just in tests.

Thanks for humoring me. :)

https://codereview.chromium.org/377553003/diff/260001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/260001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:48: 
nit: we can take out these newlines between all the overriden methods.

https://codereview.chromium.org/377553003/diff/260001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:231:
Automation::Object& automation_object = *automation->as_object;
nit: Can't this still be const?

[aboxhall, 2014-07-22 17:47:52]

https://codereview.chromium.org/377553003/diff/260001/chrome/common/extension...
File chrome/common/extensions/manifest_handlers/automation.cc (right):

https://codereview.chromium.org/377553003/diff/260001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/automation.cc:231:
Automation::Object& automation_object = *automation->as_object;
On 2014/07/22 17:36:02, Devlin wrote:
> nit: Can't this still be const?

Literally no idea what happened there. Fixed.

[aboxhall, 2014-07-22 17:47:57]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-07-22 17:49:39]

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

[commit-bot: I haz the power, 2014-07-22 20:16:08]

Change committed as 284761

[Justin Donnelly, 2014-07-23 00:39:44]

A revert of this CL has been created in
https://codereview.chromium.org/408143011/ by jdonnelly@chromium.org.

The reason for reverting is: AutomationApiTest.GetTreeByTabId was disabled
because it consistently fails on chromeos (see http://crbug.com/379927). 
Reverting this change since it re-enables that test..

[aboxhall, 2014-07-23 01:59:38]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2014-07-23 02:01:44]

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

[commit-bot: I haz the power, 2014-07-23 06:15:19]

Change committed as 284855