Move cancelable_request.cc/h to chrome/browser/common.

Move cancelable_request.cc/h to chrome/browser/common.

It is a commonly shared utility.
Also move "!chrome/browser/browser_process.h" to test section in bookmarks/DEPS

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

[Jói, 2012-09-21 09:37:39]

LGTM

[erikwright (departed), 2012-09-21 15:07:42]

[-me for review].

[kaiwang, 2012-09-21 18:28:38]

Hi ben, could you take a look at this CL. I need OWNER's approve

[Ben Goodger (Google), 2012-09-21 19:14:10]

lgtm

[commit-bot: I haz the power, 2012-09-21 19:33:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10958031/11072

[commit-bot: I haz the power, 2012-09-21 19:33:42]

Failed to apply patch for chrome/browser/common/cancelable_request.cc:
While running patch -p0 --forward --force;
patching file chrome/browser/common/cancelable_request.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/common/cancelable_request.cc.rej

[commit-bot: I haz the power, 2012-09-21 20:00:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10958031/6018

[commit-bot: I haz the power, 2012-09-21 20:01:19]

Failed to apply patch for chrome/browser/common/cancelable_request.cc:
While running patch -p0 --forward --force;
patching file chrome/browser/common/cancelable_request.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/common/cancelable_request.cc.rej

[commit-bot: I haz the power, 2012-09-21 20:08:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10958031/13011

[commit-bot: I haz the power, 2012-09-21 20:08:40]

Failed to apply patch for chrome/browser/common/cancelable_request.cc:
While running patch -p0 --forward --force;
patching file chrome/browser/common/cancelable_request.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/common/cancelable_request.cc.rej

[commit-bot: I haz the power, 2012-09-21 20:12:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/10958031/13011

[commit-bot: I haz the power, 2012-09-21 20:12:39]

Failed to apply patch for chrome/browser/common/cancelable_request.cc:
While running patch -p0 --forward --force;
patching file chrome/browser/common/cancelable_request.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/common/cancelable_request.cc.rej

[jam, 2012-10-02 06:41:47]

(I just saw this, sorry for the late comment).

I think this is an example of cleanup of the chrome\browser code that not only
can remove the need to have cancelable_request.* in chrome\browser\common, but
it can eliminate it from the codebase altogether.

cancelable_request is from long ago before we had the new bind based callbacks.
The cancelable request stuff is a bit complicated and very hard to read. There
are simpler alternatives now; ones which are more like the rest of the codebase
instead of this one-off. See the chromium-dev thread from a year ago about how
this can be removed:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5sx-puvQ1vc/vi3Gc...

[Jói, 2012-10-02 10:16:19]

Hi John,

Thanks for the pointer to the old thread where CancelableRequest is mentioned.

For the record, removing CancelableRequest altogether was the initial
plan we looked at, and as a fallback plan, not exposing it in the APIs
we were extracting (i.e. exposing only a base::Callback in the API).
I had looked into removing CancelableRequest once before myself, about
a year ago, soon after the PostTaskAndReply mechanism was available
and the thread you linked to was making the rounds, but I didn't dive
into it at the time.

It turns out it doesn't (at least at a somewhat cursory glance,
reading the code and usages for a couple of hours now, and a couple of
hours back a year ago) look so easy to completely remove it.  It has
features like allowing multiple requests in flight at once, cancelling
requests by their handle (without necessarily cancelling other
requests in flight), and a richer set of notifications than
PostTaskAndReply has
(RequestAdded/RequestRemoved/WillExecute/DidExecute).  So removing it
altogether seems, on the face of it, not trivial.  Since our focus
right now was on modularizing certain features, we decided to let it
live for now, but it was on our radar to do something about it.  Given
our discussion yesterday it might be better to prioritize getting rid
of this old mechanism over other work.

It's possible that nobody actually uses the features of allowing
multiple requests in flight, or cancelling individual requests.  If
you know this to be the case, I'd love to know about it.  If not, then
it's something we need to research.  If there is no true need for the
functionality it has beyond PostTaskAndReply, then getting rid of it
should be relatively straightforward.

Cheers,
Jói



On Tue, Oct 2, 2012 at 6:41 AM,  <jam@chromium.org> wrote:
> (I just saw this, sorry for the late comment).
>
> I think this is an example of cleanup of the chrome\browser code that not
> only
> can remove the need to have cancelable_request.* in chrome\browser\common,
> but
> it can eliminate it from the codebase altogether.
>
> cancelable_request is from long ago before we had the new bind based
> callbacks.
> The cancelable request stuff is a bit complicated and very hard to read.
> There
> are simpler alternatives now; ones which are more like the rest of the
> codebase
> instead of this one-off. See the chromium-dev thread from a year ago about
> how
> this can be removed:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5sx-puvQ1vc/vi3Gc...
>
> http://codereview.chromium.org/10958031/

[jam, 2012-10-02 16:23:19]

On Tue, Oct 2, 2012 at 3:15 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Hi John,
>
> Thanks for the pointer to the old thread where CancelableRequest is
> mentioned.
>
> For the record, removing CancelableRequest altogether was the initial
> plan we looked at, and as a fallback plan, not exposing it in the APIs
> we were extracting (i.e. exposing only a base::Callback in the API).
> I had looked into removing CancelableRequest once before myself, about
> a year ago, soon after the PostTaskAndReply mechanism was available
> and the thread you linked to was making the rounds, but I didn't dive
> into it at the time.
>
> It turns out it doesn't (at least at a somewhat cursory glance,
> reading the code and usages for a couple of hours now, and a couple of
> hours back a year ago) look so easy to completely remove it.  It has
> features like allowing multiple requests in flight at once, cancelling
> requests by their handle (without necessarily cancelling other
> requests in flight), and a richer set of notifications than
> PostTaskAndReply has
> (RequestAdded/RequestRemoved/WillExecute/DidExecute).  So removing it
> altogether seems, on the face of it, not trivial.  Since our focus
> right now was on modularizing certain features, we decided to let it
> live for now, but it was on our radar to do something about it.  Given
> our discussion yesterday it might be better to prioritize getting rid
> of this old mechanism over other work.
>
> It's possible that nobody actually uses the features of allowing
> multiple requests in flight, or cancelling individual requests.  If
> you know this to be the case, I'd love to know about it.  If not, then
> it's something we need to research.  If there is no true need for the
> functionality it has beyond PostTaskAndReply, then getting rid of it
> should be relatively straightforward.
>

Great to hear. I haven't looked at the usage in detail, so unfortunately
I'm not sure. Even if they are, I would think that they could be refactored
to not used that (i.e. none of the other code needs this!). If we
absolutely need these features that you mention, perhaps they can be
simulated using multiple WeakPtrFactory objects?

>
> Cheers,
> Jói
>
>
>
> On Tue, Oct 2, 2012 at 6:41 AM,  <jam@chromium.org> wrote:
> > (I just saw this, sorry for the late comment).
> >
> > I think this is an example of cleanup of the chrome\browser code that not
> > only
> > can remove the need to have cancelable_request.* in
> chrome\browser\common,
> > but
> > it can eliminate it from the codebase altogether.
> >
> > cancelable_request is from long ago before we had the new bind based
> > callbacks.
> > The cancelable request stuff is a bit complicated and very hard to read.
> > There
> > are simpler alternatives now; ones which are more like the rest of the
> > codebase
> > instead of this one-off. See the chromium-dev thread from a year ago
> about
> > how
> > this can be removed:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5sx-puvQ1vc/vi3Gc...
> >
> > http://codereview.chromium.org/10958031/
>

[Jói, 2012-10-02 16:49:53]

> Great to hear. I haven't looked at the usage in detail, so unfortunately I'm
> not sure. Even if they are, I would think that they could be refactored to
> not used that (i.e. none of the other code needs this!).

Yep, it's quite possible that nobody uses that functionality.  I've
put it on our agenda to look at this and suggested it to Kai as an
interim project while we figure other things out.

> [...] If we absolutely
> need these features that you mention, perhaps they can be simulated using
> multiple WeakPtrFactory objects?

Yes, that sounds like a possible approach.

Cheers,
Jói



On Tue, Oct 2, 2012 at 4:23 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Tue, Oct 2, 2012 at 3:15 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Hi John,
>>
>> Thanks for the pointer to the old thread where CancelableRequest is
>> mentioned.
>>
>> For the record, removing CancelableRequest altogether was the initial
>> plan we looked at, and as a fallback plan, not exposing it in the APIs
>> we were extracting (i.e. exposing only a base::Callback in the API).
>> I had looked into removing CancelableRequest once before myself, about
>> a year ago, soon after the PostTaskAndReply mechanism was available
>> and the thread you linked to was making the rounds, but I didn't dive
>> into it at the time.
>>
>> It turns out it doesn't (at least at a somewhat cursory glance,
>> reading the code and usages for a couple of hours now, and a couple of
>> hours back a year ago) look so easy to completely remove it.  It has
>> features like allowing multiple requests in flight at once, cancelling
>> requests by their handle (without necessarily cancelling other
>> requests in flight), and a richer set of notifications than
>> PostTaskAndReply has
>> (RequestAdded/RequestRemoved/WillExecute/DidExecute).  So removing it
>> altogether seems, on the face of it, not trivial.  Since our focus
>> right now was on modularizing certain features, we decided to let it
>> live for now, but it was on our radar to do something about it.  Given
>> our discussion yesterday it might be better to prioritize getting rid
>> of this old mechanism over other work.
>>
>> It's possible that nobody actually uses the features of allowing
>> multiple requests in flight, or cancelling individual requests.  If
>> you know this to be the case, I'd love to know about it.  If not, then
>> it's something we need to research.  If there is no true need for the
>> functionality it has beyond PostTaskAndReply, then getting rid of it
>> should be relatively straightforward.
>
>
> Great to hear. I haven't looked at the usage in detail, so unfortunately I'm
> not sure. Even if they are, I would think that they could be refactored to
> not used that (i.e. none of the other code needs this!). If we absolutely
> need these features that you mention, perhaps they can be simulated using
> multiple WeakPtrFactory objects?
>>
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Tue, Oct 2, 2012 at 6:41 AM,  <jam@chromium.org> wrote:
>> > (I just saw this, sorry for the late comment).
>> >
>> > I think this is an example of cleanup of the chrome\browser code that
>> > not
>> > only
>> > can remove the need to have cancelable_request.* in
>> > chrome\browser\common,
>> > but
>> > it can eliminate it from the codebase altogether.
>> >
>> > cancelable_request is from long ago before we had the new bind based
>> > callbacks.
>> > The cancelable request stuff is a bit complicated and very hard to read.
>> > There
>> > are simpler alternatives now; ones which are more like the rest of the
>> > codebase
>> > instead of this one-off. See the chromium-dev thread from a year ago
>> > about
>> > how
>> > this can be removed:
>> >
>> >
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5sx-puvQ1vc/vi3Gc...
>> >
>> > http://codereview.chromium.org/10958031/
>
>

[kaiwang, 2012-10-02 22:19:03]

I'll look at cancelable_request again, to figure out a way to completely remove
it :)