Add method for identifying different InfoBars

Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns an
  enum identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().

BUG=567483
Committed: https://crrev.com/ac4d93be8bc37c31f3b19749272e5d176eb8d2cf
Cr-Commit-Position: refs/heads/master@{#367176}

[gone, 2015-12-11 22:37:20]

Description was changed from

==========
Add names to identify different infobar types

BUG=
==========

to

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns a
  string identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* TODO(dfalcantara): Add histogram.

BUG=567483
==========

[gone, 2015-12-14 22:23:25]

Description was changed from

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns a
  string identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* TODO(dfalcantara): Add histogram.

BUG=567483
==========

to

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns a
  string identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().  Values are
  determined the same way about:flags does it, where the string
  is md5 hashed and some subset of the digest is converted to
  a signed integer.

BUG=567483
==========

[gone, 2015-12-14 23:22:19]

dfalcantara@chromium.org changed reviewers:
+ pkasting@chromium.org, rkaplow@chromium.org

[gone, 2015-12-14 23:22:20]

Peter: Is this a reasonable way to add an identifier to each infobar?  Went this
route after the email thread rkaplow@ sent out.

Robert: Everything make sense, metrics-wise?  The call is added in
infobar_container.cc, histograms to histograms.xml.

[Peter Kasting, 2015-12-14 23:59:14]

Another way of doing this that would be less intrusive (but slightly less
granular) would be to simply use the returned value from GetIconId() as the
value for the histogram.  These values are mostly unique except for cases where
we have no icon at all, which should be fairly rare.

That wouldn't require new strings everywhere, which are just used to hash
anyway, which seems slightly unfortunate in terms of binary size.

https://chromiumcodereview.appspot.com/1520543004/diff/200001/components/info...
File components/infobars/core/infobar_container.cc (right):

https://chromiumcodereview.appspot.com/1520543004/diff/200001/components/info...
components/infobars/core/infobar_container.cc:162:
static_cast<base::HistogramBase::Sample>(base::HashMetricName(id));
Nit: Slightly shorter to just inline:

  int hashed_id = static_cast<base::HistogramBase::Sample>(
      base::HashMetricName(infobar->delegate()->GetIdentifier()));

[gone, 2015-12-15 00:59:54]

Yeah, I agree the string definitions aren't great :/  Checking the value of
GetIconId() won't get us much granularity, though; ran a quick check of the
infobars I'm touching here and a bunch of them don't actually override
GetIconId().  There's some duplication of IDs aside from that (list below).

Maybe the right thing to do is to set up an enum?

--------------------------------------------------------------------------------
Mapping of infobars to icons:

DownloadManagerOverwriteInfoBarDelegates        No icon
RequestQuotaInfoBarDelegate
DevtoolsConfirmInfoBarDelegate
ExtensionDevToolsInfoBarDelegate
IncognitoConnectabilityInfoBarDelegate
InsecureContentInfoBarDelegate
NaClInfoBarDelegate
DataReductionProxyInfoBarDelegateAndroid
AutoSigninFirstRunInfoBarDelegate
SavePasswordInfoBarDelegate
DurableStoragePermissionInfoBarDelegateAndroid
ChromeSelectFilePolicy
ErrorInfoBarDelegate
BadFlagsPrompt
GoogleApiKeysInfoBarDelegate
ObsoleteSystemInfoBarDelegate
MockConfirmInfoBarDelegate
BrowserWindowControllerTest
    
MediaThrottleInfoBarDelegate                    IDR_INFOBAR_WARNING
PermissionUpdateInfoBarDelegate
NPAPIRemovalInfoBarDelegate

PepperBrokerInfoBarDelegate                     IDR_INFOBAR_PLUGIN_INSTALL
OutdatedPluginInfoBarDelegate
PluginMetroModeInfoBarDelegate

GeneratedPasswordSavedInfoBarDelegateAndroid    IDR_INFOBAR_SAVE_PASSWORD
SSLAddCertificateInfoBarDelegate

ReloadPluginInfoBarDelegate                     IDR_INFOBAR_PLUGIN_CRASHED
HungPluginInfoBarDelegate

KeystonePromotionInfoBarDelegate                IDR_PRODUCT_LOGO_32
DefaultBrowserInfoBarDelegate

AlternateNavInfoBarDelegate                     IDR_INFOBAR_ALT_NAV_URL
WebsiteSettingsInfoBarDelegate
    
PopupBlockedInfoBarDelegate                     IDR_BLOCKED_POPUPS

CollectedCookiesInfoBarDelegate                 IDR_INFOBAR_COOKIE

SessionCrashedInfoBarDelegate                   IDR_INFOBAR_RESTORE_SESSION

AutofillCCInfoBarDelegate                       IDR_INFOBAR_AUTOFILL_CC

TranslateInfoBarDelegate                        IDR_INFOBAR_TRANSLATE

MidiPermissionInfoBarDelegateAndroid            IDR_INFOBAR_MIDI

ProtectedMediaIdentifierInfoBarDelegateAndroid 
IDR_ANDROID_INFOBAR_PROTECTED_MEDIA_IDENTIFIER

NotificationPermissionInfobarDelegate          
IDR_ANDROID_INFOBAR_NOTIFICATIONS

AppBannerInfoBarDelegateDesktop                 IDR_INFOBAR_APP_BANNER
    
DownloadRequestInfoBarDelegateAndroid           IDR_INFOBAR_MULTIPLE_DOWNLOAD
    
ThemeInstalledInfoBarDelegate                   IDR_INFOBAR_THEME
    
GeolocationInfoBarDelegateAndroid               IDR_ANDROID_INFOBAR_GEOLOCATION
    
ThreeDAPIInfoBarDelegate                        IDR_INFOBAR_3D_BLOCKED

FullscreenInfoBarDelegate                       IDR_ANDROID_INFOBAR_FULLSCREEN
    
HungRendererInfoBarDelegate                     IDR_ANDROID_INFOBAR_FROZEN_TAB

MediaStreamInfoBarDelegateAndroid               IDR_INFOBAR_MEDIA_STREAM_CAMERA
                                                IDR_INFOBAR_MEDIA_STREAM_MIC

AppBannerInfoBarDelegateAndroid                 Uses icon defined by website

[Peter Kasting, 2015-12-15 01:28:58]

I don't really have a good gut feeling on strings vs. enum.

[gone, 2015-12-15 01:29:41]

I guess one problem with using an enum is that the InfoBarDelegate is defined in
components/, while the infobars are mostly in chrome/.  It'd be strange to
define enum values that would "break" encapsulation of the layers.

[rkaplow, 2015-12-15 18:03:37]

On 2015/12/15 01:29:41, dfalcantara wrote:
> I guess one problem with using an enum is that the InfoBarDelegate is defined
in
> components/, while the infobars are mostly in chrome/.  It'd be strange to
> define enum values that would "break" encapsulation of the layers.


This (overall) makes sense to me.
I would suggest adding some documentation for what to do when adding your own
infobar (i.e. make sure to update the enum w/ instructions) - I guess wait on
this until we decide on the solution.

I don't have a great sense for what is better - to take the string memory hit or
have a globalish enum :\

[gone, 2015-12-15 18:36:37]

Some sort of way to do compile-time hashing of a string to an int would be
great, but I'm not finding anything great that doesn't use constexpr
(http://nguillemot.blogspot.com/2012/06/side-story-compile-time-string-hashing...)
or funky looking macros
(http://stackoverflow.com/questions/2826559/compile-time-preprocessor-hashing-...).

Not sure how we'd use an enum because of the layering... best I can come up with
is to use an int within a specified range for the infobars in components/ and a
separate range for those in chrome/.

https://chromiumcodereview.appspot.com/1520543004/diff/200001/components/info...
File components/infobars/core/infobar_container.cc (right):

https://chromiumcodereview.appspot.com/1520543004/diff/200001/components/info...
components/infobars/core/infobar_container.cc:162:
static_cast<base::HistogramBase::Sample>(base::HashMetricName(id));
On 2015/12/14 23:59:14, Peter Kasting wrote:
> Nit: Slightly shorter to just inline:
> 
>   int hashed_id = static_cast<base::HistogramBase::Sample>(
>       base::HashMetricName(infobar->delegate()->GetIdentifier()));

Done.

[Peter Kasting, 2015-12-15 20:01:50]

Since we can't decide, let's just do what you have now.  LGTM

[rkaplow, 2015-12-15 20:11:13]

Can you add a bit more in the comment about the enum in infobar_delegate.h to
make it very easy for a new infobar author to get the enum value

[gone, 2015-12-15 21:02:00]

In that case, would it make sense to pull out the bit in
InfoBarContainer::AddInfoBar that computes the int from the identifier?  It'd
make it much easier to get the number for the histogram and for future metrics. 
Maybe make a new InfoBarMetrics class to support it?

[rkaplow, 2015-12-15 21:11:07]

On 2015/12/15 21:02:00, dfalcantara wrote:
> In that case, would it make sense to pull out the bit in
> InfoBarContainer::AddInfoBar that computes the int from the identifier?  It'd
> make it much easier to get the number for the histogram and for future
metrics. 
> Maybe make a new InfoBarMetrics class to support it?

You mean have the GetIdentifier() return an int (and we will recommend it is a
hash of a unique string)? That seems ok too - then we aren't keeping the strings
around.
Are you thinking it would look something like (inside a client)
{
// Hashed from base::HashMetricName("MyInfobar")
return 3823894;
}
?

Not sure if it's a bit cryptic...

[gone, 2015-12-15 21:32:11]

Was actually suggesting something like the new Patch Set 13, where a class has a
static function for computing the histogram value.  Centralizes everything into
its own class and makes it slightly cleaner (I think).

[rkaplow, 2015-12-15 22:39:29]

lgtm

seems cleaner to me too.
You can add me as an owner to the histograms if you'd like.

[gone, 2015-12-15 23:26:14]

Cool, done.

Peter: Does the new patch still look alright?

[Peter Kasting, 2015-12-16 06:13:09]

No, I view the new class as overhead with no benefit.  The old way was more
direct and clear.  There's no reason to abstract the details of recording the
histogram away from the lone callsite that wants to do this.

(Besides this, the Google style guide prohibits using classes simply to group
static functions and requires namespaces instead; see
http://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_an...
.)

[gone, 2015-12-16 19:49:07]

Alright; went back to the version without the extra static class (thanks for the
link!) and added a comment to InfoBarDelegate about using base::HashMetricName()
to update the histogram.

[gone, 2015-12-16 19:50:26]

dfalcantara@chromium.org changed reviewers:
+ isherman@chromium.org, jam@chromium.org

[gone, 2015-12-16 19:50:27]

+jam to review the mechanical changes across all the delegates

+isherman to review the changes to the histogram

[Peter Kasting, 2015-12-16 21:02:46]

https://codereview.chromium.org/1520543004/diff/300001/components/infobars/co...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/1520543004/diff/300001/components/infobars/co...
components/infobars/core/infobar_delegate.h:87: // Implementers must update the
InfoBarType enum in histograms.xml, with the
Nit: No comma

[gone, 2015-12-16 22:53:11]

https://chromiumcodereview.appspot.com/1520543004/diff/300001/components/info...
File components/infobars/core/infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/1520543004/diff/300001/components/info...
components/infobars/core/infobar_delegate.h:87: // Implementers must update the
InfoBarType enum in histograms.xml, with the
On 2015/12/16 21:02:46, Peter Kasting wrote:
> Nit: No comma

Done.

[Ilya Sherman, 2015-12-16 23:41:52]

metrics LGTM % nits:

https://codereview.chromium.org/1520543004/diff/340001/components/infobars/co...
File components/infobars/core/infobar_container.cc (right):

https://codereview.chromium.org/1520543004/diff/340001/components/infobars/co...
components/infobars/core/infobar_container.cc:160: int hashed_id =
static_cast<base::HistogramBase::Sample>(
nit: It's weird to cast to a base::HistogramBase::Sample and then store the
result in an int.  I'd pick one type and stick with it.

https://codereview.chromium.org/1520543004/diff/340001/components/infobars/co...
components/infobars/core/infobar_container.cc:160: int hashed_id =
static_cast<base::HistogramBase::Sample>(
nit: You might want to use one of the more explicit casts from
base/numerics/safe_conversions.h.

https://codereview.chromium.org/1520543004/diff/340001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1520543004/diff/340001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:16127: +<histogram name="InfoBar.Added"
enum="InfoBarTypes">
nit: Perhaps name this "Infobar.Shown"?  (Or "InfoBar" -- I'm mostly suggesting
s/Added/Shown.)

[gone, 2015-12-17 00:34:59]

https://codereview.chromium.org/1520543004/diff/340001/components/infobars/co...
File components/infobars/core/infobar_container.cc (right):

https://codereview.chromium.org/1520543004/diff/340001/components/infobars/co...
components/infobars/core/infobar_container.cc:160: int hashed_id =
static_cast<base::HistogramBase::Sample>(
On 2015/12/16 23:41:52, Ilya Sherman wrote:
> nit: You might want to use one of the more explicit casts from
> base/numerics/safe_conversions.h.

Swapped it over to storing a Sample.  Thanks for the catch.

https://codereview.chromium.org/1520543004/diff/340001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1520543004/diff/340001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:16127: +<histogram name="InfoBar.Added"
enum="InfoBarTypes">
On 2015/12/16 23:41:52, Ilya Sherman wrote:
> nit: Perhaps name this "Infobar.Shown"?  (Or "InfoBar" -- I'm mostly
suggesting
> s/Added/Shown.)

Switched to "Shown".  Also realized there was a caveat about the metric I missed
and added it to the description.

[gone, 2015-12-18 18:26:08]

dfalcantara@chromium.org changed reviewers:
+ thakis@chromium.org
- jam@chromium.org

[gone, 2015-12-18 18:26:09]

jam@ is apparently out for the holidays.  Trying my luck with thakis@.

[gone, 2015-12-18 18:27:59]

dfalcantara@chromium.org changed reviewers:
+ droger@chromium.org

[gone, 2015-12-18 18:28:00]

+droger@ for components/

[vabr (Chromium), 2015-12-18 18:31:27]

@dfalcantara -- you might also want to check the very recently added
ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.*

[droger, 2015-12-19 06:18:42]

Translate lgtm

[gone, 2015-12-22 20:21:41]

Cool, thanks for the heads up on the other infobar.  Updated & rebased.

[Nico, 2015-12-22 21:50:21]

Does this really have to return a std::string? All places return a literal as
far as I can tell, so a const char* would work just as well and would require
less copying. And an enum would be fine too and wouldn't embed all these strings
in the binary.

If this has already been discussed at length above, then lgtm stamp.

[gone, 2015-12-22 22:15:33]

Almost all the places do, yeah.  The one place it doesn't is 
SimpleAlertInfoBarDelegate, which is used to create one-off InfoBars by classes
that just need to show a message.  I've switched everything to using const char*
everywhere else, but that particular instance continues to store that as a
std::string until it's ready to be passed back.

We discussed using an enum earlier in the thread, but it got complicated because
of how the infobars were componentized.  I couldn't think of a way to avoid
breaking the layering without having components/ know about all the infobars
that are defined in chrome/ (and now in ios/).  If you've got suggestions, I'm
totally open to them.

[Nico, 2015-12-22 22:22:15]

On Tue, Dec 22, 2015 at 5:15 PM, <dfalcantara@chromium.org> wrote:

> Almost all the places do, yeah.  The one place it doesn't is
> SimpleAlertInfoBarDelegate, which is used to create one-off InfoBars by
> classes
> that just need to show a message.  I've switched everything to using const
> char*
> everywhere else, but that particular instance continues to store that as a
> std::string until it's ready to be passed back.
>
> We discussed using an enum earlier in the thread, but it got complicated
> because
> of how the infobars were componentized.  I couldn't think of a way to avoid
> breaking the layering without having components/ know about all the
> infobars
> that are defined in chrome/ (and now in ios/).  If you've got suggestions,
> I'm
> totally open to them.
>

Having an enum in components that has an entry for each info bar doesn't
seem super horrible to me. True, that enum "knows" about the other info
bars, but with the current approach you can end up with two info bars
silently returning the same string somehow, and histograms.xml needs to
know about all the info bars anyhow.


>
> https://chromiumcodereview.appspot.com/1520543004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Nico, 2015-12-22 22:22:17]

On Tue, Dec 22, 2015 at 5:15 PM, <dfalcantara@chromium.org> wrote:

> Almost all the places do, yeah.  The one place it doesn't is
> SimpleAlertInfoBarDelegate, which is used to create one-off InfoBars by
> classes
> that just need to show a message.  I've switched everything to using const
> char*
> everywhere else, but that particular instance continues to store that as a
> std::string until it's ready to be passed back.
>
> We discussed using an enum earlier in the thread, but it got complicated
> because
> of how the infobars were componentized.  I couldn't think of a way to avoid
> breaking the layering without having components/ know about all the
> infobars
> that are defined in chrome/ (and now in ios/).  If you've got suggestions,
> I'm
> totally open to them.
>

Having an enum in components that has an entry for each info bar doesn't
seem super horrible to me. True, that enum "knows" about the other info
bars, but with the current approach you can end up with two info bars
silently returning the same string somehow, and histograms.xml needs to
know about all the info bars anyhow.


>
> https://chromiumcodereview.appspot.com/1520543004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gone, 2015-12-22 22:40:03]

histograms.xml seemed kind of special in that regard... I mean it even has
entries about command line flags defined for Chrome OS (this CL was based off of
some metrics they added to about_flags.cc).

Peter: kicking back to you for input on the const char* change in the last patch
set and for input on whether switching to an enum makes more sense now than it
did when you last lgtmed.

[Peter Kasting, 2015-12-22 23:13:08]

Switching to const char* seems fine.

Regarding an enum versus const char*, I have a hard time saying one is
definitively better.  I like not having to keep a central enum of infobar types
no matter where it lives -- it's annoying to maintain, and it seems clearer to
me to specify everything about a new infobar just in the place you add it. 
However, not using an enum doesn't totally save us from that, because you still
have to update the histograms file.  Plus, updating that is really annoying, as
you have to compute the hash value to put there.  So in some sense, avoiding an
enum is actually making it _harder_ to add a new infobar rather than easier.

I guess I would say, I'm OK with either route, but perhaps an enum is slightly
better?  I'd like to know what your instincts are, though.

I wonder, if we have a central enum, whether we should further centralize
various things -- e.g. have base class functions in InfoBarDelegate to return
the infobar type and icon which are just switch statements.  We've avoided this
in the past to keep everything about new infobars specified in those new
classes, but if we have to do something central anyway, making this change might
make functionality more explicit and easier to collect data about the infobars.

[gone, 2015-12-28 20:54:51]

Yeah, I agree that having the developer explicitly compute the value of the
hashed string and add it to histograms.xml is suboptimal.  A few thoughts:

* It'll be easy for a dev to add a new infobar but not update histograms.xml
  unless we have some presubmit check for it.  PRESUBMIT.py, for example,
  trawls the codebase for new "base::UserMetricsAction" usages and requires
  devs to update actions.xml.  Feels like overkill to add a similar presubmit
  check for a single metric, though.

* It's definitely painful to compute new hashes for these things.  ChromeOS
  handles this by adding a unit test that parses a specific enum in
  histograms.xml and outputs any strings with incorrect hashes.  This still
  requires the dev to remember to explicitly add any new strings to
  histograms.xml beforehand, though.

I'm not sure if having a centralized enum will make the situation any better:

* Using an enum won't guarantee that we won't have collisions because people can
  just reuse enum values, but it'll at least be much more obvious when this
  happens.

* Using an enum means that anyone re-using the infobar component without
  chrome/ will have to keep their version of the enum up to date with chrome/.
  I thought that one of the goals of componentization was to prevent any
  dependencies on chrome/, which is suggested by components/README, but I
  could be mistaken.

All things considered, I'm inclined to use hashed strings to keep everything
de-centralized code-wise.  I'm less concerned about histograms.xml knowing
about every possible infobar because it's kind of meant to.

> I wonder, if we have a central enum, whether we should further centralize
> various things -- e.g. have base class functions in InfoBarDelegate to return
> the infobar type and icon which are just switch statements.  We've avoided
this
> in the past to keep everything about new infobars specified in those new
> classes, but if we have to do something central anyway, making this change
might
> make functionality more explicit and easier to collect data about the
infobars.

I think it'd make sense, but that'd require components knowing about what icons
exist, right?  I thought those were defined by the PNG files in the chrome/
directory.  There's also infobars that override GetVectorIconId(), which takes
precedence over GetIconId().

[Peter Kasting, 2015-12-28 22:28:32]

On 2015/12/28 20:54:51, dfalcantara wrote:
> Yeah, I agree that having the developer explicitly compute the value of the
> hashed string and add it to histograms.xml is suboptimal.  A few thoughts:
> 
> * It'll be easy for a dev to add a new infobar but not update histograms.xml
>   unless we have some presubmit check for it.  PRESUBMIT.py, for example,
>   trawls the codebase for new "base::UserMetricsAction" usages and requires
>   devs to update actions.xml.  Feels like overkill to add a similar presubmit
>   check for a single metric, though.

I think this is one place the enum has strings beat.  If there's only one enum,
in components/ (ignore layering concerns), and InfoBarDelegate has a pure
virtual method to return which infobar you are, and all the enum values have
very specific names, then it's impossible to forget to return _some_ value, and
it looks screamingly wrong if you copy-and-paste someone else's value.  So that
guarantees someone will add a new enum.  If we then put a big all-caps comment
at top and/or bottom of the enum saying to keep histograms.xml in sync, it's
both obvious and easy to do the right thing -- and if I'm the OWNER reviewing
this, I'm unlikely to let any mistakes slip by to boot.

I think that would be a sufficient setup to avoid the need for a PRESUBMIT check
for this, which I agree we should avoid.

> * Using an enum won't guarantee that we won't have collisions because people
can
>   just reuse enum values, but it'll at least be much more obvious when this
>   happens.

Yeah, I think

  InfoBarType FranksSuperRadInfoBarDelegate::GetInfoBarType() const {
    return BOBS_TOTALLY_UNCOOL_INFOBAR_DELEGATE;
  }

is so glaring it won't happen.

> * Using an enum means that anyone re-using the infobar component without
>   chrome/ will have to keep their version of the enum up to date with chrome/.
>   I thought that one of the goals of componentization was to prevent any
>   dependencies on chrome/, which is suggested by components/README, but I
>   could be mistaken.

I think if we do an enum it only makes sense to put the whole thing in one spot,
in infobar_delegate.h, and to just accept the fact that this is in principle
some kind of layering violation.  Splitting the enum into pieces has a host of
downsides and the only upside is that we avoid committing a layering violation. 
But in practice, I'm not worried about this particular layering violation.  It
doesn't cause any practical problems (e.g. exporting the wrong symbols, making
it impossible to build code based on components/ without linking in chrome/,
etc.); it's really just about the spirit of things, as far as I can tell.  If
so, that argument doesn't carry a ton of weight for me.  At the end of the day,
components/ and all our layers exist in order to facilitate building the
products we need, and we don't build one side blindly without understanding how
the other will interface with it; so if we need to put this knowledge into
components/, so be it.

> > I wonder, if we have a central enum, whether we should further centralize
> > various things -- e.g. have base class functions in InfoBarDelegate to
return
> > the infobar type and icon which are just switch statements.  We've avoided
> this
> > in the past to keep everything about new infobars specified in those new
> > classes, but if we have to do something central anyway, making this change
> might
> > make functionality more explicit and easier to collect data about the
> infobars.
> 
> I think it'd make sense, but that'd require components knowing about what
icons
> exist, right?  I thought those were defined by the PNG files in the chrome/
> directory.  There's also infobars that override GetVectorIconId(), which takes
> precedence over GetIconId().

I doubt this is a problem, at least for the PNG icons; we have various different
image resource enumerations in different places (e.g. in both ui/ and
chrome/browser/ui/) and we can hoist things above chrome/ if need be.  I don't
know offhand if the vector icons would produce more of an issue.  In the end, I
think we should probably set this consideration aside when deciding, and only
raise it if we elect to go with an enum, in the sense of maybe allowing an
additional optimization (but if not, that's fine too).

[gone, 2015-12-29 01:17:56]

Uploaded a new CL, but I'm still running it through the
bots to see what compile errors arise on the other
platforms.  Does this generally jibe with what you were
thinking?

* I didn't add unique IDs for the tests that are creating
  infobars, figuring that they wouldn't be sending metrics
  anyway.  All cases I found are assigned TEST_INFOBAR_ID.

* I just realized we already violate layering with the 
  InfoBarDelegate::As*InfoBarDelegate* methods.  An enum
  doesn't seem terrible at this point.

* I wasn't sure whether to order the new enum.  Ordering it
  could suggest to anyone missing the comment that they 
  have to add new values in alphabetical order, which would
  break the histogram.

* I forgot to make the enum comment all caps; I'll fix it
  with a new upload once all the bots finish.

> I doubt this is a problem, at least for the PNG icons; we have various
different
> image resource enumerations in different places (e.g. in both ui/ and
> chrome/browser/ui/) and we can hoist things above chrome/ if need be.  I don't
> know offhand if the vector icons would produce more of an issue.  In the end,
I
> think we should probably set this consideration aside when deciding, and only
> raise it if we elect to go with an enum, in the sense of maybe allowing an
> additional optimization (but if not, that's fine too).

Sounds good to me.

[gone, 2015-12-29 01:18:28]

Description was changed from

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns a
  string identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().  Values are
  determined the same way about:flags does it, where the string
  is md5 hashed and some subset of the digest is converted to
  a signed integer.

BUG=567483
==========

to

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns an
  enum identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().

BUG=567483
==========

[Peter Kasting, 2015-12-30 01:52:57]

LGTM

https://chromiumcodereview.appspot.com/1520543004/diff/480001/chrome/browser/...
File chrome/browser/devtools/global_confirm_info_bar.cc (right):

https://chromiumcodereview.appspot.com/1520543004/diff/480001/chrome/browser/...
chrome/browser/devtools/global_confirm_info_bar.cc:58: : INVALID_ID;
Uggghhhh.  This really shouldn't be needed; I don't think the design of this
class is correct at all.  I filed bug 573088 against myself to understand and
fix up this code.

I'm inclined to suggest that we make INVALID_ID -1 and add a DCHECK in
InfoBarContainer::AddInfoBar() that we don't get this value there (which we
never should, even with this code existing); then I can later rip this out
entirely without leaving a hole at the beginning of the histogram.

I don't just want this for cleanliness; I worry that if we leave this value,
someone later will try to use it for some kind of "infobar that I don't care
about collecting metrics on" and I don't want to leave that possibility.

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
File components/infobars/core/infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
components/infobars/core/infobar_delegate.h:67: INVALID_ID,
I'm inclined to remove "_ID" from every item as I'm not sure it adds anything
other than verbosity.

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
components/infobars/core/infobar_delegate.h:68: TEST_INFOBAR_ID,
What do you think about putting an explicit "= n" on the end of each item, so
one can cross-reference more easily to the histogram, and so we can more easily
remove dead values in the future?

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
File components/infobars/core/simple_alert_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
components/infobars/core/simple_alert_infobar_delegate.cc:31:
infobar_identifier_(infobar_identifier),
I realized while reviewing this that we could avoid this member if, instead of
adding GetIdentifier(), we implemented this change by passing the enum value
directly to AddInfoBar().  That would result in this CL touching half as many
places (since we wouldn't need to update class declarations in header files). 
However, that would preclude the possibility of reimplementing some of the
existing methods (e.g. GetInfoBarType() as switch statements in a non-virtual
base class function.  So it's probably better to keep things as you have them.

[gone, 2015-12-30 07:29:03]

Alright, I think that should be everything.  Running it through the trybots
again now.

Peter: Can you please do a sanity check?
Nico: Is the CL good to go now that we've got an enum?
Ilya/Robert: I don't think anything is fundamentally different with the metrics
stuff since we were already using ints; the values just changed.

https://chromiumcodereview.appspot.com/1520543004/diff/480001/chrome/browser/...
File chrome/browser/devtools/global_confirm_info_bar.cc (right):

https://chromiumcodereview.appspot.com/1520543004/diff/480001/chrome/browser/...
chrome/browser/devtools/global_confirm_info_bar.cc:58: : INVALID_ID;
On 2015/12/30 01:52:57, Peter Kasting wrote:
> Uggghhhh.  This really shouldn't be needed; I don't think the design of this
> class is correct at all.  I filed bug 573088 against myself to understand and
> fix up this code.
> 
> I'm inclined to suggest that we make INVALID_ID -1 and add a DCHECK in
> InfoBarContainer::AddInfoBar() that we don't get this value there (which we
> never should, even with this code existing); then I can later rip this out
> entirely without leaving a hole at the beginning of the histogram.
> 
> I don't just want this for cleanliness; I worry that if we leave this value,
> someone later will try to use it for some kind of "infobar that I don't care
> about collecting metrics on" and I don't want to leave that possibility.

Yeah, when I hit this I was a bit confused about why the class was written as it
was.  I've added a DCHECK and changed the INVALID value to -1.  Thanks for
looking into it.

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
File components/infobars/core/infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
components/infobars/core/infobar_delegate.h:67: INVALID_ID,
On 2015/12/30 01:52:57, Peter Kasting wrote:
> I'm inclined to remove "_ID" from every item as I'm not sure it adds anything
> other than verbosity.

Done.

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
components/infobars/core/infobar_delegate.h:68: TEST_INFOBAR_ID,
On 2015/12/30 01:52:57, Peter Kasting wrote:
> What do you think about putting an explicit "= n" on the end of each item, so
> one can cross-reference more easily to the histogram, and so we can more
easily
> remove dead values in the future?

Sounds good.  Alleviates a few of my concerns about the values getting out of
sync, too.

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
File components/infobars/core/simple_alert_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/1520543004/diff/480001/components/info...
components/infobars/core/simple_alert_infobar_delegate.cc:31:
infobar_identifier_(infobar_identifier),
On 2015/12/30 01:52:57, Peter Kasting wrote:
> I realized while reviewing this that we could avoid this member if, instead of
> adding GetIdentifier(), we implemented this change by passing the enum value
> directly to AddInfoBar().  That would result in this CL touching half as many
> places (since we wouldn't need to update class declarations in header files). 
> However, that would preclude the possibility of reimplementing some of the
> existing methods (e.g. GetInfoBarType() as switch statements in a non-virtual
> base class function.  So it's probably better to keep things as you have them.

Huh, that would have definitely made sense for this CL.  I agree that's probably
better to leave it as is, though, since it'll make future potential changes
easier.  I could imagine new metrics being thought up once UX starts
getting some of this data in, e.g., and we'd end up having to do this anyway.

[Peter Kasting, 2015-12-30 13:09:26]

LGTM

https://chromiumcodereview.appspot.com/1520543004/diff/520001/components/info...
File components/infobars/core/infobar_container.cc (right):

https://chromiumcodereview.appspot.com/1520543004/diff/520001/components/info...
components/infobars/core/infobar_container.cc:160:
DCHECK(infobar->delegate()->GetIdentifier() != InfoBarDelegate::INVALID);
Nit: DCHECK_NE(InfoBarDelegate::INVALID, infobar->delegate()->GetIdentifier());

[Nico, 2015-12-30 14:27:51]

rs-lgtm

[gone, 2015-12-30 18:38:43]

Thanks!

https://codereview.chromium.org/1520543004/diff/520001/components/infobars/co...
File components/infobars/core/infobar_container.cc (right):

https://codereview.chromium.org/1520543004/diff/520001/components/infobars/co...
components/infobars/core/infobar_container.cc:160:
DCHECK(infobar->delegate()->GetIdentifier() != InfoBarDelegate::INVALID);
On 2015/12/30 13:09:26, Peter Kasting wrote:
> Nit: DCHECK_NE(InfoBarDelegate::INVALID,
infobar->delegate()->GetIdentifier());

Done.

[gone, 2015-12-30 18:38:53]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2015-12-30 18:38:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, isherman@chromium.org, droger@chromium.org,
thakis@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1520543004/#ps560001
(title: "Nit again")

[commit-bot: I haz the power, 2015-12-30 18:39:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1520543004/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1520543004/560001

[commit-bot: I haz the power, 2015-12-30 20:07:35]

Description was changed from

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns an
  enum identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().

BUG=567483
==========

to

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns an
  enum identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().

BUG=567483
==========

[commit-bot: I haz the power, 2015-12-30 20:07:36]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2015-12-30 20:08:36]

Description was changed from

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns an
  enum identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().

BUG=567483
==========

to

==========
Add method for identifying different InfoBars

* Adds InfoBarDelegate::GetIdentifier(), which returns an
  enum identifying the InfoBar being created.  Generally,
  this is the name of the InfoBarDelegate, but in cases of
  classes using SimpleAlertInfoBarDelegates (or other wrapper
  classes), it uses the name of the class using the delegate.

* Adds a histogram that records which infobars were shown to
  the user via InfoBarContainer#AddInfoBar().

BUG=567483

Committed: https://crrev.com/ac4d93be8bc37c31f3b19749272e5d176eb8d2cf
Cr-Commit-Position: refs/heads/master@{#367176}
==========

[commit-bot: I haz the power, 2015-12-30 20:08:37]

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