Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2394703003:
Add deprecation messages to deprecated parts of the WebVR API. (Closed)
DescriptionAdd deprecation messages to deprecated parts of the WebVR API.
VRDisplay.getPose() and VREyeParameters.fieldOfView are marked as
deprecated, so we'll report appropriate deprecation warnings in the
console.
This change also adds metrics for secure vs. insecure origin access to WebVR
APIs.
BUG=389343
Committed: https://crrev.com/ed6ea17321313ae71b507c89bfec5ba8dda372c6
Cr-Commit-Position: refs/heads/master@{#423885}
Patch Set 1 #
Total comments: 18
Patch Set 2 : CR feedback #Patch Set 3 : Address further CR feedback #Patch Set 4 : Sync to latest #Patch Set 5 : Sync/Rebase #
Messages
Total messages: 37 (23 generated)
https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:68: UseCounter::count(*document, UseCounter::VRGetDisplays); Shouldn't this be removed since you are now counting it above? Or do we want a count of all GetDisplay calls and then also a subset of that for insecure ones? https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (left): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:109: UseCounter::count(*document, UseCounter::VRDeprecatedGetPose); Do we not need this counter anymore?
https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:68: UseCounter::count(*document, UseCounter::VRGetDisplays); On 2016/10/05 20:43:14, amp wrote: > Shouldn't this be removed since you are now counting it above? > > Or do we want a count of all GetDisplay calls and then also a subset of that for > insecure ones? Acknowledged. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (left): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:109: UseCounter::count(*document, UseCounter::VRDeprecatedGetPose); On 2016/10/05 20:43:14, amp wrote: > Do we not need this counter anymore? I moved it to the IDL, so it will generate a deprecation message as well as track the counter.
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:404: return String::format("VREyeParameters.fieldOfView is deprecated and will be removed in %s. Prefer using the projection matrices provided by VRFrameData instead. See https://www.chromestatus.com/features/4532810371039232 for more details.", milestoneString(M57)); WouldreplacedBy work? The chromestatus URL doesn't appear to have useful information for this specific case, so we probably don't need that. We lose the milestone, but that's not necessarily a bad thing. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:407: return String::format("VRDisplay.getPose() is deprecated and will be removed in %s. Prefer using VRDisplay.getFrameData() instead. See https://www.chromestatus.com/features/4532810371039232 for more details.", milestoneString(M57)); ditto https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:68: UseCounter::count(*document, UseCounter::VRGetDisplays); On 2016/10/05 21:01:03, billorr wrote: > On 2016/10/05 20:43:14, amp wrote: > > Shouldn't this be removed since you are now counting it above? > > > > Or do we want a count of all GetDisplay calls and then also a subset of that > for > > insecure ones? > > Acknowledged. We definitely don't want to call this twice (though it won't really affect data since this is per page). Unless we intend to rename the value, we should track all calls for this value. That's also more useful since we're more likely to track all uses than to want to look at two values and add them. Thus: count(VRGetDisplays) if (insecure) count(VRGetDisplaysInsecureOrigin) https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:162: if (executionContext->isSecureContext(errorMessage)) { ditto
https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:404: return String::format("VREyeParameters.fieldOfView is deprecated and will be removed in %s. Prefer using the projection matrices provided by VRFrameData instead. See https://www.chromestatus.com/features/4532810371039232 for more details.", milestoneString(M57)); On 2016/10/05 21:32:43, ddorwin wrote: > WouldreplacedBy work? > > The chromestatus URL doesn't appear to have useful information for this specific > case, so we probably don't need that. > > We lose the milestone, but that's not necessarily a bad thing. Sure - I can switch to that if we don't need the milestone. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:407: return String::format("VRDisplay.getPose() is deprecated and will be removed in %s. Prefer using VRDisplay.getFrameData() instead. See https://www.chromestatus.com/features/4532810371039232 for more details.", milestoneString(M57)); On 2016/10/05 21:32:43, ddorwin wrote: > ditto Acknowledged. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:68: UseCounter::count(*document, UseCounter::VRGetDisplays); On 2016/10/05 21:32:43, ddorwin wrote: > On 2016/10/05 21:01:03, billorr wrote: > > On 2016/10/05 20:43:14, amp wrote: > > > Shouldn't this be removed since you are now counting it above? > > > > > > Or do we want a count of all GetDisplay calls and then also a subset of that > > for > > > insecure ones? > > > > Acknowledged. > > We definitely don't want to call this twice (though it won't really affect data > since this is per page). > > Unless we intend to rename the value, we should track all calls for this value. > That's also more useful since we're more likely to track all uses than to want > to look at two values and add them. > > Thus: > count(VRGetDisplays) > if (insecure) > count(VRGetDisplaysInsecureOrigin) Acknowledged. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:162: if (executionContext->isSecureContext(errorMessage)) { On 2016/10/05 21:32:43, ddorwin wrote: > ditto Acknowledged.
All outstanding feedback is addressed. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/Deprecation.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:404: return String::format("VREyeParameters.fieldOfView is deprecated and will be removed in %s. Prefer using the projection matrices provided by VRFrameData instead. See https://www.chromestatus.com/features/4532810371039232 for more details.", milestoneString(M57)); On 2016/10/05 21:55:27, billorr wrote: > On 2016/10/05 21:32:43, ddorwin wrote: > > WouldreplacedBy work? > > > > The chromestatus URL doesn't appear to have useful information for this > specific > > case, so we probably don't need that. > > > > We lose the milestone, but that's not necessarily a bad thing. > > Sure - I can switch to that if we don't need the milestone. Done. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/Deprecation.cpp:407: return String::format("VRDisplay.getPose() is deprecated and will be removed in %s. Prefer using VRDisplay.getFrameData() instead. See https://www.chromestatus.com/features/4532810371039232 for more details.", milestoneString(M57)); On 2016/10/05 21:55:27, billorr wrote: > On 2016/10/05 21:32:43, ddorwin wrote: > > ditto > > Acknowledged. Done. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:68: UseCounter::count(*document, UseCounter::VRGetDisplays); On 2016/10/05 21:55:27, billorr wrote: > On 2016/10/05 21:32:43, ddorwin wrote: > > On 2016/10/05 21:01:03, billorr wrote: > > > On 2016/10/05 20:43:14, amp wrote: > > > > Shouldn't this be removed since you are now counting it above? > > > > > > > > Or do we want a count of all GetDisplay calls and then also a subset of > that > > > for > > > > insecure ones? > > > > > > Acknowledged. > > > > We definitely don't want to call this twice (though it won't really affect > data > > since this is per page). > > > > Unless we intend to rename the value, we should track all calls for this > value. > > That's also more useful since we're more likely to track all uses than to want > > to look at two values and add them. > > > > Thus: > > count(VRGetDisplays) > > if (insecure) > > count(VRGetDisplaysInsecureOrigin) > > Acknowledged. Done. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:68: UseCounter::count(*document, UseCounter::VRGetDisplays); On 2016/10/05 21:01:03, billorr wrote: > On 2016/10/05 20:43:14, amp wrote: > > Shouldn't this be removed since you are now counting it above? > > > > Or do we want a count of all GetDisplay calls and then also a subset of that > for > > insecure ones? > > Acknowledged. Done. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (left): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:109: UseCounter::count(*document, UseCounter::VRDeprecatedGetPose); On 2016/10/05 21:01:03, billorr wrote: > On 2016/10/05 20:43:14, amp wrote: > > Do we not need this counter anymore? > > I moved it to the IDL, so it will generate a deprecation message as well as > track the counter. Done. https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2394703003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:162: if (executionContext->isSecureContext(errorMessage)) { On 2016/10/05 21:55:27, billorr wrote: > On 2016/10/05 21:32:43, ddorwin wrote: > > ditto > > Acknowledged. Done.
Description was changed from ========== Mark parts of the WebVR API deprecated to match the spec. See https://w3c.github.io/webvr/ for the spec. VRDisplay.getPose() and VREyeParameters.fieldOfView are marked as deprecated, so we'll report appropriate deprecation warnings in the console. This change also adds metrics for secure vs. insecure origin access to WebVR APIs. BUG=389343 ========== to ========== Add deprecation messages to deprecated parts of the WebVR API. VRDisplay.getPose() and VREyeParameters.fieldOfView are marked as deprecated, so we'll report appropriate deprecation warnings in the console. This change also adds metrics for secure vs. insecure origin access to WebVR APIs. BUG=389343 ==========
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
billorr@chromium.org changed reviewers: + haraken@chromium.org, kbr@chromium.org
kbr@chromium.org: Please review changes in all files except histograms.xml haraken@chromium.org: Please review changes in histograms.xml
On 2016/10/06 00:35:11, billorr wrote: > mailto:kbr@chromium.org: Please review changes in all files except histograms.xml > > mailto:haraken@chromium.org: Please review changes in histograms.xml LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/06 00:35:11, billorr wrote: > mailto:kbr@chromium.org: Please review changes in all files except histograms.xml > > mailto:haraken@chromium.org: Please review changes in histograms.xml Clarification: kbr, Please Owners review changes in all files except histograms.xml
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM if these changes have been tested and match the spec.
The CQ bit was checked by billorr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2394703003/#ps70001 (title: "Sync/Rebase")
The CQ bit was unchecked by billorr@chromium.org
The CQ bit was checked by billorr@chromium.org
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 ========== Add deprecation messages to deprecated parts of the WebVR API. VRDisplay.getPose() and VREyeParameters.fieldOfView are marked as deprecated, so we'll report appropriate deprecation warnings in the console. This change also adds metrics for secure vs. insecure origin access to WebVR APIs. BUG=389343 ========== to ========== Add deprecation messages to deprecated parts of the WebVR API. VRDisplay.getPose() and VREyeParameters.fieldOfView are marked as deprecated, so we'll report appropriate deprecation warnings in the console. This change also adds metrics for secure vs. insecure origin access to WebVR APIs. BUG=389343 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Add deprecation messages to deprecated parts of the WebVR API. VRDisplay.getPose() and VREyeParameters.fieldOfView are marked as deprecated, so we'll report appropriate deprecation warnings in the console. This change also adds metrics for secure vs. insecure origin access to WebVR APIs. BUG=389343 ========== to ========== Add deprecation messages to deprecated parts of the WebVR API. VRDisplay.getPose() and VREyeParameters.fieldOfView are marked as deprecated, so we'll report appropriate deprecation warnings in the console. This change also adds metrics for secure vs. insecure origin access to WebVR APIs. BUG=389343 Committed: https://crrev.com/ed6ea17321313ae71b507c89bfec5ba8dda372c6 Cr-Commit-Position: refs/heads/master@{#423885} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ed6ea17321313ae71b507c89bfec5ba8dda372c6 Cr-Commit-Position: refs/heads/master@{#423885}
Message was sent while issue was closed.
billorr@, can you please send and Intent to Deprecate and Remove to blink-dev to decide on a date for removal? Lately we've been trying to avoid deprecation messages with no removal milestone, as they can tend to stay around forever, whereas ones with dates attached are audited periodically. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
