Port HistoryService::GetVisibleVisitCountToHost to CancelableTaskTracker

Port HistoryService::GetVisibleVisitCountToHost to CancelableTaskTracker

Callback no longer receive a HistoryService::Handle, but instead client
code should use the returned base::CancelableTaskTracker::TaskId to
cancel an individual task.

Simplify implementation of BrowserFeatureExtractor as all the methods
from HistoryService that are called uses base::CancelableTaskTracker by
removing the mapping from HistoryService::Handle to request and using a
scoped_ptr to deal with the lifetime of the ClientPhishingRequest object.

BUG=371818
TBR=jochen

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

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

[sdefresne, 2014-06-24 13:20:59]

blundell: Please take a look to the overall.

bryner: Please take a look at //chrome/browser/safe_browsing as I did a big
refactoring of it (cleanup as CancelableRequestProvider/Consumer is no longer
used by the HistoryService methods used). The new pattern to tie the lifetime of
the ClientPhishingRequest is following the one for the ClientMalwareRequest.

jochen: TBR for OWNERS of //chrome/browser/{download,ssl,website_settings}

[jochen (gone - plz use gerrit), 2014-06-24 14:00:33]

lgtm

[blundell, 2014-06-24 14:08:58]

LGTM

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

https://codereview.chromium.org/351553004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_types.h:515: // VisibleVisitCountToHostResult
encapsulate the result of a call to
nit: encapsulates.

https://codereview.chromium.org/351553004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_types.h:518: // Indicates whether the call to
HistoryBackend::GetVisibleVisitCountToHost
nit: should you declare the constructor/destructor is is done above?

https://codereview.chromium.org/351553004/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (left):

https://codereview.chromium.org/351553004/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:192: delete
extraction.first;  // delete request
now when the tracker is destroyed it will destroy all outstanding requests,
right?

[sdefresne, 2014-06-25 12:20:37]

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

https://codereview.chromium.org/351553004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_types.h:515: // VisibleVisitCountToHostResult
encapsulate the result of a call to
On 2014/06/24 14:08:58, blundell wrote:
> nit: encapsulates.

Done.

https://codereview.chromium.org/351553004/diff/1/chrome/browser/history/histo...
chrome/browser/history/history_types.h:518: // Indicates whether the call to
HistoryBackend::GetVisibleVisitCountToHost
On 2014/06/24 14:08:58, blundell wrote:
> nit: should you declare the constructor/destructor is is done above?

I declared it for the other structure because they contains complex objects
(std::vector) and android compilation complains that the constructor/destructor
should not be inlined for complex objects. Here the structure only contains
simple types, so I don't think the constructor/destructor are required. I'll add
them if compilation of android complains.

https://codereview.chromium.org/351553004/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/browser_feature_extractor.cc (left):

https://codereview.chromium.org/351553004/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/browser_feature_extractor.cc:192: delete
extraction.first;  // delete request
On 2014/06/24 14:08:58, blundell wrote:
> now when the tracker is destroyed it will destroy all outstanding requests,
> right?

It will invalidate all requests in flight, and since the ClientPhishingRequest
object is passed using base::Passed() then it is deleted. This is the way that
ClientMalwareRequest object lifetime is controlled.

[sdefresne, 2014-06-25 14:29:11]

+haaawk FYI

[sdefresne, 2014-06-25 17:39:31]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2014-06-25 17:39:56]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2014-06-25 17:40:03]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-25 17:41:10]

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

[commit-bot: I haz the power, 2014-06-25 22:13:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-25 23:18:30]

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

[commit-bot: I haz the power, 2014-06-25 23:18:31]

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

[Brian Ryner, 2014-06-26 01:54:32]

Sorry for the delayed response.  Noe, do you want to take a look at this for
Safe Browsing?

[sdefresne, 2014-06-26 17:24:14]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-26 17:26:03]

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

[commit-bot: I haz the power, 2014-06-26 21:25:15]

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

[commit-bot: I haz the power, 2014-06-26 21:36:03]

Change committed as 280110

[leng, 2014-06-26 22:27:47]

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

The reason for reverting is: Failures on Linux ASAN LSan Tests (2):

OnPhishingDetectionDoneDisabled
OnPhishingDetectionDoneMultiplePings
OnPhishingDetectionDoneNotPhishing
OnPhishingDetectionDoneShowInterstitia

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[sdefresne, 2014-06-30 16:08:39]

On 2014/06/26 22:27:47, leng wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/351363002/ by mailto:leng@chromium.org.
> 
> The reason for reverting is: Failures on Linux ASAN LSan Tests (2):
> 
> OnPhishingDetectionDoneDisabled
> OnPhishingDetectionDoneMultiplePings
> OnPhishingDetectionDoneNotPhishing
> OnPhishingDetectionDoneShowInterstitia
> 
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

Patch #5 fixes the memory leaks introduced by the patch. PTAL.

[blundell, 2014-07-01 07:37:27]

lgtm

[sdefresne, 2014-07-01 09:52:52]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-07-01 09:53:48]

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

[commit-bot: I haz the power, 2014-07-01 10:38:23]

Change committed as 280796