Persist declarative rules to the extension state store.

Persist declarative rules to the extension state store.

BUG=123366
TEST=no

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

[Matt Perry, 2012-06-15 21:54:41]

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
(right):

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:175:
ExtensionAPIPermission::kDeclarativeWebRequest)) {
Is there a better way to check if an extension is using declarative rules? I
tried checking for the declarative permission, but apparently
"declarativeWebRequest" does not imply "declarative".

[battre, 2012-06-15 22:56:49]

LGTM if you are sure that the cases mentions are no race conditions. I think
they might be. I can have another look when it is not so late.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
(right):

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:25:
return scoped_ptr<base::Value>(list.release());
nit: how about
return list.PassAs<base::Value>();
?

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:83:
// RulesRegistry if so.
add comment how this notification happens?

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:126:
const std::string& storage_key) {
nit: indent

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:133:
return inner_->ready_;
this contradicts the comment "Methods/variables should be used on the UI thread
unless otherwise noted."

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:151:
const std::string& storage_key)
nit: indent

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:175:
ExtensionAPIPermission::kDeclarativeWebRequest)) {
On 2012/06/15 21:54:41, Matt Perry wrote:
> Is there a better way to check if an extension is using declarative rules? I
> tried checking for the declarative permission, but apparently
> "declarativeWebRequest" does not imply "declarative".

"declarative" (and ExtensionAPIPermission::kDeclarative) is an artifact that I
forgot to delete when I migrated the declarative API definition into the
chrome.Events namespace. We could revive it for the purpose you mention.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:178:
base::Bind(&Inner::ReadFromStorage, this, extension->id()));
What do you think of moving 177-178 into a function
void ReadFromStorage(const std::string& extension_id)
and renaming 185 to ReadFromStorageCallback(...)?

I found it confusing that ReadFromStorage would consume a
scoped_ptr<base::Value>.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:210:
if (ready_)
access to ready_ on UI thread

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:211:
return;  // we've already notified our readiness
This behavior (call callback only once) is not reflected in the method comment.

I think there is a race condition possible if CheckIfReady() is called twice in
short sequence before the NotifyReadyOnRegistryThread task can be executed?

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:229:
ready_ = true;
access to ready_ on rules_registry_thread_

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
(right):

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc:154:
DCHECK_EQ(kSuccess, error);
should we pass |rules| to OnRulesChanged()? OnRulesChanged() does the same call
again. Alternatively, drop lines 152-154.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1276: extension_id,
request_id, block_time);
should we add a UMA statistic to measure the network delays at browser start
here? I am not sure how much this would tell us though as everything is slow
during browser startup. So if the UMA statistic tells us 10s, we don't know
whether the request had be answered instantly, if there was no need to load
declarative webrequest rules.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1413: if
(!rules_registry_->IsReady()) {
nice! the architecture is really flexible.

[Matt Perry, 2012-06-18 21:01:01]

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc
(right):

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:25:
return scoped_ptr<base::Value>(list.release());
On 2012/06/15 22:56:49, battre wrote:
> nit: how about
> return list.PassAs<base::Value>();
> ?

Cool, thanks!

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:83:
// RulesRegistry if so.
On 2012/06/15 22:56:49, battre wrote:
> add comment how this notification happens?

I'm not sure I understand what you mean. I added a note that the notification
happens on the RulesRegistry thread.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:126:
const std::string& storage_key) {
On 2012/06/15 22:56:49, battre wrote:
> nit: indent

Done.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:133:
return inner_->ready_;
On 2012/06/15 22:56:49, battre wrote:
> this contradicts the comment "Methods/variables should be used on the UI
thread
> unless otherwise noted."

Are you referring to ready_? ready_'s definition follows the comment "The
following are only accessible on rules_registry_thread_."

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:151:
const std::string& storage_key)
On 2012/06/15 22:56:49, battre wrote:
> nit: indent

