Re-land: Compute image description histograms on the right thread.

Original: r648795 http://crrev.com/c/1551803
Revert: r649032 http://crrev.com/c/1559413

The revert was due to calling the Chrome OS histogram code on
the wrong thread. Fixed to use the UI thread.

Original description:

The code to compute image description histograms was
hitting a DCHECK because it was running on the wrong
thread. I temporarily disabled that code, this change
re-enables it and fixes the underlying issue.

I noticed that we had code to compute accessibility
histograms that were split by platform - on some platforms
we were running the code on the UI thread, on other platforms
on a separate thread.

To solve this, I changed the code so that we can update
histograms on either thread on any platform.

TBR=jam@chromium.org

Bug: 940805
Change-Id: I1a8402e8004c2830f66e6c4249e3f883aa1490c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1570207
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#651460}
diff --git a/chrome/browser/accessibility/accessibility_labels_service.cc b/chrome/browser/accessibility/accessibility_labels_service.cc
index 64dddcc..dd31968 100644
--- a/chrome/browser/accessibility/accessibility_labels_service.cc
+++ b/chrome/browser/accessibility/accessibility_labels_service.cc
@@ -55,14 +55,12 @@
           &AccessibilityLabelsService::OnImageLabelsEnabledChanged,
           weak_factory_.GetWeakPtr()));
 
-  // Log whether the feature is enabled after startup.
-  // TODO(dmazzoni) re-enable. http://crbug.com/940805
-#if 0
-  content::BrowserAccessibilityState::GetInstance()->AddHistogramCallback(
-      base::BindRepeating(
+  // Log whether the feature is enabled after startup. This must be run on the
+  // UI thread because it accesses prefs.
+  content::BrowserAccessibilityState::GetInstance()
+      ->AddUIThreadHistogramCallback(base::BindRepeating(
           &AccessibilityLabelsService::UpdateAccessibilityLabelsHistograms,
           weak_factory_.GetWeakPtr()));
-#endif
 }
 
 AccessibilityLabelsService::AccessibilityLabelsService(Profile* profile)
diff --git a/chrome/browser/chromeos/accessibility/accessibility_manager.cc b/chrome/browser/chromeos/accessibility/accessibility_manager.cc
index 4211da35..51d11d3 100644
--- a/chrome/browser/chromeos/accessibility/accessibility_manager.cc
+++ b/chrome/browser/chromeos/accessibility/accessibility_manager.cc
@@ -1096,9 +1096,12 @@
         base::Bind(&AccessibilityManager::OnLocaleChanged,
                    base::Unretained(this)));
 
-    content::BrowserAccessibilityState::GetInstance()->AddHistogramCallback(
-        base::Bind(&AccessibilityManager::UpdateChromeOSAccessibilityHistograms,
-                   base::Unretained(this)));
+    // Compute these histograms on the main (UI) thread because they
+    // need to access PrefService.
+    content::BrowserAccessibilityState::GetInstance()
+        ->AddUIThreadHistogramCallback(base::BindOnce(
+            &AccessibilityManager::UpdateChromeOSAccessibilityHistograms,
+            base::Unretained(this)));
 
     extensions::ExtensionRegistry* registry =
         extensions::ExtensionRegistry::Get(profile);
diff --git a/content/browser/accessibility/browser_accessibility_state_impl.cc b/content/browser/accessibility/browser_accessibility_state_impl.cc
index 01b8057..cbb9640 100644
--- a/content/browser/accessibility/browser_accessibility_state_impl.cc
+++ b/content/browser/accessibility/browser_accessibility_state_impl.cc
@@ -71,22 +71,25 @@
   // Let each platform do its own initialization.
   PlatformInitialize();
 
-#if defined(OS_WIN) || defined(OS_ANDROID)
+  // Schedule calls to update histograms after a delay.
+  //
   // The delay is necessary because assistive technology sometimes isn't
   // detected until after the user interacts in some way, so a reasonable delay
   // gives us better numbers.
+
+  // Some things can be done on another thread safely.
   base::PostDelayedTaskWithTraits(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
-      base::BindOnce(&BrowserAccessibilityStateImpl::UpdateHistograms, this),
+      base::BindOnce(
+          &BrowserAccessibilityStateImpl::UpdateHistogramsOnOtherThread, this),
       base::TimeDelta::FromSeconds(ACCESSIBILITY_HISTOGRAM_DELAY_SECS));
