[Extensions Bindings] Check availability of custom API properties

[Extensions Bindings] Check availability of custom API properties

Check whether custom API properties are available to a context before
returning them on an API. Since these objects can be multiple layers
deep (e.g., privacy.websites.protectedContentEnabled), we perform
this check when returning the object, rather than when we instantiate
the API. This has the drawbacks of leaving the key in the API object
as well as only applying to custom-typed properties (as opposed to
POD-style properties), but this is sufficient for our use cases, given
their rarity.

Add tests for the same.

BUG=653596
Review-Url: https://codereview.chromium.org/2950353004
Cr-Commit-Position: refs/heads/master@{#484776}
Committed: https://chromium.googlesource.com/chromium/src/+/7918e9f775cd1a68345bd755af75939e8f282194

[Devlin, 2017-06-22 18:53:41]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 18:54:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 19:36:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 19:36:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Devlin, 2017-06-22 21:52:13]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 21:52:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 23:13:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 23:13:28]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-24 01:10:35]

Description was changed from

==========
restricted properties
BUG=
==========

to

==========
[Extensions Bindings] Check availability of custom API properties

Check whether custom API properties are available to a context before
returning them on an API. Since these objects can be multiple layers
deep (e.g., privacy.websites.protectedContentEnabled), we perform
this check when returning the object, rather than when we instantiate
the API. This has the drawbacks of leaving the key in the API object
as well as only applying to custom-typed properties (as opposed to
POD-style properties), but this is sufficient for our use cases, given
their rarity.

Add tests for the same.

BUG=653596
==========

[Devlin, 2017-06-24 01:37:18]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 01:37:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-24 02:32:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-24 02:32:36]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-26 20:31:18]

Patchset #2 (id:20001) has been deleted

[Devlin, 2017-06-26 20:31:24]

Patchset #1 (id:1) has been deleted

[Devlin, 2017-06-26 20:33:36]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-06-26 20:33:36]

Hey folks, mind taking a look?

This one's a bit interesting.  For our current purposes, restricting on custom
types is sufficient, but it's a bit odd.  However, the alternative would be to
do a potentially-multi-layer deep traversal on API instantiation for restricted
properties, which sounds... less fun.  But, it could potentially be necessary,
and it would address a bit of the weirdness.

Lemme know what you think...

[jbroman, 2017-06-27 19:49:57]

At the risk of exposing my ignorance...what's wrong with the way binding.js does
it (looking at the information exposed in the API definition, and deciding
whether to add it)?

In this case, all we care about is whether the platform is Windows or ChromeOS,
which is even better, because it can't change at runtime or vary between
contexts (right?). So it could go on the ObjectTemplate normally, without even
having to do per-context checks.

Am I missing something?

https://codereview.chromium.org/2950353004/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/2950353004/diff/40001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:672:
"privacy.websites.protectedContentEnabled": {
Does this mean you intend to deprecate the explicit "platforms" field in the
individual API .json files? The legacy code seems to use isSchemaNodeSupported
to manage this.

[Devlin, 2017-06-27 22:37:30]

On 2017/06/27 19:49:57, jbroman wrote:
> At the risk of exposing my ignorance...what's wrong with the way binding.js
does
> it (looking at the information exposed in the API definition, and deciding
> whether to add it)?
> 
> In this case, all we care about is whether the platform is Windows or
ChromeOS,
> which is even better, because it can't change at runtime or vary between
> contexts (right?). So it could go on the ObjectTemplate normally, without even
> having to do per-context checks.
> 
> Am I missing something?

Good question!

A few thoughts:
My biggest gripe about the current way binding.js does this is that it's
*different*.  Everywhere else (events and functions), we determine what's
exposed based on the features.json file.  Having two systems to do the same
thing is unfortunate, and easily leads to bugs.  And by "could easily lead to
bugs", I mean "we already have bugs".  protectedContentEnabled is enabled on
["windows", "cros", "cros touch"], but the available platforms include
"chromeos" and "chromeos touch", so... that's bad.  The property definition also
allows fewer customization options, but that could be a good or bad thing,
depending.

Also, other properties care about manifest version, which has the potential to
be problematic since it's *not* known at initialization time.  However, we no
longer support manifest v1... so in practice, we might be able to get away with
it.

All that said... this solution is also quite wart-y.  So maybe just doing what
binding.js does is better overall.  I'll upload a new patch with that and a TODO
summarizing some of these thoughts.

[Devlin, 2017-06-28 20:47:11]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 20:47:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-06-28 20:48:15]

On 2017/06/27 22:37:30, Devlin wrote:
> On 2017/06/27 19:49:57, jbroman wrote:
> > At the risk of exposing my ignorance...what's wrong with the way binding.js
> does
> > it (looking at the information exposed in the API definition, and deciding
> > whether to add it)?
> > 
> > In this case, all we care about is whether the platform is Windows or
> ChromeOS,
> > which is even better, because it can't change at runtime or vary between
> > contexts (right?). So it could go on the ObjectTemplate normally, without
even
> > having to do per-context checks.
> > 
> > Am I missing something?
> 
> Good question!
> 
> A few thoughts:
> My biggest gripe about the current way binding.js does this is that it's
> *different*.  Everywhere else (events and functions), we determine what's
> exposed based on the features.json file.  Having two systems to do the same
> thing is unfortunate, and easily leads to bugs.  And by "could easily lead to
> bugs", I mean "we already have bugs".  protectedContentEnabled is enabled on
> ["windows", "cros", "cros touch"], but the available platforms include
> "chromeos" and "chromeos touch", so... that's bad.  The property definition
also
> allows fewer customization options, but that could be a good or bad thing,
> depending.
> 
> Also, other properties care about manifest version, which has the potential to
> be problematic since it's *not* known at initialization time.  However, we no
> longer support manifest v1... so in practice, we might be able to get away
with
> it.
> 
> All that said... this solution is also quite wart-y.  So maybe just doing what
> binding.js does is better overall.  I'll upload a new patch with that and a
TODO
> summarizing some of these thoughts.

