Add UMA metrics for pop-up blocked page action on desktop

Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop 
popup actions; There is no single dismiss button, and count in 
destructor will count all the displays.

BUG=650132
Review-Url: https://codereview.chromium.org/2588273002
Cr-Commit-Position: refs/heads/master@{#441870}
Committed: https://chromium.googlesource.com/chromium/src/+/e5e6f6ee653ae95d6fb7f9a408c7370348e14fee

[charleszhao, 2016-12-20 04:53:25]

Description was changed from

==========
histogram-popup-actions
(1) Add counters for popup actions (PC only in this CL)
(2) Add unittest.

BUG=650132
==========

to

==========
histogram-popup-actions
(1) Add counters for popup actions (PC only in this CL.)
(2) Add unittest.

"Dissmiss" is not counted, because it's not very meaningful for PC popup
actions; There is no single dismiss button, and count in destructor will count
all the displays.

BUG=650132
==========

[charleszhao, 2016-12-20 05:03:29]

charleszhao@chromium.org changed reviewers:
+ dominickn@chromium.org

[charleszhao, 2016-12-20 05:03:30]

Thanks Dom,

I'll add Android and IOS counter in another CL.

[dominickn, 2016-12-20 05:31:48]

Thanks for looking at this and adding a test! I have just small things.

1. Make the title "Add UMA metrics for pop-up blocked page action on desktop"

2. Wrap the description to 80 characters

3. Use the term "desktop" instead of "PC"

4. "Dissmiss" in the description should be "Dismiss"

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/chrome_content_settings_utils.cc (right):

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/chrome_content_settings_utils.cc:25: 
Nit: remove the extra newline here (have only one)

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/chrome_content_settings_utils.h (right):

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/chrome_content_settings_utils.h:41: // UMA
histogram for the pop-ups broken puzzle piece. The enum values
Nit: change the comment to be:

"The different actions that a user can perform on the pop-up blocked page action
in the omnibox, The enum values back an UMA histogram, so they must be treated
as append-only."

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/chrome_content_settings_utils.h:52: void
RecordPopupsAction(PopupsAction action);
Nit: newline before the } that closes the namespace

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/ui/content_s...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:232:
#if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_WIN)
Is there a reason for this guard? It only excludes OS_CHROMEOS right now
(browser_tests are only run on desktop platforms), and I'm fairly sure that we
should have this running on ChromeOS too.

[charleszhao, 2016-12-20 22:04:01]

Description was changed from

==========
histogram-popup-actions
(1) Add counters for popup actions (PC only in this CL.)
(2) Add unittest.

"Dissmiss" is not counted, because it's not very meaningful for PC popup
actions; There is no single dismiss button, and count in destructor will count
all the displays.

BUG=650132
==========

to

==========
Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop popup
actions; There is no single dismiss button, and count in destructor will count
all the displays.

BUG=650132
==========

[charleszhao, 2016-12-20 22:04:21]

Description was changed from

==========
Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop popup
actions; There is no single dismiss button, and count in destructor will count
all the displays.

BUG=650132
==========

to

==========
Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop popup
actions; 
There is no single dismiss button, and count in destructor will count all the
displays.

BUG=650132
==========

[charleszhao, 2016-12-20 22:05:52]

Description was changed from

==========
Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop popup
actions; 
There is no single dismiss button, and count in destructor will count all the
displays.

BUG=650132
==========

to

==========
Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop 
popup actions; There is no single dismiss button, and count in 
destructor will count all the displays.

BUG=650132
==========

[charleszhao, 2016-12-20 22:43:50]

All done. Thanks Dom.

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/chrome_content_settings_utils.cc (right):

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/chrome_content_settings_utils.cc:25: 
On 2016/12/20 05:31:48, dominickn wrote:
> Nit: remove the extra newline here (have only one)

Done.

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/chrome_content_settings_utils.h (right):

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/chrome_content_settings_utils.h:41: // UMA
histogram for the pop-ups broken puzzle piece. The enum values
On 2016/12/20 05:31:48, dominickn wrote:
> Nit: change the comment to be:
> 
> "The different actions that a user can perform on the pop-up blocked page
action
> in the omnibox, The enum values back an UMA histogram, so they must be treated
> as append-only."

Done.

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/chrome_content_settings_utils.h:52: void
RecordPopupsAction(PopupsAction action);
On 2016/12/20 05:31:48, dominickn wrote:
> Nit: newline before the } that closes the namespace

Done.

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/ui/content_s...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/2588273002/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:232:
#if defined(OS_MACOSX) || defined(OS_LINUX) || defined(OS_WIN)
On 2016/12/20 05:31:48, dominickn wrote:
> Is there a reason for this guard? It only excludes OS_CHROMEOS right now
> (browser_tests are only run on desktop platforms), and I'm fairly sure that we
> should have this running on ChromeOS too.

