Port HistoryService::QueryRedirects{From,To} to CancelableTaskTracker

Port HistoryService::QueryRedirects{From,To} to CancelableTaskTracker

The callback no longer receive a HistoryService::Handle but instead a
base::CancelableTaskTracker::TaskId that they can use to cancel the
task via the base::CancelableTaskTracker.

Remove HistoryBackend::GetMostRecentRedirects{From,To} that backed
the HistoryBackend::QueryRedirects{From,To} as they now have the same
interface.

Remove the success and url parameters to the QueryRedirectsCallback
as the url can be bound by the caller if required (most of them don't)
and no code path used the success value.

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

[sdefresne, 2014-06-23 15:35:42]

droger: please take a look
jochen: OWNERS of refactored client code in //chrome/browser/safe_browsing

[sdefresne, 2014-06-23 15:37:42]

+haaawk since this may break clank code

[droger, 2014-06-23 15:49:48]

Where are the requests actually cancelled?

Do you actually need CancelableTaskTracker, or is CancelableCallback enough?

I think that CancelableCallback is the preferred mechanism, with the caveat that
it is not multi-thread. Do you need to cancel the task from a different thread?

[sdefresne, 2014-06-23 16:11:28]

+brettw: FYI

[droger, 2014-06-23 16:50:35]

To answer my own question about CancelableCallback: it is not possible to use it
here because CancelableCallback does not support being posted to another thread.

I only have minor / optional comments.

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1279: result->success =
GetMostRecentRedirectsFrom(url, &result->redirects);
codesearch seems partially broken so this may be wrong, but 
GetMostRecentRedirectsFrom() seems to be only called from here, maybe it would
make sense to actually inline it here?

Same for GetMostRecentRedirectsTo()

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service.cc (right):

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.cc:89: void RunWithQueryRedirectsResult(
If I understand correctly, you need this helper just because you want to use
base::Owned so that the callback owns your request.
This feels a bit strange, but I don't have a really better solution...

One way to avoid this maybe would be to have QueryRedirectsFrom return a new
QueryRedirectsResult, and then you can use PostTaskAndReplyWithResult.
It's not clear if it's better though, so feel free to keep your code as is.

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.cc:807:
base::CancelableTaskTracker::TaskId HistoryService::QueryRedirectsFrom(
Is this function actually used? I see it only called in tests.

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:24: #include
"chrome/browser/common/cancelable_request.h"
Is this still needed?

[sdefresne, 2014-06-24 13:41:00]

PTAL

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1279: result->success =
GetMostRecentRedirectsFrom(url, &result->redirects);
On 2014/06/23 16:50:35, droger wrote:
> codesearch seems partially broken so this may be wrong, but 
> GetMostRecentRedirectsFrom() seems to be only called from here, maybe it would
> make sense to actually inline it here?
> 
> Same for GetMostRecentRedirectsTo()

It's called multiple times from this file (at least three times), so I won't
inline it. GetMostRecentRedirectsTo is only called once, but there is a second
call that is commented and marked with a bug number to re-enable, so I'll also
leave the function untouched.

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service.cc (right):

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.cc:89: void RunWithQueryRedirectsResult(
On 2014/06/23 16:50:35, droger wrote:
> If I understand correctly, you need this helper just because you want to use
> base::Owned so that the callback owns your request.
> This feels a bit strange, but I don't have a really better solution...
> 
> One way to avoid this maybe would be to have QueryRedirectsFrom return a new
> QueryRedirectsResult, and then you can use PostTaskAndReplyWithResult.
> It's not clear if it's better though, so feel free to keep your code as is.

PostTaskAndReplyWithResult does not works with CancelableTaskTracker.

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.cc:807:
base::CancelableTaskTracker::TaskId HistoryService::QueryRedirectsFrom(
On 2014/06/23 16:50:35, droger wrote:
> Is this function actually used? I see it only called in tests.

It's only called from unit tests and browser tests but I don't think it is the
job of componentization to decide whether code should be removed or not. I'll
add a ticket to track removal.

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/349153006/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_service.h:24: #include
"chrome/browser/common/cancelable_request.h"
On 2014/06/23 16:50:35, droger wrote:
> Is this still needed?

Yes, there are still many usage of CancelableRequestProvider/Consumer in the
file. I'm porting them one at a time.

[droger, 2014-06-24 13:51:13]

lgtm

[jochen (gone - plz use gerrit), 2014-06-24 13:59:19]

lgtm

[sdefresne, 2014-06-24 16:25:03]

droger: as discussed, PTAL

[droger, 2014-06-24 16:34:51]

On 2014/06/24 16:25:03, sdefresne wrote:
> droger: as discussed, PTAL

Thanks, it's a simpler now.

[sdefresne, 2014-06-25 12:14:21]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-25 12:15:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sdefresne@chromium.org/349153006/60001

[commit-bot: I haz the power, 2014-06-25 16:24:46]

Change committed as 279727