Add deprecation warnings to old powerful features on insecure origins.

Add deprecation warnings to old powerful features on insecure origins.

As part of the effort to move old powerful features to secure origins
only (see https://goo.gl/rStTGz for more details), this CL adds warning
messages for the APIs in question. In particular, this adds warnings
for:
  - Geolocation
  - Fullscreen
  - Device motion
  - getUserMedia()

R=mkwst@chromium.org
BUG=481604
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194768

[mlamouri (slow - plz ping), 2015-04-30 05:23:52]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-04-30 05:23:53]

drive-by lgtm

https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
Source/core/dom/Fullscreen.cpp:215: }
No need for { and }

https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
Source/core/dom/Fullscreen.cpp:217: //}
Remove?

[timvolodine, 2015-04-30 10:47:44]

timvolodine@chromium.org changed reviewers:
+ timvolodine@chromium.org

[timvolodine, 2015-04-30 10:47:45]

lgtm DeviceMotionController.cpp
with a question though:

https://codereview.chromium.org/1115913002/diff/20001/Source/modules/device_o...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://codereview.chromium.org/1115913002/diff/20001/Source/modules/device_o...
Source/modules/device_orientation/DeviceMotionController.cpp:57:
UseCounter::countDeprecation(document().frame(),
UseCounter::DeviceMotionInsecureOrigin);
should there be a similar change in DeviceOrientationController.cpp? or is only
motion considered deprecated over non secure connections for now?

[Michael van Ouwerkerk, 2015-04-30 13:15:20]

mvanouwerkerk@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2015-04-30 13:15:21]

geolocation lgtm with nit

https://codereview.chromium.org/1115913002/diff/20001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1115913002/diff/20001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:904: return "Geolocation via
getCurrentPosition() and watchPosition() will be deprecated over insecure
origins in the future. You should consider switching your application to a
secure origin, such as HTTPS. See http://goo.gl/rStTGz for more details.";
nit: might as well use https for all the goo.gl links

[Mike West, 2015-04-30 16:17:11]

This looks like a fine approach, the code change LGTM (% the http->https note).

I don't think Blink's API OWNERs ever formally signed off on the deprecation,
however, as
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/2LXKVWYkOus/gT...
was more announcement than plan.

It would be worthwhile to either ping that thread again before landing this
patch, asking for their take on emitting console messages for these features.

[jww, 2015-04-30 17:28:45]

Thanks, all. I'll respond to the thread per Mike's suggestion and wait for
approval before committing.

https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
Source/core/dom/Fullscreen.cpp:215: }
On 2015/04/30 05:23:53, Mounir Lamouri wrote:
> No need for { and }

Done.

https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
Source/core/dom/Fullscreen.cpp:217: //}
On 2015/04/30 05:23:53, Mounir Lamouri wrote:
> Remove?

Yikes, sorry! Done.

https://codereview.chromium.org/1115913002/diff/20001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1115913002/diff/20001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:904: return "Geolocation via
getCurrentPosition() and watchPosition() will be deprecated over insecure
origins in the future. You should consider switching your application to a
secure origin, such as HTTPS. See http://goo.gl/rStTGz for more details.";
On 2015/04/30 13:15:21, Michael van Ouwerkerk wrote:
> nit: might as well use https for all the goo.gl links

For sure. At least I got it right in the comment :-) I should file a bug that
the URL shortener doesn't give HTTPS by default.

https://codereview.chromium.org/1115913002/diff/20001/Source/modules/device_o...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://codereview.chromium.org/1115913002/diff/20001/Source/modules/device_o...
Source/modules/device_orientation/DeviceMotionController.cpp:57:
UseCounter::countDeprecation(document().frame(),
UseCounter::DeviceMotionInsecureOrigin);
On 2015/04/30 10:47:45, timvolodine wrote:
> should there be a similar change in DeviceOrientationController.cpp? or is
only
> motion considered deprecated over non secure connections for now?

Yes, it should. Done!

[jww, 2015-04-30 18:10:22]