Thanks, I was thinking to prevent the test running on Andriod and IOS, which
seems not necessary.

[dominickn, 2016-12-20 22:54:59]

lgtm, thanks!

[charleszhao, 2016-12-21 00:36:52]

charleszhao@chromium.org changed reviewers:
+ jwd@chromium.org, markusheintz@google.com

[charleszhao, 2016-12-21 00:36:53]

Hi, markusheintz need owner review for these 6 files:
chrome/browser/content_settings/chrome_content_settings_utils.cc [13] 
chrome/browser/content_settings/chrome_content_settings_utils.h [13] 
chrome/browser/content_settings/tab_specific_content_settings.cc [13] 
chrome/browser/ui/content_settings/content_setting_bubble_model.cc [14] 
chrome/browser/ui/content_settings/content_setting_bubble_model.h [14]
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
[14]

Hi jwd, need owner review for 1 file:
tools/metrics/histograms/histograms.xml [10]

Thanks all.

[charleszhao, 2016-12-21 09:39:04]

The CQ bit was checked by charleszhao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 09:39:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 11:04:47]

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

[commit-bot: I haz the power, 2016-12-21 11:04:48]

Dry run: This issue passed the CQ dry run.

[jwd, 2016-12-21 19:21:34]

lgtm

[charleszhao, 2017-01-02 22:25:15]

charleszhao@chromium.org changed reviewers:
+ thakis@chromium.org
- markusheintz@google.com

[charleszhao, 2017-01-02 22:26:51]

On 2016/12/21 19:21:34, jwd wrote:
> lgtm

Thanks!

Hi, Nico, need owner review for these 6 files:
chrome/browser/content_settings/chrome_content_settings_utils.cc [13] 
chrome/browser/content_settings/chrome_content_settings_utils.h [13] 
chrome/browser/content_settings/tab_specific_content_settings.cc [13] 
chrome/browser/ui/content_settings/content_setting_bubble_model.cc [14] 
chrome/browser/ui/content_settings/content_setting_bubble_model.h [14]
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
[14]

[Nico, 2017-01-03 14:34:07]

my files lgtm with nits

https://codereview.chromium.org/2588273002/diff/20001/chrome/browser/content_...
File chrome/browser/content_settings/chrome_content_settings_utils.h (right):

https://codereview.chromium.org/2588273002/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/chrome_content_settings_utils.h:42: // action in
the omnibox, The enum values back an UMA histogram, so they must be
don't capitalize after ,

Also, Use the same sentence structure as for the other enums in this file, i.e.
"UMA histogram for actions that a cuser can perform on the pop-up blocked page
action in the omnibox. The enum values correspond to histogram entries, so do
not remove any existing values." instead of what you have

https://codereview.chromium.org/2588273002/diff/20001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2588273002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:107: 
// static

https://codereview.chromium.org/2588273002/diff/20001/chrome/browser/ui/conte...
File
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc
(right):

https://codereview.chromium.org/2588273002/diff/20001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model_browsertest.cc:243:
// Tests that each popup action is counted in the right bucket.
Nice!

[charleszhao, 2017-01-06 02:32:38]

The CQ bit was checked by charleszhao@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:33:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[charleszhao, 2017-01-06 02:35:03]

The CQ bit was unchecked by charleszhao@chromium.org

[charleszhao, 2017-01-06 03:12:10]

The CQ bit was checked by charleszhao@chromium.org

[charleszhao, 2017-01-06 03:12:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, thakis@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2588273002/#ps40001
(title: "Changed the comment of enum PopupsAction")

[commit-bot: I haz the power, 2017-01-06 03:12:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 04:09:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483672330577560,
"parent_rev": "6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507", "commit_rev":
"e5e6f6ee653ae95d6fb7f9a408c7370348e14fee"}

[commit-bot: I haz the power, 2017-01-06 04:10:14]

Description was changed from

==========
Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop 
popup actions; There is no single dismiss button, and count in 
destructor will count all the displays.

BUG=650132
==========

to

==========
Add UMA metrics for pop-up blocked page action on desktop
(1) Add counters for popup actions (desktop only in this CL.)
(2) Add unittest.

"Dismiss" is not counted, because it's not very meaningful for desktop 
popup actions; There is no single dismiss button, and count in 
destructor will count all the displays.

BUG=650132

Review-Url: https://codereview.chromium.org/2588273002
Cr-Commit-Position: refs/heads/master@{#441870}
Committed:
https://chromium.googlesource.com/chromium/src/+/e5e6f6ee653ae95d6fb7f9a408c7...
==========

[commit-bot: I haz the power, 2017-01-06 04:10:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e5e6f6ee653ae95d6fb7f9a408c7...