Add browsertest for the feedback service

Add a browsertest for the feedback service, to serve as a basis for more extensive testing for Issue 146003005.

BUG=NONE

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

[felt, 2014-03-25 21:25:44]

Hi Asanka,

This adds a browser test for the feedback service. However, it doesn't quite
work. At the end of the test (simplified snippet below):

BeginFeedbackForDownload(downloads[0]);
GetDownloads(browser(), &downloads);
ASSERT_TRUE(downloads.empty());

I've verified that BeginFeedbackForDownload is working as intended, calling
Remove() and Cancel() on the download. However, downloads isn't empty, which is
contrary to what I expected. Am I missing something here?

Thanks,
Adrienne

[asanka, 2014-03-26 18:01:30]

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3256: observer.WaitForStored();
This observer only waits for the download to begin being stored into history.
This may be too early. Use a DownloadTestObserverInProgress instead. That will
at least wait for the filename determination to complete. The download should
then be reliably dangerous when observer.WaitForEvent() returns.

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3293: GetDownloads(browser(),
&downloads);
GetDownloads() appends to the vector that's passed into it. It doesn't clear the
list. So if the list wasn't empty at this point, then it's not going to be empty
after the call even if there are no downloads.

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
File chrome/browser/download/download_test_file_activity_observer.cc (right):

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:51: void
ChangeDangerTypeForFeedbackTest() {
Suggestion: Use a more generic name since this is a shared API. Perhaps
something like SetDangerTypeForNewDownloads(content::DownloadDangerType type) ?
Make sure MockDownloadManagerDelegate preserves its previous behavior if that
method is never called.

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:52:
downloads_dangerous_ = true;
Use a more descriptive name? mark_downloads_as_dangerous_ ?

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:59:
item->GetDangerType() == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE) {
If you are forcing a danger type, I'd propose just doing it rather than
conditionalize it the item's current danger type. Also, the danger type isn't
set until ChromeDownloadManagerDelegate::DetermineDownloadTarget() returns.

[felt, 2014-06-03 00:35:22]

BLAST FROM THE PAST! Fixed these issues, except for one that I'm not sure is
possible to do cleanly (but happy for suggestions if you have any).

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3256: observer.WaitForStored();
On 2014/03/26 18:01:30, asanka wrote:
> This observer only waits for the download to begin being stored into history.
> This may be too early. Use a DownloadTestObserverInProgress instead. That will
> at least wait for the filename determination to complete. The download should
> then be reliably dangerous when observer.WaitForEvent() returns.

I think this is fixed.

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3293: GetDownloads(browser(),
&downloads);
On 2014/03/26 18:01:30, asanka wrote:
> GetDownloads() appends to the vector that's passed into it. It doesn't clear
the
> list. So if the list wasn't empty at this point, then it's not going to be
empty
> after the call even if there are no downloads.

Done.

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
File chrome/browser/download/download_test_file_activity_observer.cc (right):

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:51: void
ChangeDangerTypeForFeedbackTest() {
On 2014/03/26 18:01:30, asanka wrote:
> Suggestion: Use a more generic name since this is a shared API. Perhaps
> something like SetDangerTypeForNewDownloads(content::DownloadDangerType type)
?
> Make sure MockDownloadManagerDelegate preserves its previous behavior if that
> method is never called.

My problem here is that I don't know how to get the global danger type into the
static SetDangerous callback, which is a DownloadTargetCallback. I don't want to
modify DownloadTargetCallback, nor do I want to make a switch statement and 5
versions of SetDangerous. There must be a cleaner way that I am not thinking of,
any ideas?

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:52:
downloads_dangerous_ = true;
On 2014/03/26 18:01:30, asanka wrote:
> Use a more descriptive name? mark_downloads_as_dangerous_ ?

Done.

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:59:
item->GetDangerType() == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE) {
On 2014/03/26 18:01:30, asanka wrote:
> If you are forcing a danger type, I'd propose just doing it rather than
> conditionalize it the item's current danger type. Also, the danger type isn't
> set until ChromeDownloadManagerDelegate::DetermineDownloadTarget() returns.

I believe this should be fixed now.

[asanka, 2014-06-05 19:20:13]

Sorry about the delay!

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
File chrome/browser/download/download_test_file_activity_observer.cc (right):

https://codereview.chromium.org/197883010/diff/180001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:51: void
ChangeDangerTypeForFeedbackTest() {
On 2014/06/03 00:35:22, felt wrote:
> On 2014/03/26 18:01:30, asanka wrote:
> > Suggestion: Use a more generic name since this is a shared API. Perhaps
> > something like SetDangerTypeForNewDownloads(content::DownloadDangerType
type)
> ?
> > Make sure MockDownloadManagerDelegate preserves its previous behavior if
that
> > method is never called.
> 
> My problem here is that I don't know how to get the global danger type into
the
> static SetDangerous callback, which is a DownloadTargetCallback. I don't want
to
> modify DownloadTargetCallback, nor do I want to make a switch statement and 5
> versions of SetDangerous. There must be a cleaner way that I am not thinking
of,
> any ideas?

static void SetDangerous(const DownloadTargetCallback& callback,
      DownloadDangerType forced_danger_type,
      ...);

And when binding the callback:

base::Bind(&SetDangerous, callback, global_danger_type_)

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3244:
IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackService) {
Let's wrap this in #if defined(FULL_SAFE_BROWSING).

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
File chrome/browser/download/download_test_file_activity_observer.cc (right):

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:62:
item->GetDangerType() != content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) {
Nit: Might be simpler to just route the callback through SetDangerous whenever
mark_downloads_dangerous_ is true.

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.cc:104:
content::DownloadDangerType global_danger_type_;
Nit: maybe call it "forced_danger_type_" ? Just a suggestion, but I was a bit
confused by the global_ prefix.

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
File chrome/browser/download/download_test_file_activity_observer.h (right):

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
chrome/browser/download/download_test_file_activity_observer.h:35: // Will set
the danger type to |danger_type} for all dangerous files as long
Nit: |danger_type|

[felt, 2014-06-07 15:45:05]

Thanks Asanka.

I'm not sure what I was thinking when I wrote this 2 months ago, but present-day
me thinks I do not need the complexity of the callback part at all. The test
works fine with a regular DANGEROUS_FILE file, it doesn't need an
UNCOMMON_CONTENT file. So: simplified now.

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/197883010/diff/210001/chrome/browser/download...
chrome/browser/download/download_browsertest.cc:3244:
IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackService) {
On 2014/06/05 19:20:12, asanka wrote:
> Let's wrap this in #if defined(FULL_SAFE_BROWSING).

Done.

[asanka, 2014-06-09 20:25:35]

Great :) LGTM

[felt, 2014-06-09 20:28:50]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-06-09 20:31:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/felt@chromium.org/197883010/260004

[commit-bot: I haz the power, 2014-06-10 09:43:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  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-10 14:27:27]

Change committed as 276017