SRT Bubble

chrome/browser/component_updater/sw_reporter_installer_win.cc

Index: chrome/browser/component_updater/sw_reporter_installer_win.cc
diff --git a/chrome/browser/component_updater/sw_reporter_installer_win.cc b/chrome/browser/component_updater/sw_reporter_installer_win.cc
index c8e4f9f3b64ea4474b11a9e74fd47093d9cc11d7..08baa3c7d159b6ea17840308fa470967dbc175b2 100644
--- a/chrome/browser/component_updater/sw_reporter_installer_win.cc
+++ b/chrome/browser/component_updater/sw_reporter_installer_win.cc
@@ -27,12 +27,18 @@
 #include "base/win/registry.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/srt_global_error.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/global_error/global_error_service.h"
+#include "chrome/browser/ui/global_error/global_error_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "components/component_updater/component_updater_paths.h"
 #include "components/component_updater/component_updater_service.h"
 #include "components/component_updater/component_updater_utils.h"
 #include "components/component_updater/default_component_installer.h"
 #include "components/component_updater/pref_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
@@ -76,6 +82,10 @@ const wchar_t kSoftwareRemovalToolRegistryKey[] =
     L"Software\\Google\\Software Removal Tool";
 const wchar_t kExitCodeRegistryValueName[] = L"ExitCode";
 
+// Exit codes that identify that a cleanup is needed.
+const int kCleanupNeeded = 0;
+const int kPostRebootCleanupNeeded = 4;
+
 void ReportUmaStep(SwReporterUmaValue value) {
   UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.Step", value, SW_REPORTER_MAX);
 }
