Added a PolicyManifestHandler.

Added a PolicyManifestHandler.

This handles the "storage.managed_schema" manifest key, and validates the
schema when this key is present. The schema is then published in the descriptor
of the extensions' policy namespace.

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

[Joao da Silva, 2013-05-14 16:46:04]

This depends on https://codereview.chromium.org/15061007.

@kalman: please review; I'm not sure I got everything that needs to be done when
adding new keys to the manifest, please advise.

@mnissler: please review the policy/ changes

Thanks!

[Mattias Nissler (ping if slow), 2013-05-15 10:37:38]

Is the test data you're adding in this patch actually used?

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:103: for
(ExtensionSet::const_iterator it = set->begin(); it != set->end(); ++it) {
It seems this loop is not really needed, you could just pass the entire
extension set to the file thread and do filtering there.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:116: // The
2nd check needs to be performed on the FILE thread.
What is the 2nd check? Just spell it out :)

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:148: if
(!file_util::ReadFileToString((*it)->path().AppendASCII(path),
I was momentarily confused by the two |path|s here. Maybe rename the local to
schema_file?

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:159:
NOTREACHED();
Is this really a NOTREACHED()? Won't we hit this if I put an invalid manifest
into an extension?

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.cc (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.cc:23: const char
kInvalidPermission[] =
nit: kMissingPermission would be more accurate.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.cc:62: file =
extension->path().Append(path);
Does this make sure I can't put ../../../../../../../../etc/passwd?

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.h (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.h:12: class PolicyManifestHandler
: public extensions::ManifestHandler {
So chrome/browser/policy depends on chrome/browser/extensions again? And vice
versa?

Note that almost all ManifestHandlers are defined within
chrome/common/extensions.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.h:27:
DISALLOW_COPY_AND_ASSIGN(PolicyManifestHandler);
#include "base/basictypes.h"

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

I got up to PolicyManifestHander then started to chat to Joao about it.

Basically: a nicer model here would be for PolicyManifestHandler to grab the
file it needs, parse it into a PolicySchema, then use SetManifestData on
extension with it. Then ManagedValueStoreCache wouldn't need to do any of that
stuff, just read it out directly.

However, it doesn't look like the ManifestHandler API supports this - the schema
is read on the FILE thread, but SetManifestData isn't thread safe. Validate kind
of lets you do what you want, except it can't actually write data back (thread
safety) it can only be used to validate it.

Looping in yoz@ here - what is the intended idiom for this? Do we need to do a
bit more work in the ManifestHandler infrastrucgture to make this work? Or do we
need to just suck it up and deal with it? Because it'd be nice if an invalid
schema were an installation error. I don't know off the top of my head if we
have a similar thing elsewhere in extensions.

Quick fix: make SetManifestData thread safe. Longer fix: unify Parse and
Validate and do a sync API type model where a manifest key can declare what
thread it lives on, then it gets called on that thread (... with a few other
little things you'd need to write to make that work - for example, this combined
Parse/Validate method could return the thing it creates rather than setting it).
Just brainstorming here.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:103: for
(ExtensionSet::const_iterator it = set->begin(); it != set->end(); ++it) {
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> It seems this loop is not really needed, you could just pass the entire
> extension set to the file thread and do filtering there.

One advantage of filtering here is that the method can early-exit if there are
no extensions that have managed storage, and not make extra work for the message
loops. That said - this isn't being done, maybe it should be. And there are less
expensive ways of implementing that than creating a whole extra ExtensionSet.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:159:
NOTREACHED();
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> Is this really a NOTREACHED()? Won't we hit this if I put an invalid manifest
> into an extension?

Hopefully we are doing the schema validation at extension install time, but I
haven't reached that part of the review yet.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.h (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.h:12: class PolicyManifestHandler
: public extensions::ManifestHandler {
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> So chrome/browser/policy depends on chrome/browser/extensions again? And vice
> versa?

Looks like this only depends on chrome/common/extensions to me.

> 
> Note that almost all ManifestHandlers are defined within
> chrome/common/extensions.

The model Joao has is correct, the browser components project for extensions is
pulling the ManifestHandler implementations into where they are used. Eventually
the core of the manifest handling code will be moved in src/extensions/common.

[not at google - send to devlin, 2013-05-16 16:34:30]

Oops, actually +yoz.

[Mattias Nissler (ping if slow), 2013-05-16 18:27:54]

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.h (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.h:12: class PolicyManifestHandler
: public extensions::ManifestHandler {
On 2013/05/16 16:34:15, kalman wrote:
> On 2013/05/15 10:37:39, Mattias Nissler wrote:
> > So chrome/browser/policy depends on chrome/browser/extensions again? And
vice
> > versa?
> 
> Looks like this only depends on chrome/common/extensions to me.

Fair, I was merely talking about dependencies between the policy and extensions
subsystems.

> 
> > 
> > Note that almost all ManifestHandlers are defined within
> > chrome/common/extensions.
> 
> The model Joao has is correct, the browser components project for extensions
is
> pulling the ManifestHandler implementations into where they are used.
Eventually
> the core of the manifest handling code will be moved in src/extensions/common.

I'm totally on board with putting the ManifestHandlers next to the code that's
using the data the handler generates. By this argument, the handler belongs into
chrome/browser/extensions/api/storage, because that's where the schema
information is being used.

Side note: It should probably be renamed to StorageSchemaManifestHandler in any
case.

[Yoyo Zhou, 2013-05-16 19:14:16]

kalman: if an extension fails to Validate it does not load. So, based on my
understanding of this CL, an invalid schema is an installation error. It's just
the parsing/storing that can't be done elegantly right now, and as far as I know
we haven't had a way to do that cross-threads. It hasn't been on my radar, but I
agree a generalized Parse+Validate is probably a good long-term solution.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.cc (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.cc:75: return schema;
I'd expect !!schema or such for readability.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.h (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.h:12: class PolicyManifestHandler
: public extensions::ManifestHandler {
On 2013/05/16 18:27:54, Mattias Nissler wrote:
> On 2013/05/16 16:34:15, kalman wrote:
> > On 2013/05/15 10:37:39, Mattias Nissler wrote:
> > > So chrome/browser/policy depends on chrome/browser/extensions again? And
> vice
> > > versa?
> > 
> > Looks like this only depends on chrome/common/extensions to me.
> 
> Fair, I was merely talking about dependencies between the policy and
extensions
> subsystems.
> 
> > 
> > > 
> > > Note that almost all ManifestHandlers are defined within
> > > chrome/common/extensions.
> > 
> > The model Joao has is correct, the browser components project for extensions
> is
> > pulling the ManifestHandler implementations into where they are used.
> Eventually
> > the core of the manifest handling code will be moved in
src/extensions/common.
> 
> I'm totally on board with putting the ManifestHandlers next to the code that's
> using the data the handler generates. By this argument, the handler belongs
into
> chrome/browser/extensions/api/storage, because that's where the schema
> information is being used.
> 
> Side note: It should probably be renamed to StorageSchemaManifestHandler in
any
> case.
> 

Yes, it's better to have policy extension API stuff in its own directory (either
under policy or extensions/api) because DEPS operate at the directory level.

[Joao da Silva, 2013-05-19 13:21:35]

@bartfab: please review policy/

@mnissler: yes, the test data is used by ExtensionSettingsApiTests.

@kalman, @yoz: if validation fails then the extension doesn't load, as Yoyo
mentioned. For now the schema is being parsed twice (once at validation, and
then later when loaded for the PolicyDomainDescriptor). Ideally the PolicySchema
would be owned by the Extension as a ManifestData entry; I'll refactor that once
the thread-safety concerns are addressed.

Please have another look :-) Thanks!

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:103: for
(ExtensionSet::const_iterator it = set->begin(); it != set->end(); ++it) {
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> It seems this loop is not really needed, you could just pass the entire
> extension set to the file thread and do filtering there.

It's expected that most extensions won't have the schema, so passing only those
that do spares some work.

In any case a copy must be created here, because ExtensionService::extensions()
is UI only.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:103: for
(ExtensionSet::const_iterator it = set->begin(); it != set->end(); ++it) {
On 2013/05/16 16:34:15, kalman wrote:
> On 2013/05/15 10:37:39, Mattias Nissler wrote:
> > It seems this loop is not really needed, you could just pass the entire
> > extension set to the file thread and do filtering there.
> 
> One advantage of filtering here is that the method can early-exit if there are
> no extensions that have managed storage, and not make extra work for the
message
> loops. That said - this isn't being done, maybe it should be. And there are
less
> expensive ways of implementing that than creating a whole extra ExtensionSet.

|managed_extensions| could be created only when the first match is found. But
LoadSchemas() must be invoked even when no match is found, to possibly remove
entries for extensions that have been unloaded. I'd rather have a single path
Observe->LoadSchemas->RegisterDomain than a special-case
Observer->RegisterDomain for the no-matches case.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:116: // The
2nd check needs to be performed on the FILE thread.
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> What is the 2nd check? Just spell it out :)

Done.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:148: if
(!file_util::ReadFileToString((*it)->path().AppendASCII(path),
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> I was momentarily confused by the two |path|s here. Maybe rename the local to
> schema_file?

Done.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:159:
NOTREACHED();
On 2013/05/16 16:34:15, kalman wrote:
> On 2013/05/15 10:37:39, Mattias Nissler wrote:
> > Is this really a NOTREACHED()? Won't we hit this if I put an invalid
manifest
> > into an extension?
> 
> Hopefully we are doing the schema validation at extension install time, but I
> haven't reached that part of the review yet.

As Benjamin suspects, this is a NOTREACHED because the extension shouldn't have
validated otherwise.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.cc (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.cc:23: const char
kInvalidPermission[] =
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> nit: kMissingPermission would be more accurate.

Done.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.cc:62: file =
extension->path().Append(path);
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> Does this make sure I can't put ../../../../../../../../etc/passwd?

It doesn't, but FilePath::ReferencesParent() does. Good catch!

I doubt that /etc/passwd is valid JSON schema though ;-)

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.cc:75: return schema;
On 2013/05/16 19:14:16, Yoyo Zhou wrote:
> I'd expect !!schema or such for readability.

Done.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_manifest_handler.h (right):

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.h:12: class PolicyManifestHandler
: public extensions::ManifestHandler {
On 2013/05/16 19:14:16, Yoyo Zhou wrote:
> On 2013/05/16 18:27:54, Mattias Nissler wrote:
> > On 2013/05/16 16:34:15, kalman wrote:
> > > On 2013/05/15 10:37:39, Mattias Nissler wrote:
> > > > So chrome/browser/policy depends on chrome/browser/extensions again? And
> > vice
> > > > versa?
> > > 
> > > Looks like this only depends on chrome/common/extensions to me.
> > 
> > Fair, I was merely talking about dependencies between the policy and
> extensions
> > subsystems.
> > 
> > > 
> > > > 
> > > > Note that almost all ManifestHandlers are defined within
> > > > chrome/common/extensions.
> > > 
> > > The model Joao has is correct, the browser components project for
extensions
> > is
> > > pulling the ManifestHandler implementations into where they are used.
> > Eventually
> > > the core of the manifest handling code will be moved in
> src/extensions/common.
> > 
> > I'm totally on board with putting the ManifestHandlers next to the code
that's
> > using the data the handler generates. By this argument, the handler belongs
> into
> > chrome/browser/extensions/api/storage, because that's where the schema
> > information is being used.
> > 
> > Side note: It should probably be renamed to StorageSchemaManifestHandler in
> any
> > case.
> > 
> 
> Yes, it's better to have policy extension API stuff in its own directory
(either
> under policy or extensions/api) because DEPS operate at the directory level.

Moved to
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc.

https://codereview.chromium.org/14587009/diff/15001/chrome/browser/policy/pol...
chrome/browser/policy/policy_manifest_handler.h:27:
DISALLOW_COPY_AND_ASSIGN(PolicyManifestHandler);
On 2013/05/15 10:37:39, Mattias Nissler wrote:
> #include "base/basictypes.h"

Done.

[not at google - send to devlin, 2013-05-20 21:11:11]

> 
> Please have another look :-) Thanks!
> 

It doesn't look like you've uploaded anything new?

[bartfab (slow), 2013-05-21 12:18:15]

Indeed, you seem to have forgotten to upload the most recent revision for this
CL.

[Joao da Silva, 2013-05-21 18:14:23]

Teehee you're right! Here's the updated CL :-)

@kalman, bartfab: PTAL

[bartfab (slow), 2013-05-22 11:00:09]

After your refactorings, there is no more policy/ left to review :). I took the
liberty to look over the entire CL instead, adding a few nits.

lgtm

https://chromiumcodereview.appspot.com/14587009/diff/45001/chrome/browser/ext...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://chromiumcodereview.appspot.com/14587009/diff/45001/chrome/browser/ext...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:115: // Check
the schema file on the FILE thread, for the extensions that may
Nit 1: "Load" instead of "Check"? LoadSchemas() does check that the schema was
parsed correctly but the primary goal is to load the schema and have it
available, not just to check that it is valid.

Nit 2: I think it would be better to put the whole sentence in plural.

https://chromiumcodereview.appspot.com/14587009/diff/45001/chrome/browser/ext...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:118:
BrowserThread::FILE, FROM_HERE,
IIRC, using the FILE thread in new code is a no-no. Use the blocking pool
instead?

https://chromiumcodereview.appspot.com/14587009/diff/45001/chrome/browser/ext...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://chromiumcodereview.appspot.com/14587009/diff/45001/chrome/browser/ext...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:38:
std::string path;
Nit: #include <string>

https://chromiumcodereview.appspot.com/14587009/diff/45001/chrome/browser/ext...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:50:
std::vector<InstallWarning>* warnings) const {
Nit 1: #include <vector>
Nit 2: #include "chrome/common/extensions/install_warning.h"

[Joao da Silva, 2013-05-22 12:26:19]

You're right, it's all extensions/ code now :-) Fixed the nits, waiting for
input from kalman@.

https://codereview.chromium.org/14587009/diff/45001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/14587009/diff/45001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:115: // Check
the schema file on the FILE thread, for the extensions that may
On 2013/05/22 11:00:09, bartfab wrote:
> Nit 1: "Load" instead of "Check"? LoadSchemas() does check that the schema was
> parsed correctly but the primary goal is to load the schema and have it
> available, not just to check that it is valid.
> 
> Nit 2: I think it would be better to put the whole sentence in plural.

Done.

https://codereview.chromium.org/14587009/diff/45001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:118:
BrowserThread::FILE, FROM_HERE,
On 2013/05/22 11:00:09, bartfab wrote:
> IIRC, using the FILE thread in new code is a no-no. Use the blocking pool
> instead?

Done. Didn't know it was a no-no, I thought the blocking pool was just
encouraged over FILE.

https://codereview.chromium.org/14587009/diff/45001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/14587009/diff/45001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:38:
std::string path;
On 2013/05/22 11:00:09, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/14587009/diff/45001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:50:
std::vector<InstallWarning>* warnings) const {
On 2013/05/22 11:00:09, bartfab wrote:
> Nit 1: #include <vector>
> Nit 2: #include "chrome/common/extensions/install_warning.h"

Done.

[Joao da Silva, 2013-05-23 08:05:03]

@kalman: friendly ping

[not at google - send to devlin, 2013-05-23 16:56:18]

sorry I lost track of the review!

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:119:
weak_factory_.GetWeakPtr()));
Why make LoadSchemas static and take |self| rather than making it an instance
method and binding to a WeakPtr of |this|? Is it so that you only access
WeakPtrFactory on the UI thread?

Does copying |self| have similar issues anyway? AFAIK the only way to do
thread-safe binding in this way is via RefCountedThreadSafe? But I'm really not
up to speed in this area.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:144: // and
is valid.
seems like all of this (lines 143 - 158) should be something along the lines of:

std::string error;
scoped_ptr<policy::PolicySchema> schema =
    StorageSchemaManifestHandler::GetSchema(*it, &error);
CHECK(schema) << error;
descriptor->RegisterComponent((*it)->id(), schema.Pass());

then StorageSchemaManifestHandler::Validate could be implemented like:

return !!GetSchema(extension, error);

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:30:
"Invalid value for storage.managed_schema - must be a string";
"storage.managed_schema must be a string"

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:55: if
(!extension->HasAPIPermission(APIPermission::kStorage)) {
we should make the storage permission implied by the presence of the storage
manifest key, but until that's trivial:

https://codereview.chromium.org/15091002/
specifically
https://codereview.chromium.org/15091002/diff/32001/chrome/common/extensions/...

don't worry about it yet. but monitor it :) I'll try to remember to remind you
but it's likely I will fail.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:70:
base::StringPrintf("File does not exist: %s", file.value().c_str());
"%s referenced by storage.managed_schema does not exist" %
(file.value().c_str())

would be nice to %s all of these hard-coded references to
storage.managed_schema, by the way.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h
(right):

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:14:
class StorageSchemaManifestHandler : public ManifestHandler {
bleh is there some way to tell git that this isn't a copy? there's some flag to
git commit that configures it IIRC.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:15:
public:
pls quickly unit test the parsing here.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:24:
virtual const std::vector<std::string> Keys() const OVERRIDE;
surely one or all of these can be private

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:311: "extension_types":
"all",
hosted apps shouldn't have access, should be
extensions/platform_app/packaged_app.

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:316: "extension_types":
"all",
ditto

https://codereview.chromium.org/14587009/diff/96001/chrome/test/data/extensio...
File chrome/test/data/extensions/managed_extension/schema/managed_storage.json
(right):

https://codereview.chromium.org/14587009/diff/96001/chrome/test/data/extensio...
chrome/test/data/extensions/managed_extension/schema/managed_storage.json:2:
"$schema": "http://json-schema.org/draft-03/schema#",
just curious but is there any particular reason this needs to end with a #?

[Joao da Silva, 2013-05-24 21:31:48]

@kalman: PTAL

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/managed_value_store_cache.cc (right):

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:119:
weak_factory_.GetWeakPtr()));
On 2013/05/23 16:56:18, kalman wrote:
> Why make LoadSchemas static and take |self| rather than making it an instance
> method and binding to a WeakPtr of |this|? Is it so that you only access
> WeakPtrFactory on the UI thread?
> 

WeakPtrs can only be used on the thread that created them. So the WeakPtr here
can only be used on UI; |self| is passed so that the result of the background
task can be posted back to UI.

> Does copying |self| have similar issues anyway? AFAIK the only way to do
> thread-safe binding in this way is via RefCountedThreadSafe? But I'm really
not
> up to speed in this area.

Copying WeakPtrs across threads is fine, and destroying them in other threads is
fine too. But checking if they are still valid, and using the underlying
pointer, must always be done on the thread that owns the factory (which is the
first thread that calls GetWeakPtr(); in this case, UI).

I personally use this pattern quite a bit, and prefer it over refcounted object,
because in this way objects can still be owned by one thread and can be
explicitly destroyed at a deterministic stage during shutdown; background tasks
don't keep the object alive longer than it must.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/managed_value_store_cache.cc:144: // and
is valid.
On 2013/05/23 16:56:18, kalman wrote:
> seems like all of this (lines 143 - 158) should be something along the lines
of:
> 
> std::string error;
> scoped_ptr<policy::PolicySchema> schema =
>     StorageSchemaManifestHandler::GetSchema(*it, &error);
> CHECK(schema) << error;
> descriptor->RegisterComponent((*it)->id(), schema.Pass());
> 
> then StorageSchemaManifestHandler::Validate could be implemented like:
> 
> return !!GetSchema(extension, error);

Done.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:30:
"Invalid value for storage.managed_schema - must be a string";
On 2013/05/23 16:56:18, kalman wrote:
> "storage.managed_schema must be a string"

Done.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:55: if
(!extension->HasAPIPermission(APIPermission::kStorage)) {
On 2013/05/23 16:56:18, kalman wrote:
> we should make the storage permission implied by the presence of the storage
> manifest key, but until that's trivial:
> 
> https://codereview.chromium.org/15091002/
> specifically
>
https://codereview.chromium.org/15091002/diff/32001/chrome/common/extensions/...
> 
> don't worry about it yet. but monitor it :) I'll try to remember to remind you
> but it's likely I will fail.

I've cc'ed myself on that issue, I'll update this once that's in :-) Thanks for
the hint!

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:70:
base::StringPrintf("File does not exist: %s", file.value().c_str());
On 2013/05/23 16:56:18, kalman wrote:
> "%s referenced by storage.managed_schema does not exist" %
> (file.value().c_str())
> 

Done.

> would be nice to %s all of these hard-coded references to
> storage.managed_schema, by the way.

Done.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h
(right):

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:14:
class StorageSchemaManifestHandler : public ManifestHandler {
On 2013/05/23 16:56:18, kalman wrote:
> bleh is there some way to tell git that this isn't a copy? there's some flag
to
> git commit that configures it IIRC.

Uploaded with --no-find-copies, let's see what it does.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:15:
public:
On 2013/05/23 16:56:18, kalman wrote:
> pls quickly unit test the parsing here.

Done.

https://codereview.chromium.org/14587009/diff/96001/chrome/browser/extensions...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:24:
virtual const std::vector<std::string> Keys() const OVERRIDE;
On 2013/05/23 16:56:18, kalman wrote:
> surely one or all of these can be private

Done.

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:311: "extension_types":
"all",
On 2013/05/23 16:56:18, kalman wrote:
> hosted apps shouldn't have access, should be
> extensions/platform_app/packaged_app.

Done.

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:316: "extension_types":
"all",
On 2013/05/23 16:56:18, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/14587009/diff/96001/chrome/test/data/extensio...
File chrome/test/data/extensions/managed_extension/schema/managed_storage.json
(right):

https://codereview.chromium.org/14587009/diff/96001/chrome/test/data/extensio...
chrome/test/data/extensions/managed_extension/schema/managed_storage.json:2:
"$schema": "http://json-schema.org/draft-03/schema#",
On 2013/05/23 16:56:18, kalman wrote:
> just curious but is there any particular reason this needs to end with a #?

Yes; thanks for asking 'cause I had forgotten about it :-)

Reason 1: forward compatibility. The latest spec defines well-known $schema
constants, and this is the constant for v3. See
http://json-schema.org/latest/json-schema-core.html#anchor22.

Reason 2: this value is a valid URL that can be used to resolve a schema that
describes the schema, used for hyper-schemas. If you open this URL you'll see
that it declares its ID with this constant.

[not at google - send to devlin, 2013-05-25 00:50:57]

a few more comments but lgtm

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/14587009/diff/96001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:316: "extension_types":
"all",
On 2013/05/24 21:31:49, Joao da Silva wrote:
> On 2013/05/23 16:56:18, kalman wrote:
> > ditto
> 
> Done.

incidentally do you really need entries for both? I would have thought you'd
only need one for "storage".

https://codereview.chromium.org/14587009/diff/96001/chrome/test/data/extensio...
File chrome/test/data/extensions/managed_extension/schema/managed_storage.json
(right):

https://codereview.chromium.org/14587009/diff/96001/chrome/test/data/extensio...
chrome/test/data/extensions/managed_extension/schema/managed_storage.json:2:
"$schema": "http://json-schema.org/draft-03/schema#",
On 2013/05/24 21:31:49, Joao da Silva wrote:
> On 2013/05/23 16:56:18, kalman wrote:
> > just curious but is there any particular reason this needs to end with a #?
> 
> Yes; thanks for asking 'cause I had forgotten about it :-)
> 
> Reason 1: forward compatibility. The latest spec defines well-known $schema
> constants, and this is the constant for v3. See
> http://json-schema.org/latest/json-schema-core.html#anchor22.
> 
> Reason 2: this value is a valid URL that can be used to resolve a schema that
> describes the schema, used for hyper-schemas. If you open this URL you'll see
> that it declares its ID with this constant.

It would still be a a valid URL, and in fact the equivalent URL, without the #
though. It's odd to me that it's there. But the site you point out says to use
it, so, cool.

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:34:
std::string* error) {
DCHECK(FILE)?

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h
(right):

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:25: //
is stored in |error|.
must be called on FILE thread?

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
File
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc
(right):

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:32:
Feature::SetCurrentChannel(chrome::VersionInfo::CHANNEL_DEV);
There is Feature::ScopedCurrentChannel for this.

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:39:
//storage_schema_manifest_handler_->Register();
hm? it registers itself? when does |handler_| get deleted?

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:48:
scoped_refptr<Extension> CreateExtension(const std::string& schema) {
Does chrome/common/extensions/extension_builder.h make any of this easier?

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:170:
"}")) << error_;
if you make Validates return a testing::AssertionResult then you can stream the
error to it automatically. e.g.

testing::AssertionResult Validates(...) {
  ...
  std::string error;
  return handler_->Validate(extension.get(), &error, ...) ?
      testing::AssertionSuccess() :
      testing::AssertionFailure() << error;
}

might need parens there somewhere.

[not at google - send to devlin, 2013-05-25 00:56:59]

Incidentally I realise I've been silly and we should have just been parsing the
whole of the "storage" manifest key, not just the specific "managed_schema" part
of.

This is what c/c/e/a/manifest_types.json is for.

However, we can change it when it becomes an issue (i.e. when we add more
schemas or more metadata to storage, which I think is likely to happen at some
point).

[Joao da Silva, 2013-05-27 12:13:11]

Thanks for the comments. I'll leave the parsing of "storage.managed_schema" as
is for now then.

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc
(right):

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.cc:34:
std::string* error) {
On 2013/05/25 00:50:57, kalman wrote:
> DCHECK(FILE)?

The file operations (file_util::*) eventually hit a
"base::ThreadRestrictions::AssertIOAllowed();", which is sufficient. This
doesn't need the FILE thread exactly, just a thread that allows file IO (note
that this is actually called from the blocking pool in
managed_value_store_cache.cc)

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
File chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h
(right):

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler.h:25: //
is stored in |error|.
On 2013/05/25 00:50:57, kalman wrote:
> must be called on FILE thread?

Done.

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
File
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc
(right):

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:32:
Feature::SetCurrentChannel(chrome::VersionInfo::CHANNEL_DEV);
On 2013/05/25 00:50:57, kalman wrote:
> There is Feature::ScopedCurrentChannel for this.

Nice, done.

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:39:
//storage_schema_manifest_handler_->Register();
On 2013/05/25 00:50:57, kalman wrote:
> hm? it registers itself? when does |handler_| get deleted?

Ah, this is a leftover from a version that used a ManifestHandlerRegistry, but
then it was greatly simplified. Removed, and made the handler_ a member value.

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:48:
scoped_refptr<Extension> CreateExtension(const std::string& schema) {
On 2013/05/25 00:50:57, kalman wrote:
> Does chrome/common/extensions/extension_builder.h make any of this easier?

Yes, using that now.

https://codereview.chromium.org/14587009/diff/114002/chrome/browser/extension...
chrome/browser/extensions/api/storage/storage_schema_manifest_handler_unittest.cc:170:
"}")) << error_;
On 2013/05/25 00:50:57, kalman wrote:
> if you make Validates return a testing::AssertionResult then you can stream
the
> error to it automatically. e.g.
> 
> testing::AssertionResult Validates(...) {
>   ...
>   std::string error;
>   return handler_->Validate(extension.get(), &error, ...) ?
>       testing::AssertionSuccess() :
>       testing::AssertionFailure() << error;
> }
> 
> might need parens there somewhere.

Nice, done. Going to use this pattern more often, thanks!

[commit-bot: I haz the power, 2013-05-27 15:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/14587009/132001

[commit-bot: I haz the power, 2013-05-27 16:37:23]

Change committed as 202445