Declarative rules should be removed on uninstalling, not unloading

Declarative rules should be removed on uninstalling, not unloading

When they are removed on unloading, they are in particular removed on disabling an extension. Re-enabling the extension then does not restore these rules.

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

[vabr (Chromium), 2013-10-30 17:16:42]

Hi Jeffrey,

It'd be an honour to have your review for this.

However, the Cl is missing a key part yet -- ignoring of rules which are part of
disabled extensions.

For WebRequest rules, the current state works, because
WebRequestRulesRegistry::CreateDeltas uses WebRequestAction::Apply to complete
the deltas, and Apply ignores disabled extensions through a call to
HasPermission.
However, for Declarative Content rules this does not work, as far as I can tell
from the code, and in any case, the above lucky co-incidence is not enough,
especially if new declarative APIs are to be added. It's also not very efficient
(catching the rules of disabled extensions rather late).

Do you have an opinion on where would be a good place to filter out rules from
disabled extensions before applying them? Preferably at a place common to all
declarative APIs.

The only thing coming to my mind would be to restore the OnUnload hook to the
registries, and let them sort out disabling the rules themselves. This would
also mean we need to add an OnLoad hook to counter that.

Do you see something better?

Cheers,
Vaclav

[Jeffrey Yasskin, 2013-10-31 00:54:17]

On 2013/10/30 17:16:42, vabr (Chromium) wrote:
> However, the Cl is missing a key part yet -- ignoring of rules which are part
of
> disabled extensions.
> 
> For WebRequest rules, the current state works, because
> WebRequestRulesRegistry::CreateDeltas uses WebRequestAction::Apply to complete
> the deltas, and Apply ignores disabled extensions through a call to
> HasPermission.
> However, for Declarative Content rules this does not work, as far as I can
tell
> from the code, and in any case, the above lucky co-incidence is not enough,
> especially if new declarative APIs are to be added. It's also not very
efficient
> (catching the rules of disabled extensions rather late).

D'oh.

> Do you have an opinion on where would be a good place to filter out rules from
> disabled extensions before applying them? Preferably at a place common to all
> declarative APIs.