On 2015/04/30 17:28:45, jww wrote:
> Thanks, all. I'll respond to the thread per Mike's suggestion and wait for
> approval before committing.
> 
>
https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
> File Source/core/dom/Fullscreen.cpp (right):
> 
>
https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
> Source/core/dom/Fullscreen.cpp:215: }
> On 2015/04/30 05:23:53, Mounir Lamouri wrote:
> > No need for { and }
> 
> Done.
> 
>
https://codereview.chromium.org/1115913002/diff/20001/Source/core/dom/Fullscr...
> Source/core/dom/Fullscreen.cpp:217: //}
> On 2015/04/30 05:23:53, Mounir Lamouri wrote:
> > Remove?
> 
> Yikes, sorry! Done.
> 
>
https://codereview.chromium.org/1115913002/diff/20001/Source/core/frame/UseCo...
> File Source/core/frame/UseCounter.cpp (right):
> 
>
https://codereview.chromium.org/1115913002/diff/20001/Source/core/frame/UseCo...
> Source/core/frame/UseCounter.cpp:904: return "Geolocation via
> getCurrentPosition() and watchPosition() will be deprecated over insecure
> origins in the future. You should consider switching your application to a
> secure origin, such as HTTPS. See http://goo.gl/rStTGz for more details.";
> On 2015/04/30 13:15:21, Michael van Ouwerkerk wrote:
> > nit: might as well use https for all the goo.gl links
> 
> For sure. At least I got it right in the comment :-) I should file a bug that
> the URL shortener doesn't give HTTPS by default.
> 
>
https://codereview.chromium.org/1115913002/diff/20001/Source/modules/device_o...
> File Source/modules/device_orientation/DeviceMotionController.cpp (right):
> 
>
https://codereview.chromium.org/1115913002/diff/20001/Source/modules/device_o...
> Source/modules/device_orientation/DeviceMotionController.cpp:57:
> UseCounter::countDeprecation(document().frame(),
> UseCounter::DeviceMotionInsecureOrigin);
> On 2015/04/30 10:47:45, timvolodine wrote:
> > should there be a similar change in DeviceOrientationController.cpp? or is
> only
> > motion considered deprecated over non secure connections for now?
> 
> Yes, it should. Done!

pdr@ replied on the thread that this is good to him, so I'm going to go ahead
and commit.

[jww, 2015-04-30 18:10:29]

The CQ bit was checked by jww@chromium.org

[jww, 2015-04-30 18:10:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, mkwst@chromium.org, timvolodine@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1115913002/#ps40001
(title: "Updates from nits")

[commit-bot: I haz the power, 2015-04-30 18:11:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115913002/40001

[commit-bot: I haz the power, 2015-04-30 19:33:39]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194768

[Mike West, 2015-04-30 20:35:22]

On 2015/04/30 at 18:10:22, jww wrote:
> pdr@ replied on the thread that this is good to him, so I'm going to go ahead
and commit.

pdr@ is indeed awesome, but you need three LGTMs from API OWNERs before
deprecating (step 5 of
http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation). Please revert
this change until you have those approvals. :)

[jww, 2015-04-30 20:46:25]

On 2015/04/30 20:35:22, Mike West (traveling. slow.) wrote:
> On 2015/04/30 at 18:10:22, jww wrote:
> > pdr@ replied on the thread that this is good to him, so I'm going to go
ahead
> and commit.
> 
> pdr@ is indeed awesome, but you need three LGTMs from API OWNERs before
> deprecating (step 5 of
> http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation). Please revert
> this change until you have those approvals. :)

According to pdr@, deprecation warnings do not count as "deprecation" and do not
require the API OWNERs approval. That is, this is step 4 of
http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation). Let me know if
you still want me to revert, though.

[Mike West, 2015-04-30 23:35:31]

On 2015/04/30 at 20:46:25, jww wrote:
> According to pdr@, deprecation warnings do not count as "deprecation" and do
not require the API OWNERs approval. That is, this is step 4 of
http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation). Let me know if
you still want me to revert, though.

That strikes me as a bit strange, but you're right: I misread the process. :)

