[Battery] Allow usage from SecureContext or top-level browsing context only.

[Battery] Allow usage from SecureContext or top-level browsing context only.

Make the Battery Status API available only within a secure context that is
also a top-level browsing context. This disallows the use of the API within
framed content, as well as from any content that is not a secure context.

Details: https://github.com/w3c/battery/issues/10

WPT updated in https://github.com/w3c/web-platform-tests/pull/5871

BUG=661792
Review-Url: https://codereview.chromium.org/2880763002
Cr-Commit-Position: refs/heads/master@{#476263}
Committed: https://chromium.googlesource.com/chromium/src/+/3543d97ca7a33fb8ad48261ad252428555747896

[riju_, 2017-05-12 13:24:27]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 13:24:46]

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-05-12 14:39:15]

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

[commit-bot: I haz the power, 2017-05-12 14:39:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[riju_, 2017-05-17 12:38:02]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 12:38:15]

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-05-17 13:53:17]

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

[commit-bot: I haz the power, 2017-05-17 13:53:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[riju_, 2017-05-18 08:11:13]

Patchset #2 (id:20001) has been deleted

[riju_, 2017-05-18 08:15:31]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 08:15:49]

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-05-18 08:19:31]

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

[commit-bot: I haz the power, 2017-05-18 08:19:32]

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

[riju_, 2017-05-18 08:57:26]

Patchset #2 (id:40001) has been deleted

[riju_, 2017-05-18 09:17:26]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 09:18:08]

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-05-18 10:37:44]

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

[commit-bot: I haz the power, 2017-05-18 10:37:44]

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

[riju_, 2017-05-19 08:19:46]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 08:20:24]

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-05-19 10:30:21]

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

[commit-bot: I haz the power, 2017-05-19 10:30:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[riju_, 2017-05-22 07:24:23]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

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

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-05-22 08:44:48]

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

[commit-bot: I haz the power, 2017-05-22 08:44:49]

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

[riju_, 2017-05-22 10:33:52]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 10:34:08]

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-05-22 10:42:02]

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

[commit-bot: I haz the power, 2017-05-22 10:42:02]

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...)

[riju_, 2017-05-22 10:56:44]

Patchset #4 (id:100001) has been deleted

[riju_, 2017-05-22 11:07:58]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 11:08: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-05-22 11:16:25]

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

[commit-bot: I haz the power, 2017-05-22 11:16:26]

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...)

[riju_, 2017-05-22 11:26:05]

rijubrata.bhaumik@intel.com changed reviewers:
+ timvolodine@chromium.org

[riju_, 2017-05-22 11:26:05]

Hi Tim,

Please take a look.
We updated the Battery Status spec to address some privacy concerns
(https://github.com/w3c/battery/issues/10).

The WPT tests are imported using tool. It got reverted recently but finally it's
in now.

wpt/battery-status/battery-iframe.https-expected.txt
AND wpt/battery-status/battery-insecure-context-expected.txt
had to be changed, but git cl upload was throwing error presubmit error "The
following files are passing testharness results without console error messages,
they should be removed", so I had to use git cl upload --bypass-hooks

Is there another way to make changes to *-expected.txt in external/wpt/ but keep
git cl presubmit happy?

[timvolodine, 2017-05-25 16:01:14]

Overall looks fine to me, thanks for the patch! a few nits below

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/battery-status/detached-no-crash.html
(right):

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/battery-status/detached-no-crash.html:22: promise
= nav.getBattery();
nit: you can probably get rid of var promise and do nav.getBattery().then(..)
directly

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/battery-status/detached-no-crash.html:27:
assert_equals(error.code, DOMException.SECURITY_ERR);
nit: maybe add a shouldBeTrue / shouldBeEqualToNumber or debug to show that this
case is actually executed in the expectations..

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/NavigatorBattery.cpp (right):

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/NavigatorBattery.cpp:24:
ExecutionContext* execution_context = ExecutionContext::From(script_state);
nit: maybe one extra blank line for readability

[timvolodine, 2017-05-25 16:02:39]

On 2017/05/22 11:26:05, riju_ wrote:
> Hi Tim,
> 
> Please take a look.
> We updated the Battery Status spec to address some privacy concerns
> (https://github.com/w3c/battery/issues/10).
> 
> The WPT tests are imported using tool. It got reverted recently but finally
it's
> in now.
> 
> wpt/battery-status/battery-iframe.https-expected.txt
> AND wpt/battery-status/battery-insecure-context-expected.txt
> had to be changed, but git cl upload was throwing error presubmit error "The
> following files are passing testharness results without console error
messages,
> they should be removed", so I had to use git cl upload --bypass-hooks
> 
> Is there another way to make changes to *-expected.txt in external/wpt/ but
keep
> git cl presubmit happy?

no sure at this point, think this is first time I see this actually..

[timvolodine, 2017-05-25 16:03:44]

+cc:rbyers@ as FYI

[riju_, 2017-05-29 13:15:54]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 13:16:04]

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-05-29 13:24:33]

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