I tend to think that this should happen at the cache layer. So, the
RulesRegistryWithCache (or RulesRegistry once Fady's change goes in) would store
all the rules from creation to uninstall, but would only call AddRulesImpl on
startup if the extension is enabled (and when the extension becomes enabled).
Similarly, it would call RemoveAllRulesImpl() when the extension is disabled.

Does that sounds reasonable?

https://codereview.chromium.org/52743002/diff/120001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/52743002/diff/120001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:142:
content::Details<UnloadedExtensionInfo>(details)->extension;
This'll need to change.

[vabr (Chromium), 2013-10-31 17:13:01]

On 2013/10/31 00:54:17, Jeffrey Yasskin wrote:
> On 2013/10/30 17:16:42, vabr (Chromium) wrote:
> > However, the Cl is missing a key part yet -- ignoring of rules which are
part
> of
> > disabled extensions.
> > 
> > For WebRequest rules, the current state works, because
> > WebRequestRulesRegistry::CreateDeltas uses WebRequestAction::Apply to
complete
> > the deltas, and Apply ignores disabled extensions through a call to
> > HasPermission.
> > However, for Declarative Content rules this does not work, as far as I can
> tell
> > from the code, and in any case, the above lucky co-incidence is not enough,
> > especially if new declarative APIs are to be added. It's also not very
> efficient
> > (catching the rules of disabled extensions rather late).
> 
> D'oh.
> 
> > Do you have an opinion on where would be a good place to filter out rules
from
> > disabled extensions before applying them? Preferably at a place common to
all
> > declarative APIs.
> 
> I tend to think that this should happen at the cache layer. So, the
> RulesRegistryWithCache (or RulesRegistry once Fady's change goes in) would
store
> all the rules from creation to uninstall, but would only call AddRulesImpl on
> startup if the extension is enabled (and when the extension becomes enabled).
> Similarly, it would call RemoveAllRulesImpl() when the extension is disabled.
> 
> Does that sounds reasonable?
Thanks, Jeffrey, makes perfect sense to me.
Tomorrow is a holiday in Munich, so I'll get back to it next week. Will update
this CL with a new patch then.

Thanks,
Vaclav

> 
>
https://codereview.chromium.org/52743002/diff/120001/chrome/browser/extension...
> File chrome/browser/extensions/api/declarative/rules_registry_service.cc
> (right):
> 
>
https://codereview.chromium.org/52743002/diff/120001/chrome/browser/extension...
> chrome/browser/extensions/api/declarative/rules_registry_service.cc:142:
> content::Details<UnloadedExtensionInfo>(details)->extension;
> This'll need to change.

[vabr (Chromium), 2013-11-06 16:00:47]

Hi Jeffrey,

I'd be grateful for your opinion on the following:

RulesRegistry needs to distinguish between install/uninstall a enable/disable
changes for extensions.
For install, it needs to perform AddRules, including AddRulesImpl, and similarly
for uninstall and RemoveRules.
For enable it only needs to perform AddRulesImpl, and similarly RemoveRulesImpl
for disable.

In terms of NOTIFICATION_EXTENSION_INSTALLED,
NOTIFICATION_EXTENSION_UNINSTALLED, NOTIFICATION_EXTENSION_LOADED, and
NOTIFICATION_EXTENSION_UNLOADED events (I'll drop the common prefix below), the
following happens:
on install: INSTALLED, then LOADED
on uninstall: UNLOADED, then UNINSTALLED
on enabled: LOADED
on disabled: UNLOADED
So the most natural seems to be responding with AddRulesImpl to LOADED,
RemoveRulesImple to UNLOADED, AddRules (without calling *Impl) to INSTALLED and
RemoveRules (without calling *Impl) to UNINSTALLED.

However, there is a problem with reloading extensions.
ExtensionService::ReloadExtension causes this sequence: UNLOADED → INSTALLED →
LOADED. This misses UNINSTALLED, which would result in duplicating the rules in
RulesRegistry.
I tried to understand whether this omission is by design or not, but I could not
convince myself either way.

My favourite option so far would be erasing all rules stored for an extension in
RulesRegistry as soon as new are being saved due to the INSTALLED notification.
Obviously if ExtensionService::ReloadExtension should actually emit UNINSTALLED,
then my preferred solution changes to fixing that first.
I'm also open to any other suggestions, if you have any.

Thanks, and looking forward to hearing from you :)
Vaclav

[Jeffrey Yasskin, 2013-11-06 22:06:14]

On Wed, Nov 6, 2013 at 8:00 AM,  <vabr@chromium.org> wrote:
> Hi Jeffrey,
>
> I'd be grateful for your opinion on the following:
>
> RulesRegistry needs to distinguish between install/uninstall a
> enable/disable
> changes for extensions.
> For install, it needs to perform AddRules, including AddRulesImpl, and
> similarly
> for uninstall and RemoveRules.
> For enable it only needs to perform AddRulesImpl, and similarly
> RemoveRulesImpl
> for disable.
>
> In terms of NOTIFICATION_EXTENSION_INSTALLED,
> NOTIFICATION_EXTENSION_UNINSTALLED, NOTIFICATION_EXTENSION_LOADED, and
> NOTIFICATION_EXTENSION_UNLOADED events (I'll drop the common prefix below),
> the
> following happens:
> on install: INSTALLED, then LOADED
> on uninstall: UNLOADED, then UNINSTALLED
> on enabled: LOADED
> on disabled: UNLOADED
> So the most natural seems to be responding with AddRulesImpl to LOADED,
> RemoveRulesImple to UNLOADED,
> AddRules (without calling *Impl) to INSTALLED

Note that there aren't any rules to add on the INSTALLED event, since
the extension hasn't added them yet.

> and
> RemoveRules (without calling *Impl) to UNINSTALLED.

Sounds good.

> However, there is a problem with reloading extensions.
> ExtensionService::ReloadExtension causes this sequence: UNLOADED → INSTALLED
> →
> LOADED. This misses UNINSTALLED, which would result in duplicating the rules
> in
> RulesRegistry.
> I tried to understand whether this omission is by design or not, but I could
> not
> convince myself either way.

Argh. ReloadExtension is the bane of my existence. ReloadExtension
also causes DISABLE events sometimes, with DISABLE_RELOAD set
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext...),
and I think this lasts through the next install. Check
extension_prefs_->GetDisableReasons(extension_id) &
Extension::DISABLE_RELOAD.

> My favourite option so far would be erasing all rules stored for an
> extension in
> RulesRegistry as soon as new are being saved due to the INSTALLED
> notification.

I think this doesn't quite work because extension upgrades shouldn't
cause rules to get dropped, but I think they do cause the installed
notification.

> Obviously if ExtensionService::ReloadExtension should actually emit
> UNINSTALLED,
> then my preferred solution changes to fixing that first.
> I'm also open to any other suggestions, if you have any.
>
> Thanks, and looking forward to hearing from you :)
> Vaclav
>
>
> https://codereview.chromium.org/52743002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Extensions reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to extensions-reviews+unsubscribe@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/extensions-reviews/089e0158b....

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2013-11-07 18:23:07]