@@ -84,9 +94,40 @@ void ReportUmaStep(SwReporterUmaValue value) {
 // and then clear it from the registry as well as clear the execution state
 // from the local state. This could be called from an interruptible worker
 // thread so should be resilient to unexpected shutdown.
-void ReportAndClearExitCode(int exit_code) {
+void ReportAndClearExitCode(int exit_code, const std::string& version) {

[gab, 2014/09/24 16:22:36]

Augment method comment with |version|.

[MAD, 2014/09/24 20:40:35]

Done.

   UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.ExitCode", exit_code);
 
+  if (exit_code == kPostRebootCleanupNeeded || exit_code == kCleanupNeeded) {
+    Browser* browser =
+        chrome::FindLastActiveWithHostDesktopType(chrome::GetActiveDesktop());

[gab, 2014/09/24 16:22:36]

I don't think this is what you want. This may be NULL in numerous scenarios (see
method comment) and can also be a non-tabbed browser.

The comment @
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
hints that you want to use a ScopedTabbedBrowserDisplayer for this use case
(which will even take care of creating one for you if none exist).

[MAD, 2014/09/24 20:40:35]

The problem is that I don't have a profile here, so I can't use
ScopedTabbedBrowserDisplayer. I think it's OK if in some edge cases we don't get
a browser, we will simply try again when we do have one. But your comment about
the potential of getting a non tabbed browser is interesting, indeed we see a
weird bubble if we launch with a --app= switch... Thanks! Fixed...

On 2014/09/24 16:22:36, gab wrote:

+    if (browser) {
+      Profile* profile = browser->profile();
+      DCHECK(profile);
+      const std::string prompt_state =
+          profile->GetPrefs()->GetString(prefs::kSwReporterPromptState);
+      // Don't show the prompt again if it's been shown before.
+      if (prompt_state.empty()) {
+        profile->GetPrefs()->SetString(prefs::kSwReporterPromptState, version);

[gab, 2014/09/24 16:22:36]

Only set this pref if we actually end up showing the bubble below.

[MAD, 2014/09/24 20:40:35]

No, because we still create the global error which makes the bubble available
via the menu, and will show up as a bubble as soon as the other bubbles get
dismissed.

+        GlobalErrorService* global_error_service =
+            GlobalErrorServiceFactory::GetForProfile(profile);
+        SRTGlobalError* global_error = new SRTGlobalError(global_error_service);
+        global_error_service->AddGlobalError(global_error);

[gab, 2014/09/24 16:22:36]

This gives ownership of |global_error|, make this explicit by putting it in a
scoped_ptr above and releasing it here.

[MAD, 2014/09/24 20:40:35]

If I release it here, I won't be able to use it below...

+
+        // Do not try to show bubble if another GlobalError is already showing
+        // one.
+        const GlobalErrorService::GlobalErrorList& global_errors(
+            global_error_service->errors());
+        GlobalErrorService::GlobalErrorList::const_iterator it;
+        for (it = global_errors.begin(); it != global_errors.end(); ++it) {
+          if ((*it)->GetBubbleView())

[robertshield, 2014/09/24 13:02:12]

Just to double check: GetBubbleView() should return NULL for the |global_error|
just added line 114 since it hasn't been shown yet, right?

[MAD, 2014/09/24 20:40:35]

Yes.

+            break;
+        }
+        if (it == global_errors.end())
+          global_error->ShowBubbleView(browser);

[gab, 2014/09/24 16:22:36]

This is not safe as you gave up ownership of |global_error| above and thus
depend on the internal implementation of GlobalErrorService to have kept it
alive (the scoped_ptr syntax suggested above specifically prevents this type of
mistake).

[MAD, 2014/09/24 20:40:35]

But then if we release it above, we still can't use it here... :-/
Added comments to clarify the lifespan and ownership...

+      }
+    }
+  }
+
   base::win::RegKey srt_key(
       HKEY_CURRENT_USER, kSoftwareRemovalToolRegistryKey, KEY_WRITE);
   srt_key.DeleteValue(kExitCodeRegistryValueName);
@@ -96,7 +137,8 @@ void ReportAndClearExitCode(int exit_code) {
 // wait for termination to collect its exit code. This task could be interrupted
 // by a shutdown at anytime, so it shouldn't depend on anything external that
 // could be shutdown beforehand.
-void LaunchAndWaitForExit(const base::FilePath& exe_path) {
+void LaunchAndWaitForExit(const base::FilePath& exe_path,
+                          const std::string& version) {
   const base::CommandLine reporter_command_line(exe_path);
   base::ProcessHandle scan_reporter_process = base::kNullProcessHandle;
   if (!base::LaunchProcess(reporter_command_line,
@@ -113,16 +155,10 @@ void LaunchAndWaitForExit(const base::FilePath& exe_path) {
   base::CloseProcessHandle(scan_reporter_process);
   scan_reporter_process = base::kNullProcessHandle;
   // It's OK if this doesn't complete, the work will continue on next startup.
-  BrowserThread::PostTask(BrowserThread::UI,
-                          FROM_HERE,
-                          base::Bind(&ReportAndClearExitCode, exit_code));
-}
-
-void ExecuteReporter(const base::FilePath& install_dir) {
-  base::WorkerPool::PostTask(
+  BrowserThread::PostTask(
+      BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&LaunchAndWaitForExit, install_dir.Append(kSwReporterExeName)),
-      true);
+      base::Bind(&ReportAndClearExitCode, exit_code, version));
 }
 
 class SwReporterInstallerTraits : public ComponentInstallerTraits {
@@ -161,7 +197,7 @@ class SwReporterInstallerTraits : public ComponentInstallerTraits {
         srt_key.ReadValueDW(kExitCodeRegistryValueName, &exit_code) ==
             ERROR_SUCCESS) {
       ReportUmaStep(SW_REPORTER_REGISTRY_EXIT_CODE);
-      ReportAndClearExitCode(exit_code);
+      ReportAndClearExitCode(exit_code, version.GetString());
     }
 
     // If we can't access local state, we can't see when we last ran, so
@@ -180,7 +216,12 @@ class SwReporterInstallerTraits : public ComponentInstallerTraits {
           prefs::kSwReporterLastTimeTriggered,
           base::Time::Now().ToInternalValue());
 
-      ExecuteReporter(install_dir);
+      base::WorkerPool::PostTask(
+          FROM_HERE,
+          base::Bind(&LaunchAndWaitForExit,
+                     install_dir.Append(kSwReporterExeName),
+                     version.GetString()),
+          true);
     }
   }
 
@@ -223,10 +264,6 @@ wchar_t SwReporterInstallerTraits::version_dir_[] = {};
 
 void RegisterSwReporterComponent(ComponentUpdateService* cus,
                                  PrefService* prefs) {
-  // The Sw reporter shouldn't run if the user isn't reporting metrics.
-  if (!ChromeMetricsServiceAccessor::IsMetricsReportingEnabled())
-    return;
-
   // Install the component.
   scoped_ptr<ComponentInstallerTraits> traits(
       new SwReporterInstallerTraits(prefs));
@@ -240,4 +277,12 @@ void RegisterPrefsForSwReporter(PrefRegistrySimple* registry) {
   registry->RegisterInt64Pref(prefs::kSwReporterLastTimeTriggered, 0);
 }
 
+void RegisterProfilePrefsForSwReporter(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterStringPref(
+      prefs::kSwReporterPromptState,
+      "",
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+}
+
 }  // namespace component_updater