New issue
Advanced search Search tips

Issue 852366 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 3
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove repeated boilerplate in cloud_policy_generated.cc

Project Member Reported by olsen@chromium.org, Jun 13 2018

Issue description

4855 of the lines in cloud_policy_generated.cc are the following 19 lines:

    if (policy_proto.has_value()) {
      PolicyLevel level = POLICY_LEVEL_MANDATORY;
      bool do_set = true;
      if (policy_proto.has_policy_options()) {
        do_set = false;
        switch(policy_proto.policy_options().mode()) {
          case em::PolicyOptions::MANDATORY:
            do_set = true;
            level = POLICY_LEVEL_MANDATORY;
            break;
          case em::PolicyOptions::RECOMMENDED:
            do_set = true;
            level = POLICY_LEVEL_RECOMMENDED;
            break;
          case em::PolicyOptions::UNSET:
            break;
        }
      }
      if (do_set) {

repeated 255 times (once per policy).

A small refactoring to the generated code should be able to eliminate most of them. This would make the code a bit more readable. Making generated code more readable is advantageous, even if nobody needs to read the generated file, because it makes it easier to read and modify the generator.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e181733b85050ef2f4f70b130397a23c606d09ee

commit e181733b85050ef2f4f70b130397a23c606d09ee
Author: A Olsen <olsen@chromium.org>
Date: Wed Jun 13 16:43:00 2018

Refactor boilerplate in cloud_policy_generated.cc

cloud_policy_generated.cc has lots of boilerplate - the same code
repeated for each policy. This change moves most of the boilerplate
into a function, GetPolicyLevel. Because the file is generated, the
change must happen in the generator - generate_policy_source.py

The diff in the generated file is visible internally at
go/getpolicyleveldiff

Bug:  852366 
Change-Id: I9d6f4d3c211b79ad271504c9f32e7b11b7399a32
Reviewed-on: https://chromium-review.googlesource.com/1097318
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Roman Sorokin <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566863}
[modify] https://crrev.com/e181733b85050ef2f4f70b130397a23c606d09ee/components/policy/tools/generate_policy_source.py

Comment 2 by olsen@chromium.org, Jun 14 2018

Status: Verified (was: Assigned)
Verified - repeated boilerplate is removed from generated source.

Comment 3 by olsen@chromium.org, Jun 21 2018

Status: Started (was: Verified)
Reopening as started, will remove more repeated boilerplate.
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b

commit bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b
Author: A Olsen <olsen@chromium.org>
Date: Tue Jul 03 14:55:56 2018

Remove cloud_policy_generated.cc

Store enough metadata in policy_constants.cc that we can
use it to decode policies, and so don't need to generate any
more code in cloud_policy_generated.cc

The metadata required to decode a policy from a proto is
the key, the type (both the raw type ie integer or string,
and also whether it is JSON or external), and pointers to
both has_policyproto() and policyproto() functions, where
policyproto is the message field that holds this policy.

The metadata that is stored in policy_constants.cc looks like so -
the lines starting with &em are function pointers into the
CloudPolicySettings proto:

constexpr BooleanPolicyAccess kBooleanPolicyAccess[] = {
  {key::kAbusiveExperienceInterventionEnforce,
   &em::CloudPolicySettings::has_abusiveexperienceinterventionenforce,
   &em::CloudPolicySettings::abusiveexperienceinterventionenforce},
  {key::kAllowCrossOriginAuthPrompt,
   &em::CloudPolicySettings::has_allowcrossoriginauthprompt,
   &em::CloudPolicySettings::allowcrossoriginauthprompt},
  ...
  {nullptr, nullptr, nullptr},
};

constexpr IntegerPolicyAccess kIntegerPolicyAccess[] = {
  {key::kAdsSettingForIntrusiveAdsSites,
   &em::CloudPolicySettings::has_adssettingforintrusiveadssites,
   &em::CloudPolicySettings::adssettingforintrusiveadssites},
  {key::kArcBackupRestoreServiceEnabled,
   &em::CloudPolicySettings::has_arcbackuprestoreserviceenabled,
   &em::CloudPolicySettings::arcbackuprestoreserviceenabled},
  ...
  {nullptr, nullptr, nullptr},
};

constexpr StringPolicyAccess kStringPolicyAccess[] = {
  {key::kAllowedDomainsForApps,
   &em::CloudPolicySettings::has_alloweddomainsforapps,
   &em::CloudPolicySettings::alloweddomainsforapps,
   StringPolicyType::STRING},
  {key::kContentPackManualBehaviorHosts,
   &em::CloudPolicySettings::has_contentpackmanualbehaviorhosts,
   &em::CloudPolicySettings::contentpackmanualbehaviorhosts,
   StringPolicyType::JSON},
  {key::kNativePrintersBulkConfiguration,
   &em::CloudPolicySettings::has_nativeprintersbulkconfiguration,
   &em::CloudPolicySettings::nativeprintersbulkconfiguration,
   StringPolicyType::EXTERNAL},
  ...
  {nullptr, nullptr, nullptr},
};

constexpr StringListPolicyAccess kStringListPolicyAccess[] = {
  {key::kAllowedInputMethods,
   &em::CloudPolicySettings::has_allowedinputmethods,
   &em::CloudPolicySettings::allowedinputmethods},
  ...
  {nullptr, nullptr, nullptr},
};


Bug:  852366 
Change-Id: I1b8a0ebc0c6d5019e63d81a4cb7e1cd1cb461bb3
Reviewed-on: https://chromium-review.googlesource.com/1110219
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572212}
[modify] https://crrev.com/bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b/components/policy/BUILD.gn
[modify] https://crrev.com/bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b/components/policy/core/common/BUILD.gn
[modify] https://crrev.com/bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b/components/policy/core/common/cloud/user_cloud_policy_store_base.cc
[add] https://crrev.com/bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b/components/policy/core/common/policy_proto_decoders.cc
[add] https://crrev.com/bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b/components/policy/core/common/policy_proto_decoders.h
[modify] https://crrev.com/bc6e1f6cb751d9a841f38dd01e53f5d73ebf861b/components/policy/tools/generate_policy_source.py

Status: Verified (was: Started)
cloud_policy_generated.cc is now deleted so now it contains no repeated boilerplate.

The useful policy metadata that it stored is now in the other generated file, policy_constants.cc in a denser format (so, less repeated boilerplate).

Sign in to add a comment