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

Implement Windows GPO support for "dlpref".

This is a feature of the update protocol, which allows the
server to prioritize returning urls that can be cached
by downstream proxies.

BUG=579762
Committed: https://crrev.com/590586cdcb76d893e591e26209a57edb8700568b
Cr-Commit-Position: refs/heads/master@{#371576}

[Sorin Jianu, 2016-01-21 01:36:24]

Description was changed from

==========
Implement Windows GPO support for "dlpref".

This is a feature of the update protocol, which allows the
server to prioritize returning urls that can be cached
by downstream proxies.

BUG=579762
==========

to

==========
Implement Windows GPO support for "dlpref".

This is a feature of the update protocol, which allows the
server to prioritize returning urls that can be cached
by downstream proxies.

BUG=579762
==========

[Sorin Jianu, 2016-01-21 01:36:25]

sorin@chromium.org changed reviewers:
+ asargent@chromium.org, blundell@chromium.org, gab@chromium.org,
waffles@chromium.org

[Sorin Jianu, 2016-01-21 01:37:38]

Please, I need owners review for ios, installer, and extensions changes.

Thank you!

[Sorin Jianu, 2016-01-21 01:38:14]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 01:40:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606943007/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606943007/1

[waffles, 2016-01-21 01:56:08]

lgtm

https://codereview.chromium.org/1606943007/diff/1/chrome/installer/util/googl...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/1/chrome/installer/util/googl...
chrome/installer/util/google_update_settings.cc:780: // implementation supports
only "cacheable" as a valid policy option.
Do we want to consider an alternate sanitization policy (e.g. matches
`[a-z]{0-32}`), so that future alternatives do not require a Chrome build (just
an Omaha, Template, and Server change)?

[Sorin Jianu, 2016-01-21 02:53:55]

PTAL, thank you!

https://codereview.chromium.org/1606943007/diff/1/chrome/installer/util/googl...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/1/chrome/installer/util/googl...
chrome/installer/util/google_update_settings.cc:780: // implementation supports
only "cacheable" as a valid policy option.
On 2016/01/21 01:56:08, waffles wrote:
> Do we want to consider an alternate sanitization policy (e.g. matches
> `[a-z]{0-32}`), so that future alternatives do not require a Chrome build
(just
> an Omaha, Template, and Server change)?

I've sanitized by hand since adding a dependency on re2 in the installer seemed
an unacceptable tradeoff.

[Sorin Jianu, 2016-01-21 03:02:42]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 03:05:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606943007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606943007/40001

[commit-bot: I haz the power, 2016-01-21 05:03:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-21 05:03:48]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-01-21 08:43:17]

I don't think you need my review for anything here. components_tests.gyp has *
as OWNERS.

[Sorin Jianu, 2016-01-21 16:44:15]

Colin, I am making a change under the ios/ path, please see:
ios/chrome/browser/component_updater/ios_component_updater_configurator.cc
Thank you!

[waffles, 2016-01-21 17:33:53]

(took another look, lgtm sustained)

[asargent_no_longer_on_chrome, 2016-01-21 18:36:39]

chrome/browser/extensions lgtm

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

CC grt FYI.

installer lgtm w/ comments below

PS: It's unfortunate that this code is even in the installer/ path as it has
nothing to do with the installer per se.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:801: ERROR_SUCCESS &&
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..

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.h:49: static const wchar_t
kDownloadPreference[];
kDownloadPreferencePolicyValue to match the pattern of kUpdatePolicyValue?

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.h:50: static const wchar_t
kDownloadPreferenceCacheable[];
This constant is only ever used in the test?

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:1029:
L"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"));
base::string16(33, L'a')

to make it clearer that the test is about the size? Probably also worth testing
that base::string16(32, L'a') works.

[blundell, 2016-01-22 08:33:13]

blundell@chromium.org changed reviewers:
+ droger@chromium.org
- blundell@chromium.org

[blundell, 2016-01-22 08:33:14]

blundell -> droger for //ios (I'm not an //ios OWNER)

[droger, 2016-01-22 10:32:15]

ios LGTM

https://codereview.chromium.org/1606943007/diff/40001/chrome/browser/componen...
File chrome/browser/component_updater/chrome_component_updater_configurator.cc
(right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:126:
return std::string("");
Why not
return configurator_impl_.GetDownloadPreference();

or at least
  return std::string();

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

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:803: Validator::Validate(value))
{
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:59]

Thank you Gab. Feedback addressed or answered.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:801: ERROR_SUCCESS &&
On 2016/01/21 21:08:49, gab wrote:
> 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..

Correct.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:803: Validator::Validate(value))
{
On 2016/01/22 16:06:50, gab wrote:
> 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?

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

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.h:49: static const wchar_t
kDownloadPreference[];
On 2016/01/21 21:08:49, gab wrote:
> kDownloadPreferencePolicyValue to match the pattern of kUpdatePolicyValue?

Done.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.h:50: static const wchar_t
kDownloadPreferenceCacheable[];
On 2016/01/21 21:08:49, gab wrote:
> This constant is only ever used in the test?

The constant is used in the unit test. It had been used in the production code
until we refactored the code to accept a regex. I think it is nice to have it
defined here for documentation purposes but I can move it to the test code as
well.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings_unittest.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings_unittest.cc:1029:
L"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"));
On 2016/01/21 21:08:49, gab wrote:
> base::string16(33, L'a')
> 
> to make it clearer that the test is about the size? Probably also worth
testing
> that base::string16(32, L'a') works.

Done.

