Migrate Declarative API bindings to new JSON objects generated by JSON compiler

Migrate Declarative API bindings to new JSON objects generated by JSON compiler

This CL mirgrates the Declarative API to use the objects generated by the new JSON compiler and allows using multiple threads.

BUG=112155
TEST=no


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

[battre, 2012-02-17 17:54:35]

Hi.

I am working on this CL and get a linker error:

../../third_party/gold/gold64:
obj/chrome/common/extensions/api/libapi.a(obj/chrome/common/extensions/api/gen/chrome/common/extensions/api/api.experimental.declarative.o):
in function void
json_schema_compiler::util::PopulateListFromArray<linked_ptr<base::DictionaryValue>
>(std::vector<linked_ptr<base::DictionaryValue>,
std::allocator<linked_ptr<base::DictionaryValue> > > const&,
base::ListValue*):../../tools/json_schema_compiler/util.h:138: error: undefined
reference to
'json_schema_compiler::util::AddItemToList(linked_ptr<base::DictionaryValue>
const&, base::ListValue*)'

chrome/src$ ar t out/Debug/obj/chrome/common/extensions/api/libapi.a 
out/Debug/obj/chrome/common/extensions/api/gen/chrome/common/extensions/api/api.experimental.declarative.o
out/Debug/obj/chrome/common/extensions/api/gen/chrome/common/extensions/api/api.permissions.o
out/Debug/obj/chrome/common/extensions/api/gen/chrome/common/extensions/api/api.tabs.o
out/Debug/obj/chrome/common/extensions/api/gen/chrome/common/extensions/api/api.windows.o

Could you have a look at this please? The problem seems to be that
tools/json_schema_compiler/util.cc is not linked into libapi.a.

Thanks,
Dominic

[not at google - send to devlin, 2012-02-18 02:59:01]

Just had a browse of the CL, awesome that we're using the schema compiler again.
Chris and I will have a look at the link problems next week.

Re the change, it seems to add thread stuff as well so perhaps you were just
sending it to me as an illustration of the problem?  But the Rules
transformation stuff looks good.

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:43:
EXTENSION_FUNCTION_VALIDATE(!args_->GetString(0, &event_name));
Shame to have to do this, I suppose you have no choice given the thread
complications.

I think the check was right before though as VALDATE(args) not VALIDATE(!args)?

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:51: }
This (and other examples of the pattern) are the same as
EXTENSION_FUNCTION_VALIDATE(params.get())

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:53: if
(rules_registry_->GetOwnerThread() == content::BrowserThread::UI) {
I think this would be more clearly expressed as

content::BrowserThread::CurrentlyOn(rules_registry_->GetOwnerThread())

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_api_constants.h
(right):

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api_constants.h:15: extern
const char kInvalidEventName[];
Is this only used in declarative_api.cc? If so, you should be able to move it
there then delete this whole file.

[calamity, 2012-02-20 00:56:37]

On 2012/02/18 02:59:01, kalman wrote:
> Just had a browse of the CL, awesome that we're using the schema compiler
again.
> Chris and I will have a look at the link problems next week.
> 
> Re the change, it seems to add thread stuff as well so perhaps you were just
> sending it to me as an illustration of the problem?  But the Rules
> transformation stuff looks good.
> 
>
https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
> File chrome/browser/extensions/api/declarative/declarative_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
> chrome/browser/extensions/api/declarative/declarative_api.cc:43:
> EXTENSION_FUNCTION_VALIDATE(!args_->GetString(0, &event_name));
> Shame to have to do this, I suppose you have no choice given the thread
> complications.
> 
> I think the check was right before though as VALDATE(args) not
VALIDATE(!args)?
> 
>
https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
> chrome/browser/extensions/api/declarative/declarative_api.cc:51: }
> This (and other examples of the pattern) are the same as
> EXTENSION_FUNCTION_VALIDATE(params.get())
> 
>
https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
> chrome/browser/extensions/api/declarative/declarative_api.cc:53: if
> (rules_registry_->GetOwnerThread() == content::BrowserThread::UI) {
> I think this would be more clearly expressed as
> 
> content::BrowserThread::CurrentlyOn(rules_registry_->GetOwnerThread())
> 
>
https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
> File chrome/browser/extensions/api/declarative/declarative_api_constants.h
> (right):
> 
>
https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
> chrome/browser/extensions/api/declarative/declarative_api_constants.h:15:
extern
> const char kInvalidEventName[];
> Is this only used in declarative_api.cc? If so, you should be able to move it
> there then delete this whole file.

The linker problem has been fixed. It was an oversight while changing things to
const refs. Sorry about that.

http://www.crrev.com/122725

[battre, 2012-02-20 10:17:36]

I have addressed your comments. Thank you.

Could you please have a look at the modification necessary to chrome_tests.gypi?

Thanks,
Dominic

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:43:
EXTENSION_FUNCTION_VALIDATE(!args_->GetString(0, &event_name));
On 2012/02/18 02:59:01, kalman wrote:
> Shame to have to do this, I suppose you have no choice given the thread
> complications.
> 
> I think the check was right before though as VALDATE(args) not
VALIDATE(!args)?

Obviously. 

Done.

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:51: }
On 2012/02/18 02:59:01, kalman wrote:
> This (and other examples of the pattern) are the same as
> EXTENSION_FUNCTION_VALIDATE(params.get())

