Added UMA collection for fullscreen / mouse lock bubble re-shows.

chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc

Index: chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
diff --git a/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc b/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
index 3559f376a678aa8b5671f2a8e86a92ff8d2964b1..af0423b086d3d488bc4ed3099f3bff4bada70866 100644
--- a/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
+++ b/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h"
 
+#include "base/metrics/histogram_macros.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/exclusive_access/exclusive_access_manager.h"
@@ -14,10 +15,16 @@
 
 using content::WebContents;
 
+namespace {
+
+const char kBubbleReshowsHistogramPrefix[] =
+    "ExclusiveAccess.BubbleReshowsPerSession.";
+
+}  // namespace
+
 ExclusiveAccessControllerBase::ExclusiveAccessControllerBase(
     ExclusiveAccessManager* manager)
-    : manager_(manager), tab_with_exclusive_access_(nullptr) {
-}
+    : manager_(manager) {}
 
 ExclusiveAccessControllerBase::~ExclusiveAccessControllerBase() {
 }
@@ -66,6 +73,22 @@ void ExclusiveAccessControllerBase::Observe(
     ExitExclusiveAccessIfNecessary();
 }
 
+void ExclusiveAccessControllerBase::BubbleReshown() {
+  ++bubble_reshow_count_;
+}
+
+void ExclusiveAccessControllerBase::Exiting() {
+  // Record the number of bubble reshows during this session. Only if simplified
+  // fullscreen is enabled.
+  if (ExclusiveAccessManager::IsSimplifiedFullscreenUIEnabled()) {
+    std::string histogram_name = kBubbleReshowsHistogramPrefix;
+    histogram_name += HistogramSuffix();
+    UMA_HISTOGRAM_COUNTS_100(histogram_name, bubble_reshow_count_);

[Ilya Sherman, 2016/02/24 01:08:20]

The UMA_HISTOGRAM_* macros require that histogram names be runtime constants, so
you'll need one macro invocation per histogram name.  (Or, you could skip the
macro and call the underlying C++ functions directly.)

[Matt Giuca, 2016/02/24 03:00:31]

They do? I could do that but I'm not sure why it's necessary. (Is it for
technical reasons, or a "rule" you have?)

UMA_HISTOGRAM_COUNTS_100 calls UMA_HISTOGRAM_CUSTOM_COUNTS, which uses name
twice:

1. name is passed to STATIC_HISTOGRAM_POINTER_BLOCK which passes it to
HISTOGRAM_POINTER_USE which passes it to HistogramBase::CheckName, which expects
a const StringPiece&.
2. name is passed to base::Histogram::FactoryGet which expects a const
std::string&.

In both cases, passing a non-constant std::string is fine.

So do I have to change it?

[Matt Giuca, 2016/02/24 04:01:00]

Oh right... I just hit the DCHECK in HistogramBase::CheckName. (Not sure why I
didn't hit it in my previous testing.) Now I get it; it specifically ensures
that you don't change the name string between instances of the same static call
to UMA_HISTOGRAM_* so that it can optimize.

(This isn't really documented... there is a huge block of text in
histogram_macros.h about how this static checking works, but I just assumed it
was implementation details. There is no documentation on the UMA_HISTOGRAM_*
macros themselves that say "name must be a compile-time constant"; maybe all of
those "name" parameters should just be renamed to "constant_name" to drive the
point home?)

I moved the call to UMA_HISTOGRAM_COUNTS_100 into each subclass (with a constant
string). A bit of duplication, but the alternative is to use a switch statement
over the type of subclass, which is nasty. And I don't want to thwart the
optimization by directly calling the function.

[Ilya Sherman, 2016/02/24 06:42:34]

Yeah, it is definitely a rather epic comment.  I agree that it would be nice to
somehow make this point clearer, though I'm not sure that changing the variable
names in the macro to constant_name would be all that much clearer.  Documenting
each macro might be.

FWIW, the optimization only really matters in tight loops -- you probably don't
really need it for this code.  But, LGTM either way.

[Matt Giuca, 2016/02/25 04:27:08]

I'd rather use the macro than manually inlining it (regardless of optimization).
Otherwise I feel like I'm digging into the implementation details which may
break.

+  }
+
+  bubble_reshow_count_ = 0;
+}
+
 void ExclusiveAccessControllerBase::SetTabWithExclusiveAccess(
     WebContents* tab) {
   // Tab should never be replaced with another tab, or