This patch is now stale after my landing some refactorings to RulesRegistry.

[vabr (Chromium), 2013-11-08 07:22:26]

On 2013/11/07 18:23:07, Fady Samuel wrote:
> This patch is now stale after my landing some refactorings to RulesRegistry.

Yeah, I was prepared for that, thanks for heads-up.
Will adapt to the new code and update this soon.

[vabr (Chromium), 2013-11-19 17:05:31]

Hi Jeffrey,

Once you have time (sorry for spamming you with reviews), could you PTAL?

The fix seems to work with manual testing.
The new browser test detects the failure (i.e., fails without the fix), and runs
fine with the fix. However, one piece is missing, I noted it in a comment in the
test. I will not submit this before that gets solved, of course.

Cheers,
Vaclav

https://codereview.chromium.org/52743002/diff/120001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/52743002/diff/120001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:142:
content::Details<UnloadedExtensionInfo>(details)->extension;
On 2013/10/31 00:54:18, Jeffrey Yasskin wrote:
> This'll need to change.

Good catch, thanks! Done.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:120:
//EXPECT_TRUE(UpdateExtension(extension_id, extension_path, kChange));
This part is not yet finished -- when I used UpdateExtension like this, with
|extension_path| pointing to the current extension's directory, the test never
finished. I guess the trouble was that the version in the manifest did not
increase.

When I created a copy of that extension with an increased version number in a
second directory, then UpdateExtension complained about changed extension ID.

I'd be grateful for any advice on this.

[Jeffrey Yasskin, 2013-11-20 05:55:28]

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:111:
EnableExtension(extension_id);
Could you additionally try ReloadExtension()? I'm not exactly sure what behavior
we want for that, but I definitely want to pin it down.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:120:
//EXPECT_TRUE(UpdateExtension(extension_id, extension_path, kChange));
On 2013/11/19 17:05:31, vabr (Chromium) wrote:
> This part is not yet finished -- when I used UpdateExtension like this, with
> |extension_path| pointing to the current extension's directory, the test never
> finished. I guess the trouble was that the version in the manifest did not
> increase.
> 
> When I created a copy of that extension with an increased version number in a
> second directory, then UpdateExtension complained about changed extension ID.
> 
> I'd be grateful for any advice on this.

