Re: Don't post tasks from RulesRegistry c-tor (issue 108193008)

[va...@chromium.org]

Hi Fady, Jeffrey.

I addressed Fady's comment, and fixed a NULL de-reference I introduced.

PTAL.

Thanks,
Vaclav


https://codereview.chromium.org/108193008/diff/60001/chrome/browser/extensions/api/declarative/rules_registry.h
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/108193008/diff/60001/chrome/browser/extensions/api/declarative/rules_registry.h#newcode71
chrome/browser/extensions/api/declarative/rules_registry.h:71: return
(cache_delegate_) ? &ready_ : NULL;
On 2013/12/12 17:54:39, Fady Samuel wrote:
> What about making ready_ a scoped_ptr<OneShotEvent>? That way we don't
actually
> keep this object around in RulesRegistry if we don't need it.

Done.

https://codereview.chromium.org/108193008/

[va...@chromium.org]

Quick update: added patch set 8 to fix dereferencing of cache_delegate_ on a
wrong thread. The non-NULL-ness of cache_delegate_ and ready_ should be
equivalent, and ready_ being not NULL is even more directly what we are
interested in in MaybeProcessChangedRules.

https://codereview.chromium.org/108193008/

[fsa...@chromium.org]

lgtm

https://codereview.chromium.org/108193008/

[va...@chromium.org]

Thanks, Fady.

Jeffrey, do you have any comments?
If not, could you please give me an OWNERS approval?

Thanks,
Vaclav

https://codereview.chromium.org/108193008/

[jyas...@chromium.org]

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc
File chrome/browser/extensions/api/declarative/rules_registry.cc (left):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc#oldcode95
chrome/browser/extensions/api/declarative/rules_registry.cc:95:
base::Bind(&RulesRegistry::MarkReady, this, base::Time::Now()));
Would this CL be simpler if you just called ready_.Signal() right here,
instead of Posting a call to MarkReady()?
I'm not particularly happy about needing to check ready_ for NULL
instead of just posting tasks to it. Removing the double-check is why
OneShotEvent accepts Post()s even after it's signaled.

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc
File chrome/browser/extensions/api/declarative/rules_registry.cc
(right):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc#newcode93
chrome/browser/extensions/api/declarative/rules_registry.cc:93: // Note
that if |cache_delegate| is NULL, the Rulesregistry never signals its
capitalization: RulesRegistry

https://codereview.chromium.org/108193008/

[va...@chromium.org]

Thanks, Jeffrey.

Please see my response to your comment about calling Signal() directly.

Cheers,
Vaclav

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc
File chrome/browser/extensions/api/declarative/rules_registry.cc (left):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc#oldcode95
chrome/browser/extensions/api/declarative/rules_registry.cc:95:
base::Bind(&RulesRegistry::MarkReady, this, base::Time::Now()));

On 2013/12/17 22:55:05, Jeffrey Yasskin wrote:
> Would this CL be simpler if you just called ready_.Signal() right
here, instead
> of Posting a call to MarkReady()?
> I'm not particularly happy about needing to check ready_ for NULL
instead of
> just posting tasks to it. Removing the double-check is why
OneShotEvent accepts
> Post()s even after it's signaled.

I'm afraid calling ready_.Signal() here would lead to a DCHECK failing
CalledOnValidThread as soon as ready_.Post() is used. This is because
the constructor runs on UI thread, but the rest runs on owner_thread (IO
for WebRequestRulesRegistry).

Currently, OneShotEvent detaches from the thread it's constructed on
just to allow constructing it on another thread than it's used to.

What I can do is:
1) Allow to construct it as already signalled, or
2) provide a OneShotEvent method to detach on demand.

Does anything from the above sound good to you? Any other suggestions?

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc
File chrome/browser/extensions/api/declarative/rules_registry.cc
(right):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc#newcode93
chrome/browser/extensions/api/declarative/rules_registry.cc:93: // Note
that if |cache_delegate| is NULL, the Rulesregistry never signals its

On 2013/12/17 22:55:05, Jeffrey Yasskin wrote:
> capitalization: RulesRegistry

Done.

https://codereview.chromium.org/108193008/

[Jeffrey Yasskin]

I think exposing Detach will work better for this case, since it won't force you to keep it in a scoped_ptr.

> Does anything from the above sound good to you? Any other suggestions?
>
>
> https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc
> File chrome/browser/extensions/api/declarative/rules_registry.cc
> (right):
>
> https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensions/api/declarative/rules_registry.cc#newcode93
> chrome/browser/extensions/api/declarative/rules_registry.cc:93: // Note
> that if |cache_delegate| is NULL, the Rulesregistry never signals its
> On 2013/12/17 22:55:05, Jeffrey Yasskin wrote:
>>
>> capitalization: RulesRegistry
>
>
> Done.
>
>
> https://codereview.chromium.org/108193008/
>

> --
> 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-revi...@chromium.org.
> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/extensions-reviews/047d7b6732b6718b4b04edcdcf7f%40google.com.

[va...@chromium.org]

Hi Jeffrey,

I know you suggested 2) (explicit detach method), but I'm not sure whether
you
dismissed 1) as not getting rid of the pointer (which it does), or for some
other reason.

Currently I implemented 1) as a "weaker" change to the OneShotEvent. If you
don't like it, I'm of course happy to implement the explicit detach method
instead.

Looking forward to your comments,
Vaclav

https://codereview.chromium.org/108193008/

[jyas...@chromium.org]

lgtm



https://codereview.chromium.org/108193008/

[Jeffrey Yasskin]

On Wed, Dec 18, 2013 at 9:45 AM, <va...@chromium.org> wrote:
> Hi Jeffrey,
>
> I know you suggested 2) (explicit detach method), but I'm not sure whether
> you
> dismissed 1) as not getting rid of the pointer (which it does), or for some
> other reason.

Yeah, I hadn't looked at the constructor in enough detail. I suspect
we'll eventually have to expose Detach anyway, but your change is
good. Thanks for pushing back.

[va...@chromium.org]

On 2013/12/18 18:19:17, Jeffrey Yasskin wrote:

> To unsubscribe from this group and stop receiving emails from it, send an
email

> to mailto:chromium-revie...@chromium.org.

Thanks, Jeffrey, for your patient review! :)

Sending to the CQ now.

https://codereview.chromium.org/108193008/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/va...@chromium.org/108193008/280001


https://codereview.chromium.org/108193008/

[commi...@chromium.org]

Change committed as 241831

https://codereview.chromium.org/108193008/