RulesRegistryWithCache::ProcessChangedRules postponed

RulesRegistryWithCache::ProcessChangedRules postponed

The main change here is that ProcessChangedRules (includes converting the rules
and writing them to disk) is now postponed to when the rules registry becomes
ready. After start-up, this will just mean that ProcessChangedRules will be
added to the message loop queue.

A minor change is a new way to manage ProcessChangedRules tasks:
If there is not the UI part (which does the writing), ProcessChangedRules tasks
are not ever posted.
If there is the UI part, then at any time at most one ProcessChangedRules task
is ever scheduled. This avoids unnecessary work duplication.

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

[vabr (Chromium), 2013-05-27 09:03:30]

Hi Dominic,

Please review this fix to my recent change.

The only significant change is the diff of PS3 vs. PS2:
https://codereview.chromium.org/15911005/diff2/2001:8001/chrome/browser/exten...

The rest is just the old CL and rebasing (see this CL description).


Explanation of the fix:
In the patch set 4 of https://chromiumcodereview.appspot.com/14851011 I started
to initialise the WeakPtrFactory for the rules registry even if the profile_ is
NULL. However, I forgot to detach it after initialisation in the profile_==NULL
case. Detaching is needed, because the rules registry gets created on the UI
thread, but used and destroyed on the rules registry thread (can be IO).


Cheers,
Vaclav

[vabr (Chromium), 2013-05-27 09:07:26]

Note: the error was caught on ChromeOs Debug buildbots. I don't think there are
CrOS Debug trybots. But I was able to reproduce the error locally and see it go
away after the fix.

Vaclav

[vabr (Chromium), 2013-05-27 10:20:05]

Oh, so now the ChromeOS trybot starts to fail. I'll need to look into it more.

Please ignore this review until my further notice, sorry for the noise.

Vaclav

[vabr (Chromium), 2013-05-28 16:08:08]

Quick update:
This CL is gated on https://codereview.chromium.org/15688010/ and
https://codereview.chromium.org/15769004/.
In particular it is still not reviewable, not until those other two land safely.
I'll update this further once that happens.

Thanks for patience,
Vaclav

[vabr (Chromium), 2013-06-04 07:40:53]

On 2013/05/28 16:08:08, vabr (Chromium) wrote:
> Quick update:
> This CL is gated on https://codereview.chromium.org/15688010/ and
> https://codereview.chromium.org/15769004/.
> In particular it is still not reviewable, not until those other two land
safely.
> I'll update this further once that happens.
> 
> Thanks for patience,
> Vaclav

Hi Dominic,
This CL is now ready for review, please take a look.

The main change here is that ProcessChangedRules (includes converting the rules
and writing them to disk) is now postponed to when the rules registry becomes
ready. After start-up, this will just mean that ProcessChangedRules will be
added to the message loop queue.

A minor change is a new way to manage ProcessChangedRules tasks:
If there is not the UI part (which does the writing), ProcessChangedRules tasks
are not ever posted.
If there is the UI part, then at any time at most one ProcessChangedRules task
is ever scheduled. This avoids unnecessary work duplication.

Vaclav

[battre, 2013-06-04 08:22:34]

Can you update the CL description to what this CL actually does? I think the
relanding part is less interesting.

https://codereview.chromium.org/15911005/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.h
(right):

https://codereview.chromium.org/15911005/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.h:225: //
scheduled yet, but it is possible to schedule one.
nit: I think this should go to the typedef above.

[vabr (Chromium), 2013-06-04 09:04:27]

Hi Dominic,

Thanks, I updated the CL description and addressed your comment. Please continue
with your review.

Vaclav

[battre, 2013-06-04 09:35:40]

I think you should delete the last two paragraphs in the CL description. It is
uncommon to describe individual patch sets in the commit message. The history of
this CL is in my opinion irrelevant because the CL is so small.

LGTM

[vabr (Chromium), 2013-06-04 09:38:44]

Thanks, Dominic.

I removed the patch set descriptions, and the links to the older CLs (they are
linked to from the crbug).

Sending to CQ now.

Vaclav

[commit-bot: I haz the power, 2013-06-04 09:39:06]

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

[commit-bot: I haz the power, 2013-06-04 17:16:29]

Change committed as 203993