Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1139723002:
Make launcherSearchProvider API white-listed on stable and public on dev. (Closed)
|
Created:
5 years, 7 months ago by yawano Modified:
5 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, satorux1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake launcherSearchProvider API white-listed on stable and public on dev.
BUG=440649
Committed: https://crrev.com/e2a6d5724bd1974f7c9079cf1ce9eda47bfc46d0
Cr-Commit-Position: refs/heads/master@{#329381}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove dev channel. #Messages
Total messages: 17 (3 generated)
yawano@chromium.org changed reviewers: + benwells@chromium.org
@benwells: PTAL. Thank you! After the CL (http://crrev.com/1060733003) got lgtm and has landed, implementation of launcherSearchProvider API is completed. We want to make this API white-listed on stable and public on dev. Please note that even after this permission change, this API is still behind flag.
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:542: "channel": "dev", I don't think we should make it public on dev just yet. We still don't have a clear plan on how we're going to deal with arbitrary apps providing search results and listening to queries (security/privacy implications as well as search quality implications). Happy to enable this on stable/beta channel for File Manager, but we are in no way ready to open this up yet, even on dev. https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:547: "channel": "stable", Do you also need "channel": "beta"? Or does stable automatically imply beta and dev? (In which case, you should remove "channel": "dev" entirely.)
https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:547: "channel": "stable", On 2015/05/12 01:17:54, Matt Giuca wrote: > Do you also need "channel": "beta"? Or does stable automatically imply beta and > dev? (In which case, you should remove "channel": "dev" entirely.) Stable implies beta. This is correct, assuming public on dev and whitelisted on stable is what's desired. But I agree with Matt, this should stay whitelisted everywhere until we have ideas on how we'd make it public.
Thank you for the review! PTAL. https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:542: "channel": "dev", Okay, we will go with whilte-listed for now. https://codereview.chromium.org/1139723002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_permission_features.json:547: "channel": "stable", I simply removed dev.
lgtm
On 2015/05/12 03:28:53, benwells wrote: > lgtm Thank you!
lgtm
The CQ bit was checked by yawano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139723002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e2a6d5724bd1974f7c9079cf1ce9eda47bfc46d0 Cr-Commit-Position: refs/heads/master@{#329381}
Message was sent while issue was closed.
Er, should have changed the CL description. Oh well. If anyone comes here, the CL description should read "Make launcherSearchProvider API available on stable (but whitelisted to Files app)."
Message was sent while issue was closed.
On 2015/05/13 00:11:14, Matt Giuca wrote: > Er, should have changed the CL description. Oh well. > > If anyone comes here, the CL description should read "Make > launcherSearchProvider API available on stable (but whitelisted to Files app)." I forgot to change it, sorry. While we cannot change the description in git log, we can change description in this code review site. Should we change it? Or should we leave it for the consistency with the git log. We may be able to add it as a note in the description to this page.
Message was sent while issue was closed.
Leave it for consistency (that's why I added a note to the bottom).
Message was sent while issue was closed.
On 2015/05/13 03:28:01, Matt Giuca wrote: > Leave it for consistency (that's why I added a note to the bottom). Okay. Thank you. |