Use
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...
to build the extension in code (possibly using base::ReadFileToString to pull
background.js from the filesystem). Then you can update its contents, and it'll
keep the same ID.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:125:
EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));
You're likely to wind up with 2 rules registered here because background.js
doesn't check
http://developer.chrome.com/extensions/runtime.html#property-details-reason in
onInstalled. I suspect background.js should only register the rule when
reason=='install', so that we can check the behavior here. For user code, I
recommend blindly replacing all the rules in onInstalled, but that would obscure
the details of the behavior we want here.

You could even upgrade the extension to a version that doesn't register rules at
all, to be really sure that changes in the count are a result of the Chrome
logic rather than extension logic.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.cc:232:
CommitRemoveAllRules(extension_id);
Comment that we don't need to call RemoveAllRulesImpl because
OnExtensionUnloaded is guaranteed to be called before OnExtensionUninstalled. Or
just forward to RemoveAllRules() to be clearly safe, since removing an empty set
of rules should be pretty cheap.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:219: // storage (as
opposed to the storage of the inheriting registries, handled by
This isn't clear. Maybe "These functions remove rules from RulesRegistry
persistent storage. This contrasts to Remove[All]RulesImpl, which prevents the
rules from affecting Chrome's behavior."

Consider renaming AddRulesImpl to ActivateRules and Remove[All]RulesImpl to
Deactivate[All]Rules. Then CommitRemove[All]Rules could become Delete[All]Rules,
making the distinction somewhat easier to explain.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:205: void
RulesRegistryService::OnExtensionUninstalled(
I would consider inlining these 1-line functions that are each called from 1
place. The only exception is OnExtensionUninstalled, which is called from 2, but
I think that's few enough to make inlining it worthwhile anyway.

[vabr (Chromium), 2013-11-20 17:46:07]

Hi Jeffrey,

Thanks for your comments, I addressed them all (I hope!).

Then I discovered one more problem:
The RulesRegistryDelegate was listening to NOTIFICATION_EXTENSION_LOADED and
triggered AddRules for the rules stored in the rules store.
I believe this collides badly with the RulesRegistry now listening to this event
as well, so I removed the observing part from the delegate.
This assumes that on the browser start, before the delegate is constructed, the
extension service already knows about all extensions installed before the last
shutdown, so that these extensions are all processed by
RulesCacheDelegate::ReadRulesForInstalledExtensions. To me it looks like it
should be the case, otherwise the code looks broken already.

There is also a problem with the RenderViewHost for the background page of the
testing extension hanging during UpdateExtension.
More precisely, UpdateExtension ends with calling
ExtensionTestNotificationObserver::WaitForExtensionViewsToLoad to wait until all
RenderViewHosts associated with extensions return false on IsLoading. This
apparently does not happen.
When I forcibly pass no rules to WebRequestRulesRegistry::AddRulesImpl in
RulesRegistry::OnExtensionLoaded, then IsLoading is no more blocking by
returning true, and the test finishes (but obviously failing the expectations).
In more detail, the section of  WebRequestRulesRegistry::AddRulesImpl which is
the culprit is the block updating url_matcher_ with new condition sets.
I have yet to discover what is the connection between the URL matcher and the
incomplete loading of the extension's RenderViewHost. If you had any hypotheses
about the RenderViewHost not loading, please let me know :).