[lgarron, 2015-05-01 19:11:44]

On 2015/04/30 at 23:35:31, mkwst wrote:
> On 2015/04/30 at 20:46:25, jww wrote:
> > According to pdr@, deprecation warnings do not count as "deprecation" and do
not require the API OWNERs approval. That is, this is step 4 of
http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation). Let me know if
you still want me to revert, though.
> 
> That strikes me as a bit strange, but you're right: I misread the process. :)

re: "See https://goo.gl/rStTGz for more details" we standardized on something
slightly different in another CL (by me; don't have time to find it before lunch
because the new Rietveld search is broken). We should probably standardize on
using the same shortlink to that page.
Maybe we should even get a g.co or chrome shortlink?

[lgarron, 2015-05-01 20:53:23]

On 2015/05/01 at 19:11:44, lgarron wrote:
> On 2015/04/30 at 23:35:31, mkwst wrote:
> > On 2015/04/30 at 20:46:25, jww wrote:
> > > According to pdr@, deprecation warnings do not count as "deprecation" and
do not require the API OWNERs approval. That is, this is step 4 of
http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation). Let me know if
you still want me to revert, though.
> > 
> > That strikes me as a bit strange, but you're right: I misread the process.
:)
> 
> re: "See https://goo.gl/rStTGz for more details" we standardized on something
slightly different in another CL (by me; don't have time to find it before lunch
because the new Rietveld search is broken). We should probably standardize on
using the same shortlink to that page.
> Maybe we should even get a g.co or chrome shortlink?

Found the relevant CR: https://codereview.chromium.org/1060723002/

We used "(see: https://goo.gl/Y0ZkNV)".

[philipj_slow, 2015-05-01 22:36:57]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2015-05-01 22:36:58]

https://codereview.chromium.org/1115913002/diff/40001/Source/core/dom/Fullscr...
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/1115913002/diff/40001/Source/core/dom/Fullscr...
Source/core/dom/Fullscreen.cpp:208: // actually used. This could be used later
if a warning is shown in the
The countDeprecation is the console warning, is there any chance that this
errorMessage will ever be used? Some other clients throw exceptions using this
message, but that will not make sense for the Fullscreen API. I think these
checks should be in fullscreenIsSupported, which is the place in the spec which
allows taking this into consideration. That would also ensure that
document.fullscreenEnabled is false if fullscreen will always fail, if it's made
to return false if (frame()->settings()->strictPowerfulFeatureRestrictions()).

https://codereview.chromium.org/1115913002/diff/40001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1115913002/diff/40001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:909: case DeviceMotionInsecureOrigin:
The DeviceMotionInsecureOrigin and DeviceOrientationInsecureOrigin use counters
are both around 20%, this will amount to console spam which in turn causes
people to take deprecation messages less seriously. Is there a credible plan for
disabling device motion and device orientation on insecure origins? At this
level of usage, there's little chance that deprecation messages will bring usage
down to a level where we would normally consider breaking changes.

https://codereview.chromium.org/1115913002/diff/40001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:915: case FullscreenInsecureOrigin:
This ought to be OK, combined usage of all the fullscreen entry points isn't
terribly high:
https://www.chromestatus.com/metrics/feature/timeline/popularity/168
https://www.chromestatus.com/metrics/feature/timeline/popularity/170
https://www.chromestatus.com/metrics/feature/timeline/popularity/176
https://www.chromestatus.com/metrics/feature/timeline/popularity/177

[jww, 2015-05-05 21:29:53]

Philip, I just uploaded https://codereview.chromium.org/1125573003/ which I
think addresses some of your comments. See my inlined response below, but if you
have follow ups, it would be great to continue on the new CL.

Lucas, I disagree that this should use the link you refer to. The link I use
refers specifically to the deprecation of powerful features already allowed on
insecure origins, while the document you linked to is for new features.