New patch set uploaded using the binding.js approach.  It's much smaller.  I'm
still sad that it means we have permissions defined in two places, but... well,
one battle at a time. :)

[commit-bot: I haz the power, 2017-06-28 21:26:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 21:26:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-06-29 01:24:56]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 01:25:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 02:11:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 02:11:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-06-29 15:10:49]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 15:11:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-06-29 15:24:21]

lgtm

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
File extensions/renderer/bindings/api_binding.cc (right):

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
extensions/renderer/bindings/api_binding.cc:35: return "chromeos";
I assume you'll look into what's up with protectedContentEnabled referring to
"cros" and "cros touch", which aren't used here or in binding.js?

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
extensions/renderer/bindings/api_binding.cc:457: for (const auto& platform :
*platforms) {
super-nit: your choice, but I mildly prefer <algorithm> style here:

std::string this_platform = GetPlatformString();
auto is_this_platform = [&this_platform](const base::Value& platform) {
  return platform.is_string() && platform.GetString() == this_platform;
};
if (std::none_of(platforms->begin(), platforms->end(), is_this_platform))
  continue;

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
File extensions/renderer/bindings/api_binding_unittest.cc (right):

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
extensions/renderer/bindings/api_binding_unittest.cc:639: EXPECT_EQ("nonlinux",
GetStringPropertyFromObject(binding_object, context,
As the trybots are already indicating, this should be "\"nonlinux\"".

[commit-bot: I haz the power, 2017-06-29 16:24:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 16:24:51]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-29 21:00:44]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[Devlin, 2017-06-29 21:00:51]

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
File extensions/renderer/bindings/api_binding.cc (right):

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
extensions/renderer/bindings/api_binding.cc:35: return "chromeos";
On 2017/06/29 15:24:21, jbroman wrote:
> I assume you'll look into what's up with protectedContentEnabled referring to
> "cros" and "cros touch", which aren't used here or in binding.js?

Yep!  This just puts it more inline with the end result of the current bindings
and uses the same enums as the features files.

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
extensions/renderer/bindings/api_binding.cc:457: for (const auto& platform :
*platforms) {
On 2017/06/29 15:24:20, jbroman wrote:
> super-nit: your choice, but I mildly prefer <algorithm> style here:
> 
> std::string this_platform = GetPlatformString();
> auto is_this_platform = [&this_platform](const base::Value& platform) {
>   return platform.is_string() && platform.GetString() == this_platform;
> };
> if (std::none_of(platforms->begin(), platforms->end(), is_this_platform))
>   continue;

Oh, forgot about none_of.  I had a version that used find_if(...) != end(), but
that was clunkier than the for loop.  This is nice, though.

Done.

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
File extensions/renderer/bindings/api_binding_unittest.cc (right):

https://codereview.chromium.org/2950353004/diff/80001/extensions/renderer/bin...
extensions/renderer/bindings/api_binding_unittest.cc:639: EXPECT_EQ("nonlinux",
GetStringPropertyFromObject(binding_object, context,
On 2017/06/29 15:24:21, jbroman wrote:
> As the trybots are already indicating, this should be "\"nonlinux\"".

Yep, already done in PS4 :)

[commit-bot: I haz the power, 2017-06-29 21:01:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 22:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 22:06:25]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-07-06 22:01:15]

lgtm

[Devlin, 2017-07-06 22:24:16]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-07-06 22:24:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2950353004/#ps120001
(title: "jbroman's")

[commit-bot: I haz the power, 2017-07-06 22:24:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 00:12:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499379856734660,
"parent_rev": "a1d3a65da2d4e255a001fbcbdde583938d919fc3", "commit_rev":
"7918e9f775cd1a68345bd755af75939e8f282194"}

[commit-bot: I haz the power, 2017-07-07 00:13:02]

Description was changed from

==========
[Extensions Bindings] Check availability of custom API properties

Check whether custom API properties are available to a context before
returning them on an API. Since these objects can be multiple layers
deep (e.g., privacy.websites.protectedContentEnabled), we perform
this check when returning the object, rather than when we instantiate
the API. This has the drawbacks of leaving the key in the API object
as well as only applying to custom-typed properties (as opposed to
POD-style properties), but this is sufficient for our use cases, given
their rarity.

Add tests for the same.

BUG=653596
==========

to

==========
[Extensions Bindings] Check availability of custom API properties

Check whether custom API properties are available to a context before
returning them on an API. Since these objects can be multiple layers
deep (e.g., privacy.websites.protectedContentEnabled), we perform
this check when returning the object, rather than when we instantiate
the API. This has the drawbacks of leaving the key in the API object
as well as only applying to custom-typed properties (as opposed to
POD-style properties), but this is sufficient for our use cases, given
their rarity.

Add tests for the same.

BUG=653596

Review-Url: https://codereview.chromium.org/2950353004
Cr-Commit-Position: refs/heads/master@{#484776}
Committed:
https://chromium.googlesource.com/chromium/src/+/7918e9f775cd1a68345bd755af75...
==========

[commit-bot: I haz the power, 2017-07-07 00:13:06]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/7918e9f775cd1a68345bd755af75...