[commit-bot: I haz the power, 2017-05-29 13:24:34]

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...)

[riju_, 2017-05-30 06:35:59]

rijubrata.bhaumik@intel.com changed reviewers:
+ foolip@chromium.org

[riju_, 2017-05-30 06:36:00]

Tim@: Thanks for the comments. 

foolip@: Regarding *-expected.txt in external/wpt/, I had to use git cl upload
--bypass-hooks, as it's throwing the presubmit errors.
"The following files are passing testharness results without console error
messages, they should be removed"

Any suggestions how to fix this issue ?

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/battery-status/detached-no-crash.html
(right):

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/battery-status/detached-no-crash.html:22: promise
= nav.getBattery();
On 2017/05/25 16:01:13, timvolodine wrote:
> nit: you can probably get rid of var promise and do nav.getBattery().then(..)
> directly

Done.

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/battery-status/detached-no-crash.html:27:
assert_equals(error.code, DOMException.SECURITY_ERR);
On 2017/05/25 16:01:14, timvolodine wrote:
> nit: maybe add a shouldBeTrue / shouldBeEqualToNumber or debug to show that
this
> case is actually executed in the expectations..

Done.

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/NavigatorBattery.cpp (right):

https://codereview.chromium.org/2880763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/NavigatorBattery.cpp:24:
ExecutionContext* execution_context = ExecutionContext::From(script_state);
On 2017/05/25 16:01:14, timvolodine wrote:
> nit: maybe one extra blank line for readability

Done.

[riju_, 2017-05-30 07:03:00]

The CQ bit was checked by rijubrata.bhaumik@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 07:03:18]

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-05-30 09:17:21]

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

[commit-bot: I haz the power, 2017-05-30 09:17:22]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-05-31 14:32:54]

On 2017/05/30 06:36:00, riju_ wrote:
> Tim@: Thanks for the comments. 
> 
> foolip@: Regarding *-expected.txt in external/wpt/, I had to use git cl upload
> --bypass-hooks, as it's throwing the presubmit errors.
> "The following files are passing testharness results without console error
> messages, they should be removed"
> 
> Any suggestions how to fix this issue ?

Just delete those files. Looks like they've now been replaced by empty files?

[riju_, 2017-05-31 16:14:09]

Patchset #7 (id:180001) has been deleted

[riju_, 2017-06-01 06:58:50]

Patchset #7 (id:200001) has been deleted

[riju_, 2017-06-01 08:26:34]

Patchset #7 (id:220001) has been deleted

[riju_, 2017-06-01 08:37:33]

Patchset #8 (id:260001) has been deleted

[riju_, 2017-06-01 10:17:01]

Patchset #8 (id:280001) has been deleted

[riju_, 2017-06-01 10:17:09]

Patchset #7 (id:240001) has been deleted

[riju_, 2017-06-01 10:20:40]

On 2017/05/31 14:32:54, foolip wrote:
> On 2017/05/30 06:36:00, riju_ wrote:
> > Tim@: Thanks for the comments. 
> > 
> > foolip@: Regarding *-expected.txt in external/wpt/, I had to use git cl
upload
> > --bypass-hooks, as it's throwing the presubmit errors.
> > "The following files are passing testharness results without console error
> > messages, they should be removed"
> > 
> > Any suggestions how to fix this issue ?
> 
> Just delete those files. Looks like they've now been replaced by empty files?

