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

chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc

 // Copyright (c) 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 
 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() {
 }
 
 GURL ExclusiveAccessControllerBase::GetExclusiveAccessBubbleURL() const {
   return manager_->GetExclusiveAccessBubbleURL();
 }
 
 GURL ExclusiveAccessControllerBase::GetURLForExclusiveAccessBubble() const {
   if (tab_with_exclusive_access_)
@@ skipping 28 matching lines @@
 void ExclusiveAccessControllerBase::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type);
   if (content::Details<content::LoadCommittedDetails>(details)
           ->is_navigation_to_different_page())
     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
   // UpdateNotificationRegistrations would need updating.
   DCHECK(tab_with_exclusive_access_ == tab ||
          tab_with_exclusive_access_ == nullptr || tab == nullptr);
   tab_with_exclusive_access_ = tab;
   UpdateNotificationRegistrations();
 }
 
 void ExclusiveAccessControllerBase::UpdateNotificationRegistrations() {
   if (tab_with_exclusive_access_ && registrar_.IsEmpty()) {
     registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                    content::Source<content::NavigationController>(
                        &tab_with_exclusive_access_->GetController()));
   } else if (!tab_with_exclusive_access_ && !registrar_.IsEmpty()) {
     registrar_.RemoveAll();
   }
 }