1
edit
Changes
→Review Issues
## [http://developer.apple.com/library/mac/#documentation/GraphicsImaging/Reference/Quartz_Services_Ref/Reference/reference.html CGAssociateMouseAndMouseCursorPosition and CGGetLastMouseDelta] on OS X
## [http://www.x.org/archive/X11R6.8.2/doc/XGrabPointer.3.html XGrabPointer] on Linux
====Review Issues====
* <s>Rewrite DOM node removal unlocking logic to use nsIMutatationObserver</s> humph, diogogmt
* <s>Space, bracing, tabs, etc style nits</s> humph
* <s>clientX, clientY, screenX, screenY values should follow spec as per [https://bugzilla.mozilla.org/show_bug.cgi?id=633602#c55 scheib's comment]</s> diogogmt
* <s>Confirm |aEvent->lastRefPoint = nsIntPoint(bounds.width/2, bounds.height/2);| that / 2 is right</s> diogogmt
* <s>"nsCOMPtr<nsIContent> mMouseLockedElement; - Shouldn't this be a static member variable? Or how does the patch handle cases when the document which has iframe and mouse moves over those iframes." Probably wants a test for this case, too.</s> diogogmt
* <s>"nsDOMMouseLockable::ShouldLock...QI to nsINode and check IsInDoc()"</s> diogogmt
* <s>"nsRefPtr<nsMouseLockableRequest> request = new nsMouseLockableRequest(aSuccessCallback, aFailureCallback); -- You should store aTarget (QI'ed to nsINode or nsIContent or Element or nsIDOMEventTarget) so that when calling callback you push Cx to stack; nsCxPusher pusher: // defined in nsContentUtils.h NS_ENSURE_STATE(pusher.Push(target));"</s> - humph
* <s>"nsDOMMouseLockable looks like it should be cycle collectable"</s> humph
* <s>Break .pointer out of Navigator, and add nsIDOMMozNavigatorPointer (not sure if this name is best) as a new interface to Navigator, similar to nsIDOMMozNavigatorBattery.</s> - humph, diogogmt
* <s>Rename for moz* prefix. The API should be probably prefixed. So, navigator.mozPointer and all the interfaces should start with nsIDOMMoz. Similar to what webkit* is doing:</s> - humph
** <s>navigator.webkitPointer.unlock();</s>
** <s>navigator.webkitPointer.lock();</s>
** <s>navigator.webkitPointer.isLocked();</s>
** <s>document.body.addEventListener("webkitpointerlocklost", ...</s>
** <s>mouseMoveEvent.webkitMovementX</s>
** <s>mouseMoveEvent.webkitMovementY</s>
* Follow discussion unfolding here for other changes: http://lists.w3.org/Archives/Public/public-webapps/2011OctDec/1558.html
* 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===
|-
|9
|<s>"Mouse lock must succeed only if the window is in focus"|'''johnno'''</s>|'''test_UserAgentIsActive.html'''
|-
|10
|<s>"Mouse lock must succeed only if...the user-agent is the active application of the operating system"</s>
|'''johnno'''
|'''test_UserAgentIsActive.html'''
|-
|11
# Prefer <code>i++;</code> to <code>i+=1;</code>
# Many tests require the harness and focus, ensure that those tests are correctly written ie:
#* For Harness: See [[#Using the Test Harness|here]]#* For Focus: Ensure the test resembles the following format-<pre><!-- snip --><body onload="runTests();">See [[#Waiting For Focus]]
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>106</s>|<s>test_UserAgentIsActive</s>|<s>diogogmt</s>
|
|<s>X||X|X|northwind</s>
|
|<s>X</s>
|<s>X</s>
|<s>northwind</s>
|
|<s><b>IMPOSSIBLE IN MOCHITEST</b></s>
|
|
|<s>
* northWind Comments prior to diogo re-writing user agent test:
** navigator.userAgent does not indicate focus status of browser.
** test should be testing for full screen if user agent is in focus then testing that full screen is unavailable when user agent is not in focus.
** test uses tabs, should use spaces.
</s>
|-
|<s>109</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>110</s>|<s>test_exitMouselockOnLoseFocus.html</s>||||||<s>diogogmt</s>
|
|<s>X</s>
|<s>X</s>
|<s>X</s>
|
|<s>northWind</s>
|<s>X</s>
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|114
|test_mousePos.html
|diogogmt
|
|
|
|
|A summary of the test: *Move the mouse outside the browser boundaries. *Move the mouse back inside the browser. *Checks if movementXY were set to zero, since the last refPoint is unknownThe problem is that synthesizeMouse doesn't move the mouse outside the boundaries of the specified element. If the X/Y coords passed to synthesizeMouse are bigger than the X/Y of the element receiving the event, it won't move the cursor. I'm logging all the screen/client/movementXY values, and if the coords passed to synthesizeMouse are bigger than the boundaries of the element receiving the mousemove, the test will break, since the mouse is never moved. [http://pastebin.com/MhP62BtC code]
|-
|<s>115</s>|<s>file_mouselocklost.html</s>|<s>diogogmt</s>
|
|
|
|
|<s>rhung</s>
|
|<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>
|
|||-|119|file_fullscreen.html|northWind|X|X|X|X<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>|file_constantxy<s>file_screenXYclientXYConst.html</s>|<s>diogogmt</s>|<s>X</s>|<s>X</s>
|
|
|
|<s>anachid</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>|<bs>FIXING[https://github.com/diogogmt/mozilla-central/tree/mouselock-tests-constantXY branch]</bs>||<s>* Anachid (formatting review on file commit version '''1f5fa3e0a5f7730f33eb5d3f7c51105fac4cb93d'''):</s><s>** Line 8 exceeds 80 character limit by 7 characters.</s><s>** Remove line 99 [ console.log("start"); ]</s><s>** Line 100 and 103 could be re-done to document.getElementById("div") instead of having a div variable, thereby removing variable space and risk of a bug</s><s>** Extra line breaks (should be removed) on line 74,77,78</s>
|-
|108
|file_defaultUnlock.html
|diogogmt
|
|
|
|-
|<s>103</s>|<s>file_DOMtree.html</s>||<s>diogogmt</s>
|
|
|
|
|<s>rhung</s>
|
|<s><b>FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY</b></s>
|
|
|
|'''FIXING'''
|
|
|
|X
|
|X
|
|
|northWind
|
|northWind'''PEER REVIEWED, SEE COMMENTS'''
|
|
|
* northWind:
** hangs intermittently, most likely requires focus, see [[#Waiting For Focus|here]]
** 39,41: not required, handled by harness
** 51: remove line, setup being called at better time @343
** 60: should use === for identity as opposed to ==
** 60: should add document.mozFullScreen && to condition
** 61, 69: needs failure callback with is(false, ...) and SimpleTest.finish()
** 68: should add && document.mozFullScreenElement === outer to condition
** 68: whitespace at end of line
** <s>78: looked at this briefly, are we sure that synthMs will cause the cursor to cross over the 'one' div?</s>
** 172: Col 45: "the" should be "a Left"
** 183-214: Why not testing for mousemove?
|-
|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>file_syntheticMouseEvent.html</s>||<s>diogogmt</s>
|
|
|
|
|<s>rhung</s>
|
|
|
|
|<s>Merged with MouseEvents test</s>
|-
|105
|file_targetOutOfFocus.html
|diogogmt
|
|
|
|
| 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
|
|
|X
|
|X
|
|
|121
|test_mouseLockable.html
(rename to file_mozPointerLock.html)
|
|
|X
|X
|X
|X
|
|
|
|
|-
|122
|file_nestedFullScreen.html
|diogogmt
|
|
|
|X
|
|-|122|file_nestedFullScreen.htmlmjschranz
|
|<b>REVIEWED, SEE COMMENTS</b>
|
|
|[https://github.com/diogogmt/mozilla-central/blob/mouselock-tests/dom/tests/mochitest/pointerlock/file_nestedFullScreen.html file_nestedFullScreen]
* mjschranz
** Line 48: isLocked method call is missing opening/closing 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. Fix previous error and double check all tests still pass when harness is run.
|-
|<s>123</s>
|<s>file_unlockOnUnload.html</s>
|<s>northWind</s>
|
|
|
|X
|
|<s>rhung</s>
|
|<s>'''FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY'''</s>
|
|
|
|-
|<s>123120</s>|<s>file_unlockOnUnloadfile_differentDOM.html</s>|<s>northWinddiogogmt</s>|
|
|
|
|
|<s>rhung</s>
|
|<s>'''FIXED, PEER REVIEWED, AND ADDED TO REPOSITORY'''</s>
|
|
|
|}
<!--
====Test Reviews[DEPRECATED]====
# <s>test_syntheticMouseEvent.html</s>
# <s>test_userPref.html</s> - mjschranz
-->
====Waiting For Focus====