Also, since both the preceding CLs are already committed, this CL is now based
off ToT, so the patch should be locally applicable, if you wanted to try it out.
(I'm not expecting you to do so :), just saying.)

Cheers,
Vaclav

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:111:
EnableExtension(extension_id);
On 2013/11/20 05:55:28, Jeffrey Yasskin wrote:
> Could you additionally try ReloadExtension()? I'm not exactly sure what
behavior
> we want for that, but I definitely want to pin it down.

I added ReloadExtension to the test, with the expectation that ReloadExtension
will not change the number of active rules for the tested extension.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:120:
//EXPECT_TRUE(UpdateExtension(extension_id, extension_path, kChange));
On 2013/11/20 05:55:28, Jeffrey Yasskin wrote:
> On 2013/11/19 17:05:31, vabr (Chromium) wrote:
> > This part is not yet finished -- when I used UpdateExtension like this, with
> > |extension_path| pointing to the current extension's directory, the test
never
> > finished. I guess the trouble was that the version in the manifest did not
> > increase.
> > 
> > When I created a copy of that extension with an increased version number in
a
> > second directory, then UpdateExtension complained about changed extension
ID.
> > 
> > I'd be grateful for any advice on this.
> 
> Use
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...
> to build the extension in code (possibly using base::ReadFileToString to pull
> background.js from the filesystem). Then you can update its contents, and
it'll
> keep the same ID.

Done.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:125:
EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));
On 2013/11/20 05:55:28, Jeffrey Yasskin wrote:
> You're likely to wind up with 2 rules registered here because background.js
> doesn't check
> http://developer.chrome.com/extensions/runtime.html#property-details-reason in
> onInstalled. I suspect background.js should only register the rule when
> reason=='install', so that we can check the behavior here. For user code, I
> recommend blindly replacing all the rules in onInstalled, but that would
obscure
> the details of the behavior we want here.
> 
> You could even upgrade the extension to a version that doesn't register rules
at
> all, to be really sure that changes in the count are a result of the Chrome
> logic rather than extension logic.

I added bot updating and reloading with no rules.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.cc:232:
CommitRemoveAllRules(extension_id);
On 2013/11/20 05:55:28, Jeffrey Yasskin wrote:
> Comment that we don't need to call RemoveAllRulesImpl because
> OnExtensionUnloaded is guaranteed to be called before OnExtensionUninstalled.
Or
> just forward to RemoveAllRules() to be clearly safe, since removing an empty
set
> of rules should be pretty cheap.

Done -- I went for calling RemoveAllRules. Thanks for this observation!

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:219: // storage (as
opposed to the storage of the inheriting registries, handled by
On 2013/11/20 05:55:28, Jeffrey Yasskin wrote:
> This isn't clear. Maybe "These functions remove rules from RulesRegistry
> persistent storage. This contrasts to Remove[All]RulesImpl, which prevents the
> rules from affecting Chrome's behavior."
> 
> Consider renaming AddRulesImpl to ActivateRules and Remove[All]RulesImpl to
> Deactivate[All]Rules. Then CommitRemove[All]Rules could become
Delete[All]Rules,
> making the distinction somewhat easier to explain.


I was also unhappy about the naming.

However, following your other suggestion, I merged the Commit* back to the
non-impl Remove* methods. For RemoveRules there was actually no reason for me to
separate those, and RemoveAllRules can be called during uninstalling including
the impl part, as you observed.

[vabr (Chromium), 2013-11-21 13:21:50]

On 2013/11/20 17:46:07, vabr (Chromium) wrote:
> There is also a problem with the RenderViewHost for the background page of the
> testing extension hanging during UpdateExtension.
> More precisely, UpdateExtension ends with calling
> ExtensionTestNotificationObserver::WaitForExtensionViewsToLoad to wait until
all
> RenderViewHosts associated with extensions return false on IsLoading. This
> apparently does not happen.
> When I forcibly pass no rules to WebRequestRulesRegistry::AddRulesImpl in
> RulesRegistry::OnExtensionLoaded, then IsLoading is no more blocking by
> returning true, and the test finishes (but obviously failing the
expectations).
> In more detail, the section of  WebRequestRulesRegistry::AddRulesImpl which is
> the culprit is the block updating url_matcher_ with new condition sets.
> I have yet to discover what is the connection between the URL matcher and the
> incomplete loading of the extension's RenderViewHost. If you had any
hypotheses
> about the RenderViewHost not loading, please let me know :).

Ha, found it -- the extension blocks its own background page.
Summarised the problem as http://crbug.com/322008. I need to first fix that bug,
and then this CL should hopefully work.

Cheers,
Vaclav

[Jeffrey Yasskin, 2013-11-22 00:35:53]