-#else
-  // On MacOS, UpdateHistograms should be called on the UI thread because it
-  // needs to be able to access PrefService.
+
+  // Other things must be done on the UI thread (e.g. to access PrefService).
   base::PostDelayedTaskWithTraits(
       FROM_HERE, {BrowserThread::UI},
-      base::BindOnce(&BrowserAccessibilityStateImpl::UpdateHistograms, this),
+      base::BindOnce(&BrowserAccessibilityStateImpl::UpdateHistogramsOnUIThread,
+                     this),
       base::TimeDelta::FromSeconds(ACCESSIBILITY_HISTOGRAM_DELAY_SECS));
-#endif
 }
 
 BrowserAccessibilityStateImpl::~BrowserAccessibilityStateImpl() {
@@ -136,20 +139,27 @@
   return accessibility_mode_ == ui::kAXModeComplete;
 }
 
-void BrowserAccessibilityStateImpl::AddHistogramCallback(
-    base::Closure callback) {
-  histogram_callbacks_.push_back(std::move(callback));
+void BrowserAccessibilityStateImpl::AddUIThreadHistogramCallback(
+    base::OnceClosure callback) {
+  ui_thread_histogram_callbacks_.push_back(std::move(callback));
+}
+
+void BrowserAccessibilityStateImpl::AddOtherThreadHistogramCallback(
+    base::OnceClosure callback) {
+  other_thread_histogram_callbacks_.push_back(std::move(callback));
 }
 
 void BrowserAccessibilityStateImpl::UpdateHistogramsForTesting() {
-  UpdateHistograms();
+  UpdateHistogramsOnUIThread();
+  UpdateHistogramsOnOtherThread();
 }
 
-void BrowserAccessibilityStateImpl::UpdateHistograms() {
-  UpdatePlatformSpecificHistograms();
+void BrowserAccessibilityStateImpl::UpdateHistogramsOnUIThread() {
+  UpdatePlatformSpecificHistogramsOnUIThread();
 
-  for (size_t i = 0; i < histogram_callbacks_.size(); ++i)
-    histogram_callbacks_[i].Run();
+  for (auto& callback : ui_thread_histogram_callbacks_)
+    std::move(callback).Run();
+  ui_thread_histogram_callbacks_.clear();
 
   // TODO(dmazzoni): remove this in M59 since Accessibility.ModeFlag
   // supercedes it.  http://crbug.com/672205
@@ -162,6 +172,14 @@
                             switches::kForceRendererAccessibility));
 }
 
+void BrowserAccessibilityStateImpl::UpdateHistogramsOnOtherThread() {
+  UpdatePlatformSpecificHistogramsOnOtherThread();
+
+  for (auto& callback : other_thread_histogram_callbacks_)
+    std::move(callback).Run();
+  other_thread_histogram_callbacks_.clear();
+}
+
 void BrowserAccessibilityStateImpl::OnAXModeAdded(ui::AXMode mode) {
   AddAccessibilityModeFlags(mode);
 }
@@ -173,7 +191,10 @@
 #if !defined(OS_ANDROID) && !defined(OS_WIN) && !defined(OS_MACOSX)
 void BrowserAccessibilityStateImpl::PlatformInitialize() {}
 
-void BrowserAccessibilityStateImpl::UpdatePlatformSpecificHistograms() {}
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnUIThread() {}
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnOtherThread() {}
 #endif
 
 void BrowserAccessibilityStateImpl::AddAccessibilityModeFlags(ui::AXMode mode) {
diff --git a/content/browser/accessibility/browser_accessibility_state_impl.h b/content/browser/accessibility/browser_accessibility_state_impl.h
index 30dd348..2af968c 100644
--- a/content/browser/accessibility/browser_accessibility_state_impl.h
+++ b/content/browser/accessibility/browser_accessibility_state_impl.h
@@ -57,7 +57,8 @@
   void ResetAccessibilityMode() override;
   void OnScreenReaderDetected() override;
   bool IsAccessibleBrowser() override;
-  void AddHistogramCallback(base::Closure callback) override;
+  void AddUIThreadHistogramCallback(base::OnceClosure callback) override;
+  void AddOtherThreadHistogramCallback(base::OnceClosure callback) override;
 
   void UpdateHistogramsForTesting() override;
 
@@ -85,17 +86,22 @@
 
   // Called a short while after startup to allow time for the accessibility
   // state to be determined. Updates histograms with the current state.
-  void UpdateHistograms();
+  // Two variants - one for things that must be run on the UI thread, and
+  // another that can be run on another thread.
+  void UpdateHistogramsOnUIThread();
+  void UpdateHistogramsOnOtherThread();
 
   // Leaky singleton, destructor generally won't be called.
   ~BrowserAccessibilityStateImpl() override;
 
   void PlatformInitialize();
-  void UpdatePlatformSpecificHistograms();
+  void UpdatePlatformSpecificHistogramsOnUIThread();
+  void UpdatePlatformSpecificHistogramsOnOtherThread();
 
   ui::AXMode accessibility_mode_;
 
-  std::vector<base::Closure> histogram_callbacks_;
+  std::vector<base::OnceClosure> ui_thread_histogram_callbacks_;
+  std::vector<base::OnceClosure> other_thread_histogram_callbacks_;
 
   bool disable_hot_tracking_;
 
diff --git a/content/browser/accessibility/browser_accessibility_state_impl_android.cc b/content/browser/accessibility/browser_accessibility_state_impl_android.cc
index 7622a81..1cc7b7a 100644
--- a/content/browser/accessibility/browser_accessibility_state_impl_android.cc
+++ b/content/browser/accessibility/browser_accessibility_state_impl_android.cc
@@ -21,8 +21,12 @@
   Java_BrowserAccessibilityState_registerAnimatorDurationScaleObserver(env);
 }
 
