Opt out of download feedback from settings

Adds the ability to opt out of Safe Browsing download feedback from settings.

Notes:
- The download feedback pref is an enum with a ternary (not binary) value. As a result, need to set up a special handler to toggle the checkbox appropriately.
- Moved DownloadReportingStatus enum from DownloadFeedbackDialogView into DownloadFeedbackService. This lets BrowserOptionsHandler use the enum.

-------
How to test (currently Win & CrOS only):

A
1. Go to chrome://settings on a clean build; search for 'Privacy'
2. Verify that "Send suspicious downloaded files to Google" is not checked

B
1. Visit http://testsafebrowsing.appspot.com/, click on #5
2. Click discard in the download item
3. Click 'No thanks' in the dialog
4. Go to chrome://settings; search for 'Privacy'
5. Verify that "Send suspicious downloaded files to Google" is not checked

C
1. Clear your user preferences (e.g., delete the Preferences file in your user data directory)
2. Visit http://testsafebrowsing.appspot.com/, click on #5
3. Click discard in the download item
4. Click 'OK' in the dialog
5. Go to chrome://settings; search for 'Privacy'
6. Verify that "Send suspicious downloaded files to Google" is checked
-----------

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

[felt, 2014-02-11 04:13:51]

Hi Dan,

Can you please review this CL?

One small open question: what is the metric="..." field? Am I supposed to put
the value somewhere, like in histograms.xml?

Thanks,
Adrienne

[Dan Beam, 2014-02-12 22:53:01]

lgtm w/nits

> what is the metric="..." field?

metric="..." corresponds to a user metric action (just a count of how many times
this checkbox has been checked, i think...)

adding a tools/metrics/ owner (isherman@) as you might need to update
tools/metrics/actions/{chromeactions.txt,extract_actions.py}

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
chrome/browser/ui/webui/options/browser_options_handler.cc:732:
base::Bind(&BrowserOptionsHandler::DownloadFeedbackStatusChanged,
nit: HandleDownloadFeedbackStatusChanged

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
chrome/browser/ui/webui/options/browser_options_handler.cc:1555:
args->GetBoolean(0, &enabled);
nit:

  bool success = args->GetBoolean(0, &enabled);
  DCHECK(success);

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
chrome/browser/ui/webui/options/browser_options_handler.cc:1565: }
nit:

  pref_service->SetInteger(
      prefs::kSafeBrowsingDownloadReportingEnabled,
      enabled ? DownloadFeedbackService::kDownloadReportingEnabled :
                DownloadFeedbackService::kDownloadReportingDisabled);

[Dan Beam, 2014-02-12 22:55:48]

actually, wait -- can't you use a boolean and determine whether the pref has
ever been set via PrefService::HasPrefPath() instead of making it an enum with 3
states (one of which is "hasn't been set, essentially")?

https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...

[Dan Beam, 2014-02-12 22:56:15]

use a boolean => register as a boolean pref**

[felt, 2014-02-13 05:17:58]

asvitkine@, can you tell me what I should do with the metric="..." field?
tools/metrics/actions/chromeactions.txt doesn't look like I ought to edit it
directly. Is there a scraping script I'm supposed to run?

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
chrome/browser/ui/webui/options/browser_options_handler.cc:732:
base::Bind(&BrowserOptionsHandler::DownloadFeedbackStatusChanged,
On 2014/02/12 22:53:04, Dan Beam wrote:
> nit: HandleDownloadFeedbackStatusChanged

Done.

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
chrome/browser/ui/webui/options/browser_options_handler.cc:1555:
args->GetBoolean(0, &enabled);
On 2014/02/12 22:53:04, Dan Beam wrote:
> nit:
> 
>   bool success = args->GetBoolean(0, &enabled);
>   DCHECK(success);

Done.

https://chromiumcodereview.appspot.com/153353006/diff/100001/chrome/browser/u...
chrome/browser/ui/webui/options/browser_options_handler.cc:1565: }
On 2014/02/12 22:53:04, Dan Beam wrote:
> nit:
> 
>   pref_service->SetInteger(
>       prefs::kSafeBrowsingDownloadReportingEnabled,
>       enabled ? DownloadFeedbackService::kDownloadReportingEnabled :
>                 DownloadFeedbackService::kDownloadReportingDisabled);

