Log the creation of more dialog box types.

Log the creation of more dialog box types.

Increase the number of dialog box types that have their creation
logged. These are simple cases, where the logging is in the
constructor. Future cls will include more complex cases.

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

[pdyson, 2017-04-20 05:16:58]

Description was changed from

==========
Log the creation of more dialog box types.

Increase the number of dialog box types that have their creation
logged. These are simple cases, where the logging is in the
contructor. Future cls will include more complex cases.

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

to

==========
Log the creation of more dialog box types.

Increase the number of dialog box types that have their creation
logged. These are simple cases, where the logging is in the
constructor. Future cls will include more complex cases.

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

[pdyson, 2017-04-20 05:16:59]

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

[msw, 2017-04-20 18:06:02]

lgtm without 'bubble' in name/label.

https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
chrome/browser/ui/browser_dialogs.h:215: EXTENSION_INSTALLED_BUBBLE = 23,
q: why name this with 'bubble', but not others, eg. SAVE_CARD for
SaveCardBubbleViews and GLOBAL_ERROR for GlobalErrorBubbleView, etc.? I'd
probably skip adding bubble to any ENUM values, unless it's more critical to
describing the value, like CONFIRM_BUBBLE

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

https://codereview.chromium.org/2833603003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:89432: +  <int value="23"
label="Extension Installed Bubble (Sideloaded)"/>
ditto nit: remove "Bubble" here

[pdyson, 2017-04-21 00:49:53]

Mike - a question about removing "bubble".

https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
chrome/browser/ui/browser_dialogs.h:215: EXTENSION_INSTALLED_BUBBLE = 23,
On 2017/04/20 at 18:06:02, msw wrote:
> q: why name this with 'bubble', but not others, eg. SAVE_CARD for
SaveCardBubbleViews and GLOBAL_ERROR for GlobalErrorBubbleView, etc.? I'd
probably skip adding bubble to any ENUM values, unless it's more critical to
describing the value, like CONFIRM_BUBBLE

I was concerned that having EXTENSION_INSTALL and EXTENSION_INSTALLED would lead
to confusion, so I called out which one was "bubble". I realize that this is
inconsistent with other "bubble" dialogs, but I went with what I see as clarity
over consistency. However, I could remove "_BUBBLE" and it would still be clear
in the dashboard, although still possibly confusing in the code.

What do you think?

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

https://codereview.chromium.org/2833603003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:89432: +  <int value="23"
label="Extension Installed Bubble (Sideloaded)"/>
On 2017/04/20 at 18:06:02, msw wrote:
> ditto nit: remove "Bubble" here

I've fine with removing "bubble" here as "(Sideloaded)" clearly distinguishes
this from "Extension Install".

[msw, 2017-04-21 01:58:19]

https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
chrome/browser/ui/browser_dialogs.h:215: EXTENSION_INSTALLED_BUBBLE = 23,
On 2017/04/21 00:49:53, pdyson wrote:
> On 2017/04/20 at 18:06:02, msw wrote:
> > q: why name this with 'bubble', but not others, eg. SAVE_CARD for
> SaveCardBubbleViews and GLOBAL_ERROR for GlobalErrorBubbleView, etc.? I'd
> probably skip adding bubble to any ENUM values, unless it's more critical to
> describing the value, like CONFIRM_BUBBLE
> 
> I was concerned that having EXTENSION_INSTALL and EXTENSION_INSTALLED would
lead
> to confusion, so I called out which one was "bubble". I realize that this is
> inconsistent with other "bubble" dialogs, but I went with what I see as
clarity
> over consistency. However, I could remove "_BUBBLE" and it would still be
clear
> in the dashboard, although still possibly confusing in the code.
> 
> What do you think?

I think *INSTALL AND *INSTALLED is fine. As you said, you can clarify the labels
in the dashboard, and the code points to the appropriate classes.

[pdyson, 2017-04-21 03:36:16]

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

[commit-bot: I haz the power, 2017-04-21 03:36: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-04-21 05:17:18]

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

[commit-bot: I haz the power, 2017-04-21 05:17:19]

Dry run: This issue passed the CQ dry run.

[pdyson, 2017-04-24 05:36:56]

On 2017/04/21 at 01:58:19, msw wrote:
>
https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
> File chrome/browser/ui/browser_dialogs.h (right):
> 
>
https://codereview.chromium.org/2833603003/diff/1/chrome/browser/ui/browser_d...
> chrome/browser/ui/browser_dialogs.h:215: EXTENSION_INSTALLED_BUBBLE = 23,
> On 2017/04/21 00:49:53, pdyson wrote:
> > On 2017/04/20 at 18:06:02, msw wrote:
> > > q: why name this with 'bubble', but not others, eg. SAVE_CARD for
> > SaveCardBubbleViews and GLOBAL_ERROR for GlobalErrorBubbleView, etc.? I'd
> > probably skip adding bubble to any ENUM values, unless it's more critical to
> > describing the value, like CONFIRM_BUBBLE
> > 
> > I was concerned that having EXTENSION_INSTALL and EXTENSION_INSTALLED would
lead
> > to confusion, so I called out which one was "bubble". I realize that this is
> > inconsistent with other "bubble" dialogs, but I went with what I see as
clarity
> > over consistency. However, I could remove "_BUBBLE" and it would still be
clear
> > in the dashboard, although still possibly confusing in the code.
> > 
> > What do you think?
> 
> I think *INSTALL AND *INSTALLED is fine. As you said, you can clarify the
labels in the dashboard, and the code points to the appropriate classes.

Ok. I've removed "_BUBBLE".

[pdyson, 2017-04-24 05:37:17]

Jesse, can you take a look at this now?

[jwd, 2017-04-24 21:11:24]

lgtm

[pdyson, 2017-04-25 23:23:42]

The CQ bit was checked by pdyson@chromium.org

[pdyson, 2017-04-25 23:23:42]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2833603003/#ps20001
(title: "remove "_BUBBLE"")

[commit-bot: I haz the power, 2017-04-25 23:24:27]

CQ is trying da patch.

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

[pdyson, 2017-04-25 23:24:29]

The CQ bit was unchecked by pdyson@chromium.org

[pdyson, 2017-04-25 23:49:34]

The CQ bit was checked by pdyson@chromium.org

[pdyson, 2017-04-25 23:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2833603003/#ps40001
(title: "Fix simple merge conflict.")

[commit-bot: I haz the power, 2017-04-25 23:50:05]

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-04-26 01:21:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493164174666840,
"parent_rev": "187f69b644f33ff3dbec23972e3b18dcc1222930", "commit_rev":
"106455b2f89608b7225f03fe71593332aa4eddfe"}

[commit-bot: I haz the power, 2017-04-26 01:22:05]

Description was changed from

==========
Log the creation of more dialog box types.

Increase the number of dialog box types that have their creation
logged. These are simple cases, where the logging is in the
constructor. Future cls will include more complex cases.

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

to

==========
Log the creation of more dialog box types.

Increase the number of dialog box types that have their creation
logged. These are simple cases, where the logging is in the
constructor. Future cls will include more complex cases.

BUG=705331

Review-Url: https://codereview.chromium.org/2833603003
Cr-Commit-Position: refs/heads/master@{#467195}
Committed:
https://chromium.googlesource.com/chromium/src/+/106455b2f89608b7225f03fe7159...
==========

[commit-bot: I haz the power, 2017-04-26 01:22:07]

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