-void BrowserAccessibilityStateImpl::UpdatePlatformSpecificHistograms() {
-  // NOTE: this method is run from the file thread to reduce jank, since
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnUIThread() {}
+
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnOtherThread() {
+  // NOTE: this method is run from another thread to reduce jank, since
   // there's no guarantee these system calls will return quickly. Be careful
   // not to add any code that isn't safe to run from a non-main thread!
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
diff --git a/content/browser/accessibility/browser_accessibility_state_impl_mac.mm b/content/browser/accessibility/browser_accessibility_state_impl_mac.mm
index fde90e81..e91b463 100644
--- a/content/browser/accessibility/browser_accessibility_state_impl_mac.mm
+++ b/content/browser/accessibility/browser_accessibility_state_impl_mac.mm
@@ -59,8 +59,8 @@
       base::BindOnce(&SetupAccessibilityDisplayOptionsNotifier));
 }
 
-void BrowserAccessibilityStateImpl::UpdatePlatformSpecificHistograms() {
-  // NOTE: This function is running on the file thread.
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnUIThread() {
   NSWorkspace* workspace = [NSWorkspace sharedWorkspace];
 
   SEL sel = @selector(accessibilityDisplayShouldIncreaseContrast);
@@ -86,4 +86,7 @@
   }
 }
 
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnOtherThread() {}
+
 }  // namespace content
diff --git a/content/browser/accessibility/browser_accessibility_state_impl_win.cc b/content/browser/accessibility/browser_accessibility_state_impl_win.cc
index 641f406..2c66497 100644
--- a/content/browser/accessibility/browser_accessibility_state_impl_win.cc
+++ b/content/browser/accessibility/browser_accessibility_state_impl_win.cc
@@ -89,10 +89,15 @@
       new gfx::SingletonHwndObserver(base::BindRepeating(&OnWndProc)));
 }
 
-void BrowserAccessibilityStateImpl::UpdatePlatformSpecificHistograms() {
-  // NOTE: this method is run from the file thread to reduce jank, since
-  // there's no guarantee these system calls will return quickly. Be careful
-  // not to add any code that isn't safe to run from a non-main thread!
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnUIThread() {}
+
+void BrowserAccessibilityStateImpl::
+    UpdatePlatformSpecificHistogramsOnOtherThread() {
+  // NOTE: this method is run from another thread to reduce jank, since
+  // there's no guarantee these system calls will return quickly. Code that
+  // needs to run in the UI thread can be run in
+  // UpdatePlatformSpecificHistogramsOnUIThread instead.
 
   AUDIODESCRIPTION audio_description = {0};
   audio_description.cbSize = sizeof(AUDIODESCRIPTION);
diff --git a/content/public/browser/browser_accessibility_state.h b/content/public/browser/browser_accessibility_state.h
index 484214b..389acd3 100644
--- a/content/public/browser/browser_accessibility_state.h
+++ b/content/public/browser/browser_accessibility_state.h
@@ -56,7 +56,14 @@
   // browser starts up, when accessibility state histograms are updated.
   // Use this to register a method to update additional accessibility
   // histograms.
-  virtual void AddHistogramCallback(base::Closure callback) = 0;
+  //
+  // Use this variant for a callback that must be run on the UI thread,
+  // for example something that needs to access prefs.
+  virtual void AddUIThreadHistogramCallback(base::OnceClosure callback) = 0;
+
+  // Use this variant for a callback that's better to run on another
+  // thread, for example something that may block or run slowly.
+  virtual void AddOtherThreadHistogramCallback(base::OnceClosure callback) = 0;
 
   virtual void UpdateHistogramsForTesting() = 0;
 };