> Then I discovered one more problem:
> The RulesRegistryDelegate was listening to NOTIFICATION_EXTENSION_LOADED and
> triggered AddRules for the rules stored in the rules store.
> I believe this collides badly with the RulesRegistry now listening to this
event
> as well, so I removed the observing part from the delegate.
> This assumes that on the browser start, before the delegate is constructed,
the
> extension service already knows about all extensions installed before the last
> shutdown, so that these extensions are all processed by
> RulesCacheDelegate::ReadRulesForInstalledExtensions. To me it looks like it
> should be the case, otherwise the code looks broken already.

After reading some of the initialization code in ExtensionService, I think this
assumption is correct.

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:111:
EnableExtension(extension_id);
On 2013/11/20 17:46:07, vabr (Chromium) wrote:
> On 2013/11/20 05:55:28, Jeffrey Yasskin wrote:
> > Could you additionally try ReloadExtension()? I'm not exactly sure what
> behavior
> > we want for that, but I definitely want to pin it down.
> 
> I added ReloadExtension to the test, with the expectation that ReloadExtension
> will not change the number of active rules for the tested extension.

Actually, you've tested that ReloadExtension removes the rules and then fires
onInstalled(reason=='install'), which adds them back. That's fine, but should be
commented more explicitly. (and I think it becomes simpler if you move the
ReloadExtension step to between 3 and 4.)

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:60: const
std::string kRedirectToDataConstants(base::StringPrintf(
We ban global variables of string type, because they slow down startup, and it's
simpler not to have separate rules for unittests. If you need to do this, make
it a function that returns a string.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:70: "    new
chrome.declarativeWebRequest.RequestMatcher({})\n"
Nice debugging for http://crbug.com/322008. To fix it here, you could add
"schemes: ['http']" to the RequestMatcher.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:95: const
std::string kRedirectToDataBackground(
I'd do these concatenations inline in the test, to make it clearer what's being
written.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:105:
kRedirectToDataInstallRules));
You don't need to install rules if there aren't any rules.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:156: // helpers
in a TestExtensionDir, because we need the extension to persist
d'oh

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:175:
ExtensionTestMessageListener ready("ready", true);
For literal parameters, I like including the name in a comment:
/*will_reply=*/true

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:176: int change
= 1;  // expected change of the number of installed extensions
I prefer commenting the meaning of the "1" with a comment rather than a
variable, to make it clear that nothing else depends on or changes the value of
the variable.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:215:
kRedirectToDataBackgroundNoRules);
If you move this background.js write to section 4, I think you won't need
section 5 at all. The current effect of section 4 is just to verify that an
Update causes a call to onInstalled(reason=='update'), which should be tested
elsewhere.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:225: // 6.
Update again, with the code adding rules back.
I think you can remove section 6 after moving ReloadExtension to between 3 and
4.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:253: // 9.
Install the extension again.
What extra behavior are 9 and 10 checking? (Comment that and not just what the
code is doing.)

[vabr (Chromium), 2013-11-22 10:41:50]

Hi Jeffrey,

Thanks for comments!
I addressed all of them, with the exception of "d'oh". :)

