Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831553002:
policy_templates.json: Remove duplicate captions (Closed)
|
Created:
3 years, 8 months ago by Thiemo Nagel Modified:
3 years, 8 months ago Reviewers:
emaxx CC:
chromium-reviews, tnagel+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionpolicy_templates.json: Remove duplicate captions
Remove instances in which the caption has been copied to the
description verbatim.
BUG=none
Review-Url: https://codereview.chromium.org/2831553002
Cr-Commit-Position: refs/heads/master@{#465993}
Committed: https://chromium.googlesource.com/chromium/src/+/95491b1213b02a0054ead98d4b434a6b4da5b55b
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments #Patch Set 3 : Rebase #
Total comments: 5
Patch Set 4 : Rebase #Messages
Total messages: 30 (19 generated)
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tnagel@chromium.org changed reviewers: + emaxx@chromium.org
Hi Maksim, could you please take a look at this piece of cleanup? -- Thanks, Thiemo
LGTM with nits https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... File components/policy/resources/policy_templates.json (left): https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:4065: 'desc': '''Allow <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph> to handle the listed content types. nit: Maybe better to keep this line for this policy, because otherwise there's no description left for the "policy is set" case. Maybe rephrase this line a little bit, so that it's not the same as title (but I'm also fine with keeping the original line, as this policy has already been deprecated). https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7716: 'desc': '''The default behavior for sites not in any content pack. nit: Maybe keep some descriptive line of text here too? https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:9517: 'desc': '''This setting controls the minimum PIN length. nit: Maybe keep some descriptive line of text here too? https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:9536: 'desc': '''This setting controls the maximum PIN length. nit: Maybe keep some descriptive line of text here too? https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:6027: 'desc': '''If enabled screenshots cannot be taken using keyboard shortcuts or extension APIs. nit: While you're here, maybe add a comma after "enabled".
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! Could you maybe take another look? https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... File components/policy/resources/policy_templates.json (left): https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:4065: 'desc': '''Allow <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph> to handle the listed content types. On 2017/04/19 13:56:27, emaxx wrote: > nit: Maybe better to keep this line for this policy, because otherwise there's > no description left for the "policy is set" case. Maybe rephrase this line a > little bit, so that it's not the same as title (but I'm also fine with keeping > the original line, as this policy has already been deprecated). Thanks, rephrased. https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:7716: 'desc': '''The default behavior for sites not in any content pack. On 2017/04/19 13:56:27, emaxx wrote: > nit: Maybe keep some descriptive line of text here too? Imho the duplicate line adds no value. As it's for internal use only, I'd prefer not to spend effort on it. https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:9517: 'desc': '''This setting controls the minimum PIN length. On 2017/04/19 13:56:27, emaxx wrote: > nit: Maybe keep some descriptive line of text here too? Done. https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:9536: 'desc': '''This setting controls the maximum PIN length. On 2017/04/19 13:56:27, emaxx wrote: > nit: Maybe keep some descriptive line of text here too? Done. https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2831553002/diff/1/components/policy/resources... components/policy/resources/policy_templates.json:6027: 'desc': '''If enabled screenshots cannot be taken using keyboard shortcuts or extension APIs. On 2017/04/19 13:56:27, emaxx wrote: > nit: While you're here, maybe add a comma after "enabled". Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with a nit https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9392: 'desc': '''If the policy is set, the configured minimal PIN length is nit: s/minimal/minimum/? (I think it's more correct here) https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9396: If the policy is not set, a minimal PIN length of 6 digits is nit: s/minimal/minimum/?
https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9392: 'desc': '''If the policy is set, the configured minimal PIN length is On 2017/04/19 15:53:26, emaxx wrote: > nit: s/minimal/minimum/? (I think it's more correct here) I think it's the opposite... ;) Minimum is a noun, whereas minimal is an adjective. An adjective-noun construction seems more valid to me than a noun-noun construction. I'd suggest looping in Bartosz in case you have doubts. https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9396: If the policy is not set, a minimal PIN length of 6 digits is On 2017/04/19 15:53:26, emaxx wrote: > nit: s/minimal/minimum/? Same as above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9392: 'desc': '''If the policy is set, the configured minimal PIN length is On 2017/04/19 16:01:08, Thiemo Nagel wrote: > On 2017/04/19 15:53:26, emaxx wrote: > > nit: s/minimal/minimum/? (I think it's more correct here) > > I think it's the opposite... ;) > > Minimum is a noun, whereas minimal is an adjective. An adjective-noun > construction seems more valid to me than a noun-noun construction. I'd suggest > looping in Bartosz in case you have doubts. Feel free to land it as is (it's just a small stupid nit), but I don't think that minimum can be only a noun, it can be an adjective too: https://www.merriam-webster.com/dictionary/minimum My understanding was that in Maths and in other "formal" contexts the difference between "minimum" and "minimal" is that the former speaks about the global minima, while the latter - about the local ones. But I may be wrong.
On 2017/04/20 10:55:28, emaxx wrote: > Still LGTM > > https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... > File components/policy/resources/policy_templates.json (right): > > https://codereview.chromium.org/2831553002/diff/40001/components/policy/resou... > components/policy/resources/policy_templates.json:9392: 'desc': '''If the policy > is set, the configured minimal PIN length is > On 2017/04/19 16:01:08, Thiemo Nagel wrote: > > On 2017/04/19 15:53:26, emaxx wrote: > > > nit: s/minimal/minimum/? (I think it's more correct here) > > > > I think it's the opposite... ;) > > > > Minimum is a noun, whereas minimal is an adjective. An adjective-noun > > construction seems more valid to me than a noun-noun construction. I'd > suggest > > looping in Bartosz in case you have doubts. > > Feel free to land it as is (it's just a small stupid nit), but I don't think > that minimum can be only a noun, it can be an adjective too: > https://www.merriam-webster.com/dictionary/minimum > My understanding was that in Maths and in other "formal" contexts the difference > between "minimum" and "minimal" is that the former speaks about the global > minima, while the latter - about the local ones. But I may be wrong. Thanks for the correction! I wasn't aware that "minimum" can be both a noun and an adjective. I still like my version better, though. :P
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by tnagel@chromium.org
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/2831553002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492692364229610,
"parent_rev": "588eca456e606a5697043317e5d777fad880c6f8", "commit_rev":
"95491b1213b02a0054ead98d4b434a6b4da5b55b"}
Message was sent while issue was closed.
Description was changed from ========== policy_templates.json: Remove duplicate captions Remove instances in which the caption has been copied to the description verbatim. BUG=none ========== to ========== policy_templates.json: Remove duplicate captions Remove instances in which the caption has been copied to the description verbatim. BUG=none Review-Url: https://codereview.chromium.org/2831553002 Cr-Commit-Position: refs/heads/master@{#465993} Committed: https://chromium.googlesource.com/chromium/src/+/95491b1213b02a0054ead98d4b43... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/95491b1213b02a0054ead98d4b43... |
