Cleanup Tool WebUI: Add functionality to the Scan button

chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc

Index: chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc
diff --git a/chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc b/chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc
index 1a84b1aebcf12e1a89f46067f1b4f0e15f51f126..29847e7c90bad950129409048d5ed08b93ec8d2e 100644
--- a/chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc
+++ b/chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.cc
@@ -5,7 +5,31 @@
 #include "chrome/browser/ui/webui/cleanup_tool/cleanup_action_handler.h"
 
 #include "base/bind.h"
+#include "base/timer/timer.h"
 #include "base/values.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/component_updater/sw_reporter_installer_win.h"
+
+namespace {
+
+bool IsCleanupComponentRegistered() {
+  component_updater::ComponentUpdateService* cus =
+      g_browser_process->component_updater();
+  std::vector<std::string> component_ids = cus->GetComponentIDs();
+
+  return std::find(component_ids.begin(), component_ids.end(),
+                   component_updater::kSwReporterComponentId) !=
+         component_ids.end();
+}
+
+// TODO(proberge): Localize strings once they are finalized.
+constexpr char kDetectionOkText[] = "No problems detected";
+constexpr char kDetectionUwSText[] = "2 potentially harmful programs detected";
+constexpr char kDetectionTimeText[] = "Last scanned today";
+constexpr char kCleanupUnsupportedText[] = "Chrome Cleanup is not supported.";
+constexpr char kUnsupportedHelpText[] = "Please try reinstalling Chrome";

[tommycli, 2017/04/21 18:24:21]

Passing localized text back via the C++ handler seems... odd.

The usual pattern we use is to pass back an integer code (an enum), and then
generate the appropriate localized string on the JavaScript side.

Are the strings really not "finalized"? If they appear in any mocks, I would
just add the strings to the grd files and update later if the mocks change...

[tommycli, 2017/04/21 18:28:21]

To elaborate... we usually like to design our internal APIs with similar
quality standards as external APIs. If we pass status as localized text instead
of fixed codes (in an enum), it becomes impossible to programatically
query what happened -- all we can do is display the text.

[proberge, 2017/04/21 21:04:32]

From the mocks I was implementing, two of the strings (kDetectionUwSText and
kDetectionTimeText) required plurality support that did not seem available from
the WebUI's JS. It seems like I have to localize them in C++.

For example:
<message name="IDS_CLEANUP_TOOL_DAYS_LAST_RUN" desc="A message on the Chrome
Cleanup Tool page, shown to inform when the most recent scan for Unwanted
Software was done. [ICU Syntax]">
  {DAYS_COUNT, plural,
   =0 {Last scanned today}
   =1 {Last scanned yesterday}
   other {Last scanned # days ago}}
</message>

Is there an elegant way to resolve this? I might be able to push back on the
string to get some that are more easily localizable.
just add the strings to the grd files and update later if the mocks change...

Is there a way to add strings to the grd files without staging them for
translation? I'd like to avoid wasting translation resources.

[tommycli, 2017/04/21 21:57:02]

It appears there's translation_expectations.pyl as documented here:
https://www.chromium.org/developers/design-documents/ui-localization

However, I would probably just strive to get these strings finalized in time for
branch cut. I don't believe they are translated before that time anyways.

As for plurality, you might be stuck passing the strings from C++ in that case.

I see the clear browsing data dialog also passes the strings from C++ in that
case:
https://cs.chromium.org/chromium/src/chrome/browser/browsing_data/browsing_da...

Hope that helps... I think our general pattern is -- avoid sending the localized
strings from C++ if possible, but in this case maybe it's unavoidable.

[alito, 2017/04/21 22:12:37]

Re not translating strings: I've noticed that some messages in grd files have
the attribute "translateable" set to false. I can't find any official
documentation about it though...

[proberge, 2017/04/25 17:39:32]

Looks like we only have until May 11th to get strings in for M60. I'll start a
fire under our UX person to provide better strings before then.
It's unlikely any of the current ones will be used, so I'll skip on translating
them.

+
+}  // namespace
 
 CleanupActionHandler::CleanupActionHandler() {}
 
@@ -16,6 +40,9 @@ void CleanupActionHandler::RegisterMessages() {
       "requestLastScanResult",
       base::Bind(&CleanupActionHandler::HandleRequestLastScanResult,
                  base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "startScan", base::Bind(&CleanupActionHandler::HandleStartScan,
+                              base::Unretained(this)));
 }
 
 void CleanupActionHandler::HandleRequestLastScanResult(
@@ -27,12 +54,46 @@ void CleanupActionHandler::HandleRequestLastScanResult(
 
   base::DictionaryValue last_scan_results;
   // TODO(proberge): Return real information about the last run.
-  // TODO(proberge): Localize strings once they are finalized.
   last_scan_results.SetBoolean("hasScanResults", false);
   last_scan_results.SetBoolean("isInfected", false);
-  last_scan_results.SetString("detectionStatusText", "No problems detected");
-  last_scan_results.SetString("detectionTimeText", "Last scanned today");
+  last_scan_results.SetString("detectionStatusText", kDetectionOkText);
+  last_scan_results.SetString("detectionTimeText", kDetectionTimeText);
 
   AllowJavascript();
   ResolveJavascriptCallback(base::Value(webui_callback_id), last_scan_results);
 }
+
+void CleanupActionHandler::HandleStartScan(const base::ListValue* args) {
+  std::string webui_callback_id;
+  CHECK_EQ(1U, args->GetSize());
+  bool success = args->GetString(0, &webui_callback_id);
+  DCHECK(success);
+
+  if (!IsCleanupComponentRegistered()) {
+    base::DictionaryValue scan_results;
+    scan_results.SetBoolean("hasScanResults", false);
+    scan_results.SetBoolean("isInfected", false);
+    scan_results.SetString("detectionStatusText", kCleanupUnsupportedText);
+    scan_results.SetString("detectionTimeText", kUnsupportedHelpText);
+
+    AllowJavascript();
+    ResolveJavascriptCallback(base::Value(webui_callback_id), scan_results);
+    return;
+  }
+
+  component_updater::RegisterUserInitiatedSwReporterScan(
+      base::Bind(&CleanupActionHandler::ReportScanResults,
+                 base::Unretained(this), webui_callback_id));
+}
+
+void CleanupActionHandler::ReportScanResults(const std::string& callback_id) {
+  base::DictionaryValue scan_results;
+  // TODO(proberge): Return real information about the scan.
+  scan_results.SetBoolean("hasScanResults", true);
+  scan_results.SetBoolean("isInfected", true);
+  scan_results.SetString("detectionStatusText", kDetectionUwSText);
+  scan_results.SetString("detectionTimeText", kDetectionTimeText);
+
+  AllowJavascript();

[tommycli, 2017/04/21 18:24:21]

I assume this scan can take a long time, where JavaScript may become disallowed.

In that case, our suggested pattern is to use a weak pointer factory and cancel
the callback when javascript is disallowed. See example usage of
callback_weak_ptr_factory_.

https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/profile...

[proberge, 2017/04/21 21:04:32]

Makes a lot of sense. Thanks for the example usage!

+  ResolveJavascriptCallback(base::Value(callback_id), scan_results);
+}