Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846993002:
[PointerLock] Add null check before dispatching click event (Closed)
Description[PointerLock] Add null check before dispatching click event
BUG=706802
Review-Url: https://codereview.chromium.org/2846993002
Cr-Commit-Position: refs/heads/master@{#469510}
Committed: https://chromium.googlesource.com/chromium/src/+/92650052047f581f5c7a400e379b24d0f2f15495
Patch Set 1 #Patch Set 2 : rbyers's comments: Use wpt/pointerlock/; Rename test #
Messages
Total messages: 34 (20 generated)
The CQ bit was checked by chongz@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.
Description was changed from ========== Check |element_| before dispatching click event BUG= ========== to ========== Check |element_| before dispatching click event BUG=706802 ==========
Description was changed from ========== Check |element_| before dispatching click event BUG=706802 ========== to ========== [PointerLock] Add null check before dispatching click event BUG=706802 ==========
chongz@chromium.org changed reviewers: + scheib@chromium.org
scheib@ PTAL, thanks!
lgtm
chongz@chromium.org changed reviewers: + rbyers@chromium.org
rbyers@ PTAL at the wpt, thanks! Can I put it under "wpt/pointerevents/pointerlock/" like this or should I create a top level dir "wpt/pointerlock/"?
On 2017/04/28 14:59:05, chongz wrote:
> rbyers@ PTAL at the wpt, thanks!
The way the test is described ("doesn't crash"), it's more of an implementation
test than an interop test. You could argue that this test should just live in
blink instead of WPT. Then again, validating the exact behavior of pointerlock
when it's target is removed on mouseup could still be a useful interop test - it
should just check the exact behavior instead of being described as "verifying
the browser doesn't crash". shceib@ WDYT?
> Can I put it under "wpt/pointerevents/pointerlock/" like this or should I
create
> a top level dir "wpt/pointerlock/"?
Since this test doesn't actually use pointerevents, it shouldn't be in the
pointerevents directory (the top-level directory indicates the primary spec
being tested, and the test results in that directory are used as part of
evaluating the interop/maturity status for that spec). There is already a
pointerlock directory in WPT, including with a test that's fairly similar to
this:
https://github.com/w3c/web-platform-tests/blob/66ffd47cfc514ac8a30f540187c25b....
But apparently we're not importing those tests into blink yet. I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=716766 for that (just
importing is pretty trivial - the manual tests won't run by default).
On 2017/04/29 16:11:32, Rick Byers wrote:
> On 2017/04/28 14:59:05, chongz wrote:
> > rbyers@ PTAL at the wpt, thanks!
>
> The way the test is described ("doesn't crash"), it's more of an
implementation
> test than an interop test. You could argue that this test should just live in
> blink instead of WPT. Then again, validating the exact behavior of
pointerlock
> when it's target is removed on mouseup could still be a useful interop test -
it
> should just check the exact behavior instead of being described as "verifying
> the browser doesn't crash". shceib@ WDYT?
IMHO 'don't crash' tests are valid in WPT because everyone can crash... and it's
not overly implementation specific. Easy to see others making this mistake too.
If trying to expand this test to be larger... I suppose we could check the
target parent doesn't receive any events. (It never should, events are
dispatched with bubbles==false).
- ;) scheib
>
> > Can I put it under "wpt/pointerevents/pointerlock/" like this or should I
> create
> > a top level dir "wpt/pointerlock/"?
>
> Since this test doesn't actually use pointerevents, it shouldn't be in the
> pointerevents directory (the top-level directory indicates the primary spec
> being tested, and the test results in that directory are used as part of
> evaluating the interop/maturity status for that spec). There is already a
> pointerlock directory in WPT, including with a test that's fairly similar to
> this:
>
https://github.com/w3c/web-platform-tests/blob/66ffd47cfc514ac8a30f540187c25b....
> But apparently we're not importing those tests into blink yet. I filed
> https://bugs.chromium.org/p/chromium/issues/detail?id=716766 for that (just
> importing is pretty trivial - the manual tests won't run by default).
The CQ bit was checked by chongz@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...
rbyers@ I've renamed the test and moved it to "wpt/pointerlock/", PTAL again,
thanks!
On 2017/04/29 23:22:46, scheib wrote:
> On 2017/04/29 16:11:32, Rick Byers wrote:
> > On 2017/04/28 14:59:05, chongz wrote:
> > > rbyers@ PTAL at the wpt, thanks!
> >
> > The way the test is described ("doesn't crash"), it's more of an
> implementation
> > test than an interop test. You could argue that this test should just live
in
> > blink instead of WPT. Then again, validating the exact behavior of
> pointerlock
> > when it's target is removed on mouseup could still be a useful interop test
-
> it
> > should just check the exact behavior instead of being described as
"verifying
> > the browser doesn't crash". shceib@ WDYT?
>
> IMHO 'don't crash' tests are valid in WPT because everyone can crash... and
it's
> not overly implementation specific. Easy to see others making this mistake
too.
>
> If trying to expand this test to be larger... I suppose we could check the
> target parent doesn't receive any events. (It never should, events are
> dispatched with bubbles==false).
>
> - ;) scheib
>
> >
> > > Can I put it under "wpt/pointerevents/pointerlock/" like this or should I
> > create
> > > a top level dir "wpt/pointerlock/"?
> >
> > Since this test doesn't actually use pointerevents, it shouldn't be in the
> > pointerevents directory (the top-level directory indicates the primary spec
> > being tested, and the test results in that directory are used as part of
> > evaluating the interop/maturity status for that spec). There is already a
> > pointerlock directory in WPT, including with a test that's fairly similar to
> > this:
> >
>
https://github.com/w3c/web-platform-tests/blob/66ffd47cfc514ac8a30f540187c25b....
> > But apparently we're not importing those tests into blink yet. I filed
> > https://bugs.chromium.org/p/chromium/issues/detail?id=716766 for that (just
> > importing is pretty trivial - the manual tests won't run by default).
Here is the CL to import "wpt/pointerlock/":
https://codereview.chromium.org/2853343002/
(I assume the bot can handle directory merging and won't conflict with this
patch)
Also I think my test is necessary as
https://github.com/w3c/web-platform-tests/blob/master/pointerlock/pointerlock...
doesn't cover the "mouseup" case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rbyers@google.com changed reviewers: + rbyers@google.com
LGTM Thanks! And thanks for importing that directory (https://codereview.chromium.org/2853343002).
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2846993002/#ps20001 (title: "rbyers's comments: Use wpt/pointerlock/; Rename test")
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
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...)
On 2017/05/03 14:45:36, commit-bot: I haz the power wrote: > 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...) Hmm it seems that @google email won't work... rbyers@ do you mind stamping again? Thanks!
rbyers@ can you stamp again with the @chromium account? Thanks!
On 2017/05/04 15:39:24, chongz wrote: > rbyers@ can you stamp again with the @chromium account? Thanks! Damn, sorry - multi-signin has been really buggy for me lately. LGTM
The CQ bit was checked by chongz@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": 20001, "attempt_start_ts": 1493934307915970,
"parent_rev": "74c96e8453a7a5da13fb1ebef083a06b1566f2d2", "commit_rev":
"92650052047f581f5c7a400e379b24d0f2f15495"}
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493934307915970,
"parent_rev": "74c96e8453a7a5da13fb1ebef083a06b1566f2d2", "commit_rev":
"92650052047f581f5c7a400e379b24d0f2f15495"}
Message was sent while issue was closed.
Description was changed from ========== [PointerLock] Add null check before dispatching click event BUG=706802 ========== to ========== [PointerLock] Add null check before dispatching click event BUG=706802 Review-Url: https://codereview.chromium.org/2846993002 Cr-Commit-Position: refs/heads/master@{#469510} Committed: https://chromium.googlesource.com/chromium/src/+/92650052047f581f5c7a400e379b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/92650052047f581f5c7a400e379b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
