Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2387793002:
Whitelist for metricsPrivate.getIsCrashReportingEnabled (Closed)
DescriptionWhitelist for metricsPrivate.getIsCrashReportingEnabled
Whitelists the metricsPrivate.getIsCrashReportingEnabled() extension API
function separately from the rest of the metricsPrivate API.
This should prompt developers to get an OWNERS review for new usages of
getIsCrashReportingEnabled(). It's implemented as a feature whitelist instead of
a permission whitelist so that existing uses will continue to work. To that end,
also whitelists all extensions that are already whitelisted for the
metricsPrivate permission as a starting point.
BUG=374199, 647397
Committed: https://crrev.com/3d1a7fba3851df28c1dfd7c786233131ce17cf93
Cr-Commit-Position: refs/heads/master@{#422911}
Patch Set 1 #Patch Set 2 : . #
Total comments: 10
Patch Set 3 : Reduce whitelist length #Patch Set 4 : update comment #Patch Set 5 : rebase #Messages
Total messages: 20 (6 generated)
Description was changed from ========== Whitelist for metricsPrivate.getIsCrashReportingEnabled Whitelists the metricsPrivate.getIsCrashReportingEnabled() extension API function separately from the rest of the metricsPrivate API. This should prompt developers to get an OWNERS review for new usages of getIsCrashReportingEnabled(). It's implemented as a feature whitelist instead of a permission whitelist so that existing uses will continue to work. To that end, also whitelists all extensions that are already whitelisted for the metricsPrivate permission as a starting point. BUG= ========== to ========== Whitelist for metricsPrivate.getIsCrashReportingEnabled Whitelists the metricsPrivate.getIsCrashReportingEnabled() extension API function separately from the rest of the metricsPrivate API. This should prompt developers to get an OWNERS review for new usages of getIsCrashReportingEnabled(). It's implemented as a feature whitelist instead of a permission whitelist so that existing uses will continue to work. To that end, also whitelists all extensions that are already whitelisted for the metricsPrivate permission as a starting point. BUG=374199,647397 ==========
michaelpg@chromium.org changed reviewers: + isherman@chromium.org, rdevlin.cronin@chromium.org
As decided earlier this week, we whitelist this function separately and document metrics ownership. LMK how the implementation looks. Context: https://codereview.chromium.org/2331343012/#msg31 https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:602: // metrics_private OWNERS approval of the usage before adding entries or //tools/metrics/OWNERS?
https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:599: "whitelist": [ Can you clarify what the intent here is? Right now, this basically just prevents webui from using this function, while allowing everything else. Additionally, any new extensions will need to be whitelisted both here and in _permissions_features.json in order to use it. Is that the desired behavior?
https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:599: "whitelist": [ On 2016/10/01 01:01:58, Devlin (catching up) wrote: > Can you clarify what the intent here is? Right now, this basically just > prevents webui from using this function, while allowing everything else. webui doesn't use it; if it does we'd do the same thing for chrome URLs > Additionally, any new extensions will need to be whitelisted both here and in > _permissions_features.json in order to use it. Is that the desired behavior? The desired behavior is: 1. New extensions which need to use chrome.metricsPrivate just whitelist themselves in _permissions_features.json, same as today 2. New extensions which specifically want to use chrome.metricsPrivate.getIsCrashReportingEnabled() wonder why the function doesn't exist, find this _api_features.json entry, read the comment, and either: a) Follow instructions, asking a metrics owner for a review b) Ignore the comment and whitelist themselves (hopefully a reviewer would notice; to that end, maybe better to put a comment at the end of the list?) Basically the metrics ppl are concerned about misuse of this function and want people to include them in reviews before using the function in new places.
https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:599: "whitelist": [ On 2016/10/01 01:47:09, michaelpg wrote: > On 2016/10/01 01:01:58, Devlin (catching up) wrote: > > Can you clarify what the intent here is? Right now, this basically just > > prevents webui from using this function, while allowing everything else. > > webui doesn't use it; if it does we'd do the same thing for chrome URLs > > > Additionally, any new extensions will need to be whitelisted both here and in > > _permissions_features.json in order to use it. Is that the desired behavior? > > The desired behavior is: > > 1. New extensions which need to use chrome.metricsPrivate just whitelist > themselves in _permissions_features.json, same as today > 2. New extensions which specifically want to use > chrome.metricsPrivate.getIsCrashReportingEnabled() wonder why the function > doesn't exist, find this _api_features.json entry, read the comment, and either: > a) Follow instructions, asking a metrics owner for a review > b) Ignore the comment and whitelist themselves (hopefully a reviewer would > notice; to that end, maybe better to put a comment at the end of the list?) > > Basically the metrics ppl are concerned about misuse of this function and want > people to include them in reviews before using the function in new places. This is a pretty monstrous list. Let's see if we can't narrow it down a bit. I'll follow up with you offline.
Thanks for following up on this! https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:588: "default_parent": true What does default_parent mean? https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:599: "whitelist": [ On 2016/10/01 02:03:44, Devlin (catching up) wrote: > On 2016/10/01 01:47:09, michaelpg wrote: > > On 2016/10/01 01:01:58, Devlin (catching up) wrote: > > > Can you clarify what the intent here is? Right now, this basically just > > > prevents webui from using this function, while allowing everything else. > > > > webui doesn't use it; if it does we'd do the same thing for chrome URLs > > > > > Additionally, any new extensions will need to be whitelisted both here and > in > > > _permissions_features.json in order to use it. Is that the desired > behavior? > > > > The desired behavior is: > > > > 1. New extensions which need to use chrome.metricsPrivate just whitelist > > themselves in _permissions_features.json, same as today > > 2. New extensions which specifically want to use > > chrome.metricsPrivate.getIsCrashReportingEnabled() wonder why the function > > doesn't exist, find this _api_features.json entry, read the comment, and > either: > > a) Follow instructions, asking a metrics owner for a review > > b) Ignore the comment and whitelist themselves (hopefully a reviewer would > > notice; to that end, maybe better to put a comment at the end of the list?) > > > > Basically the metrics ppl are concerned about misuse of this function and want > > people to include them in reviews before using the function in new places. > > This is a pretty monstrous list. Let's see if we can't narrow it down a bit. > I'll follow up with you offline. +1 to narrowing if possible. I bet most of these extensions don't rely on this API currently -- IIRC, only ~5 extensions actually use it. https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:602: // metrics_private OWNERS approval of the usage before adding entries On 2016/09/30 21:10:34, michaelpg wrote: > or //tools/metrics/OWNERS? Yes, the intent is for //tools/metrics/OWNERS to take a look at any new uses of this API =)
On 2016/10/03 21:11:10, Ilya Sherman wrote: > Thanks for following up on this! > > https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... > File chrome/common/extensions/api/_api_features.json (right): > > https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... > chrome/common/extensions/api/_api_features.json:588: "default_parent": true > What does default_parent mean? > > https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... > chrome/common/extensions/api/_api_features.json:599: "whitelist": [ > On 2016/10/01 02:03:44, Devlin (catching up) wrote: > > On 2016/10/01 01:47:09, michaelpg wrote: > > > On 2016/10/01 01:01:58, Devlin (catching up) wrote: > > > > Can you clarify what the intent here is? Right now, this basically just > > > > prevents webui from using this function, while allowing everything else. > > > > > > webui doesn't use it; if it does we'd do the same thing for chrome URLs > > > > > > > Additionally, any new extensions will need to be whitelisted both here and > > in > > > > _permissions_features.json in order to use it. Is that the desired > > behavior? > > > > > > The desired behavior is: > > > > > > 1. New extensions which need to use chrome.metricsPrivate just whitelist > > > themselves in _permissions_features.json, same as today > > > 2. New extensions which specifically want to use > > > chrome.metricsPrivate.getIsCrashReportingEnabled() wonder why the function > > > doesn't exist, find this _api_features.json entry, read the comment, and > > either: > > > a) Follow instructions, asking a metrics owner for a review > > > b) Ignore the comment and whitelist themselves (hopefully a reviewer > would > > > notice; to that end, maybe better to put a comment at the end of the list?) > > > > > > Basically the metrics ppl are concerned about misuse of this function and > want > > > people to include them in reviews before using the function in new places. > > > > This is a pretty monstrous list. Let's see if we can't narrow it down a bit. > > I'll follow up with you offline. > > +1 to narrowing if possible. I bet most of these extensions don't rely on this > API currently -- IIRC, only ~5 extensions actually use it. it's more than 5: https://crbug.com/647397#c3 > > https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... > chrome/common/extensions/api/_api_features.json:602: // metrics_private OWNERS > approval of the usage before adding entries > On 2016/09/30 21:10:34, michaelpg wrote: > > or //tools/metrics/OWNERS? > > Yes, the intent is for //tools/metrics/OWNERS to take a look at any new uses of > this API =)
On 2016/10/03 21:43:21, michaelpg wrote: > On 2016/10/03 21:11:10, Ilya Sherman wrote: > > Thanks for following up on this! > > > > > https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... > > File chrome/common/extensions/api/_api_features.json (right): > > > > > https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... > > chrome/common/extensions/api/_api_features.json:588: "default_parent": true > > What does default_parent mean? > > > > > https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... > > chrome/common/extensions/api/_api_features.json:599: "whitelist": [ > > On 2016/10/01 02:03:44, Devlin (catching up) wrote: > > > On 2016/10/01 01:47:09, michaelpg wrote: > > > > On 2016/10/01 01:01:58, Devlin (catching up) wrote: > > > > > Can you clarify what the intent here is? Right now, this basically just > > > > > prevents webui from using this function, while allowing everything else. > > > > > > > > > webui doesn't use it; if it does we'd do the same thing for chrome URLs > > > > > > > > > Additionally, any new extensions will need to be whitelisted both here > and > > > in > > > > > _permissions_features.json in order to use it. Is that the desired > > > behavior? > > > > > > > > The desired behavior is: > > > > > > > > 1. New extensions which need to use chrome.metricsPrivate just whitelist > > > > themselves in _permissions_features.json, same as today > > > > 2. New extensions which specifically want to use > > > > chrome.metricsPrivate.getIsCrashReportingEnabled() wonder why the function > > > > doesn't exist, find this _api_features.json entry, read the comment, and > > > either: > > > > a) Follow instructions, asking a metrics owner for a review > > > > b) Ignore the comment and whitelist themselves (hopefully a reviewer > > would > > > > notice; to that end, maybe better to put a comment at the end of the > list?) > > > > > > > > Basically the metrics ppl are concerned about misuse of this function and > > want > > > > people to include them in reviews before using the function in new places. > > > > > > This is a pretty monstrous list. Let's see if we can't narrow it down a > bit. > > > I'll follow up with you offline. > > > > +1 to narrowing if possible. I bet most of these extensions don't rely on > this > > API currently -- IIRC, only ~5 extensions actually use it. > > it's more than 5: https://crbug.com/647397#c3 Ah, I was thinking of extension names, but not taking into account that there are a few versions of each extension. Thanks for doing the research!
Narrowed the list down best I could. PTAL. https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:599: "whitelist": [ On 2016/10/01 02:03:44, Devlin wrote: > On 2016/10/01 01:47:09, michaelpg wrote: > > On 2016/10/01 01:01:58, Devlin (catching up) wrote: > > > Can you clarify what the intent here is? Right now, this basically just > > > prevents webui from using this function, while allowing everything else. > > > > webui doesn't use it; if it does we'd do the same thing for chrome URLs > > > > > Additionally, any new extensions will need to be whitelisted both here and > in > > > _permissions_features.json in order to use it. Is that the desired > behavior? > > > > The desired behavior is: > > > > 1. New extensions which need to use chrome.metricsPrivate just whitelist > > themselves in _permissions_features.json, same as today > > 2. New extensions which specifically want to use > > chrome.metricsPrivate.getIsCrashReportingEnabled() wonder why the function > > doesn't exist, find this _api_features.json entry, read the comment, and > either: > > a) Follow instructions, asking a metrics owner for a review > > b) Ignore the comment and whitelist themselves (hopefully a reviewer would > > notice; to that end, maybe better to put a comment at the end of the list?) > > > > Basically the metrics ppl are concerned about misuse of this function and want > > people to include them in reviews before using the function in new places. > > This is a pretty monstrous list. Let's see if we can't narrow it down a bit. > I'll follow up with you offline. Done. https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:602: // metrics_private OWNERS approval of the usage before adding entries On 2016/10/03 21:11:10, Ilya Sherman wrote: > On 2016/09/30 21:10:34, michaelpg wrote: > > or //tools/metrics/OWNERS? > > Yes, the intent is for //tools/metrics/OWNERS to take a look at any new uses of > this API =) Done.
LGTM, thanks.
https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/2387793002/diff/20001/chrome/common/extension... chrome/common/extensions/api/_api_features.json:588: "default_parent": true On 2016/10/03 21:11:10, Ilya Sherman wrote: > What does default_parent mean? metricsPrivate is a "complex" feature (it's an array of 2 objects). To use metricsPrivate as a parent (automatically done for metricsPrivate.foo, as below) we have to specify which of these two objects its children should inherit from. See: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/_features.m...
lgtm; thanks for your investigation, Michael!
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2387793002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Whitelist for metricsPrivate.getIsCrashReportingEnabled Whitelists the metricsPrivate.getIsCrashReportingEnabled() extension API function separately from the rest of the metricsPrivate API. This should prompt developers to get an OWNERS review for new usages of getIsCrashReportingEnabled(). It's implemented as a feature whitelist instead of a permission whitelist so that existing uses will continue to work. To that end, also whitelists all extensions that are already whitelisted for the metricsPrivate permission as a starting point. BUG=374199,647397 ========== to ========== Whitelist for metricsPrivate.getIsCrashReportingEnabled Whitelists the metricsPrivate.getIsCrashReportingEnabled() extension API function separately from the rest of the metricsPrivate API. This should prompt developers to get an OWNERS review for new usages of getIsCrashReportingEnabled(). It's implemented as a feature whitelist instead of a permission whitelist so that existing uses will continue to work. To that end, also whitelists all extensions that are already whitelisted for the metricsPrivate permission as a starting point. BUG=374199,647397 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Whitelist for metricsPrivate.getIsCrashReportingEnabled Whitelists the metricsPrivate.getIsCrashReportingEnabled() extension API function separately from the rest of the metricsPrivate API. This should prompt developers to get an OWNERS review for new usages of getIsCrashReportingEnabled(). It's implemented as a feature whitelist instead of a permission whitelist so that existing uses will continue to work. To that end, also whitelists all extensions that are already whitelisted for the metricsPrivate permission as a starting point. BUG=374199,647397 ========== to ========== Whitelist for metricsPrivate.getIsCrashReportingEnabled Whitelists the metricsPrivate.getIsCrashReportingEnabled() extension API function separately from the rest of the metricsPrivate API. This should prompt developers to get an OWNERS review for new usages of getIsCrashReportingEnabled(). It's implemented as a feature whitelist instead of a permission whitelist so that existing uses will continue to work. To that end, also whitelists all extensions that are already whitelisted for the metricsPrivate permission as a starting point. BUG=374199,647397 Committed: https://crrev.com/3d1a7fba3851df28c1dfd7c786233131ce17cf93 Cr-Commit-Position: refs/heads/master@{#422911} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3d1a7fba3851df28c1dfd7c786233131ce17cf93 Cr-Commit-Position: refs/heads/master@{#422911} |
