Some easy NewRunnableMethod conversions in safe_browsing.

Some easy NewRunnableMethod conversions in safe_browsing.

Also a harder one of a CancelableTask timeout task, which was replaced
with a callback-owned flag.


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

[Scott Hess - ex-Googler, 2011-11-13 00:33:28]

Mostly mundane conversions, except for the timeout_task conversion, which is why
I posted it your way.  Previously, the code registered a timeout_task, which was
cancelled if the primary task ran first.  Now, it uses a flag to cancel the
timeout, which flag is base::Owned by the timeout task's callback.  If the
primary task runs first, it sets the flag and the timeout task short-circuits.

Mostly this seemed less baroque than having a weak-ptr factory in the check
structure.  I considered a weak-ptr factory in the service instance, but then
realized that since checks are considered as pointers, it might be possible to
have a new check which re-used an old check's pointer, and thinking in that
direction quickly leads me to want to rewrite code rather than get this
conversion done quickly.

[awong, 2011-11-14 20:13:37]

comment inline.

http://codereview.chromium.org/8548003/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/8548003/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:109: // will not fire.
Wow. complicated.

So the flag is owned by the timeout closure only, and the "non-timeout" paths
know to disable themselves because "client" is gone?

I'm tempted to favor the more baroque style of embedding the factory in the
Check object, if only because people understand the lifetimes.  If you embedded
the factory, it'd be clear that on delete of |check|, the timeout callback would
just not fire.

As is, I actually read through the patch multiple times before I could satisfy
myself that I understood where each of these objects was expected to be deleted
with respect to the callbacks that might be outstanding for them.

[Scott Hess - ex-Googler, 2011-11-14 23:21:54]

http://codereview.chromium.org/8548003/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/8548003/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:109: // will not fire.
On 2011/11/14 20:13:37, awong wrote:
> Wow. complicated.
>
> So the flag is owned by the timeout closure only, and the "non-timeout" paths
> know to disable themselves because "client" is gone?

I think so.  Author has left the project ...

> I'm tempted to favor the more baroque style of embedding the factory in the
> Check object, if only because people understand the lifetimes.  If you
embedded
> the factory, it'd be clear that on delete of |check|, the timeout callback
would
> just not fire.
>
> As is, I actually read through the patch multiple times before I could satisfy
> myself that I understood where each of these objects was expected to be
deleted
> with respect to the callbacks that might be outstanding for them.

I've made that change.

My initial approach was to have the SafeBrowsingCheck constructor take the
pointer to put in the factory.  This lead to having to modify some unit tests,
which lead to having to read up on gmock ACTION_P macros, which lead to wailing
and teeth-gnashing, but even once all of that was fixed I ended up with an
insane error message (constructing w/o a parameter gave the expected error,
constructing with the appropriate parameter gave an error "Declaration has no
effect").  So I backed it out to this version.

Another option would be to have a default-to-NULL constructor (or
with-and-without constructors), though I understand that to be against our style
guidelines.

YET ANOTHER OPTION would be to leave it alone and hope that someone finds time
to refactor this code.

[awong, 2011-11-15 00:30:37]

LGTM

On 2011/11/14 23:21:54, shess wrote:
>
http://codereview.chromium.org/8548003/diff/1/chrome/browser/safe_browsing/sa...
> File chrome/browser/safe_browsing/safe_browsing_service.h (right):
> 
>
http://codereview.chromium.org/8548003/diff/1/chrome/browser/safe_browsing/sa...
> chrome/browser/safe_browsing/safe_browsing_service.h:109: // will not fire.
> On 2011/11/14 20:13:37, awong wrote:
> > Wow. complicated.
> >
> > So the flag is owned by the timeout closure only, and the "non-timeout"
paths
> > know to disable themselves because "client" is gone?
> 
> I think so.  Author has left the project ...
> 
> > I'm tempted to favor the more baroque style of embedding the factory in the
> > Check object, if only because people understand the lifetimes.  If you
> embedded
> > the factory, it'd be clear that on delete of |check|, the timeout callback
> would
> > just not fire.
> >
> > As is, I actually read through the patch multiple times before I could
satisfy
> > myself that I understood where each of these objects was expected to be
> deleted
> > with respect to the callbacks that might be outstanding for them.
> 
> I've made that change.
> 
> My initial approach was to have the SafeBrowsingCheck constructor take the
> pointer to put in the factory.  This lead to having to modify some unit tests,
> which lead to having to read up on gmock ACTION_P macros, which lead to
wailing
> and teeth-gnashing, but even once all of that was fixed I ended up with an
> insane error message (constructing w/o a parameter gave the expected error,
> constructing with the appropriate parameter gave an error "Declaration has no
> effect").  So I backed it out to this version.
>
> Another option would be to have a default-to-NULL constructor (or
> with-and-without constructors), though I understand that to be against our
style
> guidelines.
> 
> YET ANOTHER OPTION would be to leave it alone and hope that someone finds time
> to refactor this code.

yikes.

As is, I think the current change is "good enough" as the readability seems
about equal.

I know there is a clear hole in the cancellation semantics of Callback<>/Bind().
 However, I've been hesitant to try and craft a new API cause I don't think I
get all the use cases.

The CancelableRequest structure in content has the right structure, but it is
ridiculously complex for most use cases.  More so than even WeakPtrFactory. :-/

Thanks for wading through the C++ gobbledygook.

[Scott Hess - ex-Googler, 2011-11-15 18:55:43]

The Linux trybot failure seems consistent for earlier and later numbered tries
which do not include my change.  So I'm pushing the button.

[commit-bot: I haz the power, 2011-11-15 18:55:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/8548003/5001

[Scott Hess - ex-Googler, 2011-11-15 18:58:15]

On 2011/11/15 00:30:37, awong wrote:
> LGTM

Thanks.

> On 2011/11/14 23:21:54, shess wrote:
> > YET ANOTHER OPTION would be to leave it alone and hope that someone finds
time
> > to refactor this code.
> 
> yikes.
> 
> As is, I think the current change is "good enough" as the readability seems
> about equal.

I've been thinking I should give it a pass and convert a bunch of manual
thread-bouncing stuff to PostTaskAndReply(), and generally clean up the
ref-counting over-use in this code.  So the chance isn't non-zero :-).

[commit-bot: I haz the power, 2011-11-15 21:17:21]

Change committed as 110166