Enforce the 'requirements' field in manifests.

Enforce the 'requirements' field in manifests.

Implemented for the "plugins" and "3d" options.

BUG=133027, 130405
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155861

[Aaron Boodman, 2012-07-16 21:16:12]

http://codereview.chromium.org/10689097/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10689097/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_service.cc:1936: if
(!requirements_provider_->Supports(*extension->manifest(), &error)) {
Didn't it look like some of the requirements needed to be checked
asynchronously?

http://codereview.chromium.org/10689097/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10689097/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:1681: if
(!manifest_->HasKey(keys::kRequirements)) {
No need for braces here.

http://codereview.chromium.org/10689097/diff/1/chrome/common/extensions/requi...
File chrome/common/extensions/requirements_provider.h (right):

http://codereview.chromium.org/10689097/diff/1/chrome/common/extensions/requi...
chrome/common/extensions/requirements_provider.h:14: class RequirementsProvider
{
I don't think this needs to be abstract for now. The implementation is all going
to be in c/b/e. Just create a concrete RequirementsProvider in c/b/e?

[eaugusti, 2012-07-20 00:37:30]

Any suggestions on testing it?

[eaugusti, 2012-07-20 00:39:43]

Also, the "plugins" requirements option is kind of weird. 

The "3d" states that it wants some features using the "features" key.
But plugins is a single item, either you have them or you don't.

Should there be some features for the "plugins" requirement, right now I just
ignore any values that plugins has.

[Mihai Parparita -not on Chrome, 2012-07-20 00:46:08]

On Thu, Jul 19, 2012 at 5:39 PM, <eaugusti@chromium.org> wrote:

> Also, the "plugins" requirements option is kind of weird.
>
> The "3d" states that it wants some features using the "features" key.
> But plugins is a single item, either you have them or you don't.
>
> Should there be some features for the "plugins" requirement, right now I
> just
> ignore any values that plugins has.
>

The "plugins" requirement actually looks like:

"plugins": {
  "npapi": true
}

See
https://cs.corp.google.com/codesearch#google3/javatests/com/google/chrome/web...
the test data on the webstore side.

Mihai

[Aaron Boodman, 2012-07-22 17:46:25]

Since this feature has turned out to be a lot more complicated than I thought, I
put together a quick design doc:

https://docs.google.com/a/google.com/document/d/10jKBlzas_0WmYuNeLTJMRPOKtYvD...

We can talk this out in person on Monday.

http://codereview.chromium.org/10689097/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10689097/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1942: if
(!requirements_provider_->Supports(extension, &errors, &async, this)) {
I think that this is going to result in a GPU process getting created at each
browser startup, which seems undesirable. Can you verify?

In any case, we're also going to need to check at install time so that we can
make installation throw an error, rather than installing successfully and then
going into disabled state.

http://codereview.chromium.org/10689097/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10689097/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:354: // Called to report an
extension that declares requirements that are not meet
s/meet/met/

http://codereview.chromium.org/10689097/diff/6001/chrome/browser/extensions/r...
File chrome/browser/extensions/requirements_provider.h (right):

http://codereview.chromium.org/10689097/diff/6001/chrome/browser/extensions/r...
chrome/browser/extensions/requirements_provider.h:22: class RequirementsProvider
{
Since this class isn't 'providing' things right now, how about changing the name
to RequirementsChecker.

http://codereview.chromium.org/10689097/diff/6001/chrome/browser/extensions/r...
chrome/browser/extensions/requirements_provider.h:31: bool Supports(const
Extension* extsnion, std::list<string16>* errors,
This is a kind of odd interface. How about making it always provide the result
asynchronously, for consistency.

[eaugusti, 2012-07-26 23:24:13]

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/crx_installer.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/crx_installer.cc:405: UTF8ToUTF16(JoinString(errors, '
'))));
Errors should probably be joined and formatted better.

[Aaron Boodman, 2012-07-30 12:15:11]

Was this meant for me to review. GPUFeatureChecker is not included in this CL.

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.cc (right):

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:55: base::Unretained(this)));
Unretained is almost always a bad idea. If this class goes away while the task
is outstanding, the callback will crash. If you use WeakPtr instead, it will
prevent the callback from being executed if this class goes away.

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.h (right):

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:8: #include <vecotr>
opps

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:33: content::BrowserThread::ID
callback_thread);
Is it possible to just assume that the callback thread is the thread Check() is
called upon?

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:40: void IsCSS3dAvailable(bool
available);
CSS3D would be more consistent with the other uses of capitalization here.

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:46: static bool
checked_for_webgl_;
Is this cache necessary? I thought when I looked at the underlying code that we
call to determine if these things are supported, that it does its own caching.

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:56: base::Lock
async_requirement_checks_lock_;
You should not need this lock. The asynchronous checks are going to call back on
the UI thread, which can only do one thing at a time. Right?

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:58:
scoped_refptr<GPUFeatureChecker> webgl_checker_;
These should not need to be refcounted either. The new hotness is WeakPtr. See
base/memory/weak_ptr.h and base/callback.h (the section on weak pointers) for
documentation.

Basically, the callee accepts a base::Callback and just blindly calls it. The
caller passes a Callback wrapping a WeakPtr. The infrastructure makes sure that
the callback is a no-op of the wrapped WeakPtr is invalid.

So, for example, the Callback passed to Check() should be weak (though this
class doesn't care about that detail). And it looks like GPUFeatureChecker also
uses callbacks. In that case, those should be wrapping
WeakPtr<RequirementsChecker>.

[eaugusti, 2012-07-30 20:03:17]

Back to you.
GPUFeatureChecker was already added in crrev.com/145058

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/requirements_checker.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:55: base::Unretained(this)));
On 2012/07/30 12:15:11, Aaron Boodman wrote:
> Unretained is almost always a bad idea. If this class goes away while the task
> is outstanding, the callback will crash. If you use WeakPtr instead, it will
> prevent the callback from being executed if this class goes away.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/requirements_checker.h (right):

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:8: #include <vecotr>
On 2012/07/30 12:15:11, Aaron Boodman wrote:
> opps

Done.

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:33: content::BrowserThread::ID
callback_thread);
Because of GPUFeatureChecker (GPUDataManager), we need make sure that this is
called on the UI thread (see comment for RequirementsChecker::AsWeakPtr()).

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:40: void IsCSS3dAvailable(bool
available);
On 2012/07/30 12:15:11, Aaron Boodman wrote:
> CSS3D would be more consistent with the other uses of capitalization here.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:46: static bool
checked_for_webgl_;
You're right.

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:56: base::Lock
async_requirement_checks_lock_;
On 2012/07/30 12:15:11, Aaron Boodman wrote:
> You should not need this lock. The asynchronous checks are going to call back
on
> the UI thread, which can only do one thing at a time. Right?

Done.

https://chromiumcodereview.appspot.com/10689097/diff/14001/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:58:
scoped_refptr<GPUFeatureChecker> webgl_checker_;
GPUFeatureChecker is already RefCountedThreadSafe (crrev.com/145058), should I
refactor it?

[Aaron Boodman, 2012-08-01 03:58:54]

Argh, I meant to finish this before you left PST time. Sorry for the high
latency.

Also, I think Yoyo should take a pass through this because he has looked deeply
in this area more recently than me. Moving Mihai to cc.

=== Actual review below ===

Cool, yeah, I think this basic structure is good.

Also, I think that you still need to handle the case where Chrome updates. I
can't believe Chrome doesn't already store a 'lastVersion' preference so that it
can do migrations like this, but I don't see it. So we might have to add that.
Anyway it can be a separate CL.

I'll have to do some research to answer the testing question. But I think it
should be possible.

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.h (right):

http://codereview.chromium.org/10689097/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:33: content::BrowserThread::ID
callback_thread);
On 2012/07/30 20:03:17, eaugusti wrote:
> Because of GPUFeatureChecker (GPUDataManager), we need make sure that this is
> called on the UI thread (see comment for RequirementsChecker::AsWeakPtr()).

I think that we can require this class to also only be used on the UI thread (it
is common in Chrome for classes to be thread-specific).  Just document it in the
class header: "This class should only be used from the UI thread."

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:398:
base::Bind(&CrxInstaller::RequirementsChecked,
Why run this on the file thread? It seems like this is just introducing more
thread hops. If you have it call back on UI thread, then RequirementsCheck can
just directly call ConfirmInstall() rather than PostTask'ing to it.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:553:
std::vector<std::string>::iterator it;
Since you don't need |it| outside the for loop define it there.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:559:
const_cast<Extension*>(extension_.get())->AddInstallWarnings(
You can avoid this const_cast by de-consting extension_ and following that
thread. It ends not to far away at SandboxedUnpacker::Client::OnUnpackSucceeded.
The Extension* that is passed into that is non-const.

None of these actually need to be const at this point in extension's lifetime.
We only need it to be const when it gets added to ExtensionService because at
that point it can be modified on multiple threads concurrently.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.h:207: void RunRequirementsChecker();
Needs comments.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (left):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:644: 
stray edit

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:837: // An extension's granted
permissions need to be migrated if the
stray edit

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:66: // Any errors that arose from
unmet unmet requirements.
duplicate 'unmet'

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:698: ListValue* errors_list = new
ListValue();
Are there other functions in this class doing the same pattern of
std::vector<std::string> -> ListValue? If so, please pull out a
UpdateExtensionPrefList() or someting.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:709: if
(ReadExtensionPrefList(extension_id, kUnsupportedRequirements,
Same comment about potentially pulling out duplicated code.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2146: initial_enable = false;
Oh cool. Since we already had code that was initially disabling extensions on
install, I guess we can assume that it already works, and the comment in the doc
about carefully making sure this doesn't confuse clients doesn't apply.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2149: } else if
(extension_prefs_->HasUnsupportedRequirements(id)) {
Can you check whether state == DISABLE_UNSUPPORTED_REQUIREMENTS instead? If so,
can you remove the unsupported requirements pref?

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/installed_loader.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/installed_loader.cc:79: if
(extension_prefs_->HasUnsupportedRequirements(extension->id())) {
Won't the extension be disabled if it is installed and has unsupported
requirements? How do we get here?

Secondly, I don't think we need to show these warnings for installed extensions,
only unpacked ones.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:20: const char* kWebGlError =
"WebGL is not supported";
These need to be messages in generated_resources.grd so that they can be
localized.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:31: :
async_requirement_checks_(0) {
Nit: Maybe rename this to pending_requirements_checks_?

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:55: DictionaryValue*
requirement_value;
= NULL;

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:56: if
(!requirements_value->GetDictionaryWithoutPathExpansion(*it,
first param on each line must align.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:66: ListValue* features;
= NULL;

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:71: continue;
This is a different kind of error (whee). This should be handled in extension.cc
with the other errors there. Perhaps it would make sense to parse requirements
into a struct which is a member of Extension so that they can be more easily
handled here.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:74: std::string feature;
define feature within for block

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:75: base::ListValue::iterator
it;
define it within for statement

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:96: if
(!async_requirement_checks_) {
Don't use boolean comparisons for integers.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:102: // from the use of
async_requirement_checks_.
Hm, I know that I spec'd RequirementsChecker::Check to maybe be re-entrant, but
now that I see the code, I think maybe that was a bad idea. If you instead make
RequirementsChecker guarantee to always callback asynchronously, I think it will
make it less error-prone and easier to maintain.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:114: void
RequirementsChecker::IsWebGLAvailable(bool available) {
Traditionally a name like Foo::IsMonkey is asking Foo whether it is a monkey (it
would return a boolean). In this case you are essentially telling Foo about
monkeyness. So the method name should be: SetIsWebGLAvailable. Or maybe
SetWebGLAvailability.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:127: if
(!--async_requirement_checks_) {
Compare to zero instead.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.h (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:31: content::BrowserThread::ID
callback_thread);
Yeah, so I think you can just let this callback on the UI thread, which will be
the same thread it is created on.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:46: virtual
base::WeakPtr<RequirementsChecker> AsWeakPtr() OVERRIDE;
Nice catch, but do we have a need to call this from a non-UI thread? I think it
is probably sufficient to just say that it is UI-thread-only in the comments.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:49: friend class
GPUFeatureChecker;
Curiosity: Is this needed so GPUFeatureChecker can call Is*Available() ?

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:197:
extension_service_->extension_prefs()->HasUnsupportedRequirements(
Yeah, I don't think we want to do this. Did I say this in the doc or something?

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:628: } else if
(prefs->HasUnsupportedRequirements(extension_id)) {
Seems like you could check the disable reason instead. This is also stored in
prefs.

[Aaron Boodman, 2012-08-01 04:03:57]

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.cc:559:
const_cast<Extension*>(extension_.get())->AddInstallWarnings(
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> None of these actually need to be const at this point in extension's lifetime.
> We only need it to be const when it gets added to ExtensionService because at
> that point it can be modified on multiple threads concurrently.

s/modified/accessed/

[Yoyo Zhou, 2012-08-02 22:23:30]

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.h:207: void RunRequirementsChecker();
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Needs comments.

How about "Check Requirements"?

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.h:355: bool
error_on_unsported_requirements_;
Did you mean 'unsupported'?

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.cc (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:20: const char* kWebGlError =
"WebGL is not supported";
Naming nit: kWebGLError

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:70:
errors_.push_back("Improperly formatted requirement features for 3D");
Define literal strings at the top.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.h (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:46: virtual
base::WeakPtr<RequirementsChecker> AsWeakPtr() OVERRIDE;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Nice catch, but do we have a need to call this from a non-UI thread? I think
it
> is probably sufficient to just say that it is UI-thread-only in the comments.

There is also WeakPtrFactory which gives a bit more control over who can get a
weak pointer. Something to look into, not necessarily better for this case.

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:59: // Every requirments that
needs to be resolved asynchroniously will add to
typo: requirement, asynchronously

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/unpacked_installer.h (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/unpacked_installer.h:54: void
RunRequirementsChecker();
Same comment here (CheckRequirements). Seeing this common definition makes me
think it should be defined as a RequirementsChecker::Delegate interface. Aaron,
thoughts?

[eaugusti, 2012-08-03 01:06:25]

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/crx_installer.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/crx_installer.cc:398:
base::Bind(&CrxInstaller::RequirementsChecked,
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Why run this on the file thread? It seems like this is just introducing more
> thread hops. If you have it call back on UI thread, then RequirementsCheck can
> just directly call ConfirmInstall() rather than PostTask'ing to it.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/crx_installer.cc:553:
std::vector<std::string>::iterator it;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Since you don't need |it| outside the for loop define it there.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/crx_installer.cc:559:
const_cast<Extension*>(extension_.get())->AddInstallWarnings(
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> You can avoid this const_cast by de-consting extension_ and following that
> thread. It ends not to far away at
SandboxedUnpacker::Client::OnUnpackSucceeded.
> The Extension* that is passed into that is non-const.
> 
> None of these actually need to be const at this point in extension's lifetime.
> We only need it to be const when it gets added to ExtensionService because at
> that point it can be modified on multiple threads concurrently.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/crx_installer.h (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/crx_installer.h:207: void RunRequirementsChecker();
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Needs comments.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/crx_installer.h:355: bool
error_on_unsported_requirements_;
On 2012/08/02 22:23:30, Yoyo Zhou wrote:
> Did you mean 'unsupported'?

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/extension_prefs.cc (left):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:644: 
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> stray edit

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:837: // An extension's granted
permissions need to be migrated if the
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> stray edit

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:66: // Any errors that arose from
unmet unmet requirements.
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> duplicate 'unmet'

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:698: ListValue* errors_list = new
ListValue();
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Are there other functions in this class doing the same pattern of
> std::vector<std::string> -> ListValue? If so, please pull out a
> UpdateExtensionPrefList() or someting.

No one uses this exact functionality, but I can still pull it out for later.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:709: if
(ReadExtensionPrefList(extension_id, kUnsupportedRequirements,
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Same comment about potentially pulling out duplicated code.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/extension_service.cc:2146: initial_enable = false;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Oh cool. Since we already had code that was initially disabling extensions on
> install, I guess we can assume that it already works, and the comment in the
doc
> about carefully making sure this doesn't confuse clients doesn't apply.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/extension_service.cc:2149: } else if
(extension_prefs_->HasUnsupportedRequirements(id)) {
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Can you check whether state == DISABLE_UNSUPPORTED_REQUIREMENTS instead? If
so,
> can you remove the unsupported requirements pref?

If we remove the unsupported requirements in the perfs, then we will be unable
to display the warnings on updated/sync'd extensions after a restart.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/installed_loader.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/installed_loader.cc:79: if
(extension_prefs_->HasUnsupportedRequirements(extension->id())) {
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Won't the extension be disabled if it is installed and has unsupported
> requirements? How do we get here?
> 
> Secondly, I don't think we need to show these warnings for installed
extensions,
> only unpacked ones.

The extension will remain disabled, but I think that we need the warnings. It
will be a bit awkward if you can see the requirement warnings when the extension
gets sync'd the first time, but then never after that.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/requirements_checker.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:20: const char* kWebGlError =
"WebGL is not supported";
On 2012/08/02 22:23:30, Yoyo Zhou wrote:
> Naming nit: kWebGLError

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:31: :
async_requirement_checks_(0) {
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Nit: Maybe rename this to pending_requirements_checks_?

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:55: DictionaryValue*
requirement_value;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> = NULL;

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:56: if
(!requirements_value->GetDictionaryWithoutPathExpansion(*it,
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> first param on each line must align.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:66: ListValue* features;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> = NULL;

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:70:
errors_.push_back("Improperly formatted requirement features for 3D");
On 2012/08/02 22:23:30, Yoyo Zhou wrote:
> Define literal strings at the top.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:71: continue;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> This is a different kind of error (whee). This should be handled in
extension.cc
> with the other errors there. Perhaps it would make sense to parse requirements
> into a struct which is a member of Extension so that they can be more easily
> handled here.

Done, I like that struct.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:74: std::string feature;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> define feature within for block

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:75: base::ListValue::iterator
it;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> define it within for statement

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:96: if
(!async_requirement_checks_) {
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Don't use boolean comparisons for integers.

:(

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:102: // from the use of
async_requirement_checks_.
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Hm, I know that I spec'd RequirementsChecker::Check to maybe be re-entrant,
but
> now that I see the code, I think maybe that was a bad idea. If you instead
make
> RequirementsChecker guarantee to always callback asynchronously, I think it
will
> make it less error-prone and easier to maintain.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:114: void
RequirementsChecker::IsWebGLAvailable(bool available) {
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Traditionally a name like Foo::IsMonkey is asking Foo whether it is a monkey
(it
> would return a boolean). In this case you are essentially telling Foo about
> monkeyness. So the method name should be: SetIsWebGLAvailable. Or maybe
> SetWebGLAvailability.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.cc:127: if
(!--async_requirement_checks_) {
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Compare to zero instead.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/requirements_checker.h (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:31: content::BrowserThread::ID
callback_thread);
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Yeah, so I think you can just let this callback on the UI thread, which will
be
> the same thread it is created on.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:46: virtual
base::WeakPtr<RequirementsChecker> AsWeakPtr() OVERRIDE;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Nice catch, but do we have a need to call this from a non-UI thread? I think
it
> is probably sufficient to just say that it is UI-thread-only in the comments.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:49: friend class
GPUFeatureChecker;
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Curiosity: Is this needed so GPUFeatureChecker can call Is*Available() ?

Yes, but it is totally unnecessary. I must be going crazy.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/requirements_checker.h:59: // Every requirments that
needs to be resolved asynchroniously will add to
On 2012/08/02 22:23:30, Yoyo Zhou wrote:
> typo: requirement, asynchronously

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
File chrome/browser/extensions/unpacked_installer.h (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ext...
chrome/browser/extensions/unpacked_installer.h:54: void
RunRequirementsChecker();
On 2012/08/02 22:23:30, Yoyo Zhou wrote:
> Same comment here (CheckRequirements). Seeing this common definition makes me
> think it should be defined as a RequirementsChecker::Delegate interface.
Aaron,
> thoughts?

Done.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ui/...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:197:
extension_service_->extension_prefs()->HasUnsupportedRequirements(
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Yeah, I don't think we want to do this. Did I say this in the doc or
something?

This wasn't in the doc, but I think it is the best way to persistently show
these warnings.

https://chromiumcodereview.appspot.com/10689097/diff/13021/chrome/browser/ui/...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:628: } else if
(prefs->HasUnsupportedRequirements(extension_id)) {
On 2012/08/01 03:58:54, Aaron Boodman wrote:
> Seems like you could check the disable reason instead. This is also stored in
> prefs.

Done.

[Aaron Boodman, 2012-08-06 20:36:06]

nice

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
File chrome/browser/extensions/unpacked_installer.h (right):

http://codereview.chromium.org/10689097/diff/13021/chrome/browser/extensions/...
chrome/browser/extensions/unpacked_installer.h:54: void
RunRequirementsChecker();
On 2012/08/02 22:23:30, Yoyo Zhou wrote:
> Same comment here (CheckRequirements). Seeing this common definition makes me
> think it should be defined as a RequirementsChecker::Delegate interface.
Aaron,
> thoughts?

I'm kinda in to using WeakPtr callbacks where Chrome would traditionally use
delegates. The advantage is that the framework automatically handles the
lifetime issues for you. If we used a delegate here we'd have to manually ensure
it did not get called back after it had been deleted.

http://codereview.chromium.org/10689097/diff/26030/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10689097/diff/26030/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:450: &value_list))
Needs curlies.

http://codereview.chromium.org/10689097/diff/26030/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:664: 
stray edit

http://codereview.chromium.org/10689097/diff/26030/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:884: 
stray edit

http://codereview.chromium.org/10689097/diff/26030/chrome/browser/extensions/...
File chrome/browser/extensions/installed_loader.cc (right):

http://codereview.chromium.org/10689097/diff/26030/chrome/browser/extensions/...
chrome/browser/extensions/installed_loader.cc:79: if (extension &&
stray edit?

http://codereview.chromium.org/10689097/diff/26030/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10689097/diff/26030/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1719: &requirement_value)) {
nit: previous indentation style was fine.

http://codereview.chromium.org/10689097/diff/26030/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1728: ListValue* features;
= NULL;

[eaugusti, 2012-08-17 23:25:01]

I'm pretty sure I addressed all the issues over the last two patch sets, but I
may have missed something.

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/browser/ext...
File chrome/browser/extensions/extension_prefs.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:450: &value_list))
On 2012/08/06 20:36:07, Aaron Boodman wrote:
> Needs curlies.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:664: 
On 2012/08/06 20:36:07, Aaron Boodman wrote:
> stray edit

Done.

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/browser/ext...
chrome/browser/extensions/extension_prefs.cc:884: 
On 2012/08/06 20:36:07, Aaron Boodman wrote:
> stray edit

Done.

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/browser/ext...
File chrome/browser/extensions/installed_loader.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/browser/ext...
chrome/browser/extensions/installed_loader.cc:79: if (extension &&
On 2012/08/06 20:36:07, Aaron Boodman wrote:
> stray edit?

Done.

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/common/exte...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/common/exte...
chrome/common/extensions/extension.cc:1719: &requirement_value)) {
On 2012/08/06 20:36:07, Aaron Boodman wrote:
> nit: previous indentation style was fine.

Done.

https://chromiumcodereview.appspot.com/10689097/diff/26030/chrome/common/exte...
chrome/common/extensions/extension.cc:1728: ListValue* features;
On 2012/08/06 20:36:07, Aaron Boodman wrote:
> = NULL;

Done.

https://chromiumcodereview.appspot.com/10689097/diff/33001/chrome/browser/ext...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/10689097/diff/33001/chrome/browser/ext...
chrome/browser/extensions/extension_service.h:404: bool has_requirement_errors);
Since we no longer put the requirement errors in the extensions Install
warnings, there is no need to pass the errors into ExtensionService.

[Aaron Boodman, 2012-08-21 23:04:21]

http://codereview.chromium.org/10689097/diff/17008/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/app/generated_resou...
chrome/app/generated_resources.grd:4562: +        Cannot enable, not all
requirements are met.
It would be better to show the specific requirement that was not met.

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.h:211: void
RequirementsChecked(std::vector<std::string> errors);
OnRequirementsChecked

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.cc (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
This change needs lots of tests:

- manifest tests for the new 'requirements' field.
- integration tests that try to install an extension various ways that doesn't
meet requirements and fails.

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.h (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:20: // This class lives on the
UI  thread. This includes any calls to AsWeakPtr().
extra space after "UI"

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:20: // This class lives on the
UI  thread. This includes any calls to AsWeakPtr().
Validates the 'requirements' extension manifest field. This is an asynchronous
process that involves several threads, but the public interface of this class
(including constructor and destructor) must only be used on the UI thread.

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:630:
ExtensionErrorReporter::GetInstance()->ReportError(
I think that we should do this inside ExtensionService::EnableExtension()
instead.

http://codereview.chromium.org/10689097/diff/17008/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1718: if (*it == "plugins") {
Is it possible to use the JSON Schema Compiler here? Perhaps different CL. Ask
CalPoly peeps how hard.

http://codereview.chromium.org/10689097/diff/17008/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1720: requirement_value->begin_keys();
indent either 2 or 4 from "D". Don't care which.

[eaugusti, 2012-08-24 19:30:52]

http://codereview.chromium.org/10689097/diff/17008/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/app/generated_resou...
chrome/app/generated_resources.grd:4562: +        Cannot enable, not all
requirements are met.
On 2012/08/21 23:04:21, Aaron Boodman wrote:
> It would be better to show the specific requirement that was not met.

Ok. To do this we will need a value in the prefs. I am going to pass around a
bitmask instead of the localized strings. Then we can just use a utility
function to pull the localized strings out of the bitmask.

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
chrome/browser/extensions/crx_installer.h:211: void
RequirementsChecked(std::vector<std::string> errors);
On 2012/08/21 23:04:21, Aaron Boodman wrote:
> OnRequirementsChecked

Done.

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.cc (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/08/21 23:04:21, Aaron Boodman wrote:
> This change needs lots of tests:
> 
> - manifest tests for the new 'requirements' field.
> - integration tests that try to install an extension various ways that doesn't
> meet requirements and fails.

TEST TIME!

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
File chrome/browser/extensions/requirements_checker.h (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/extensions/...
chrome/browser/extensions/requirements_checker.h:20: // This class lives on the
UI  thread. This includes any calls to AsWeakPtr().
On 2012/08/21 23:04:21, Aaron Boodman wrote:
> extra space after "UI"

Done.

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:630:
ExtensionErrorReporter::GetInstance()->ReportError(
On 2012/08/21 23:04:21, Aaron Boodman wrote:
> I think that we should do this inside ExtensionService::EnableExtension()
> instead.

No move. Discussed irl.

http://codereview.chromium.org/10689097/diff/17008/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10689097/diff/17008/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1718: if (*it == "plugins") {
On 2012/08/21 23:04:21, Aaron Boodman wrote:
> Is it possible to use the JSON Schema Compiler here? Perhaps different CL. Ask
> CalPoly peeps how hard.

Done.

http://codereview.chromium.org/10689097/diff/17008/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1720: requirement_value->begin_keys();
On 2012/08/21 23:04:21, Aaron Boodman wrote:
> indent either 2 or 4 from "D". Don't care which.

Done.

[eaugusti, 2012-08-31 00:47:49]

Now recheck the requirements before enabling. 
Also, I removed the extra crx files.

[Aaron Boodman, 2012-09-04 21:48:57]

LGTM - land it already

http://codereview.chromium.org/10689097/diff/56001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10689097/diff/56001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:81:
requirements_checker_(new extensions::RequirementsChecker()) {
Nit: Can delay creating this until needed.

[eaugusti, 2012-09-04 23:52:15]

jhawkins - Can you look over the c/b/ui (only real change is in
extension_settings_handler).

http://codereview.chromium.org/10689097/diff/56001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10689097/diff/56001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:81:
requirements_checker_(new extensions::RequirementsChecker()) {
On 2012/09/04 21:48:57, Aaron Boodman wrote:
> Nit: Can delay creating this until needed.

Done.

[James Hawkins, 2012-09-05 00:01:51]

c/b/ui LGTM

[commit-bot: I haz the power, 2012-09-05 03:50:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eaugusti@chromium.org/10689097/59001

[commit-bot: I haz the power, 2012-09-05 03:50:49]

Failed to apply patch for chrome/browser/extensions/unpacked_installer.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/unpacked_installer.cc
Hunk #3 FAILED at 94.
Hunk #4 succeeded at 111 (offset 1 line).
Hunk #5 FAILED at 137.
Hunk #6 FAILED at 198.
Hunk #7 succeeded at 209 (offset -4 lines).
Hunk #8 succeeded at 219 (offset -4 lines).
Hunk #9 succeeded at 227 (offset -4 lines).
3 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/extensions/unpacked_installer.cc.rej

[commit-bot: I haz the power, 2012-09-05 18:44:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eaugusti@chromium.org/10689097/49006

[commit-bot: I haz the power, 2012-09-05 21:35:55]

Try job failure for 10689097-49006 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-07 16:55:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eaugusti@chromium.org/10689097/51004

[commit-bot: I haz the power, 2012-09-07 16:55:53]

Failed to apply patch for chrome/browser/extensions/app_process_apitest.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/app_process_apitest.cc
Hunk #1 FAILED at 253.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/app_process_apitest.cc.rej

[commit-bot: I haz the power, 2012-09-07 21:33:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eaugusti@chromium.org/10689097/55065

[commit-bot: I haz the power, 2012-09-07 23:02:58]

Try job failure for 10689097-55065 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...