Log the creation of dialog boxes using DialogDelegateView and BubbleDialogDelegateView.

Log the creation of dialog boxes using DialogDelegateView and BubbleDialogDelegateView.

BUG=705331
Review-Url: https://codereview.chromium.org/2867683002
Cr-Commit-Position: refs/heads/master@{#471137}
Committed: https://chromium.googlesource.com/chromium/src/+/fe27ed665852a667acdba62f8a77a1d9969170da

[pdyson, 2017-05-08 04:32:48]

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

[commit-bot: I haz the power, 2017-05-08 04:33:13]

Dry run: CQ is trying da patch.

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

[pdyson, 2017-05-08 05:11:30]

pdyson@chromium.org changed reviewers:
+ jwd@chromium.org

[commit-bot: I haz the power, 2017-05-08 06:05:26]

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

[commit-bot: I haz the power, 2017-05-08 06:05:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jwd, 2017-05-08 22:37:16]

LGTM with xml change.

https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:11856: +<histogram
name="Dialog.BubbleDialogDelegateView.Creation"
Can you refactor these histograms using the histogram suffixes. You can do this
using ordering="prefix". More info about suffixes is at the top of this file.
This is just a change to the XML.

[pdyson, 2017-05-09 04:24:15]

On 2017/05/08 at 22:37:16, jwd wrote:
> LGTM with xml change.
> 
>
https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:11856: +<histogram
name="Dialog.BubbleDialogDelegateView.Creation"
> Can you refactor these histograms using the histogram suffixes. You can do
this using ordering="prefix". More info about suffixes is at the top of this
file. This is just a change to the XML.

I made this change, but it was not as easy as just changing the xml.

The existing metric Dialog.Creation uses the enum DialogName, but for all of the
other Dialog.*.Creation metrics we need to use the enum BooleanCreated. I
couldn't see a way of doing this using histogram_suffixes.

So I made a new histogram called Dialog.Create (instead of Creation) and then
made Dialog.DelegateView.Create and Dialog.BubbleDialogDelegateView.Create using
histogram_suffixes. This meant I had to change the metric in the code from
*.Creation to *.Create.

Dialog.Delegate.Creation is now the odd one out. To be consistent it should be
Dialog.Delegate.Create, but as Dialog.Delegate.Creation already exists, we would
need to deprecate that (in another cl). Please, let me know if we should do that
or leave Dialog.Delegate.Creation is as it.

I also got a warning when uploading that the histogram names
Dialog.BubbleDialogDelegateView.Create and Dialog.DelegateView.Create have no
match is histograms.xml. I think I did it correctly, but am cautious as I got
the warning.

[pdyson, 2017-05-09 04:24:28]

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

[commit-bot: I haz the power, 2017-05-09 04:25:40]

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, 2017-05-09 06:08:33]

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

[commit-bot: I haz the power, 2017-05-09 06:08:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jwd, 2017-05-09 20:17:15]

On 2017/05/09 04:24:15, pdyson wrote:
> On 2017/05/08 at 22:37:16, jwd wrote:
> > LGTM with xml change.
> > 
> >
>
https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
> > tools/metrics/histograms/histograms.xml:11856: +<histogram
> name="Dialog.BubbleDialogDelegateView.Creation"
> > Can you refactor these histograms using the histogram suffixes. You can do
> this using ordering="prefix". More info about suffixes is at the top of this
> file. This is just a change to the XML.
> 
> I made this change, but it was not as easy as just changing the xml.
> 
> The existing metric Dialog.Creation uses the enum DialogName, but for all of
the
> other Dialog.*.Creation metrics we need to use the enum BooleanCreated. I
> couldn't see a way of doing this using histogram_suffixes.
> 
> So I made a new histogram called Dialog.Create (instead of Creation) and then
> made Dialog.DelegateView.Create and Dialog.BubbleDialogDelegateView.Create
using
> histogram_suffixes. This meant I had to change the metric in the code from
> *.Creation to *.Create.
Ah, shoot, I hadn't thought about the enum for Dialog.Creation.
> 
> Dialog.Delegate.Creation is now the odd one out. To be consistent it should be
> Dialog.Delegate.Create, but as Dialog.Delegate.Creation already exists, we
would
> need to deprecate that (in another cl). Please, let me know if we should do
that
> or leave Dialog.Delegate.Creation is as it.
Unfortunately, I think we should move it to the consistent name since it landed
in M60, which hasn't branched yet, so it doesn't cause a big analysis headache.
> 
> I also got a warning when uploading that the histogram names
> Dialog.BubbleDialogDelegateView.Create and Dialog.DelegateView.Create have no
> match is histograms.xml. I think I did it correctly, but am cautious as I got
> the warning.
It does look fine. Turns out the presubmit is not very clever, and doesn't check
this. The histograms.xml can always be fixed later if there are problems.