Let me know what you think.
Thanks,
Vaclav

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:60: const
std::string kRedirectToDataConstants(base::StringPrintf(
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> We ban global variables of string type, because they slow down startup, and
it's
> simpler not to have separate rules for unittests. If you need to do this, make
> it a function that returns a string.

Good point.
I went for a macro holding the shared title string. Creating a function hides
the fact that this is a compile time constant.
But if you find that ugly, I'll make a function instead.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:70: "    new
chrome.declarativeWebRequest.RequestMatcher({})\n"
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> Nice debugging for http://crbug.com/322008. To fix it here, you could add
> "schemes: ['http']" to the RequestMatcher.

Done.
Also for the similar extension in
chrome/test/data/extensions/api_test/declarative/redirect_to_data/ .

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:95: const
std::string kRedirectToDataBackground(
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> I'd do these concatenations inline in the test, to make it clearer what's
being
> written.

Done.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:105:
kRedirectToDataInstallRules));
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> You don't need to install rules if there aren't any rules. 

Done.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:156: // helpers
in a TestExtensionDir, because we need the extension to persist
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> d'oh

Is this d'oh the "do something about it before landing" kind, or the "oh my,
interesting times at some undefined point in the future" kind? :)

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:175:
ExtensionTestMessageListener ready("ready", true);
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> For literal parameters, I like including the name in a comment:
> /*will_reply=*/true

Done.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:176: int change
= 1;  // expected change of the number of installed extensions
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> I prefer commenting the meaning of the "1" with a comment rather than a
> variable, to make it clear that nothing else depends on or changes the value
of
> the variable.

Done.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:215:
kRedirectToDataBackgroundNoRules);
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> If you move this background.js write to section 4, I think you won't need
> section 5 at all. The current effect of section 4 is just to verify that an
> Update causes a call to onInstalled(reason=='update'), which should be tested
> elsewhere.

Done.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:225: // 6.
Update again, with the code adding rules back.
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> I think you can remove section 6 after moving ReloadExtension to between 3 and
> 4.

Done.

https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:253: // 9.
Install the extension again.
On 2013/11/22 00:35:54, Jeffrey Yasskin wrote:
> What extra behavior are 9 and 10 checking? (Comment that and not just what the
> code is doing.)

Originally I thought I would test that reloading was different from updating in
that it did replace the rules. But now I realised that it's the same, so I'm
dropping 9 & 10.

[Jeffrey Yasskin, 2013-11-22 18:42:44]

LGTM, after fixing the Reload comment or ordering in the test.

The "d'oh" was just that I'm sad we couldn't remove the version of the extension
in the filesystem.

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:171:
ExtensionTestMessageListener ready("ready", /*will_reply=*/true);
nit: I think will_reply should actually be false here since you never call
Reply().

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:181: // 2.
Disable the extension. Rules no more active, but are still registered.
grammar nit: "Rules are no longer active"

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:193: // 4.
Reload the extension. Rules are still active.
I think this comment isn't quite describing what actually happens. Looking at
your previous section 7, the message that was sent was "ready", not
"ready_noop", indicating that the reload re-added the rules. If the reload
actually doesn't remove the rules, but still runs the
onInstalled(reason="install"), then do this after the "without the code" (i.e.
after step 5) to be clear that reload actually doesn't drop the rules.

[vabr (Chromium), 2013-11-22 21:16:44]

Thanks, Jeffrey.

I'm not sure I addressed your last e-mail satisfactorily, please check.

Cheers,
Vaclav

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:171:
ExtensionTestMessageListener ready("ready", /*will_reply=*/true);
On 2013/11/22 18:42:44, Jeffrey Yasskin wrote:
> nit: I think will_reply should actually be false here since you never call
> Reply().

Done.

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:181: // 2.
Disable the extension. Rules no more active, but are still registered.
On 2013/11/22 18:42:44, Jeffrey Yasskin wrote:
> grammar nit: "Rules are no longer active"

Done.

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:193: // 4.
Reload the extension. Rules are still active.
On 2013/11/22 18:42:44, Jeffrey Yasskin wrote:
> I think this comment isn't quite describing what actually happens. Looking at
> your previous section 7, the message that was sent was "ready", not
> "ready_noop", indicating that the reload re-added the rules. If the reload
> actually doesn't remove the rules, but still runs the
> onInstalled(reason="install"), then do this after the "without the code" (i.e.
> after step 5) to be clear that reload actually doesn't drop the rules.

The previous patch was still broken, and part 7 was one of the broken parts.
Reload does not invoke onInstalled(reason="install"). I think it might invoke
onInstalled(reason="update") when the source of the extension is upgraded in
between. In this case it should not invoke onInstalled at all.

On the other hand, rules are now not flushed on unload (and uninstalled is not
called during reload).