Done.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:175:
ExtensionAPIPermission::kDeclarativeWebRequest)) {
On 2012/06/15 22:56:49, battre wrote:
> On 2012/06/15 21:54:41, Matt Perry wrote:
> > Is there a better way to check if an extension is using declarative rules? I
> > tried checking for the declarative permission, but apparently
> > "declarativeWebRequest" does not imply "declarative".
> 
> "declarative" (and ExtensionAPIPermission::kDeclarative) is an artifact that I
> forgot to delete when I migrated the declarative API definition into the
> chrome.Events namespace. We could revive it for the purpose you mention.

I'll leave as-is for now but add a TODO.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:178:
base::Bind(&Inner::ReadFromStorage, this, extension->id()));
On 2012/06/15 22:56:49, battre wrote:
> What do you think of moving 177-178 into a function
> void ReadFromStorage(const std::string& extension_id)
> and renaming 185 to ReadFromStorageCallback(...)?
> 
> I found it confusing that ReadFromStorage would consume a
> scoped_ptr<base::Value>.

Done.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:210:
if (ready_)
On 2012/06/15 22:56:49, battre wrote:
> access to ready_ on UI thread

Doh! Good catch.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:211:
return;  // we've already notified our readiness
On 2012/06/15 22:56:49, battre wrote:
> This behavior (call callback only once) is not reflected in the method
comment.

Added comments to CheckIfReady and OnReady.

> I think there is a race condition possible if CheckIfReady() is called twice
in
> short sequence before the NotifyReadyOnRegistryThread task can be executed?

Good catch - moving the ready_ check to the RulesRegistry thread fixes this.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_storage_delegate.cc:229:
ready_ = true;
On 2012/06/15 22:56:49, battre wrote:
> access to ready_ on rules_registry_thread_

That is the intended thread.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
(right):

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc:154:
DCHECK_EQ(kSuccess, error);
On 2012/06/15 22:56:49, battre wrote:
> should we pass |rules| to OnRulesChanged()? OnRulesChanged() does the same
call
> again. Alternatively, drop lines 152-154.

Oops. I originally did do that, but decided it would be better for the delegate
to decide whether or not to fetch the rules. I've dropped these lines.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1276: extension_id,
request_id, block_time);
On 2012/06/15 22:56:49, battre wrote:
> should we add a UMA statistic to measure the network delays at browser start
> here? I am not sure how much this would tell us though as everything is slow
> during browser startup. So if the UMA statistic tells us 10s, we don't know
> whether the request had be answered instantly, if there was no need to load
> declarative webrequest rules.