[Sorin Jianu, 2016-01-25 20:05:22]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-25 20:11:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606943007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606943007/80001

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

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:803: Validator::Validate(value))
{
On 2016/01/25 20:04:58, Sorin Jianu wrote:
> On 2016/01/22 16:06:50, gab wrote:
> > 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?
> 
> I tried to follow the existing structure of the code and encapsulate the
> validation logic in a function.

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.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.h:50: static const wchar_t
kDownloadPreferenceCacheable[];
On 2016/01/25 20:04:58, Sorin Jianu wrote:
> On 2016/01/21 21:08:49, gab wrote:
> > This constant is only ever used in the test?
> 
> The constant is used in the unit test. It had been used in the production code
> until we refactored the code to accept a regex. I think it is nice to have it
> defined here for documentation purposes but I can move it to the test code as
> well.

I'm not convinced there is a point to document a string which nothing points to
(i.e. unless reading the code from top to bottom no one will ever stumble upon
this...). Plus, as-is there is no comment so it's not really documented either
:-).

I'd say it's best for the API to document how it handles its input (something
that encapsulates this + regex) and then move this to the test (+ comment on
definition in test that it should trigger the documented regex)

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

Thank you Gab! PTAL.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:803: Validator::Validate(value))
{
On 2016/01/25 20:33:50, gab wrote:
> On 2016/01/25 20:04:58, Sorin Jianu wrote:
> > On 2016/01/22 16:06:50, gab wrote:
> > > 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?
> > 
> > I tried to follow the existing structure of the code and encapsulate the
> > validation logic in a function.
> 
> 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.

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.

[Sorin Jianu, 2016-01-25 21:28:13]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-25 21:30:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606943007/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606943007/120001

[commit-bot: I haz the power, 2016-01-25 22:51:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-25 22:51:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sorin Jianu, 2016-01-26 02:37:40]

The CQ bit was checked by sorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 02:37:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606943007/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606943007/140001

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

lgtm with one more question about the policy's documentation

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:803: Validator::Validate(value))
{
On 2016/01/25 21:27:56, Sorin Jianu wrote:
> On 2016/01/25 20:33:50, gab wrote:
> > On 2016/01/25 20:04:58, Sorin Jianu wrote:
> > > On 2016/01/22 16:06:50, gab wrote:
> > > > 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?
> > > 
> > > I tried to follow the existing structure of the code and encapsulate the
> > > validation logic in a function.
> > 
> > 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.
> 
> 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. 
> 

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.

https://codereview.chromium.org/1606943007/diff/120001/chrome/installer/util/...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/1606943007/diff/120001/chrome/installer/util/...
chrome/installer/util/google_update_settings.h:275: // following regex: 
`[a-zA-z]{0-32}`.
So a policy of "abc12345" means something? Or are the meaningful values that
should be documented?

[commit-bot: I haz the power, 2016-01-26 04:14:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-26 04:14:49]

Dry run: This issue passed the CQ dry run.

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

thank you, feedback answered.

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
File chrome/installer/util/google_update_settings.cc (right):

https://codereview.chromium.org/1606943007/diff/40001/chrome/installer/util/g...
chrome/installer/util/google_update_settings.cc:803: Validator::Validate(value))
{
On 2016/01/26 03:44:55, gab wrote:
> On 2016/01/25 21:27:56, Sorin Jianu wrote:
> > On 2016/01/25 20:33:50, gab wrote:
> > > On 2016/01/25 20:04:58, Sorin Jianu wrote:
> > > > On 2016/01/22 16:06:50, gab wrote:
> > > > > 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?
> > > > 
> > > > I tried to follow the existing structure of the code and encapsulate the
> > > > validation logic in a function.
> > > 
> > > 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.
> > 
> > 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. 
> > 
> 
> 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.

Acknowledged.

https://codereview.chromium.org/1606943007/diff/120001/chrome/installer/util/...
File chrome/installer/util/google_update_settings.h (right):

https://codereview.chromium.org/1606943007/diff/120001/chrome/installer/util/...
chrome/installer/util/google_update_settings.h:275: // following regex: 
`[a-zA-z]{0-32}`.
On 2016/01/26 03:44:55, gab wrote:
> So a policy of "abc12345" means something? Or are the meaningful values that
> should be documented?

Done.

[Sorin Jianu, 2016-01-26 18:14:48]

The CQ bit was checked by sorin@chromium.org

[Sorin Jianu, 2016-01-26 18:14:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, droger@chromium.org, asargent@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1606943007/#ps160001
(title: "whitespace comments only")

[commit-bot: I haz the power, 2016-01-26 18:15:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1606943007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1606943007/160001

[commit-bot: I haz the power, 2016-01-26 20:09:49]

Description was changed from

==========
Implement Windows GPO support for "dlpref".

This is a feature of the update protocol, which allows the
server to prioritize returning urls that can be cached
by downstream proxies.

BUG=579762
==========

to

==========
Implement Windows GPO support for "dlpref".

This is a feature of the update protocol, which allows the
server to prioritize returning urls that can be cached
by downstream proxies.

BUG=579762
==========

[commit-bot: I haz the power, 2016-01-26 20:09:50]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-26 20:11:44]

Description was changed from

==========
Implement Windows GPO support for "dlpref".

This is a feature of the update protocol, which allows the
server to prioritize returning urls that can be cached
by downstream proxies.

BUG=579762
==========

to

==========
Implement Windows GPO support for "dlpref".

This is a feature of the update protocol, which allows the
server to prioritize returning urls that can be cached
by downstream proxies.

BUG=579762

Committed: https://crrev.com/590586cdcb76d893e591e26209a57edb8700568b
Cr-Commit-Position: refs/heads/master@{#371576}
==========

[commit-bot: I haz the power, 2016-01-26 20:11:45]

Patchset 9 (id:??) landed as
https://crrev.com/590586cdcb76d893e591e26209a57edb8700568b
Cr-Commit-Position: refs/heads/master@{#371576}