Thanks, Yes, I was thinking the same. 
Expectations were added in the Import wpt ..
https://chromium-review.googlesource.com/c/509768/

BTW, I am doing
git rm
third_party/WebKit/LayoutTests/external/wpt/battery-status/battery-iframe.https-expected.txt

git commit -a
git cl upload --no-find-copies

Still rietveld shows M and not D 

Tim@ : any more concerns for this CL ?

[timvolodine, 2017-06-01 10:24:39]

On 2017/06/01 10:20:40, riju_ wrote:
> On 2017/05/31 14:32:54, foolip wrote:
> > On 2017/05/30 06:36:00, riju_ wrote:
> > > Tim@: Thanks for the comments. 
> > > 
> > > foolip@: Regarding *-expected.txt in external/wpt/, I had to use git cl
> upload
> > > --bypass-hooks, as it's throwing the presubmit errors.
> > > "The following files are passing testharness results without console error
> > > messages, they should be removed"
> > > 
> > > Any suggestions how to fix this issue ?
> > 
> > Just delete those files. Looks like they've now been replaced by empty
files?
> 
> Thanks, Yes, I was thinking the same. 
> Expectations were added in the Import wpt ..
> https://chromium-review.googlesource.com/c/509768/
> 
> BTW, I am doing
> git rm
>
third_party/WebKit/LayoutTests/external/wpt/battery-status/battery-iframe.https-expected.txt
> 
> git commit -a
> git cl upload --no-find-copies
> 
> Still rietveld shows M and not D 
> 
> Tim@ : any more concerns for this CL ?

lgtm on my side, thanks!

[riju_, 2017-06-01 11:10:49]

The CQ bit was checked by rijubrata.bhaumik@intel.com

[commit-bot: I haz the power, 2017-06-01 11:10:59]

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-01 13:00:06]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496315449664810,
"parent_rev": "5d0edd94b0f86092c267bc9f5fc77396a994a135", "commit_rev":
"3543d97ca7a33fb8ad48261ad252428555747896"}

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

Description was changed from

==========
[Battery] Allow usage from SecureContext or top-level browsing context only.

Make the Battery Status API available only within a secure context that is
also a top-level browsing context. This disallows the use of the API within
framed content, as well as from any content that is not a secure context.

Details: https://github.com/w3c/battery/issues/10

WPT updated in https://github.com/w3c/web-platform-tests/pull/5871

BUG=661792
==========

to

==========
[Battery] Allow usage from SecureContext or top-level browsing context only.

Make the Battery Status API available only within a secure context that is
also a top-level browsing context. This disallows the use of the API within
framed content, as well as from any content that is not a secure context.

Details: https://github.com/w3c/battery/issues/10

WPT updated in https://github.com/w3c/web-platform-tests/pull/5871

BUG=661792

Review-Url: https://codereview.chromium.org/2880763002
Cr-Commit-Position: refs/heads/master@{#476263}
Committed:
https://chromium.googlesource.com/chromium/src/+/3543d97ca7a33fb8ad48261ad252...
==========

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

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3543d97ca7a33fb8ad48261ad252...

[mlamouri (slow - plz ping), 2017-06-13 13:03:18]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/2937553005/ by mlamouri@chromium.org.

The reason for reverting is: This CL should have gotten a API OWNER lgtm with an
Intent to Ship. If the change is accepted by the API OWNERS, we should re-land
this..

[Rick Byers, 2017-06-16 18:36:52]

On 2017/05/25 16:03:44, timvolodine wrote:
> +cc:rbyers@ as FYI

mlamouri@ is right - we should have an "intent to remove" discussion about
taking away this capability.  See
https://www.chromium.org/blink/removing-features.  Sorry I didn't say that when
cc'd on the review.  And please don't feel at all bad about it - the bar and
process is non-obvious, it's expected that we'll sometimes make mistakes on both
sides.  I'm just sorry for the surprise here.

Doesn't mean we shouldn't do exactly this of course, let's just talk about the
tradeoffs on blink-dev and the WG: https://github.com/w3c/battery/issues/10