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 See [[#Waiting For Focus]] One or more tests have to deal with switching the test resembles lock between multiple elements, and the following format-<pre><!-- snip --><body onload="runTests();">[[Changes to Fullscreen Unlock|spec is changing]] on that front.
====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||X</s>
|
|<s>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<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====
Many tests will intermittently hang if they don't wait for focus. To have a test wait for focus, ensure that the test incorporates the following template:
<pre>
<!-- snip -->
<body onload="runTests();">
<!-- other tags that you may need go here -->
<script>
function runTests() {
SimpleTest.waitForExplicitFinish();
SimpleTest.waitForFocus(function() {
// Test code goes here
});
}
</script>
</body>
<!-- /snip --></pre>
====Using the Test Harness====