[jwd, 2017-05-09 20:58:41]

Still LGTM

[pdyson, 2017-05-10 00:54:02]

On 2017/05/09 at 20:17:15, jwd wrote:
> On 2017/05/09 04:24:15, pdyson wrote:
> > On 2017/05/08 at 22:37:16, jwd wrote:
> > > LGTM with xml change.
> > > 
> > >
> >
https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
> > > File tools/metrics/histograms/histograms.xml (right):
> > > 
> > >
> >
https://codereview.chromium.org/2867683002/diff/1/tools/metrics/histograms/hi...
> > > tools/metrics/histograms/histograms.xml:11856: +<histogram
> > name="Dialog.BubbleDialogDelegateView.Creation"
> > > Can you refactor these histograms using the histogram suffixes. You can do
> > this using ordering="prefix". More info about suffixes is at the top of this
> > file. This is just a change to the XML.
> > 
> > I made this change, but it was not as easy as just changing the xml.
> > 
> > The existing metric Dialog.Creation uses the enum DialogName, but for all of
the
> > other Dialog.*.Creation metrics we need to use the enum BooleanCreated. I
> > couldn't see a way of doing this using histogram_suffixes.
> > 
> > So I made a new histogram called Dialog.Create (instead of Creation) and
then
> > made Dialog.DelegateView.Create and Dialog.BubbleDialogDelegateView.Create
using
> > histogram_suffixes. This meant I had to change the metric in the code from
> > *.Creation to *.Create.
> Ah, shoot, I hadn't thought about the enum for Dialog.Creation.
> > 
> > Dialog.Delegate.Creation is now the odd one out. To be consistent it should
be
> > Dialog.Delegate.Create, but as Dialog.Delegate.Creation already exists, we
would
> > need to deprecate that (in another cl). Please, let me know if we should do
that
> > or leave Dialog.Delegate.Creation is as it.
> Unfortunately, I think we should move it to the consistent name since it
landed in M60, which hasn't branched yet, so it doesn't cause a big analysis
headache.

Ok. I'll fix this in another CL.

> > 
> > I also got a warning when uploading that the histogram names
> > Dialog.BubbleDialogDelegateView.Create and Dialog.DelegateView.Create have
no
> > match is histograms.xml. I think I did it correctly, but am cautious as I
got
> > the warning.
> It does look fine. Turns out the presubmit is not very clever, and doesn't
check this. The histograms.xml can always be fixed later if there are problems.

[pdyson, 2017-05-10 00:54:42]

pdyson@chromium.org changed reviewers:
+ msw@chromium.org

[pdyson, 2017-05-11 00:14:53]

Mike, I'm ready for you to take a look at this now.

[msw, 2017-05-11 00:54:40]

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

https://codereview.chromium.org/2867683002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:11858: +  <summary>Counts the number
times dialog boxes are created.</summary>
I think we need to get more specific here... you have two or three fairly
closely related metrics and it's not clear to casual observers how they are
related (overlap, comparison, etc.) For example, BubbleDialogDelegateView is-a
DialogDelegateView, and DialogDelegateView is-a DialogDelegate, so you'll record
all three creat[e|ion] metrics for one BubbleDialogDelegateView instantiation,
and you'll record both DialogDelegateView and DialogDelegate for one non-bubble
DialogDelegateView instantiation, right? This seems broken.