Done.

[felt, 2014-02-13 05:22:34]

pkasting, I'll need your OWNERS review for chrome/browser/ui/views/download/*.

But first: Dan suggested that I remove the DownloadReportingStatus enum, which
has states for the dialog never being shown, being enabled, and being disabled.
Instead, I would add a boolean pref and then check HasPrefPath() to see if it's
ever been set. 
-- One of the benefits of using the enum is that it's very explicit which case
is being handled in download_item_view.cc.
-- OTOH, if I use the boolean/HasPrefPath() then I don't need custom handling
for the ternary pref state in browser_options_handler.cc.

Do you have a strong preference either way? Thought I'd check before
refactoring.

Adrienne

[Alexei Svitkine (slow), 2014-02-13 15:55:51]

I think the metric= field does indeed introduce a new User Action.

Can you run "tools/metrics/actions/extract_actions.py --hash" and add the
updated chromeactions.txt to this CL?

[Peter Kasting, 2014-02-13 18:22:48]

On 2014/02/13 05:22:34, felt wrote:
> pkasting, I'll need your OWNERS review for chrome/browser/ui/views/download/*.
> 
> But first: Dan suggested that I remove the DownloadReportingStatus enum, which
> has states for the dialog never being shown, being enabled, and being
disabled.
> Instead, I would add a boolean pref and then check HasPrefPath() to see if
it's
> ever been set. 

Didn't we go round and round this issue on
https://codereview.chromium.org/147593002/ ?  I'm sort of surprised this needs
to immediately be refactored right after it landed.

[felt, 2014-02-13 21:02:43]

On 2014/02/13 15:55:51, Alexei Svitkine wrote:
> I think the metric= field does indeed introduce a new User Action.
> 
> Can you run "tools/metrics/actions/extract_actions.py --hash" and add the
> updated chromeactions.txt to this CL?

OK, done.

[felt, 2014-02-13 21:03:49]

On 2014/02/13 18:22:48, Peter Kasting wrote:
> On 2014/02/13 05:22:34, felt wrote:
> > pkasting, I'll need your OWNERS review for
chrome/browser/ui/views/download/*.
> > 
> > But first: Dan suggested that I remove the DownloadReportingStatus enum,
which
> > has states for the dialog never being shown, being enabled, and being
> disabled.
> > Instead, I would add a boolean pref and then check HasPrefPath() to see if
> it's
> > ever been set. 
> 
> Didn't we go round and round this issue on
> https://codereview.chromium.org/147593002/ ?  I'm sort of surprised this needs
> to immediately be refactored right after it landed.

You're right, I didn't entirely understand how the prefs worked when I wrote
that CL. In lieu of a time machine, I have hopefully corrected that in this CL.
Now using a boolean pref. PTAL.

[Peter Kasting, 2014-02-13 22:17:51]

LGTM

https://chromiumcodereview.appspot.com/153353006/diff/300001/chrome/browser/u...
File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/153353006/diff/300001/chrome/browser/u...
chrome/browser/ui/views/download/download_feedback_dialog_view.cc:56:
callback.Run(false /* disabled */);
Nit: Don't add these sorts of parameter-explaining comments (here or below).

