Schedule posttask to decide Uma optin option offering

chrome/browser/ui/views/session_crashed_bubble_view.cc

Index: chrome/browser/ui/views/session_crashed_bubble_view.cc
diff --git a/chrome/browser/ui/views/session_crashed_bubble_view.cc b/chrome/browser/ui/views/session_crashed_bubble_view.cc
index c143725e9d37c7fda63cdbc6955a009186089e46..e1f1aebd64e4601b98919d02ed6146632770d249 100644
--- a/chrome/browser/ui/views/session_crashed_bubble_view.cc
+++ b/chrome/browser/ui/views/session_crashed_bubble_view.cc
@@ -6,6 +6,9 @@
 
 #include <vector>
 
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"

[Alexei Svitkine (slow), 2014/05/14 14:09:57]

Is this include needed?

[yao, 2014/05/14 14:53:28]

Done.

 #include "base/prefs/pref_service.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -20,6 +23,7 @@
 #include "chrome/common/url_constants.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/chromium_strings.h"
@@ -39,6 +43,9 @@ using views::GridLayout;
 
 namespace {
 
+// Define a macro to record the current source location.
+#define FROM_HERE FROM_HERE_WITH_EXPLICIT_FUNCTION(__FUNCTION__)

[Alexei Svitkine (slow), 2014/05/14 14:09:57]

Why is this needed? Can you include a header that defines this instead?

[yao, 2014/05/14 14:53:28]

Done.

+
 // Fixed width of the column holding the description label of the bubble.
 const int kWidthOfDescriptionText = 320;
 
@@ -71,12 +78,38 @@ void SessionCrashedBubbleView::Show(Browser* browser) {
   if (browser->profile()->IsOffTheRecord())
     return;
 
+  // Schedule a task to run ShouldOfferMetricsReporting() on FILE thread. The
+  // reason of doing this is that GoogleUpdateSettings::GetCollectStatsConsent()
+  // can't be called from UI thread, but current function might be called from

[Alexei Svitkine (slow), 2014/05/14 14:09:57]

Nit: "might be" -> "is"

[yao, 2014/05/14 14:53:28]

When the browser starts up, it's not called by UI thread. Only when the user
switches profiles, it's called by UI thread.

(Based on when the crash is observed)

[Alexei Svitkine (slow), 2014/05/14 15:02:54]

It should always be called on the UI thread. If it's not, then there's another
bug.

(However, it's quite possible that during browser start up the IO checks are not
turned on yet.)

To confirm, you can add the following at the top of the function (and probably
good to have in general):

 DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[yao, 2014/05/14 21:00:26]

Done.

+  // UI thread. After function ShouldOfferMetricsReporting returns,
+  // calls SessionCrashedBubbleView::ShowForReal with the returned value as a
+  // parameter to create and show the bubble.
+  base::Callback<bool(void)> decideMetricsReportingFn = base::Bind(
+      &ShouldOfferMetricsReporting);

[Alexei Svitkine (slow), 2014/05/14 14:09:57]

Nit: The code would be less verbose if you inline these into the PostTask(...)
invocation. (Plus you're not using the right naming convention for them
anyway...)

[yao, 2014/05/14 14:53:28]

Done.

+
+  base::Callback<void(bool)> afterMetricsReportingDecidedFn =
+      base::Bind(&SessionCrashedBubbleView::ShowForReal, browser);
+
+  content::BrowserThread::PostTaskAndReplyWithResult(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      decideMetricsReportingFn,
+      afterMetricsReportingDecidedFn);
+}
+
+// static
+void SessionCrashedBubbleView::ShowForReal(Browser* browser,
+                                           bool offer_uma_optin) {
   views::View* anchor_view =
       BrowserView::GetBrowserViewForBrowser(browser)->toolbar()->app_menu();
   content::WebContents* web_contents =
       browser->tab_strip_model()->GetActiveWebContents();
   SessionCrashedBubbleView* crash_bubble =
       new SessionCrashedBubbleView(anchor_view, browser, web_contents);
+  // Record the return value of function ShouldOfferMetricsReporting() in a
+  // member of the object.
+  crash_bubble->offer_uma_optin_ = offer_uma_optin;

[Alexei Svitkine (slow), 2014/05/14 14:09:57]

Make it a ctor parameter instead.

[yao, 2014/05/14 14:53:28]

Done.

+  // Show the bubble.
   views::BubbleDelegateView::CreateBubble(crash_bubble)->Show();
 }
 
@@ -91,6 +124,7 @@ SessionCrashedBubbleView::SessionCrashedBubbleView(
       restore_button_(NULL),
       close_(NULL),
       uma_option_(NULL),
+      offer_uma_optin_(false),
       started_navigation_(false) {
   set_close_on_deactivate(false);
   registrar_.Add(
@@ -188,7 +222,7 @@ void SessionCrashedBubbleView::Init() {
   layout->AddPaddingRow(0, kMarginHeight);
 
   // Metrics reporting option.
-  if (ShouldOfferMetricsReporting())
+  if (offer_uma_optin_)
     CreateUmaOptinView(layout);
 
   set_color(kWhiteBackgroundColor);