Collect more UMA data about dialogs and site engagement.

Collect more UMA data about dialogs and site engagement.

BUG=644268
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/3bfa5646649fe5439357abaed781a5b4803d1946
Cr-Commit-Position: refs/heads/master@{#419783}

[Avi (use Gerrit), 2016-09-19 17:40:11]

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

[Avi (use Gerrit), 2016-09-19 17:40:11]

Description was changed from

==========
Collect more UMA data about dialogs and site engagement.

BUG=644268
TEST=none
==========

to

==========
Collect more UMA data about dialogs and site engagement.

BUG=644268
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-19 17:40:38]

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

[Avi (use Gerrit), 2016-09-19 19:39:07]

avi@chromium.org changed reviewers:
+ asvitkine@chromium.org, dcheng@chromium.org, dominickn@chromium.org,
ojan@chromium.org

[Avi (use Gerrit), 2016-09-19 19:39:08]

Alexei for uma
Daniel for Blink
Dominick/Ojan for what stats I'm collecting

[dcheng, 2016-09-19 19:43:46]

Blink LGTM

[Alexei Svitkine (slow), 2016-09-19 19:54:59]

lgtm

https://codereview.chromium.org/2352753002/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2352753002/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:132:
base::Bind(&SaveUnloadUmaStats, engagement_score, callback);
Er, shouldn't you be doing something with the return value of base::Bind() - or
is this some unusual use of base::Bind() I'm not familiar with? If the latter,
please add a comment to help readers like me.

[commit-bot: I haz the power, 2016-09-19 20:22:14]

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

[commit-bot: I haz the power, 2016-09-19 20:22:14]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-09-19 20:46:00]

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

[Avi (use Gerrit), 2016-09-19 20:46:22]

https://codereview.chromium.org/2352753002/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2352753002/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:132:
base::Bind(&SaveUnloadUmaStats, engagement_score, callback);
On 2016/09/19 19:54:59, Alexei Svitkine (very slow) wrote:
> Er, shouldn't you be doing something with the return value of base::Bind() -
or
> is this some unusual use of base::Bind() I'm not familiar with? If the latter,
> please add a comment to help readers like me.

Ugh.

I forgot to save, I did git cl format, and then my text editor reloaded.

Fixed; thank you.

[commit-bot: I haz the power, 2016-09-19 20:46:30]

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-09-19 23:02:53]

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

[commit-bot: I haz the power, 2016-09-19 23:02:54]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-20 07:22:09]

Stats lgtm % nits

https://codereview.chromium.org/2352753002/diff/20001/chrome/browser/ui/javas...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2352753002/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:95:
namespace {
Nit: this file already has an anonymous namespace above; should this go there
too?

https://codereview.chromium.org/2352753002/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:127:
SiteEngagementService* site_engagement_service =
Nit: inline profile variable?

https://codereview.chromium.org/2352753002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2352753002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1605: int32_t
message_length = static_cast<int32_t>(message.length());
Nit: size_t? Or uint32_t?

https://codereview.chromium.org/2352753002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2352753002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2268: int32_t message_length =
static_cast<int32_t>(message.length());
Nit: size_t? Or uint32_t?

[ojan, 2016-09-20 13:28:34]

lgtm

[Avi (use Gerrit), 2016-09-20 14:18:50]

https://codereview.chromium.org/2352753002/diff/20001/chrome/browser/ui/javas...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2352753002/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:95:
namespace {
On 2016/09/20 07:22:09, dominickn wrote:
> Nit: this file already has an anonymous namespace above; should this go there
> too?

It's the spliced-in callback for below, so I wanted to keep it right there.

https://codereview.chromium.org/2352753002/diff/20001/chrome/browser/ui/javas...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:127:
SiteEngagementService* site_engagement_service =
On 2016/09/20 07:22:09, dominickn wrote:
> Nit: inline profile variable?

Done.

https://codereview.chromium.org/2352753002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2352753002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1605: int32_t
message_length = static_cast<int32_t>(message.length());
On 2016/09/20 07:22:09, dominickn wrote:
> Nit: size_t? Or uint32_t?

I stole this from earlier UMA code I wrote, and it didn't compile without the
cast.

https://codereview.chromium.org/2352753002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2352753002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2268: int32_t message_length =
static_cast<int32_t>(message.length());
On 2016/09/20 07:22:09, dominickn wrote:
> Nit: size_t? Or uint32_t?

ditto.

[Avi (use Gerrit), 2016-09-20 14:24:00]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-09-20 14:24:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dcheng@chromium.org, dominickn@chromium.org,
ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2352753002/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2016-09-20 14:24:10]

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-09-20 16:09:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-20 16:10:22]

Description was changed from

==========
Collect more UMA data about dialogs and site engagement.

BUG=644268
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Collect more UMA data about dialogs and site engagement.

BUG=644268
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/3bfa5646649fe5439357abaed781a5b4803d1946
Cr-Commit-Position: refs/heads/master@{#419783}
==========

[commit-bot: I haz the power, 2016-09-20 16:10:24]

Patchset 3 (id:??) landed as
https://crrev.com/3bfa5646649fe5439357abaed781a5b4803d1946
Cr-Commit-Position: refs/heads/master@{#419783}