Does the updated comment make sense now?

[Jeffrey Yasskin, 2013-11-22 21:39:52]

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:193: // 4.
Reload the extension. Rules are still active.
On 2013/11/22 21:16:44, vabr (Chromium) wrote:
> On 2013/11/22 18:42:44, Jeffrey Yasskin wrote:
> > I think this comment isn't quite describing what actually happens. Looking
at
> > your previous section 7, the message that was sent was "ready", not
> > "ready_noop", indicating that the reload re-added the rules. If the reload
> > actually doesn't remove the rules, but still runs the
> > onInstalled(reason="install"), then do this after the "without the code"
(i.e.
> > after step 5) to be clear that reload actually doesn't drop the rules.
> 
> The previous patch was still broken, and part 7 was one of the broken parts.
> Reload does not invoke onInstalled(reason="install"). I think it might invoke
> onInstalled(reason="update") when the source of the extension is upgraded in
> between. In this case it should not invoke onInstalled at all.
> 
> On the other hand, rules are now not flushed on unload (and uninstalled is not
> called during reload).
> 
> Does the updated comment make sense now?

Given that behavior, you should move the Reload section after section 5, so that
there's no way the onInstalled event could possible re-add the rules. then if
the test passes, we've proven that reload doesn't flush the rules.

https://codereview.chromium.org/52743002/diff/770001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/770001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:181: // 2.
Disable the extension. Rules are no more active, but are still
grammar nit: The more important change is s/more/longer/ or "not active
anymore".

[vabr (Chromium), 2013-11-22 21:52:38]

Thanks Jeffrey,

Both comments addressed, the test still passes locally. :)

Vaclav

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:193: // 4.
Reload the extension. Rules are still active.
On 2013/11/22 21:39:52, Jeffrey Yasskin wrote:
> On 2013/11/22 21:16:44, vabr (Chromium) wrote:
> > On 2013/11/22 18:42:44, Jeffrey Yasskin wrote:
> > > I think this comment isn't quite describing what actually happens. Looking
> at
> > > your previous section 7, the message that was sent was "ready", not
> > > "ready_noop", indicating that the reload re-added the rules. If the reload
> > > actually doesn't remove the rules, but still runs the
> > > onInstalled(reason="install"), then do this after the "without the code"
> (i.e.
> > > after step 5) to be clear that reload actually doesn't drop the rules.
> > 
> > The previous patch was still broken, and part 7 was one of the broken parts.
> > Reload does not invoke onInstalled(reason="install"). I think it might
invoke
> > onInstalled(reason="update") when the source of the extension is upgraded in
> > between. In this case it should not invoke onInstalled at all.
> > 
> > On the other hand, rules are now not flushed on unload (and uninstalled is
not
> > called during reload).
> > 
> > Does the updated comment make sense now?
> 
> Given that behavior, you should move the Reload section after section 5, so
that
> there's no way the onInstalled event could possible re-add the rules. then if
> the test passes, we've proven that reload doesn't flush the rules.

Done.

https://codereview.chromium.org/52743002/diff/770001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/52743002/diff/770001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:181: // 2.
Disable the extension. Rules are no more active, but are still
On 2013/11/22 21:39:54, Jeffrey Yasskin wrote:
> grammar nit: The more important change is s/more/longer/ or "not active
> anymore".

Ah, sorry I overlooked that. Done.

[Jeffrey Yasskin, 2013-11-22 21:55:23]

LGTM again. Thanks for bearing with me!

[vabr (Chromium), 2013-11-22 22:02:12]

On 2013/11/22 21:55:23, Jeffrey Yasskin wrote:
> LGTM again. Thanks for bearing with me!

You've been of a great help again, thanks Jeffrey!

Sending to a CQ now.

[commit-bot: I haz the power, 2013-11-22 22:03:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/52743002/860001

[commit-bot: I haz the power, 2013-11-23 01:58:12]

Change committed as 236918