Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599113002:
Add the first layout tests for WebVR to test navigator.getVRDisplays with mocked vr_service mojo in… (Closed)
DescriptionAdd the first layout tests for WebVR to test navigator.getVRDisplays with mocked vr_service mojo interfaces.
We will add more layout tests to cover more WebVR APIs later with same
approach.
BUG=675325
Committed: https://crrev.com/6a27a59ca9f7ccba5bf70846aa767de4ad3cdebb
Cr-Commit-Position: refs/heads/master@{#441088}
Patch Set 1 #
Total comments: 6
Messages
Total messages: 29 (15 generated)
leilei@chromium.org changed reviewers: + bajones@chromium.org, bsheedy@chromium.org, shaobo.yan@intel.com
leilei@chromium.org changed reviewers: + dglazkov@chromium.org, esprehn@chromium.org
lgtm
On 2016/12/22 18:25:53, dglazkov wrote: > lgtm Stealth vacation LGTM. (Shh! Don't tell the family that I'm looking at work stuff!) On a more serious note, thanks for getting this rolling at such an awkward time of year. I'm happy to jump in and help flesh out the test coverage when I'm back from leave at the beginning of the year.
On 2016/12/22 21:28:46, bajones (OOO till January) wrote: > On 2016/12/22 18:25:53, dglazkov wrote: > > lgtm > > Stealth vacation LGTM. (Shh! Don't tell the family that I'm looking at work > stuff!) > > On a more serious note, thanks for getting this rolling at such an awkward time > of year. I'm happy to jump in and help flesh out the test coverage when I'm back > from leave at the beginning of the year. Thanks for doing code review during vacation and offering help! Brian is working on adding more test cases, we will let you know if we need any help:) BTW, I will land this patch after holiday, I don't want to land something before long holiday and worry about if I will break others during holiday:) Happy holidays everyone!
The CQ bit was checked by leilei@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. Thx for helps on make it work!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by leilei@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.
The CQ bit was checked by leilei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1483418522360500, "parent_rev":
"d7722f1f783287f00a0288d0cfccc96d03e39b3e", "commit_rev":
"4a6bad1ec9e0f81f11e1eee4fe552b4207f56047"}
Message was sent while issue was closed.
Description was changed from ========== Add the first layout tests for WebVR to test navigator.getVRDisplays with mocked vr_service mojo interfaces. We will add more layout tests to cover more WebVR APIs later with same approach. BUG=675325 ========== to ========== Add the first layout tests for WebVR to test navigator.getVRDisplays with mocked vr_service mojo interfaces. We will add more layout tests to cover more WebVR APIs later with same approach. BUG=675325 Review-Url: https://codereview.chromium.org/2599113002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Add the first layout tests for WebVR to test navigator.getVRDisplays with mocked vr_service mojo interfaces. We will add more layout tests to cover more WebVR APIs later with same approach. BUG=675325 Review-Url: https://codereview.chromium.org/2599113002 ========== to ========== Add the first layout tests for WebVR to test navigator.getVRDisplays with mocked vr_service mojo interfaces. We will add more layout tests to cover more WebVR APIs later with same approach. BUG=675325 Committed: https://crrev.com/6a27a59ca9f7ccba5bf70846aa767de4ad3cdebb Cr-Commit-Position: refs/heads/master@{#441088} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6a27a59ca9f7ccba5bf70846aa767de4ad3cdebb Cr-Commit-Position: refs/heads/master@{#441088}
Message was sent while issue was closed.
yzshen@chromium.org changed reviewers: + yzshen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right): https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:6: 'mojo/public/js/connection', Please note that "connection" is deprecated and being removed soon. https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:21: this.router_ = new router.Router(handle); Please don't use router directly, this doesn't do validation of incoming messages properly. An example of the correct/recommended way: https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/s...
Message was sent while issue was closed.
https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right): https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:6: 'mojo/public/js/connection', On 2017/01/03 22:29:05, yzshen1 wrote: > Please note that "connection" is deprecated and being removed soon. What do you suggest using in its place? Or should using the solution you suggested below handle this for us? https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:21: this.router_ = new router.Router(handle); On 2017/01/03 22:29:05, yzshen1 wrote: > Please don't use router directly, this doesn't do validation of incoming > messages properly. > > An example of the correct/recommended way: > https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/s... I'll look into changing this, then.
Message was sent while issue was closed.
https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right): https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:6: 'mojo/public/js/connection', On 2017/01/03 22:39:06, bsheedy wrote: > On 2017/01/03 22:29:05, yzshen1 wrote: > > Please note that "connection" is deprecated and being removed soon. > > What do you suggest using in its place? Or should using the solution you > suggested below handle this for us? With bindings.BindingSet, connection is not needed anymore. https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:21: this.router_ = new router.Router(handle); On 2017/01/03 22:39:06, bsheedy wrote: > On 2017/01/03 22:29:05, yzshen1 wrote: > > Please don't use router directly, this doesn't do validation of incoming > > messages properly. > > > > An example of the correct/recommended way: > > > https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/s... > > I'll look into changing this, then. Thanks for fixing it!
Message was sent while issue was closed.
On 2017/01/03 22:39:06, bsheedy wrote: > https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right): > > https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:6: > 'mojo/public/js/connection', > On 2017/01/03 22:29:05, yzshen1 wrote: > > Please note that "connection" is deprecated and being removed soon. > > What do you suggest using in its place? Or should using the solution you > suggested below handle this for us? > > https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:21: this.router_ > = new router.Router(handle); > On 2017/01/03 22:29:05, yzshen1 wrote: > > Please don't use router directly, this doesn't do validation of incoming > > messages properly. > > > > An example of the correct/recommended way: > > > https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/s... > > I'll look into changing this, then. I am working on a patch for this. Still compiling. Let me see whether it works... (It may not if the current code has validation errors ignored.) I will let you know soon. :)
Message was sent while issue was closed.
On 2017/01/03 22:41:54, yzshen1 wrote: > On 2017/01/03 22:39:06, bsheedy wrote: > > > https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js (right): > > > > > https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:6: > > 'mojo/public/js/connection', > > On 2017/01/03 22:29:05, yzshen1 wrote: > > > Please note that "connection" is deprecated and being removed soon. > > > > What do you suggest using in its place? Or should using the solution you > > suggested below handle this for us? > > > > > https://codereview.chromium.org/2599113002/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/vr/resources/mock-vr-service.js:21: > this.router_ > > = new router.Router(handle); > > On 2017/01/03 22:29:05, yzshen1 wrote: > > > Please don't use router directly, this doesn't do validation of incoming > > > messages properly. > > > > > > An example of the correct/recommended way: > > > > > > https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/s... > > > > I'll look into changing this, then. > > I am working on a patch for this. Still compiling. Let me see whether it > works... (It may not if the current code has validation errors ignored.) > > I will let you know soon. :) FYI: https://codereview.chromium.org/2608383002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