Done.

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1413: if
(!rules_registry_->IsReady()) {
On 2012/06/15 22:56:49, battre wrote:
> nice! the architecture is really flexible.

Agreed, I was pleasantly surprised with how simple this was to hook up. Great
job with the rules API and and webRequest hooks!

http://codereview.chromium.org/10560013/diff/7001/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_api.cc:1421:
blocked_requests_[request->identifier()].num_handlers_blocking++;
BTW I had to add more initialization of blocked_requests_ here to mimic what
DispatchEvent does.

[battre, 2012-06-18 21:33:42]

Great, LGTM

[battre, 2012-06-19 05:51:08]

There is one more thing that came to my mind: We persist now all rules, not only
those that were registered by vNext extensions. Is this desired?

[Matt Perry, 2012-06-19 18:35:59]

On 2012/06/19 05:51:08, battre wrote:
> There is one more thing that came to my mind: We persist now all rules, not
only
> those that were registered by vNext extensions. Is this desired?

I don't understand the distinction. I thought only vNext extensions have access
to the rules API.

[battre, 2012-06-19 18:46:13]

On 2012/06/19 18:35:59, Matt Perry wrote:
> On 2012/06/19 05:51:08, battre wrote:
> > There is one more thing that came to my mind: We persist now all rules, not
> only
> > those that were registered by vNext extensions. Is this desired?
> 
> I don't understand the distinction. I thought only vNext extensions have
access
> to the rules API.

Ok, I did not know what we limit the access to vNext extensions. From a
technological point of view there is nothing that prevents a background page to
set up declarative rules. That is how our current browser tests work.

[Matt Perry, 2012-06-19 18:48:12]

On 2012/06/19 18:46:13, battre wrote:
> On 2012/06/19 18:35:59, Matt Perry wrote:
> > On 2012/06/19 05:51:08, battre wrote:
> > > There is one more thing that came to my mind: We persist now all rules,
not
> > only
> > > those that were registered by vNext extensions. Is this desired?
> > 
> > I don't understand the distinction. I thought only vNext extensions have
> access
> > to the rules API.
> 
> Ok, I did not know what we limit the access to vNext extensions. From a
> technological point of view there is nothing that prevents a background page
to
> set up declarative rules. That is how our current browser tests work.

Do those rules do anything? Doesn't the extension need access to the
declarativeWebRequest API?

[battre, 2012-06-19 18:50:36]

On 2012/06/19 18:48:12, Matt Perry wrote:
> On 2012/06/19 18:46:13, battre wrote:
> > On 2012/06/19 18:35:59, Matt Perry wrote:
> > > On 2012/06/19 05:51:08, battre wrote:
> > > > There is one more thing that came to my mind: We persist now all rules,
> not
> > > only
> > > > those that were registered by vNext extensions. Is this desired?
> > > 
> > > I don't understand the distinction. I thought only vNext extensions have
> > access
> > > to the rules API.
> > 
> > Ok, I did not know what we limit the access to vNext extensions. From a
> > technological point of view there is nothing that prevents a background page
> to
> > set up declarative rules. That is how our current browser tests work.
> 
> Do those rules do anything? Doesn't the extension need access to the
> declarativeWebRequest API?

Yes, the extension needs permission to use the declarative web request API.

[Matt Perry, 2012-06-19 18:53:37]

On 2012/06/19 18:50:36, battre wrote:
> On 2012/06/19 18:48:12, Matt Perry wrote:
> > On 2012/06/19 18:46:13, battre wrote:
> > > On 2012/06/19 18:35:59, Matt Perry wrote:
> > > > On 2012/06/19 05:51:08, battre wrote:
> > > > > There is one more thing that came to my mind: We persist now all
rules,
> > not
> > > > only
> > > > > those that were registered by vNext extensions. Is this desired?
> > > > 
> > > > I don't understand the distinction. I thought only vNext extensions have
> > > access
> > > > to the rules API.
> > > 
> > > Ok, I did not know what we limit the access to vNext extensions. From a
> > > technological point of view there is nothing that prevents a background
page
> > to
> > > set up declarative rules. That is how our current browser tests work.
> > 
> > Do those rules do anything? Doesn't the extension need access to the
> > declarativeWebRequest API?
> 
> Yes, the extension needs permission to use the declarative web request API.

OK, and the declarativeWebRequest permission is restricted to dev, so we should
be OK until we release vNext. Right?

[battre, 2012-06-19 18:56:45]

On 2012/06/19 18:53:37, Matt Perry wrote:
> On 2012/06/19 18:50:36, battre wrote:
> > On 2012/06/19 18:48:12, Matt Perry wrote:
> > > On 2012/06/19 18:46:13, battre wrote:
> > > > On 2012/06/19 18:35:59, Matt Perry wrote:
> > > > > On 2012/06/19 05:51:08, battre wrote:
> > > > > > There is one more thing that came to my mind: We persist now all
> rules,
> > > not
> > > > > only
> > > > > > those that were registered by vNext extensions. Is this desired?
> > > > > 
> > > > > I don't understand the distinction. I thought only vNext extensions
have
> > > > access
> > > > > to the rules API.
> > > > 
> > > > Ok, I did not know what we limit the access to vNext extensions. From a
> > > > technological point of view there is nothing that prevents a background
> page
> > > to
> > > > set up declarative rules. That is how our current browser tests work.
> > > 
> > > Do those rules do anything? Doesn't the extension need access to the
> > > declarativeWebRequest API?
> > 
> > Yes, the extension needs permission to use the declarative web request API.
> 
> OK, and the declarativeWebRequest permission is restricted to dev, so we
should
> be OK until we release vNext. Right?

Yes.

[battre, 2012-06-26 07:59:55]

http://codereview.chromium.org/10560013/diff/21003/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

http://codereview.chromium.org/10560013/diff/21003/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.cc:1425:
request->identifier(), response));
I think this is actually incorrect. We need to decrement the block count but we
also need to process the declarative rules.