Schedule posttask to decide Uma optin option offering

chrome/browser/ui/views/session_crashed_bubble_view.cc

 // Copyright 2014 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/views/session_crashed_bubble_view.h"
 
 #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"
 #include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/ui/options/options_util.h"
 #include "chrome/browser/ui/startup/session_crashed_bubble.h"
 #include "chrome/browser/ui/startup/startup_browser_creator_impl.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/common/pref_names.h"
 #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"
 #include "grit/generated_resources.h"
 #include "grit/ui_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/controls/button/checkbox.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
 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;
 
 // Margins width for the top rows to compensate for the bottom panel for which
 // we don't want any margin.
 const int kMarginWidth = 12;
 const int kMarginHeight = kMarginWidth;
 
 // The color of the background of the sub panel to offer UMA optin.
 const SkColor kLightGrayBackgroundColor = 0xFFF0F0F0;
@@ skipping 12 matching lines @@
 #endif  // defined(GOOGLE_CHROME_BUILD)
 }
 
 }  // namespace
 
 // static
 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();
 }
 
 SessionCrashedBubbleView::SessionCrashedBubbleView(
     views::View* anchor_view,
     Browser* browser,
     content::WebContents* web_contents)
     : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
       content::WebContentsObserver(web_contents),
       browser_(browser),
       web_contents_(web_contents),
       restore_button_(NULL),
       close_(NULL),
       uma_option_(NULL),
+      offer_uma_optin_(false),
       started_navigation_(false) {
   set_close_on_deactivate(false);
   registrar_.Add(
       this,
       chrome::NOTIFICATION_TAB_CLOSING,
       content::Source<content::NavigationController>(&(
           web_contents->GetController())));
   browser->tab_strip_model()->AddObserver(this);
 }
 
@@ skipping 77 matching lines @@
 
   layout->StartRow(0, kTextColumnSetId);
   layout->AddView(text_label);
   layout->AddPaddingRow(0, kMarginHeight);
 
   layout->StartRow(0, kButtonColumnSetId);
   layout->AddView(restore_button_);
   layout->AddPaddingRow(0, kMarginHeight);
 
   // Metrics reporting option.
-  if (ShouldOfferMetricsReporting())
+  if (offer_uma_optin_)
     CreateUmaOptinView(layout);
 
   set_color(kWhiteBackgroundColor);
   set_margins(gfx::Insets());
   Layout();
 }
 
 void SessionCrashedBubbleView::CreateUmaOptinView(GridLayout* layout) {
   // Checkbox for metric reporting setting.
   uma_option_ = new views::Checkbox(
@@ skipping 86 matching lines @@
 }
 
 void SessionCrashedBubbleView::CloseBubble() {
   GetWidget()->Close();
 }
 
 bool ShowSessionCrashedBubble(Browser* browser) {
   SessionCrashedBubbleView::Show(browser);
   return true;
 }