Implement Windows GPO support for "dlpref" in component updater.

chrome/installer/util/google_update_settings.cc

Index: chrome/installer/util/google_update_settings.cc
diff --git a/chrome/installer/util/google_update_settings.cc b/chrome/installer/util/google_update_settings.cc
index e32e185d354c26607c7908f55f5c52ebb02a78c3..7451b8e9b538d593d288f872edaea9fa9a4d028b 100644
--- a/chrome/installer/util/google_update_settings.cc
+++ b/chrome/installer/util/google_update_settings.cc
@@ -8,6 +8,7 @@
 
 #include <algorithm>
 #include <limits>
+#include <vector>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
@@ -36,6 +37,10 @@ using installer::InstallationState;
 const wchar_t GoogleUpdateSettings::kPoliciesKey[] =
     L"SOFTWARE\\Policies\\Google\\Update";
 const wchar_t GoogleUpdateSettings::kUpdatePolicyValue[] = L"UpdateDefault";
+const wchar_t GoogleUpdateSettings::kDownloadPreference[] =
+    L"DownloadPreference";
+const wchar_t GoogleUpdateSettings::kDownloadPreferenceCacheable[] =
+    L"cacheable";
 const wchar_t GoogleUpdateSettings::kUpdateOverrideValuePrefix[] = L"Update";
 const wchar_t GoogleUpdateSettings::kCheckPeriodOverrideMinutes[] =
     L"AutoUpdateCheckPeriodMinutes";
@@ -770,6 +775,38 @@ bool GoogleUpdateSettings::ReenableAutoupdates() {
   return true;
 }
 
+// Reads and sanitizes the value of
+// "HKLM\SOFTWARE\Policies\Google\Update\DownloadPreference". A valid
+// group policy option must be a single alpha numeric word of up to 32
+// characters.
+base::string16 GoogleUpdateSettings::GetDownloadPreference() {
+  // Validates that the argument value matches `[a-zA-z]{0-32}`.
+  struct Validator {
+   public:
+    static bool Validate(const base::string16& value) {
+      const size_t kMaxValueLength = 32;
+      if (value.size() > kMaxValueLength)
+        return false;
+      for (auto ch : value) {
+        if (!base::IsAsciiAlpha(ch))
+          return false;
+      }
+      return true;
+    }
+  };
+
+  RegKey policy_key;
+  base::string16 value;
+  if (policy_key.Open(HKEY_LOCAL_MACHINE, kPoliciesKey, KEY_QUERY_VALUE) ==
+          ERROR_SUCCESS &&

[gab, 2016/01/21 21:08:49]

Is this git cl format's output? Would feel more natural to me to wrap the last
parameter of the Open() call and have this line be:

                 , KEY_QUERY_VALUE) == ERROR_SUCESS &&

If it's git cl format though then I guess that's okay..

[Sorin Jianu, 2016/01/25 20:04:58]

Correct.

+      policy_key.ReadValue(kDownloadPreference, &value) == ERROR_SUCCESS &&
+      Validator::Validate(value)) {

[gab, 2016/01/22 16:06:50]

Actually, why use a static method inside a local class? Can't the body of that
method just be in the if's body below?

[Sorin Jianu, 2016/01/25 20:04:58]

I tried to follow the existing structure of the code and encapsulate the
validation logic in a function.

[gab, 2016/01/25 20:33:50]

Hmmm I don't see code defining a local static method inside a struct in this
file?

Regardless though, you can move lines 787 to 794 in the body of this if and
forgo the helper method altogether.

[Sorin Jianu, 2016/01/25 21:27:56]

I meant I followed the structure of:
if (open(key) &&
    read(value) &&
    validate(value))....

What we are discussing here is whether to abstract out the validation step, as
above, or code up the validation inside the function itself. I prefer writing
functions.

To me this is a minor point, I think the code looks wors, and it is more
error-prone due to the multiple returns. I inlined the code anyway. 

[gab, 2016/01/26 03:44:55]

Thanks, I prefer the latest version inline. To me the advantage of having a
separate function is either code re-use or testability, never of which was being
used here; hence my preference.

[Sorin Jianu, 2016/01/26 18:09:48]

Acknowledged.

+    return value;
+  }
+
+  return base::string16();
+}
+
 void GoogleUpdateSettings::RecordChromeUpdatePolicyHistograms() {
   const bool is_multi_install = InstallUtil::IsMultiInstall(
       BrowserDistribution::GetDistribution(), IsSystemInstall());