Instead, consider a typedef for the callback in the .h file that describes what
the param means.  In the limit, if this still isn't clear at all, switch from
using a bool to an enum so that these callsites become self-documenting.  (I
think in this case that's overkill.)

https://chromiumcodereview.appspot.com/153353006/diff/300001/chrome/browser/u...
chrome/browser/ui/views/download/download_feedback_dialog_view.cc:99: return
true;
Nit: Because the bodies of these two functions are so similar, maybe they should
both be something akin to:

  return OnButtonClicked(false);

...where OnButtonClicked() is the shared implementation, which takes the
pref/callback value, and always returns true (for caller convenience).

[felt, 2014-02-14 00:21:49]

https://codereview.chromium.org/153353006/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right):

https://codereview.chromium.org/153353006/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_feedback_dialog_view.cc:56:
callback.Run(false /* disabled */);
On 2014/02/13 22:17:52, Peter Kasting wrote:
> Nit: Don't add these sorts of parameter-explaining comments (here or below).
> 
> Instead, consider a typedef for the callback in the .h file that describes
what
> the param means.  In the limit, if this still isn't clear at all, switch from
> using a bool to an enum so that these callsites become self-documenting.  (I
> think in this case that's overkill.)

Done, added typedef for the callback.

https://codereview.chromium.org/153353006/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_feedback_dialog_view.cc:99: return
true;
On 2014/02/13 22:17:52, Peter Kasting wrote:
> Nit: Because the bodies of these two functions are so similar, maybe they
should
> both be something akin to:
> 
>   return OnButtonClicked(false);
> 
> ...where OnButtonClicked() is the shared implementation, which takes the
> pref/callback value, and always returns true (for caller convenience).

Done. Also merged ReleaseDialogStatusHold into the new OnButtonClicked method,
since it was only called within Cancel() and Accept().

[felt, 2014-02-14 00:24:03]

Elliot, can I get an OWNERS review for chrome/browser/profiles/profile.cc? Tiny
change -- just a renamed pref.

Adrienne

[Elliot Glaysher, 2014-02-14 00:31:24]

owners stamp lgtm

[felt, 2014-02-14 05:32:27]

alexei, please do an OWNERS review for tools/metrics. thanks!

[Alexei Svitkine (slow), 2014-02-14 14:50:24]

metrics lgtm

[felt, 2014-02-14 15:18:51]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-02-14 15:19:25]

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

[commit-bot: I haz the power, 2014-02-14 15:19:33]

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

[commit-bot: I haz the power, 2014-02-14 15:19:34]

Failed to apply patch for tools/metrics/actions/chromeactions.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/metrics/actions/chromeactions.txt
  Hunk #1 FAILED at 1447.
  Hunk #2 succeeded at 1486 (offset 5 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
tools/metrics/actions/chromeactions.txt.rej

Patch:       tools/metrics/actions/chromeactions.txt
Index: tools/metrics/actions/chromeactions.txt
diff --git a/tools/metrics/actions/chromeactions.txt
b/tools/metrics/actions/chromeactions.txt
index
4b7e1090569de7e13dde4a953f61e028ca6b6601..ca8618e90e395b18f2af9465ad3b24bea7a2923e
100644
--- a/tools/metrics/actions/chromeactions.txt
+++ b/tools/metrics/actions/chromeactions.txt
@@ -1447,6 +1447,8 @@
 0xc8faba7110ff04c3	Options_Homepage_ShowHomeButton
 0x26cdb65b6a92b56a	Options_Homepage_UseNewTab
 0xca6dd0b172e6df26	Options_Homepage_UseURL
+0x5238873285e397d8	Options_HotwordCheckbox_Disable
+0xce7307701f1a21dd	Options_HotwordCheckbox_Enable
 0x3462530e3c04ab92	Options_ImagesCheckbox_Disable
 0x3b55d5acba24ebad	Options_ImagesCheckbox_Enable
 0xe82fe68817c3bf54	Options_InstantEnabled_Disable
@@ -1481,6 +1483,8 @@
 0x6b2fbab68d97bb5b	Options_SSL3_Enable
 0x08e75d386343cc27	Options_SafeBrowsingCheckbox_Disable
 0xc951b246a33514ac	Options_SafeBrowsingCheckbox_Enable
+0x96ebcd98112d0930	Options_SafeBrowsingDownloadFeedbackCheckbox_Disable
+0xab0f183ebf8995bb	Options_SafeBrowsingDownloadFeedbackCheckbox_Enable
 0x7653dc09eac4e2bf	Options_SearchEngineChanged
 0x47fd936f55f6abd4	Options_SensitivitySlider_Changed
 0xb6e98e1635e14c7d	Options_SetAsDefaultBrowser

[felt, 2014-02-14 15:27:47]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-02-14 15:28:59]

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

[commit-bot: I haz the power, 2014-02-14 17:33:34]

Change committed as 251363