1
edit
Changes
→Review Issues
* Finish tests, updated to changes above.
* <s>Update patch to trunk</s> - humph
* Figure out why DOMMouseScroll events aren't being retargeted to the locked element
* Look into roc's suggestion from bug 722449 comment 8, namely, using getBoundingClientRect vs. GetPrimaryFrame()
* Maybe use nsStubMutationObserver instead of nsIMutationObserver?
* Extensions to the Document Interface. Add pointerLockElement attribute and exitPointerLock method - diogogmt
* Update Element interface to have requestPointerLock - diogogmt
* <s>Dispatch pointerlockchange or pointerlockerror events instead of firing a callback when pointer gets locked</s> - diogogmt
* Update mochitests to use new pointerlock API
* Add expect number of tests to mochitests
* Update patch to trunk
===Tests===
One or more tests have to deal with switching the lock between multiple elements, and the [[Changes to Fullscreen Unlock|spec is changing]] on that front.
====New Test Reviews [Updated 1/19/2012]====This is a new list of test. The following tests will need to confirm to the new prefixes in accordance to Patchv3 of [https://bugzilla.mozilla.org/show_bug.cgi?id=633602 Bug 633602]. Please add your name beside each test(s) you are assessing/reviewing/fixing/peer reviewing.Once the cause for rejection is clear, indicate it by placing an X in any of the columns and optionally filling out the comments column. Indicate the action taken to resolve the issue in the action taken column. Example actions include:# '''DELETED'''# '''FIXED''' Other actions are also acceptable where appropriate.Once fixed, please pull request to rhung's [https://github.com/rhung/mozilla-central/tree/mouselock-tests/dom/tests/mochitest/mouselockrhung's mouselock-tests] branch and indicate that you've done so in the pull requested column. Ensure that you add '''[POST-SUBMIT]''' to the name of your pull request. {| border=1 style="border: 1px solid darkgray;"| Num| Name| Primary Reviewer| Timing issues?| Needs Harness?| Needs focus?| Formatting issues?| Other? (Indicate in comments)| Peer Reviewers| Peer reviewed?| Action Taken| Pull requested?| bgcolor="red" style="text-align:center;" | Updated with Patchv3 | Comments|- | colspan=14 bgcolor="yellow" style="text-align:center;" | '''Ensure you have actually tested your test before sending a pull request'''|-| 1| test_FullScreenHarness.html| Anachid| X (no action)| X (no action)| X (no action)| X (no action)| -| | | '''FIXED'''| | X| |-| 2| file_DOMtree.html| Anachid| X (fixed)| X (fixed)| X (fixed)| X (no action)| -| | | '''FIXED''' | | X| |-| 3| file_nestedFullScreen.html| Anachid| X (fixed)| X (fixed)| X (fixed)| X (no action)| -| | | '''FIXED''' | | X| |-| 4| file_differentDOM.html| Anachid| X (no action)| X (no action)| X (no action)| X (no action)| -| | | '''FIXED''' | | X| |-| 5| file_fullscreen.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| Test needs to completely re-done. Cases like <br>* ok(false, "Mouse locked without fullscreen mode"); will always fail!* Flow of the test will causes scope problems.* Currently fixing.|-| 6| file_doubleLockCallBack.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| |-| 7| file_unlockOnUnload.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| |-| 8| file_exitMouseLockOnLoseFocus.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| |-| 9| test_mousePos.html| rhung||||| Doesn't seem like it can be done via mochitest||| '''REMOVED'''||| As mentioned in the previous comments, synthesizeMouse cannot be moved beyond the browser. It might need to be a litmus test.|-| 10| file_mouselocklost.html| Anachid| X| X| X| X| -| | | '''FIXED''' | | X| Removed some test that were repeating. Otherwise, it works|-| 11| file_movementXY.html| rhung| X| X| X| X| -| | | '''Fixing''' | | X||-| 12| file_targetOutOfFocus.html| rhung| X| X| X| X (fixed)| -| | | '''FIXED''' | | X| Minor changes, mostly styling.|-| 13| test_mouseLockable.html| rhung| X| X| X| X| -| | | '''FIXED & RENAMED''' | | X| Updated to test_MozPointerLock.html|-| 14| file_MouseEvents.html| rhung| X| X| X| X| Leak issue might be platform specific.| | | '''FIXED?''' | | X| Works without chance of hanging or random success and failures.|-| 15| file_limitlessScroll.html| rhung|||||| | | '''MERGED WITH file_movementXY''' | |||-| 16| file_defaultUnlock.html| rhung|||||| | | '''FIXED''' | | X||-| 17| file_userPref.html| rhung|||||| | | '''FIXED''' || X||-| 18| file_constantXY.html| rhung|||||| | | '''FIXING''' || X| Needs proper error messages, perhaps formatting issues. Test runs fine though.|-|} New Tests (not sure if it will work on mochitest, and the test will possibly fail as dave is working on this patch)# When there is a parent element with multiple child element, have a test to check that all mouse events fired inside the child element are suppressed and is captured by the parent element. (this may also include the scenario where there is an iframe in the parent element)# A Test to check that repeated move of the mouse to a certain direction that will exceed the screen size does not break the mouselock (this may conflict with file_movementXY.html and we may need to add more test cases) ====Changes To Be Applied To Submitted Tests[Deprecated]====
The following table is used to maintain a small note on the task for each test. It will include note from the review by Mozilla and issue we overlooked.
|}
====Remaining Test Reviews[Deprecated]====
The following tests need investigation. Please add your name beside each test(s) you are assessing/reviewing/fixing/peer reviewing.
Once the cause for rejection is clear, indicate it by placing an X in any of the columns and optionally filling out the comments column. Indicate the action taken to resolve the issue in the action taken column. Example actions include:
| Action Taken
| Pull requested?
| bgcolor="red" style="text-align:center;" | Updated with Patchv3
| Comments
|-
| colspan=13 14 bgcolor="yellow" style="text-align:center;" | '''Ensure you have actually tested your test before sending a pull request'''
|-
|-
|
|<s><b>IMPOSSIBLE IN MOCHITEST</b></s>
|
|
|<s>
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|X
|
|
* <s>For Peer Reviewer: https://github.com/rhung/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/mouselock/file_doubleLockCallBack.html</s>
|<s>X</s>
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|
|test_mousePos.html
|diogogmt
|
|
|
|
|<s><b>PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|<s>Original file didn't need any changes. Passed all test criterias</s>
<s>Awaiting peer review to cross it off the list</s>
|-
|<s>119</s>|<s>file_fullscreen.html</s>|<s>northWind</s>|<s>X</s>|<s>X</s>|<s>X</s>|<s>X</s>|<s>X</s>|<s>rhung</s>||<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|X
|
|'''READ COMMENTS'''|X|<s>Lines </s> <s>49</s>, <s>66</s><s>, and 76 are too long. 76 in particular. </s> <s>Needs comment at the beginning to describe what the test does.</s>
|-
|<s>107</s>
|
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|<s>[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests-constantXY/dom/tests/mochitest/mouselock/file_constantXY.html file_constantXY]</s>
|file_defaultUnlock.html
|diogogmt
|
|
|
|
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|
|
|'''FIXING'''
|
|
|
|
|'''PEER REVIEWED, SEE COMMENTS'''
|
|
|
|-
|116
|file_movementfile_movementXY.html
|diogogmt
|
|
|
| mjschranz
|
|<b>REVIEWED, READY FOR PULL REQUEST</b>
|
|
|[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/pointerlock/file_movementXY.html file_movementXY]|* mjschranz** As far as I can tell, there doesn't appear to be any styling errors. The tests run just fine and based on what I understand about it and what is listed in the comments the tests appear to function as intended.
|-
|<s>120</s>
|
|<s>rhung</s>
|
|
|
|
|
| mjschranz
|
|<b>REVIEWED, SEE COMMENTS</b>
|
|
|[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/pointerlock/file_targetOutOfFocus.html file_targetOutOfFocus]|* mjschranz** Line 41: the word focus is surrounded by single quotation marks, should be double ( "" ).** No other styling issues seen. Tests run and appear to be functioning as per the specs.
|-
|118
|file_userPref.html
|
|
|
|X
|
|X
|
|
|121
|test_mouseLockable.html
|
|
|X
|X
|X
|X
|
|
|X
|
| mjschranz
|
|<b>REVIEWED, SEE COMMENTS</b>
|
|
|[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests-nestedFullScreen/dom/tests/mochitest/mouselockpointerlock/file_nestedFullScreen.html file_nestedFullScreen][https* mjschranz** Line 48:isLocked method call is missing opening//githubclosing brackets. The test still passes when FullScreenHarness is run however we obviously need to fix that and ensure it's still locked as intended.** No other styling issues seen.com/diogogmt/mozilla-central/tree/mouselock-Fix previous error and double check all tests-constantXY branch]still pass when harness is run.
|-
|<s>123</s>
|
|<s>'''FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY'''</s>
|
|
|
|
|<s>'''FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY'''</s>
|
|
|
