Remove validation retry logic from DeviceSettingsService

Remove validation retry logic from DeviceSettingsService

A benefit of removing policy validation against system time is that
there are no transient policy validation errors anymore and thus the
retry logic in DeviceSettingsService can be scrapped.

Also removing the unused STORE_POLICY_ERROR status code.

BUG=701045
Review-Url: https://codereview.chromium.org/2922453002
Cr-Commit-Position: refs/heads/master@{#476611}
Committed: https://chromium.googlesource.com/chromium/src/+/efffaf05f0a47a357ab6955c0a3caf7a4a06ef65

[Thiemo Nagel, 2017-06-01 11:48:02]

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

[commit-bot: I haz the power, 2017-06-01 11:48:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-06-01 11:55:26]

tnagel@chromium.org changed reviewers:
+ emaxx@chromium.org

[Thiemo Nagel, 2017-06-01 11:55:27]

Hey Maksim,

could you please take a look?

Thank you!
Thiemo

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/session_manager_operation.cc (left):

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/session_manager_operation.cc:161: if
(!policy->ParseFromString(policy_blob) || !policy->IsInitialized()) {
Funny enough, IsInitialized() maps to true [1].  Thus dropping it for clearer
code.

[1]
https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/de...

[commit-bot: I haz the power, 2017-06-01 12:18:34]

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

[commit-bot: I haz the power, 2017-06-01 12:18:36]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-06-01 18:51:17]

LGTM with notes.

And thanks for doing this cleanup!

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/device_settings_service.h:62: // Status codes
for Load() and Store().
Should we then rename all enum items to change the "STORE_" prefix?
(Not suggesting to do this in the same CL.)

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/session_manager_operation.cc (left):

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/session_manager_operation.cc:161: if
(!policy->ParseFromString(policy_blob) || !policy->IsInitialized()) {
On 2017/06/01 11:55:26, Thiemo Nagel wrote:
> Funny enough, IsInitialized() maps to true [1].  Thus dropping it for clearer
> code.
> 
> [1]
>
https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/de...

Not sure about this change. It's identical to "true" only whenever there are no
required fields in the protobuf message. Even though it's very unlikely that a
required field will be introduced into PolicyFetchResponse now, it's still a bit
cleaner to still catch this in the code.

[Thiemo Nagel, 2017-06-02 09:30:23]

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

[commit-bot: I haz the power, 2017-06-02 09:30:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2017-06-02 09:30:53]

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/device_settings_service.h:62: // Status codes
for Load() and Store().
On 2017/06/01 18:51:17, emaxx wrote:
> Should we then rename all enum items to change the "STORE_" prefix?
> (Not suggesting to do this in the same CL.)

I agree that it's a bit confusing.  Maybe STORE_ is intended to refer to the
underlying DeviceCloudPolicyStore?  -- Anyways, I'll do a follow-up CL.

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
File chrome/browser/chromeos/settings/session_manager_operation.cc (left):

https://codereview.chromium.org/2922453002/diff/1/chrome/browser/chromeos/set...
chrome/browser/chromeos/settings/session_manager_operation.cc:161: if
(!policy->ParseFromString(policy_blob) || !policy->IsInitialized()) {
> Not sure about this change. It's identical to "true" only whenever there are
no
> required fields in the protobuf message. Even though it's very unlikely that a
> required field will be introduced into PolicyFetchResponse now, it's still a
bit
> cleaner to still catch this in the code.

Thanks for explaining the intention of IsInitialized()!  I don't feel very
strongly about this, but given that the proto docs recommend against required
fields, I don't think we'll ever add any and thus the code seems to be clearer
without.

Anyways, I'll drop it, probably not worth to discuss further.

[Thiemo Nagel, 2017-06-02 09:31:00]

The CQ bit was unchecked by tnagel@chromium.org

[Thiemo Nagel, 2017-06-02 09:31:06]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2017-06-02 09:31:06]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2922453002/#ps20001
(title: "Adding back IsInitialized()")

[commit-bot: I haz the power, 2017-06-02 09:31:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 10:00:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496395866125600,
"parent_rev": "4eeb1274a17cd247778ee11666e0b18d9114e0ea", "commit_rev":
"efffaf05f0a47a357ab6955c0a3caf7a4a06ef65"}

[commit-bot: I haz the power, 2017-06-02 10:00:55]

Description was changed from

==========
Remove validation retry logic from DeviceSettingsService

A benefit of removing policy validation against system time is that
there are no transient policy validation errors anymore and thus the
retry logic in DeviceSettingsService can be scrapped.

Also removing the unused STORE_POLICY_ERROR status code.

BUG=701045
==========

to

==========
Remove validation retry logic from DeviceSettingsService

A benefit of removing policy validation against system time is that
there are no transient policy validation errors anymore and thus the
retry logic in DeviceSettingsService can be scrapped.

Also removing the unused STORE_POLICY_ERROR status code.

BUG=701045

Review-Url: https://codereview.chromium.org/2922453002
Cr-Commit-Position: refs/heads/master@{#476611}
Committed:
https://chromium.googlesource.com/chromium/src/+/efffaf05f0a47a357ab6955c0a3c...
==========

[commit-bot: I haz the power, 2017-06-02 10:00:56]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/efffaf05f0a47a357ab6955c0a3c...