not quite. EXTENSION_FUNCTION_VALIDATE sets bad_message_ = true.

OTOH, if somebody sends us a request for a non-existing rules registry, that
should not happen, so let's change this.

Done.

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:53: if
(rules_registry_->GetOwnerThread() == content::BrowserThread::UI) {
On 2012/02/18 02:59:01, kalman wrote:
> I think this would be more clearly expressed as
> 
> content::BrowserThread::CurrentlyOn(rules_registry_->GetOwnerThread())

Done.

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_api_constants.h
(right):

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api_constants.h:15: extern
const char kInvalidEventName[];
On 2012/02/18 02:59:01, kalman wrote:
> Is this only used in declarative_api.cc? If so, you should be able to move it
> there then delete this whole file.

Done.

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/chrome_tests....
chrome/chrome_tests.gypi:2470: 'common/extensions/api/api.gyp:api',
Why is this necessary? should this not be inherited from browser?

Without this, the compiler does not find the generated include files:

../../chrome/browser/extensions/api/declarative/rules_registry.h:12:67: error:
chrome/common/extensions/api/experimental.declarative.h: No such file or
directory

[battre, 2012-02-20 10:18:39]

Hi Matt,

could you please review this?

Thanks,
Dominic

[not at google - send to devlin, 2012-02-20 11:50:51]

lgtm

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://chromiumcodereview.appspot.com/9422003/diff/2002/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:51: }
On 2012/02/20 10:17:36, battre wrote:
> On 2012/02/18 02:59:01, kalman wrote:
> > This (and other examples of the pattern) are the same as
> > EXTENSION_FUNCTION_VALIDATE(params.get())
> 
> not quite. EXTENSION_FUNCTION_VALIDATE sets bad_message_ = true.
> 
> OTOH, if somebody sends us a request for a non-existing rules registry, that
> should not happen, so let's change this.
> 
> Done.

Ah crap I think I put this comment on the wrong blob. I meant to put it on like
line 66/79/94 etc, such that

scoped_ptr<AddRules::Params> params(...);
if (!params.get()) {
  bad_message_ = true;
  return;
}

becomes

scoped_ptr<AddRules::Params> params(...);
EXTENSION_FUNCTION_VALIDATE(params.get());

and same for the other examples of that.

That said -- in this case you're right too, particularly now that the
declarative API is only available internally, there should never be a request
for a nonexistent rules registry. It might be nice to mention this in a comment.

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/chrome_tests....
chrome/chrome_tests.gypi:2470: 'common/extensions/api/api.gyp:api',
On 2012/02/20 10:17:36, battre wrote:
> Why is this necessary? should this not be inherited from browser?
> 
> Without this, the compiler does not find the generated include files:
> 
> ../../chrome/browser/extensions/api/declarative/rules_registry.h:12:67: error:
> chrome/common/extensions/api/experimental.declarative.h: No such file or
> directory

Hm. Damn, we will need to look into that, but it's probably to do with the way
that include paths are set up... the .h files get generated to some intermediate
build directory and possibly that's only visible during the "chrome" compile
stage, so it doesn't get shared.

It actually looks like protobuf compilation has the same problem (2 lines
above), which is interesting because it's what we based the gyp stuff on.

Chris, any idea?

https://chromiumcodereview.appspot.com/9422003/diff/9001/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://chromiumcodereview.appspot.com/9422003/diff/9001/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:61:
SendResponse(error_.empty() && !bad_message_);
I think that you don't need to check for bad_message_ since SendResponseImpl
(extension_function.cc) kills the renderer anyway if it's true.

https://chromiumcodereview.appspot.com/9422003/diff/9001/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://chromiumcodereview.appspot.com/9422003/diff/9001/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:56:
std::vector<std::string> rule_identifiers;  // Empty to get all rules.
Just noticed this... comment is inaccurate (post last review(s)) I think.

[Matt Perry, 2012-02-21 23:43:41]

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/browser/exten...
chrome/browser/extensions/api/declarative/declarative_api.cc:47: if
(content::BrowserThread::CurrentlyOn(rules_registry_->GetOwnerThread())) {
If rules_registry_ is owned by a different thread, it probably isn't safe to
call methods on it. Especially since the implementation calls through to a
delegate.

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/browser/exten...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://chromiumcodereview.appspot.com/9422003/diff/4001/chrome/browser/exten...
chrome/browser/extensions/api/declarative/rules_registry.h:24: typedef
extensions::api::experimental_declarative::Rule Rule;
I believe "using extensions::api::experimental_declarative::Rule" would be more
idiomatic here (not 100% it works).

[battre, 2012-02-28 22:29:17]

I have addressed your comments and added unit tests that I promised to add once
everything is migrated to the new JSON objects.

In order to reliably post the OnExtensionUnloaded from RulesRegistryService to
RulesRegistry instances on the correct thread, I needed to make the
RulesRegistry RefCountedThreadSafe.

I think the threading should be safe now.

Bear with me if clang fails. It is pretty late and I don't want to wait for the
try bots.

Best regards,
Dominic

http://codereview.chromium.org/9422003/diff/2002/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9422003/diff/2002/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative/declarative_api.cc:51: }
On 2012/02/20 11:50:51, kalman wrote:
> On 2012/02/20 10:17:36, battre wrote:
> > On 2012/02/18 02:59:01, kalman wrote:
> > > This (and other examples of the pattern) are the same as
> > > EXTENSION_FUNCTION_VALIDATE(params.get())
> > 
> > not quite. EXTENSION_FUNCTION_VALIDATE sets bad_message_ = true.
> > 
> > OTOH, if somebody sends us a request for a non-existing rules registry, that
> > should not happen, so let's change this.
> > 
> > Done.
> 
> Ah crap I think I put this comment on the wrong blob. I meant to put it on
like
> line 66/79/94 etc, such that
> 
> scoped_ptr<AddRules::Params> params(...);
> if (!params.get()) {
>   bad_message_ = true;
>   return;
> }
> 
> becomes
> 
> scoped_ptr<AddRules::Params> params(...);
> EXTENSION_FUNCTION_VALIDATE(params.get());
> 
> and same for the other examples of that.

I wanted to do this change after landing http://codereview.chromium.org/9416060/
because PostTaskAndReply cannot rely the result of one function to another.
Ideally, this function would become some thing like this:

PostTaskAndReplyWithStatus(
    message_loop_proxy, FROM_HERE,
    base::Bind(&RulesFunction::RunImplOnCorrectThread, this),
    base::Bind(&RulesFunction::SendResponse, this),
    new bool);

I.e. we can get rid of SendResponseOnUIThread() entirely and pass the boolean
return value of RunImplOnCorrectThread to SendResponse.

Anyway, I don't see harm in letting RunImplOnCorrectThread return a boolean and
ignoring it for now.

Done.

> That said -- in this case you're right too, particularly now that the
> declarative API is only available internally, there should never be a request
> for a nonexistent rules registry. It might be nice to mention this in a
comment.

Done.

http://codereview.chromium.org/9422003/diff/4001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9422003/diff/4001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative/declarative_api.cc:47: if
(content::BrowserThread::CurrentlyOn(rules_registry_->GetOwnerThread())) {
On 2012/02/21 23:43:42, Matt Perry wrote:
> If rules_registry_ is owned by a different thread, it probably isn't safe to
> call methods on it. Especially since the implementation calls through to a
> delegate. 

I have added a comment to the declaration of GetOwnerThread, that calling this
function must be safe from any thread. This should be the case for a simple {
return content::BrowserThread::UI; } style function.

I had to make the RulesRegistries RefCountedThreadSafe, though. Otherwise, there
is no safe way of posting a the OnExtensionUnloaded message from
RulesRegistryService (which owns the RulesRegiestries) to the RulesRegistries.

http://codereview.chromium.org/9422003/diff/4001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

http://codereview.chromium.org/9422003/diff/4001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative/rules_registry.h:24: typedef
extensions::api::experimental_declarative::Rule Rule;
On 2012/02/21 23:43:42, Matt Perry wrote:
> I believe "using extensions::api::experimental_declarative::Rule" would be
more
> idiomatic here (not 100% it works).

This does not work.

http://codereview.chromium.org/9422003/diff/9001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9422003/diff/9001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative/declarative_api.cc:61:
SendResponse(error_.empty() && !bad_message_);
On 2012/02/20 11:50:51, kalman wrote:
> I think that you don't need to check for bad_message_ since SendResponseImpl
> (extension_function.cc) kills the renderer anyway if it's true.

Done. - My goal is to get rid of SendResponseOnUIThread anyway (see other
comment about passing the return value of RunImplOnCorrectThread to
SendResponse).

http://codereview.chromium.org/9422003/diff/9001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

http://codereview.chromium.org/9422003/diff/9001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:56:
std::vector<std::string> rule_identifiers;  // Empty to get all rules.
On 2012/02/20 11:50:51, kalman wrote:
> Just noticed this... comment is inaccurate (post last review(s)) I think.

Done.

[Matt Perry, 2012-02-28 23:01:52]

lgtm

http://codereview.chromium.org/9422003/diff/4001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

http://codereview.chromium.org/9422003/diff/4001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/declarative/declarative_api.cc:47: if
(content::BrowserThread::CurrentlyOn(rules_registry_->GetOwnerThread())) {
On 2012/02/28 22:29:18, battre wrote:
> On 2012/02/21 23:43:42, Matt Perry wrote:
> > If rules_registry_ is owned by a different thread, it probably isn't safe to
> > call methods on it. Especially since the implementation calls through to a
> > delegate. 
> 
> I have added a comment to the declaration of GetOwnerThread, that calling this
> function must be safe from any thread. This should be the case for a simple {
> return content::BrowserThread::UI; } style function.

I guess that works, as long as:
- RulesRegistry and its delegate are guaranteed to outlive the scoped of this
function.
- InitializingRulesRegistry's delegate never changes.

[commit-bot: I haz the power, 2012-02-29 19:06:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/9422003/30001

[commit-bot: I haz the power, 2012-02-29 21:05:05]

Change committed as 124249