Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1646893002:
Whitelist MR extensions for settingsPrivate API. (Closed)
|
Created:
4 years, 10 months ago by amp Modified:
4 years, 10 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, imcheng, apacible Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhitelist MR extensions for settingsPrivate API.
BUG=582130
Committed: https://crrev.com/ebf6571a42e69d9bf09bccbbfd75c5dd651de9bb
Cr-Commit-Position: refs/heads/master@{#372215}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Updated allowed channel and extension types. #
Total comments: 2
Patch Set 3 : Formatting #Messages
Total messages: 12 (4 generated)
amp@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:935: "extension_types": ["extension", "platform_app"], I tried adding a "location": "external_component", line to the second entry here, but it gave me an error when I tried it to use the API so I have left it with just the whitelist for now.
https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:934: "channel": "trunk", Assuming you want to use this anywhere other than trunk, this will need to be updated (if it's your intent to only use it on trunk for now, then this is fine). https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:935: "extension_types": ["extension", "platform_app"], On 2016/01/28 17:59:11, amp wrote: > I tried adding a > "location": "external_component", > line to the second entry here, but it gave me an error when I tried it to use > the API so I have left it with just the whitelist for now. Huh. Interesting. With a whitelist, it doesn't really matter, but I still would have expected it to work. Are you sure that on your dev build MR is added as an external component (don't sweat it if it's a pain to debug). All that said, please do remove "platform_app" here.
https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:934: "channel": "trunk", On 2016/01/28 19:04:11, Devlin wrote: > Assuming you want to use this anywhere other than trunk, this will need to be > updated (if it's your intent to only use it on trunk for now, then this is > fine). Yes, we will want to have this released to stable for M50. Is it ok to set this to stable here since it's now limited by a whitelist, or do we need further approval to do that? I've set it to stable for now. https://codereview.chromium.org/1646893002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:935: "extension_types": ["extension", "platform_app"], On 2016/01/28 19:04:11, Devlin wrote: > On 2016/01/28 17:59:11, amp wrote: > > I tried adding a > > "location": "external_component", > > line to the second entry here, but it gave me an error when I tried it to use > > the API so I have left it with just the whitelist for now. > > Huh. Interesting. With a whitelist, it doesn't really matter, but I still > would have expected it to work. Are you sure that on your dev build MR is added > as an external component (don't sweat it if it's a pain to debug). > > All that said, please do remove "platform_app" here. Right, thanks for the clarification. My dev build is just a regular extension so it would fail. The release extension is an external_component but that would be harder to test with this limitation. And platform_app should be removed in latest patch.
lgtm https://codereview.chromium.org/1646893002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1646893002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:927: "settingsPrivate": [ We're quite inconsistent in style, but I'd prefer to see us using the { on the previous line, a la line 874. It feels more concise.
https://codereview.chromium.org/1646893002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1646893002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:927: "settingsPrivate": [ On 2016/01/28 22:55:59, Devlin wrote: > We're quite inconsistent in style, but I'd prefer to see us using the { on the > previous line, a la line 874. It feels more concise. Done.
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1646893002/#ps40001 (title: "Formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1646893002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1646893002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Whitelist MR extensions for settingsPrivate API. BUG=582130 ========== to ========== Whitelist MR extensions for settingsPrivate API. BUG=582130 Committed: https://crrev.com/ebf6571a42e69d9bf09bccbbfd75c5dd651de9bb Cr-Commit-Position: refs/heads/master@{#372215} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ebf6571a42e69d9bf09bccbbfd75c5dd651de9bb Cr-Commit-Position: refs/heads/master@{#372215} |