https://codereview.chromium.org/1115913002/diff/40001/Source/core/dom/Fullscr...
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/1115913002/diff/40001/Source/core/dom/Fullscr...
Source/core/dom/Fullscreen.cpp:208: // actually used. This could be used later
if a warning is shown in the
On 2015/05/01 22:36:58, philipj_UTC2 wrote:
> The countDeprecation is the console warning, is there any chance that this
> errorMessage will ever be used? Some other clients throw exceptions using this
> message, but that will not make sense for the Fullscreen API. I think these
> checks should be in fullscreenIsSupported, which is the place in the spec
which
> allows taking this into consideration. That would also ensure that
> document.fullscreenEnabled is false if fullscreen will always fail, if it's
made
> to return false if (frame()->settings()->strictPowerfulFeatureRestrictions()).

I think that's an accurate description of where it should be blocked in the long
term, but it appears to me to be the wrong place to measure usage. That is, I
don't think we don't want to measure that fullscreen is being used until
requestFullscreen is actually called.

https://codereview.chromium.org/1115913002/diff/40001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1115913002/diff/40001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:909: case DeviceMotionInsecureOrigin:
On 2015/05/01 22:36:58, philipj_UTC2 wrote:
> The DeviceMotionInsecureOrigin and DeviceOrientationInsecureOrigin use
counters
> are both around 20%, this will amount to console spam which in turn causes
> people to take deprecation messages less seriously. Is there a credible plan
for
> disabling device motion and device orientation on insecure origins? At this
> level of usage, there's little chance that deprecation messages will bring
usage
> down to a level where we would normally consider breaking changes.

That's a great point, and my mistake for not checking those stats earlier. Those
usage numbers are incredibly high, so I'm suspicious that something funky is
going on (a la some common library automatically registers a 'devicemotion'
event), but until we can figure out what's going on, I'll remove the deprecation
message.

[philipj_slow, 2015-05-06 07:43:55]

https://codereview.chromium.org/1115913002/diff/40001/Source/core/dom/Fullscr...
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/1115913002/diff/40001/Source/core/dom/Fullscr...
Source/core/dom/Fullscreen.cpp:208: // actually used. This could be used later
if a warning is shown in the
On 2015/05/05 21:29:53, jww wrote:
> On 2015/05/01 22:36:58, philipj_UTC2 wrote:
> > The countDeprecation is the console warning, is there any chance that this
> > errorMessage will ever be used? Some other clients throw exceptions using
this
> > message, but that will not make sense for the Fullscreen API. I think these
> > checks should be in fullscreenIsSupported, which is the place in the spec
> which
> > allows taking this into consideration. That would also ensure that
> > document.fullscreenEnabled is false if fullscreen will always fail, if it's
> made
> > to return false if
(frame()->settings()->strictPowerfulFeatureRestrictions()).
> 
> I think that's an accurate description of where it should be blocked in the
long
> term, but it appears to me to be the wrong place to measure usage. That is, I
> don't think we don't want to measure that fullscreen is being used until
> requestFullscreen is actually called.

The fullscreen element ready check is only used in requestFullscreen() in both
spec and implementation. (I didn't check this when I commented, though, so your
skepticism is warranted.)

If you want to move this to the ready check I'll be happy to review, but there's
no hurry really.

https://codereview.chromium.org/1115913002/diff/40001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1115913002/diff/40001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.cpp:909: case DeviceMotionInsecureOrigin:
On 2015/05/05 21:29:53, jww wrote:
> On 2015/05/01 22:36:58, philipj_UTC2 wrote:
> > The DeviceMotionInsecureOrigin and DeviceOrientationInsecureOrigin use
> counters
> > are both around 20%, this will amount to console spam which in turn causes
> > people to take deprecation messages less seriously. Is there a credible plan
> for
> > disabling device motion and device orientation on insecure origins? At this
> > level of usage, there's little chance that deprecation messages will bring
> usage
> > down to a level where we would normally consider breaking changes.
> 
> That's a great point, and my mistake for not checking those stats earlier.
Those
> usage numbers are incredibly high, so I'm suspicious that something funky is
> going on (a la some common library automatically registers a 'devicemotion'
> event), but until we can figure out what's going on, I'll remove the
deprecation
> message.

Thanks!