Extension BaseFeatureProvider minor cleanups. Don't crash in release mode if we failed to parse a p…

Extension BaseFeatureProvider minor cleanups. Don't crash in release mode if we failed to parse a permissions file.

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

[justinlin, 2013-02-15 00:26:33]

ptal, and lmk what you think? i don't feel strongly about this approach, but
it's one possible way to address these crashes.

i suspect it's the JSONReader that's just dying/failing randomly sometimes
that's the root cause.

[not at google - send to devlin, 2013-02-15 16:33:34]

There are just so many similar places we could DCHECK rather than CHECK, I don't
want to add that unless we really need to - let's see how that bug turns out.

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.cc:626: // don't want to crash the whole
browser in release mode.
Link to bug instead of this.

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/base_feature_provider.cc (right):

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:53: if (!value) {
link to bug

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:55: << error_message;
If we're going to clean up this stuff, something like

scoped_ptr<Value> value(base::JSONReader::ReadAndReturnError(...))
DCHECK(value) << ...;
scoped_ptr<DictionaryValue> value_as_dict;
if (value) {
  CHECK(value->IsType(...));
  value_as_dict = value.PassAs<DictionaryValue>();
} else {
  value_as_dict.reset(new DictionaryValue());
}
return make_scoped_ptr(new BaseFeatureProvider(*dictionary_value, factory));

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:58:
dictionary_value.reset(static_cast<DictionaryValue*>(value));
(This means that the DictionaryValue created on line 52 is unnecessary)

[justinlin, 2013-02-21 09:18:23]

thanks for the review. addressed the comments.

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.cc:626: // don't want to crash the whole
browser in release mode.
On 2013/02/15 16:33:34, kalman wrote:
> Link to bug instead of this.

Done.

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/base_feature_provider.cc (right):

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:53: if (!value) {
On 2013/02/15 16:33:34, kalman wrote:
> link to bug

Done.

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:55: << error_message;
On 2013/02/15 16:33:34, kalman wrote:
> If we're going to clean up this stuff, something like
> 
> scoped_ptr<Value> value(base::JSONReader::ReadAndReturnError(...))
> DCHECK(value) << ...;
> scoped_ptr<DictionaryValue> value_as_dict;
> if (value) {
>   CHECK(value->IsType(...));
>   value_as_dict = value.PassAs<DictionaryValue>();
> } else {
>   value_as_dict.reset(new DictionaryValue());
> }
> return make_scoped_ptr(new BaseFeatureProvider(*dictionary_value, factory));

Done, except I can't seem to use PassAs<DictionaryValue>() for this particular
downcast. I don't see any doc in scoped_ptr about PassAs supporting downcasts..

../../base/memory/scoped_ptr.h:202:9: error: no matching constructor for
initialization of 'base::internal::scoped_ptr_impl<base::DictionaryValue,
base::DefaultDeleter<base::DictionaryValue> >
::Data'
      : data_(other->release(), other->get_deleter()) {
        ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/memory/scoped_ptr.h:339:40: note: in instantiation of function
template specialization 'base::internal::scoped_ptr_impl<base::DictionaryValue,
base::DefaultDeleter<base::DictionaryV
alue> >::scoped_ptr_impl<base::Value, base::DefaultDeleter<base::Value> >'
requested here
  scoped_ptr(scoped_ptr<U, V> other) : impl_(&other.impl_) {
                                       ^

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:58:
dictionary_value.reset(static_cast<DictionaryValue*>(value));
On 2013/02/15 16:33:34, kalman wrote:
> (This means that the DictionaryValue created on line 52 is unnecessary)

True. It was mostly for safety in case another "if" clause gets added and
doesn't initialize the pointer, but your way looks better.

[not at google - send to devlin, 2013-02-21 16:20:09]

lgtm

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/base_feature_provider.cc (right):

https://codereview.chromium.org/12255041/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:55: << error_message;
On 2013/02/21 09:18:23, justinlin wrote:
> On 2013/02/15 16:33:34, kalman wrote:
> > If we're going to clean up this stuff, something like
> > 
> > scoped_ptr<Value> value(base::JSONReader::ReadAndReturnError(...))
> > DCHECK(value) << ...;
> > scoped_ptr<DictionaryValue> value_as_dict;
> > if (value) {
> >   CHECK(value->IsType(...));
> >   value_as_dict = value.PassAs<DictionaryValue>();
> > } else {
> >   value_as_dict.reset(new DictionaryValue());
> > }
> > return make_scoped_ptr(new BaseFeatureProvider(*dictionary_value, factory));
> 
> Done, except I can't seem to use PassAs<DictionaryValue>() for this particular
> downcast. I don't see any doc in scoped_ptr about PassAs supporting
downcasts..
> 
> ../../base/memory/scoped_ptr.h:202:9: error: no matching constructor for
> initialization of 'base::internal::scoped_ptr_impl<base::DictionaryValue,
> base::DefaultDeleter<base::DictionaryValue> >
> ::Data'
>       : data_(other->release(), other->get_deleter()) {
>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../base/memory/scoped_ptr.h:339:40: note: in instantiation of function
> template specialization
'base::internal::scoped_ptr_impl<base::DictionaryValue,
> base::DefaultDeleter<base::DictionaryV
> alue> >::scoped_ptr_impl<base::Value, base::DefaultDeleter<base::Value> >'
> requested here
>   scoped_ptr(scoped_ptr<U, V> other) : impl_(&other.impl_) {
>                                        ^

Bummer, sorry about that.

[commit-bot: I haz the power, 2013-02-21 18:40:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/12255041/6001

[commit-bot: I haz the power, 2013-02-21 22:10:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/12255041/6001

[commit-bot: I haz the power, 2013-02-21 23:54:21]

Change committed as 183906