Add gallery permissions to Media Galleries Preferences

Add gallery permissions to Media Galleries Preferences

BUG=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150474

[Evan Stade, 2012-08-02 17:47:31]

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:82: bool
LookupGalleryByPath(const FilePath& path,
I thought we said that this would never fail, rather it fills in the
MediaGalleryPrefInfo as if for a new gallery.

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:88: const FilePath&
path,
why take 3 of the members of a MediaGalleryPrefInfo struct, rather than the
struct itself?

[vandebo (ex-Chrome), 2012-08-02 17:51:38]

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:82: bool
LookupGalleryByPath(const FilePath& path,
On 2012/08/02 17:47:31, Evan Stade wrote:
> I thought we said that this would never fail, rather it fills in the
> MediaGalleryPrefInfo as if for a new gallery.

Oops... right, I'll fix that.

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:88: const FilePath&
path,
On 2012/08/02 17:47:31, Evan Stade wrote:
> why take 3 of the members of a MediaGalleryPrefInfo struct, rather than the
> struct itself?

Because not all the fields can be valid and because some callers may not already
have the info in a struct, so they'd have to put it into one to call this
method, whereas if you have it it one, you just reference the fields at the call
site.

[Evan Stade, 2012-08-02 18:00:54]

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:301: if
(gallery_info->second.type == MediaGalleryPrefInfo::kAutoDetected)
seems like in this case rather than returning you should clear the permission in
the ExtensionPrefs. Otherwise a blacklisted gallery can't become unblacklisted.

Also, I don't know if it makes a difference, but in the case of

!has_permission, !has all galleries

the ExtensionsPref setting should be cleared as well, rather than set to false.

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:88: const FilePath&
path,
On 2012/08/02 17:51:38, vandebo wrote:
> On 2012/08/02 17:47:31, Evan Stade wrote:
> > why take 3 of the members of a MediaGalleryPrefInfo struct, rather than the
> > struct itself?
> 
> Because not all the fields can be valid and because some callers may not
already
> have the info in a struct, so they'd have to put it into one to call this
> method, whereas if you have it it one, you just reference the fields at the
call
> site.

are these "some callers" real or hypothetical?

[vandebo (ex-Chrome), 2012-08-02 18:26:02]

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:301: if
(gallery_info->second.type == MediaGalleryPrefInfo::kAutoDetected)
On 2012/08/02 18:00:54, Evan Stade wrote:
> seems like in this case rather than returning you should clear the permission
in
> the ExtensionPrefs. Otherwise a blacklisted gallery can't become
unblacklisted.
> 
> Also, I don't know if it makes a difference, but in the case of
> 
> !has_permission, !has all galleries
> 
> the ExtensionsPref setting should be cleared as well, rather than set to
false.

That all sounds right, will do that

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:82: bool
LookupGalleryByPath(const FilePath& path,
On 2012/08/02 17:51:38, vandebo wrote:
> On 2012/08/02 17:47:31, Evan Stade wrote:
> > I thought we said that this would never fail, rather it fills in the
> > MediaGalleryPrefInfo as if for a new gallery.
> 
> Oops... right, I'll fix that.

Done.

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:88: const FilePath&
path,
On 2012/08/02 18:00:54, Evan Stade wrote:
> On 2012/08/02 17:51:38, vandebo wrote:
> > On 2012/08/02 17:47:31, Evan Stade wrote:
> > > why take 3 of the members of a MediaGalleryPrefInfo struct, rather than
the
> > > struct itself?
> > 
> > Because not all the fields can be valid and because some callers may not
> already
> > have the info in a struct, so they'd have to put it into one to call this
> > method, whereas if you have it it one, you just reference the fields at the
> call
> > site.
> 
> are these "some callers" real or hypothetical?

Doesn't matter, my point is that I think it's bad form to pass a partially
filled struct and doing it this way doesn't hurt the case where you already have
it in a struct.

[Evan Stade, 2012-08-02 18:36:35]

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.h:88: const FilePath&
path,
On 2012/08/02 18:26:02, vandebo wrote:
> On 2012/08/02 18:00:54, Evan Stade wrote:
> > On 2012/08/02 17:51:38, vandebo wrote:
> > > On 2012/08/02 17:47:31, Evan Stade wrote:
> > > > why take 3 of the members of a MediaGalleryPrefInfo struct, rather than
> the
> > > > struct itself?
> > > 
> > > Because not all the fields can be valid and because some callers may not
> > already
> > > have the info in a struct, so they'd have to put it into one to call this
> > > method, whereas if you have it it one, you just reference the fields at
the
> > call
> > > site.
> > 
> > are these "some callers" real or hypothetical?
> 
> Doesn't matter, my point is that I think it's bad form to pass a partially
> filled struct and doing it this way doesn't hurt the case where you already
have
> it in a struct.

Technically, you could get rid of the AddMediaGalleryByPath function, but it's
preferable to leave it just for ease of use (as you have done). I see this as
the same thing.

http://codereview.chromium.org/10821077/diff/2003/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/2003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:221:
gallery_info.display_name = display_name;
should you Compute if empty?

[vandebo (ex-Chrome), 2012-08-02 18:56:00]

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/6008/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:301: if
(gallery_info->second.type == MediaGalleryPrefInfo::kAutoDetected)
On 2012/08/02 18:26:02, vandebo wrote:
> On 2012/08/02 18:00:54, Evan Stade wrote:
> > seems like in this case rather than returning you should clear the
permission
> in
> > the ExtensionPrefs. Otherwise a blacklisted gallery can't become
> unblacklisted.
> > 
> > Also, I don't know if it makes a difference, but in the case of
> > 
> > !has_permission, !has all galleries
> > 
> > the ExtensionsPref setting should be cleared as well, rather than set to
> false.
> 
> That all sounds right, will do that

Done.

http://codereview.chromium.org/10821077/diff/2003/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/2003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:221:
gallery_info.display_name = display_name;
On 2012/08/02 18:36:35, Evan Stade wrote:
> should you Compute if empty?

It's a bug if the caller passes an empty display name.  I'd rather have that
apparent than possibly subtly papered over.

[Evan Stade, 2012-08-02 19:04:18]

http://codereview.chromium.org/10821077/diff/2003/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/2003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:150:
prefs::kMediaGalleriesRememberedGalleries);
just discovered via testing that |list| can be NULL and it will crash when
deref'ed.

[vandebo (ex-Chrome), 2012-08-02 19:16:59]

Also added a default gallery

https://chromiumcodereview.appspot.com/10821077/diff/2003/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

https://chromiumcodereview.appspot.com/10821077/diff/2003/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:150:
prefs::kMediaGalleriesRememberedGalleries);
On 2012/08/02 19:04:18, Evan Stade wrote:
> just discovered via testing that |list| can be NULL and it will crash when
> deref'ed.

Done.

[Evan Stade, 2012-08-02 21:07:16]

https://chromiumcodereview.appspot.com/10821077/diff/2003/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

https://chromiumcodereview.appspot.com/10821077/diff/2003/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:221:
gallery_info.display_name = display_name;
On 2012/08/02 18:56:00, vandebo wrote:
> On 2012/08/02 18:36:35, Evan Stade wrote:
> > should you Compute if empty?
> 
> It's a bug if the caller passes an empty display name.  I'd rather have that
> apparent than possibly subtly papered over.

if that's a bug you should DCHECK it.

[Evan Stade, 2012-08-02 22:07:57]

http://codereview.chromium.org/10821077/diff/7010/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/7010/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:50: if
(!dict->GetString(kMediaGalleriesPrefIdKey, &string_type))
believe this should be TypeKey

http://codereview.chromium.org/10821077/diff/7010/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:238:
gallery_info.display_name = display_name;
what about device_id?

[Elliot Glaysher, 2012-08-02 23:23:51]

http://codereview.chromium.org/10821077/diff/7010/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

http://codereview.chromium.org/10821077/diff/7010/chrome/browser/media_galler...
chrome/browser/media_gallery/media_file_system_registry.cc:67: //Profile*
profile = Profile::FromBrowserContext(rph->GetBrowserContext());
stray line?

[vandebo (ex-Chrome), 2012-08-03 18:43:10]

Unit tests are disabled for now, still finishing them up.

https://chromiumcodereview.appspot.com/10821077/diff/2003/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

https://chromiumcodereview.appspot.com/10821077/diff/2003/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:221:
gallery_info.display_name = display_name;
On 2012/08/02 21:07:16, Evan Stade wrote:
> On 2012/08/02 18:56:00, vandebo wrote:
> > On 2012/08/02 18:36:35, Evan Stade wrote:
> > > should you Compute if empty?
> > 
> > It's a bug if the caller passes an empty display name.  I'd rather have that
> > apparent than possibly subtly papered over.
> 
> if that's a bug you should DCHECK it.

Done.

[Evan Stade, 2012-08-03 18:53:43]

just nits

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:53: if
(string_type.compare(kMediaGalleriesTypeAutoDetectedValue) == 0) {
string_type == kMediaGalleriesTypeAutoDetectedValue

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:56: } else if
(string_type.compare(kMediaGalleriesTypeUserAddedValue) == 0) {
no need for else after a return.

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:323: if
(gallery->second.type != MediaGalleryPrefInfo::kBlackListed) {
no curlies

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:326: NOT_REACHED();
no underscore (right?)

[vandebo (ex-Chrome), 2012-08-03 19:03:07]

+asargent for extension changes
+thestig for chrome changes

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:53: if
(string_type.compare(kMediaGalleriesTypeAutoDetectedValue) == 0) {
On 2012/08/03 18:53:43, Evan Stade wrote:
> string_type == kMediaGalleriesTypeAutoDetectedValue

Done.

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:56: } else if
(string_type.compare(kMediaGalleriesTypeUserAddedValue) == 0) {
On 2012/08/03 18:53:43, Evan Stade wrote:
> no need for else after a return.

Done.

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:323: if
(gallery->second.type != MediaGalleryPrefInfo::kBlackListed) {
On 2012/08/03 18:53:43, Evan Stade wrote:
> no curlies

I prefer curlies if there is an else.  I omit them if it's exactly two lines,
one for the conditional and one for the body.

https://chromiumcodereview.appspot.com/10821077/diff/4005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:326: NOT_REACHED();
On 2012/08/03 18:53:43, Evan Stade wrote:
> no underscore (right?)

Done.

[Lei Zhang, 2012-08-03 19:22:04]

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.h:65: std::string
GetDeviceIdFromPath(const FilePath& path);
nit: const and return const std::string& ?

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:70: const
DictionaryValue* dict, MediaGalleryPrefInfo* out_gallery_info) {
use a const-ref for dict?

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:102:
dict->SetString(kMediaGalleriesTypeKey,
why not just set a |value| variable here, and call SetString() after the
switch() ?

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:183: for
(ListValue::const_iterator it = list->begin(); it != list->end(); it++) {
it++ -> ++it

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:232: for
(ListValue::iterator it = list->begin(); it != list->end(); ++it) {
const_iterator

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.h:96:
MediaGalleryPrefId AddGalleryByPath(const FilePath& path);
nit: add TODO(username) Remove ?

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences_unittest.cc:101:
printf("All: %zu\n", galleries_for_all.size());
debug code?

[asargent_no_longer_on_chrome, 2012-08-03 20:10:16]

chrome/browser/extensions/* LGTM

[vandebo (ex-Chrome), 2012-08-03 20:27:27]

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.h:65: std::string
GetDeviceIdFromPath(const FilePath& path);
On 2012/08/03 19:22:04, Lei Zhang wrote:
> nit: const and return const std::string& ?

const Done.  method may create the return value though.

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:70: const
DictionaryValue* dict, MediaGalleryPrefInfo* out_gallery_info) {
On 2012/08/03 19:22:04, Lei Zhang wrote:
> use a const-ref for dict?

Done.

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:102:
dict->SetString(kMediaGalleriesTypeKey,
On 2012/08/03 19:22:04, Lei Zhang wrote:
> why not just set a |value| variable here, and call SetString() after the
> switch() ?

Ehh, doesn't seem any cleaner, but done.

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:183: for
(ListValue::const_iterator it = list->begin(); it != list->end(); it++) {
On 2012/08/03 19:22:04, Lei Zhang wrote:
> it++ -> ++it

Done.

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.cc:232: for
(ListValue::iterator it = list->begin(); it != list->end(); ++it) {
On 2012/08/03 19:22:04, Lei Zhang wrote:
> const_iterator

Done.

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences.h (right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences.h:96:
MediaGalleryPrefId AddGalleryByPath(const FilePath& path);
On 2012/08/03 19:22:04, Lei Zhang wrote:
> nit: add TODO(username) Remove ?

Done.

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
File chrome/browser/media_gallery/media_galleries_preferences_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10821077/diff/5005/chrome/browser/medi...
chrome/browser/media_gallery/media_galleries_preferences_unittest.cc:101:
printf("All: %zu\n", galleries_for_all.size());
On 2012/08/03 19:22:04, Lei Zhang wrote:
> debug code?

Yea, tests are still coming along.  Removed.

[Evan Stade, 2012-08-06 23:40:38]

http://codereview.chromium.org/10821077/diff/5005/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/5005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:70: const
DictionaryValue* dict, MediaGalleryPrefInfo* out_gallery_info) {
On 2012/08/03 20:27:27, vandebo wrote:
> On 2012/08/03 19:22:04, Lei Zhang wrote:
> > use a const-ref for dict?
> 
> Done.

how is that an improvement? const ref tends to imply you can make a copy if you
want.

[Lei Zhang, 2012-08-07 00:04:53]

http://codereview.chromium.org/10821077/diff/5005/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_galleries_preferences.cc (right):

http://codereview.chromium.org/10821077/diff/5005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_galleries_preferences.cc:70: const
DictionaryValue* dict, MediaGalleryPrefInfo* out_gallery_info) {
On 2012/08/06 23:40:38, Evan Stade wrote:
> On 2012/08/03 20:27:27, vandebo wrote:
> > On 2012/08/03 19:22:04, Lei Zhang wrote:
> > > use a const-ref for dict?
> > 
> > Done.
> 
> how is that an improvement? const ref tends to imply you can make a copy if
you
> want.

The c++ style guide says "In fact it is a very strong convention in Google code
that input arguments are values or const references while output arguments are
pointers." I like it because the callee has 1 less potential NULL pointer to
defend against.

[Evan Stade, 2012-08-07 01:18:48]

On 2012/08/07 00:04:53, Lei Zhang wrote:
>
http://codereview.chromium.org/10821077/diff/5005/chrome/browser/media_galler...
> File chrome/browser/media_gallery/media_galleries_preferences.cc (right):
> 
>
http://codereview.chromium.org/10821077/diff/5005/chrome/browser/media_galler...
> chrome/browser/media_gallery/media_galleries_preferences.cc:70: const
> DictionaryValue* dict, MediaGalleryPrefInfo* out_gallery_info) {
> On 2012/08/06 23:40:38, Evan Stade wrote:
> > On 2012/08/03 20:27:27, vandebo wrote:
> > > On 2012/08/03 19:22:04, Lei Zhang wrote:
> > > > use a const-ref for dict?
> > > 
> > > Done.
> > 
> > how is that an improvement? const ref tends to imply you can make a copy if
> you
> > want.
> 
> The c++ style guide says "In fact it is a very strong convention in Google
code
> that input arguments are values or const references while output arguments are
> pointers." I like it because the callee has 1 less potential NULL pointer to
> defend against.

Right after that quote it also says "Input parameters may be const pointers" and
gives an entire paragraph about permitting the use of const*. The convention in
chrome does not align with what that style guide claims, but there do seem to be
a few places where we use const DictionaryValue&, so I guess I don't care too
much.

[Evan Stade, 2012-08-07 01:27:48]

also forgot to LGTM

[asargent_no_longer_on_chrome, 2012-08-07 16:02:52]

>
> Right after that quote it also says "Input parameters may be const
> pointers" and
> gives an entire paragraph about permitting the use of const*. The
> convention in
> chrome does not align with what that style guide claims, but there do seem
> to be
> a few places where we use const DictionaryValue&, so I guess I don't care
> too
> much.


 FWIW, in the extensions code I'm pretty sure we follow the convention
mentioned in the style guide that const pointer is only used when it would
make sense for that input parameter to possibly be null (and the function
will check for this and handle it appropriately); otherwise we default to
const reference.

[Lei Zhang, 2012-08-07 23:13:43]

lgtm

[commit-bot: I haz the power, 2012-08-07 23:36:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/10821077/11018

[commit-bot: I haz the power, 2012-08-08 00:55:37]

Change committed as 150474