Change HistoryService::QueryURL to use CancelableTaskTracker

Change HistoryService::QueryURL to use CancelableTaskTracker

The callback no longer receive an HistoryService::Handle as first parameter
and the caller should use the returned base::CancelableTaskTrasker::TaskId
to cancel a request early.

Instead of passing a CancelableRequestConsumer to the query, the caller
pass a base::CancelableTaskTracker. The lifetime of the callback can be
associated to the request by using base::Passed().

Remove unused types and fix usage of the values passed to the result
callback (check success before using the URLRow or the VisitVector).

BUG=371818
TBR=bauerb,sky,asargent,gavinp,shess,pkasting

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

[sdefresne, 2014-06-06 14:58:19]

blundell: Please take a look.
brettw: FYI

[blundell, 2014-06-10 08:14:36]

Could you update the CL description to explain what the porting process
involved? Thanks!

[sdefresne, 2014-06-11 08:33:14]

Done. Please have another look.

[blundell, 2014-06-11 11:08:41]

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/custom_ho...
File chrome/browser/custom_home_pages_table_model.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/custom_ho...
chrome/browser/custom_home_pages_table_model.cc:234: if (taskId !=
base::CancelableTaskTracker::kBadTaskId)
nit: this if seems unnecessary.

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:315: if (success &&
!visits.empty()) {
The value of success is the same as the value of visits being non-NULL (in the
old version of the callback)?

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_service.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_service.cc:722: return
tracker->PostTaskAndReply(thread_->message_loop_proxy().get(),
Could PostTaskAndReplyWithResult simplify the  passing of the success value from
QueryURL to RunWithQueryURLResult?

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_unittest.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_unittest.cc:1061: CancelableRequestConsumer
consumer_;
This is still used by other code in this file I take it?

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1857: const history::VisitVector&
visists) {
nit: could you rename this to visits while you're here?

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (left):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:333:
StorePendingQuery(handle, request, callback);
Can StorePendingQuery() and GetPendingQuery() go away now?

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:331:
base::Passed(&owned_request),
Is there a reason this can't just be base::Unretained() as well?

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service_unittest.cc:76: if (success)
{
same question as before re: success.

[sdefresne, 2014-06-11 16:46:26]

PTAL

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/custom_ho...
File chrome/browser/custom_home_pages_table_model.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/custom_ho...
chrome/browser/custom_home_pages_table_model.cc:234: if (taskId !=
base::CancelableTaskTracker::kBadTaskId)
On 2014/06/11 11:08:40, blundell wrote:
> nit: this if seems unnecessary.

Done.

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/history/history_api.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/history/history_api.cc:315: if (success &&
!visits.empty()) {
On 2014/06/11 11:08:40, blundell wrote:
> The value of success is the same as the value of visits being non-NULL (in the
> old version of the callback)?

The QueryURL callback description says "If success is false, neither the row nor
the vector will be valid." So the code incorrectly used the visits parameter
previously. When success is false, the pointer was non-NULL but the vector was
empty.

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_service.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_service.cc:722: return
tracker->PostTaskAndReply(thread_->message_loop_proxy().get(),
On 2014/06/11 11:08:40, blundell wrote:
> Could PostTaskAndReplyWithResult simplify the  passing of the success value
from
> QueryURL to RunWithQueryURLResult?

Done.

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_unittest.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_unittest.cc:1061: CancelableRequestConsumer
consumer_;
On 2014/06/11 11:08:40, blundell wrote:
> This is still used by other code in this file I take it?

Yes.

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:1857: const history::VisitVector&
visists) {
On 2014/06/11 11:08:40, blundell wrote:
> nit: could you rename this to visits while you're here?

Done.

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (left):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:333:
StorePendingQuery(handle, request, callback);
On 2014/06/11 11:08:40, blundell wrote:
> Can StorePendingQuery() and GetPendingQuery() go away now?

No, they are still used by other functions that post tasks using
CancelableRequestConsumer/CancelableRequestProvider.

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/browser_feature_extractor.cc:331:
base::Passed(&owned_request),
On 2014/06/11 11:08:40, blundell wrote:
> Is there a reason this can't just be base::Unretained() as well?

The description of ExtractFeatures explain why:
  // Begins extraction of the browser features.  We take ownership
  // of the request object until |callback| is called (see DoneCallback above)
  // and will write the extracted features to the feature map.  Once the
  // feature extraction is complete, |callback| is run on the UI thread.  We
  // take ownership of the |callback| object.  |info| may not be valid after
  // ExtractFeatures returns.  This method must run on the UI thread.

The ExtractFeatures function launch a succession of asynchronous request
(StartExtractFeatures is just one such step). When an asynchronous request is in
progress, the ClientPhishingRequest value was associated to the request Handle
(this is what StorePendingQuery/GetPendingQuery do). When the
BrowserFeatureExtractor is killed, all the objects in that map were deleted.

With the new code, we just pass the pointer with base::Passed which means that
the object will be owned by the callback until it is called, or deleted when the
callback is deleted. Since all the pending requests are killed when the
CancelableTaskTracker is deleted, then the lifetime of the ClientPhishingRequest
is the same with old code and new code.

Do you want me to put this in a comment somewhere? This will most likely be the
pattern used by all the functions at the end of the migration. If you want me to
add the comment and this is your only nit, could you lgtm with nit?

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/314293005/diff/60001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_service_unittest.cc:76: if (success)
{
On 2014/06/11 11:08:41, blundell wrote:
> same question as before re: success.

Same answer, except that row was never NULL, so the code was incorrect (it is a
unit test, so I guess they never tested the error path).

[blundell, 2014-06-12 09:21:18]

LGTM with suggestion

https://codereview.chromium.org/314293005/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/314293005/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/browser_feature_extractor.cc:316:
ClientPhishingRequest* request,
How much noise would it add to pass this parameter as a scoped_ptr?

https://codereview.chromium.org/314293005/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/browser_feature_extractor.cc:326:
scoped_ptr<ClientPhishingRequest> owned_request(request);
If you don't do what I suggest above, yes, it would be good to add a comment
here.

[sdefresne, 2014-06-12 11:18:01]

https://codereview.chromium.org/314293005/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (right):

https://codereview.chromium.org/314293005/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/browser_feature_extractor.cc:316:
ClientPhishingRequest* request,
On 2014/06/12 09:21:18, blundell wrote:
> How much noise would it add to pass this parameter as a scoped_ptr?

Yes, this is putting much noise in the rest of the file.

https://codereview.chromium.org/314293005/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/browser_feature_extractor.cc:326:
scoped_ptr<ClientPhishingRequest> owned_request(request);
On 2014/06/12 09:21:18, blundell wrote:
> If you don't do what I suggest above, yes, it would be good to add a comment
> here.

Done.

[sdefresne, 2014-06-12 12:51:36]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-12 12:55:45]

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

[sdefresne, 2014-06-12 14:57:34]

TBR-ing OWNERS of refactored client code

TBR=bauerb for //chrome/browser/browsing_data
TBR=sky for //chrome/browser/custom_home_pages_table_model.*
TBR=asargent for //chrome/browser/extensions/api
TBR=gavinp for //chrome/browser/prerender
TBR=shess for //chrome/browser/safe_browsing
TBR=pkasting for //chrome/browser/search_engines

[commit-bot: I haz the power, 2014-06-12 15:17:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-12 15:23:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-12 15:23:46]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[sdefresne, 2014-06-12 15:56:52]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-12 15:58:29]

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

[asargent_no_longer_on_chrome, 2014-06-12 16:14:31]

extensions lgtm

[Peter Kasting, 2014-06-12 17:22:35]

LGTM

[Scott Hess - ex-Googler, 2014-06-12 18:04:35]

LGTM for safe_browsing.  Your option if you want to consider my nits elsewhere,
either in an update to this CL (if an opportunity rises) or a cleanup CL.

https://codereview.chromium.org/314293005/diff/160001/chrome/browser/custom_h...
File chrome/browser/custom_home_pages_table_model.cc (right):

https://codereview.chromium.org/314293005/diff/160001/chrome/browser/custom_h...
chrome/browser/custom_home_pages_table_model.cc:258:
observer_->OnItemsChanged(static_cast<int>(entry_index), 1);
This case seems wrong ... but assigning a size_t to an int above also seems
wrong.  Maybe entry_index should be a size_t?

https://codereview.chromium.org/314293005/diff/160001/chrome/browser/history/...
File chrome/browser/history/history_backend.cc (right):

https://codereview.chromium.org/314293005/diff/160001/chrome/browser/history/...
chrome/browser/history/history_backend.cc:1004: if (result.success &&
want_visits) {
Can drop the {} to be consistent with the style in this file.

[commit-bot: I haz the power, 2014-06-12 18:08:52]

Change committed as 276739

[sdefresne, 2014-06-12 18:15:34]

On 2014/06/12 18:04:35, shess wrote:
> LGTM for safe_browsing.  Your option if you want to consider my nits
elsewhere,
> either in an update to this CL (if an opportunity rises) or a cleanup CL.
> 
>
https://codereview.chromium.org/314293005/diff/160001/chrome/browser/custom_h...
> File chrome/browser/custom_home_pages_table_model.cc (right):
> 
>
https://codereview.chromium.org/314293005/diff/160001/chrome/browser/custom_h...
> chrome/browser/custom_home_pages_table_model.cc:258:
> observer_->OnItemsChanged(static_cast<int>(entry_index), 1);
> This case seems wrong ... but assigning a size_t to an int above also seems
> wrong.  Maybe entry_index should be a size_t?
> 
>
https://codereview.chromium.org/314293005/diff/160001/chrome/browser/history/...
> File chrome/browser/history/history_backend.cc (right):
> 
>
https://codereview.chromium.org/314293005/diff/160001/chrome/browser/history/...
> chrome/browser/history/history_backend.cc:1004: if (result.success &&
> want_visits) {
> Can drop the {} to be consistent with the style in this file.

Thank you. I'll address those in a followup CL.

[Feng Qian, 2014-06-12 22:09:14]

A revert of this CL has been created in
https://codereview.chromium.org/330703005/ by feng@chromium.org.

The reason for reverting is: Break Clank ToT build:

https://uberchromegw.corp.google.com/i/clank.tot/builders/instrumentation-yak....