https://codereview.chromium.org/2867683002/diff/20001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate.cc (right):

https://codereview.chromium.org/2867683002/diff/20001/ui/views/window/dialog_...
ui/views/window/dialog_delegate.cc:36:
UMA_HISTOGRAM_BOOLEAN("Dialog.Delegate.Creation", true);
nit: maybe this should be Dialog.DialogDelegate.Create (or Creation)?

https://codereview.chromium.org/2867683002/diff/20001/ui/views/window/dialog_...
ui/views/window/dialog_delegate.cc:242:
UMA_HISTOGRAM_BOOLEAN("Dialog.DelegateView.Create", true);
nit: maybe this should be Dialog.DialogDelegateView.Create?

[pdyson, 2017-05-11 05:54:01]

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

https://codereview.chromium.org/2867683002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:11858: +  <summary>Counts the number
times dialog boxes are created.</summary>
On 2017/05/11 at 00:54:39, msw wrote:
> I think we need to get more specific here... you have two or three fairly
closely related metrics and it's not clear to casual observers how they are
related (overlap, comparison, etc.) For example, BubbleDialogDelegateView is-a
DialogDelegateView, and DialogDelegateView is-a DialogDelegate, so you'll record
all three creat[e|ion] metrics for one BubbleDialogDelegateView instantiation,
and you'll record both DialogDelegateView and DialogDelegate for one non-bubble
DialogDelegateView instantiation, right? This seems broken.

I've added to the comments here and below to make this clearer. We have found a
very large gap between the number of dialogs that we are counting specifically
in Dialog.Creation and the overall number in Dialog.Delegate.Creation. So we
will start counting the classes in between to see where the discrepancy might
lie. There is also another cl coming where we count many more specific child
classes. This current cl will allow us to see where the gaps are more precisely.

https://codereview.chromium.org/2867683002/diff/20001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate.cc (right):

https://codereview.chromium.org/2867683002/diff/20001/ui/views/window/dialog_...
ui/views/window/dialog_delegate.cc:242:
UMA_HISTOGRAM_BOOLEAN("Dialog.DelegateView.Create", true);
On 2017/05/11 at 00:54:40, msw wrote:
> nit: maybe this should be Dialog.DialogDelegateView.Create?

Done.

I have another cl to change Dialog.Delegate.Creation to be of the form
Dialog.*.Create. So I'll make that Dialog.DialogDelegate.Create so this is all
consistent.

[pdyson, 2017-05-11 05:54:12]

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

[commit-bot: I haz the power, 2017-05-11 05:54:56]

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, 2017-05-11 08:06:35]

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

[commit-bot: I haz the power, 2017-05-11 08:06:36]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-11 17:02:46]

lgtm

[pdyson, 2017-05-12 00:01:17]

The CQ bit was checked by pdyson@chromium.org

[pdyson, 2017-05-12 00:01:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2867683002/#ps30001
(title: "Expand descriptions.")

[commit-bot: I haz the power, 2017-05-12 00:02:53]

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-05-12 00:18:24]

CQ is committing da patch.

Bot data: {"patchset_id": 30001, "attempt_start_ts": 1494547277472160,
"parent_rev": "9601a9bbabcf3aad7d64f0d192561dc80dd374c2", "commit_rev":
"fe27ed665852a667acdba62f8a77a1d9969170da"}

[commit-bot: I haz the power, 2017-05-12 00:18:35]

Description was changed from

==========
Log the creation of dialog boxes using DialogDelegateView and
BubbleDialogDelegateView.

BUG=705331
==========

to

==========
Log the creation of dialog boxes using DialogDelegateView and
BubbleDialogDelegateView.

BUG=705331

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

[commit-bot: I haz the power, 2017-05-12 00:18:36]

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