Collects stats on how user interacts with the bubble.

Collects stats on how user interacts with the bubble.

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

[Alexei Svitkine (slow), 2014-07-02 15:07:26]

https://codereview.chromium.org/341823005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/341823005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:94:
prefs::kMetricsReportingEnabled)->IsUserModifiable();
Hmm, I just noticed that this is wrong. Since you're running
ShouldOfferMetricsReporting on the FILE thread now, it's not allowed to access
local_state or g_browser_process, which should be being done on the UI thread.

Please fix this in a separate CL before landing this change. I suggest just
removing this helper and having line 163 simply call
GoogleUpdateSettings::GetCollectStatsConsent() and the doing the
FindPreference() and histogram code back on the UI thread in ShowForReal().

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:76:
callback_before_close_(base::Bind(&EmptyCallback)) {
I think you can initialize it with the default ctor and then just do:

if (!callback_before_close_.is_null())
  callback_before_close_.Run();

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.h (right):

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.h:83: void SetCloseCallback(base::Closure c)
{callback_before_close_ = c;}
This should use hacker_style (i.e. set_before_close_callback()) if you're
meaning to keep it inline in the .h file. Else, move its body to the .cc file.

Also, spaces between {}'s.

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.h:122: base::Closure callback_before_close_;
Nit: Rename to before_close_callback_

[yao, 2014-07-07 17:47:46]

Hi Jesse, 

Since Alexei is OOO, could you do the review for me? Thanks!

Yiyao

https://codereview.chromium.org/341823005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/341823005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:94:
prefs::kMetricsReportingEnabled)->IsUserModifiable();
On 2014/07/02 15:07:26, asvitkine (OOO back July 14th) wrote:
> Hmm, I just noticed that this is wrong. Since you're running
> ShouldOfferMetricsReporting on the FILE thread now, it's not allowed to access
> local_state or g_browser_process, which should be being done on the UI thread.
> 
> Please fix this in a separate CL before landing this change. I suggest just
> removing this helper and having line 163 simply call
> GoogleUpdateSettings::GetCollectStatsConsent() and the doing the
> FindPreference() and histogram code back on the UI thread in ShowForReal().

CL on the way.

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:76:
callback_before_close_(base::Bind(&EmptyCallback)) {
On 2014/07/02 15:07:26, asvitkine (OOO back July 14th) wrote:
> I think you can initialize it with the default ctor and then just do:
> 
> if (!callback_before_close_.is_null())
>   callback_before_close_.Run();

Done.

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.h (right):

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.h:83: void SetCloseCallback(base::Closure c)
{callback_before_close_ = c;}
On 2014/07/02 15:07:26, asvitkine (OOO back July 14th) wrote:
> This should use hacker_style (i.e. set_before_close_callback()) if you're
> meaning to keep it inline in the .h file. Else, move its body to the .cc file.
> 
> Also, spaces between {}'s.

Done.

https://codereview.chromium.org/341823005/diff/120001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.h:122: base::Closure callback_before_close_;
On 2014/07/02 15:07:26, asvitkine (OOO back July 14th) wrote:
> Nit: Rename to before_close_callback_

Done.

[jwd, 2014-07-09 15:37:52]

https://codereview.chromium.org/341823005/diff/200001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/341823005/diff/200001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:73:
SESSION_CRASHED_BUBBLE_ALREADY_UMA_OPTIN,
Is this being used anywhere anymore?

[yao, 2014-07-09 15:47:03]

https://codereview.chromium.org/341823005/diff/200001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/341823005/diff/200001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:73:
SESSION_CRASHED_BUBBLE_ALREADY_UMA_OPTIN,
On 2014/07/09 15:37:52, Jesse Doherty wrote:
> Is this being used anywhere anymore?

Done.

[jwd, 2014-07-09 18:08:18]

lgtm

[yao, 2014-07-09 18:10:10]

+msw for owner's review, Thanks

[msw, 2014-07-09 20:03:08]

I don't think changing BubbleFrameView is the right approach. Are you actually
concerned with specifically when the (x) button is pressed, or should this stat
also measure the use of the [Esc] key to close the bubble? It looks like the
later, since SESSION_CRASHED_BUBBLE_IGNORED also measures tab
navigation/closing/dragging methods of closing the bubble.

Instead, consider simply measuring bubble close via a OnWidgetDestroying
override. You can optionally report a *_CLOSE count anytime this happens, or
alternatively use a SessionCrashedBubbleView flag to record invocation of the
[Restore] button, and just record a *_IGNORED count when that flag isn't set on
close.

[yao, 2014-07-10 19:23:49]

Hi Mike, 

I changed it as how you suggested (with a flag).

Thanks,
Yiyao

[msw, 2014-07-10 19:58:37]

lgtm with nits

https://codereview.chromium.org/341823005/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/341823005/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:363: restored_ = true;
nit: you should probably record the histogram value and set this flag in
RestorePreviousSession, just in case there are (or will eventually be) other
ways to invoke that.

https://codereview.chromium.org/341823005/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:390: }
nit: revert this (indent was correct).

[yao, 2014-07-10 20:19:12]

Thanks for your review Mike :)

https://codereview.chromium.org/341823005/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/341823005/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:363: restored_ = true;
On 2014/07/10 19:58:37, msw wrote:
> nit: you should probably record the histogram value and set this flag in
> RestorePreviousSession, just in case there are (or will eventually be) other
> ways to invoke that.

Done.

https://codereview.chromium.org/341823005/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:390: }
On 2014/07/10 19:58:37, msw wrote:
> nit: revert this (indent was correct).

Done.

[yao, 2014-07-10 21:40:05]

The CQ bit was checked by yiyaoliu@chromium.org

[commit-bot: I haz the power, 2014-07-10 21:41:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/341823005/280001

[commit-bot: I haz the power, 2014-07-10 22:40:48]

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

[commit-bot: I haz the power, 2014-07-10 22:44:40]

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

[commit-bot: I haz the power, 2014-07-10 22:44:41]

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

[yao, 2014-07-10 22:50:46]

Hi Ilya, 

Could you do an owner's review for file histograms.xml?

Thanks, 
Yiyao

[Ilya Sherman, 2014-07-10 23:16:45]

LGTM % nits:

https://codereview.chromium.org/341823005/diff/280001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/341823005/diff/280001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:45766: +  <int value="1" label="There is
error when showing the bubble."/>
nit: "is error" -> "was an error" to match the tense of the other labels.

https://codereview.chromium.org/341823005/diff/280001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:45768: +  <int value="3" label="User was
already optin to UMA"/>
nit: "optin" -> "opted in"

https://codereview.chromium.org/341823005/diff/280001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:45769: +  <int value="4" label="User
chose to optin to UMA"/>
nit: "optin" -> "opt in".

[yao, 2014-07-11 03:40:41]

The CQ bit was checked by yiyaoliu@chromium.org

[commit-bot: I haz the power, 2014-07-11 03:42:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/341823005/300001

[commit-bot: I haz the power, 2014-07-11 09:26:40]

Change committed as 282592