Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10082033:
Add tests for fullscreen mode and mouse lock mode. (Closed)
|
Created:
8 years, 8 months ago by dyu1 Modified:
8 years, 8 months ago CC:
chromium-reviews, anantha, dyu1 Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd tests for fullscreen mode and mouse lock mode.
-testPrefsLineEntryForFullscreenAllowed
-testPrefsLineEntryForFullscreenExit
-testPatternsForAllowFullscreenAndPointerLock
-testPatternsForAllowMouseLock
-testNoMouseLockRequest
-testUnableToLockMouse
-SearchForTextOutsideOfContainer
Added additional tests:
-testEnterTabFSWhileInBrowserFS
-testNoMouseLockInBrowserFS
-testMouseLockExitWhenBrowserLoseFocus
-ExitTabFSToBrowserFS
-F11KeyExistsTabAndBrowserFS
BUG=122481
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133584
Patch Set 1 : #
Total comments: 22
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Total comments: 30
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 16 (0 generated)
@scheib: please comment on test coverage.
http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:28: driver = self.NewWebDriver() why not use self._driver? http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:130: self.assertTrue(self.WaitUntil(self.IsFullscreenForTab)) why not call LaunchFullscreenMode() instead of 127-130? The button to click could be an arg http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:138: def EnableMouseLockMode(self): _ prefix? http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:139: """Helper function to enable mouse lock mode.""" Declare the expectations. That the fullscreen_mouselock.html page should be open, etc etc http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:151: '/'.join(self.GetHttpURLForDataPath('').split('/')[0:3]) + ',*') this is incomprehensible. Don't split. Just remove '/files/' from the end. hostname_pattern = re.sub('/files/$', '', self.GetHttpURLForDataPath('')) ALso, it would be better to compute it in setUp() if you're going to use it in several tests. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:168: def testPatternsForAllowFullscreenAndPointerLock(self): the test names are getting too big. Makes it difficult to disable tests in PYAUTO_TESTS (because of the 80 chars per line limit). Make it smaller: testPatternsFS() is fine.
http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:104: def LaunchFullscreenMode(self): Consider naming this so it's clear that it doesn't accept the fullscreen transition. E.g. LaunchFullscreenAndExpectPrompt. Ok, that name is too long, but it's confusing when used in tests below and then actions are taken to accept the dialog. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:115: self.assertTrue(self.IsFullscreenBubbleDisplayed()) FYI: Above these lines are duplicated when testing the automation hooks. In production test code you should only need one of the asserts: IsFullscreenBubbleDisplayed OR IsFullscreenBubbleDisplayingButtons OR IsFullscreenPermissionRequested. This code could easily get away with only IsFullscreenPermissionRequested. But since it's not duplicated all over it won't do any harm. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:133: self.assertTrue(self.WaitUntil(self.IsMouseLockPermissionRequested)) Please add a comment noting that this wait shouldn't be needed. It is there only because of http://code.google.com/p/chromium/issues/detail?id=123396. One should be able to click the "fullscreen & mouselock" button on that page, Then accept both in a single action. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:229: lambda: not self.IsMouseLockPermissionRequested())) I'm concerned that there is a race condition here. Mouse lock permission will be be requested before we make the request, so it's likely to stay in that state even after a click event. We must wait for some other signal that the click has registered and that mouse lock state has changed. This is possible, but we must modify the HTML to do it. Callbacks are specified in the mouse lock API, and you can see that test page logs out success or failure of mouse lock transitions. I recommend changing this test and testNoMouseLockRequest to wait for the JavaScript response before checking on the PermissionRequested state.
Addressed the comments and one question for scheib. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:28: driver = self.NewWebDriver() On 2012/04/16 19:42:12, Nirnimesh wrote: > why not use self._driver? Done. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:104: def LaunchFullscreenMode(self): On 2012/04/16 21:37:48, scheib wrote: > Consider naming this so it's clear that it doesn't accept the fullscreen > transition. E.g. LaunchFullscreenAndExpectPrompt. Ok, that name is too long, but > it's confusing when used in tests below and then actions are taken to accept the > dialog. Done. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:115: self.assertTrue(self.IsFullscreenBubbleDisplayed()) There could be a chance for additional duplicates so I kept IsFullscreenPermissionRequested. On 2012/04/16 21:37:48, scheib wrote: > FYI: Above these lines are duplicated when testing the automation hooks. In > production test code you should only need one of the asserts: > IsFullscreenBubbleDisplayed OR IsFullscreenBubbleDisplayingButtons OR > IsFullscreenPermissionRequested. > > This code could easily get away with only IsFullscreenPermissionRequested. But > since it's not duplicated all over it won't do any harm. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:130: self.assertTrue(self.WaitUntil(self.IsFullscreenForTab)) On 2012/04/16 19:42:12, Nirnimesh wrote: > why not call LaunchFullscreenMode() instead of 127-130? The button to click > could be an arg Done. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:133: self.assertTrue(self.WaitUntil(self.IsMouseLockPermissionRequested)) On 2012/04/16 21:37:48, scheib wrote: > Please add a comment noting that this wait shouldn't be needed. It is there only > because of http://code.google.com/p/chromium/issues/detail?id=123396. One should > be able to click the "fullscreen & mouselock" button on that page, Then accept > both in a single action. Done. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:138: def EnableMouseLockMode(self): I didn't make this private since later on you'll be able to mouse lock without depending on fullscreen. On 2012/04/16 19:42:12, Nirnimesh wrote: > _ prefix? http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:139: """Helper function to enable mouse lock mode.""" On 2012/04/16 19:42:12, Nirnimesh wrote: > Declare the expectations. That the fullscreen_mouselock.html page should be > open, etc etc Done. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:151: '/'.join(self.GetHttpURLForDataPath('').split('/')[0:3]) + ',*') Done and moved to setUp() On 2012/04/16 19:42:12, Nirnimesh wrote: > this is incomprehensible. Don't split. Just remove '/files/' from the end. > hostname_pattern = re.sub('/files/$', '', self.GetHttpURLForDataPath('')) > > ALso, it would be better to compute it in setUp() if you're going to use it in > several tests. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:168: def testPatternsForAllowFullscreenAndPointerLock(self): On 2012/04/16 19:42:12, Nirnimesh wrote: > the test names are getting too big. Makes it difficult to disable tests in > PYAUTO_TESTS (because of the 80 chars per line limit). Make it smaller: > testPatternsFS() is fine. Done. http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:229: lambda: not self.IsMouseLockPermissionRequested())) If the js callback is successful then check the two asserts and if it callback fails then assert an error? On 2012/04/16 21:37:48, scheib wrote: > I'm concerned that there is a race condition here. Mouse lock permission will be > be requested before we make the request, so it's likely to stay in that state > even after a click event. We must wait for some other signal that the click has > registered and that mouse lock state has changed. This is possible, but we must > modify the HTML to do it. Callbacks are specified in the mouse lock API, and you > can see that test page logs out success or failure of mouse lock transitions. I > recommend changing this test and testNoMouseLockRequest to wait for the > JavaScript response before checking on the PermissionRequested state.
http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:229: lambda: not self.IsMouseLockPermissionRequested())) On 2012/04/16 23:21:27, dyu1 wrote: > If the js callback is successful then check the two asserts and if it callback > fails then assert an error? > > On 2012/04/16 21:37:48, scheib wrote: > > I'm concerned that there is a race condition here. Mouse lock permission will > be > > be requested before we make the request, so it's likely to stay in that state > > even after a click event. We must wait for some other signal that the click > has > > registered and that mouse lock state has changed. This is possible, but we > must > > modify the HTML to do it. Callbacks are specified in the mouse lock API, and > you > > can see that test page logs out success or failure of mouse lock transitions. > I > > recommend changing this test and testNoMouseLockRequest to wait for the > > JavaScript response before checking on the PermissionRequested state. > If you can get your python test code to wait for some notification from JS, then I would recommend having the pointer lock callbacks (both success and failure) simply notify the test to continue. Then have the test assert that mouse lock and permissions are true or false as appropriate (this test and the test above expect different states).
http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:109: def LaunchFSAndExpectPrompt(self, buttonAction='enterFullscreen'): prefix function name with underscore if meant to be used internally to this class http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:109: def LaunchFSAndExpectPrompt(self, buttonAction='enterFullscreen'): button_action, here and everywhere else in this file http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:114: until after approving the prompt. (optional) maybe we should say that this helper will fail the current test if it's not successful http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:118: click enterFullscreen. indent by 4 more spaces http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:138: """Helper function to enable fullscreen and mouse lock mode.""" nit: try to be consistent with capitalization of "Fullscreen" and "Mouse Lock" when talking about those modes. For example, it's capitalized in line 132 above, but not in the current line http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:139: self.LaunchFSAndExpectPrompt('enterFullscreenAndLockMouse1') button_action= http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:171: content_settings['pattern_pairs']) nit: should we have a msg='' argument to provide an explanation in case the assertion fails? If so, you may want to consider doing the same for the other assertions added to this CL (wherever you think it makes sense and would be helpful) http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:183: def testPatternsForFSAndML(self): there's some inconsistency in function names added to this CL. Sometimes, you abbreviate "Fullscreen" as "FS" and "MouseLock" as "ML". For clarity, maybe we should avoid abbreviations entirely, unless the function names are getting way too long. In this case, I don't think the function name would be too long if we didn't abbreviate. http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:225: self.assertTrue(self.IsMouseLocked()) Perhaps the () are unnecessary, (e.g., see line 147 above). Similar comment at line 224 above
https://chromiumcodereview.appspot.com/10082033/diff/5005/chrome/test/functio... File chrome/test/functional/fullscreen_mouselock.py (right): https://chromiumcodereview.appspot.com/10082033/diff/5005/chrome/test/functio... chrome/test/functional/fullscreen_mouselock.py:225: self.assertTrue(self.IsMouseLocked()) On 2012/04/17 17:57:14, dennis_jeffrey wrote: > Perhaps the () are unnecessary, (e.g., see line 147 above). Similar comment at > line 224 above Sorry, please ignore this comment. The parens are needed here because we're calling assertTrue, which expects a "regular value" input argument, not a callable. So we actually want to invoke the function argument before assertTrue() is invoked. In contrast, line 147 above is calling WaitUntil, which expects a callable input argument - so we should not have parens there (otherwise, the expression would evaluate to the result of the function call, rather than to the function itself).
http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/2001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:229: lambda: not self.IsMouseLockPermissionRequested())) On 2012/04/16 23:55:42, scheib wrote: > On 2012/04/16 23:21:27, dyu1 wrote: > > If the js callback is successful then check the two asserts and if it callback > > fails then assert an error? > > > > On 2012/04/16 21:37:48, scheib wrote: > > > I'm concerned that there is a race condition here. Mouse lock permission > will > > be > > > be requested before we make the request, so it's likely to stay in that > state > > > even after a click event. We must wait for some other signal that the click > > has > > > registered and that mouse lock state has changed. This is possible, but we > > must > > > modify the HTML to do it. Callbacks are specified in the mouse lock API, and > > you > > > can see that test page logs out success or failure of mouse lock > transitions. > > I > > > recommend changing this test and testNoMouseLockRequest to wait for the > > > JavaScript response before checking on the PermissionRequested state. > > > > If you can get your python test code to wait for some notification from JS, then > I would recommend having the pointer lock callbacks (both success and failure) > simply notify the test to continue. Then have the test assert that mouse lock > and permissions are true or false as appropriate (this test and the test above > expect different states). Done. http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:109: def LaunchFSAndExpectPrompt(self, buttonAction='enterFullscreen'): On 2012/04/17 17:57:14, dennis_jeffrey wrote: > prefix function name with underscore if meant to be used internally to this > class Done. http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:109: def LaunchFSAndExpectPrompt(self, buttonAction='enterFullscreen'): On 2012/04/17 17:57:14, dennis_jeffrey wrote: > button_action, here and everywhere else in this file Done. http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:114: until after approving the prompt. On 2012/04/17 17:57:14, dennis_jeffrey wrote: > (optional) maybe we should say that this helper will fail the current test if > it's not successful Done. http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:118: click enterFullscreen. On 2012/04/17 17:57:14, dennis_jeffrey wrote: > indent by 4 more spaces Done. http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:138: """Helper function to enable fullscreen and mouse lock mode.""" On 2012/04/17 17:57:14, dennis_jeffrey wrote: > nit: try to be consistent with capitalization of "Fullscreen" and "Mouse Lock" > when talking about those modes. For example, it's capitalized in line 132 > above, but not in the current line Done. http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:139: self.LaunchFSAndExpectPrompt('enterFullscreenAndLockMouse1') Done On 2012/04/17 17:57:14, dennis_jeffrey wrote: > button_action= http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:171: content_settings['pattern_pairs']) I added it although the assertion error shows that the hostname does not match when the test fails. On 2012/04/17 17:57:14, dennis_jeffrey wrote: > nit: should we have a msg='' argument to provide an explanation in case the > assertion fails? If so, you may want to consider doing the same for the other > assertions added to this CL (wherever you think it makes sense and would be > helpful) http://codereview.chromium.org/10082033/diff/5005/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:183: def testPatternsForFSAndML(self): This test is disabled in PYAUTO_TESTS. The name is fully spelled out is too long in that file. See Nirnimesh's original comments about 80 chars limit. On 2012/04/17 17:57:14, dennis_jeffrey wrote: > there's some inconsistency in function names added to this CL. Sometimes, you > abbreviate "Fullscreen" as "FS" and "MouseLock" as "ML". For clarity, maybe we > should avoid abbreviations entirely, unless the function names are getting way > too long. In this case, I don't think the function name would be too long if we > didn't abbreviate.
LGTM % minor comments. http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:33: self._driver = self.NewWebDriver() Why repeat? http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:178: def testPrefsLineEntryForFullscreenAllowed(self): LineEntry -> Prefs http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:179: """Verify line entry when fullscreen is allowed.""" it's not clear what you mean by 'line entry'. Change to: prefs? http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:189: def testPrefsLineEntryForFullscreenExit(self): LineEntry -> Prefs http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:324: import time move to the top of this file
http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:33: self._driver = self.NewWebDriver() On 2012/04/23 19:16:26, Nirnimesh wrote: > Why repeat? Done. http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:178: def testPrefsLineEntryForFullscreenAllowed(self): On 2012/04/23 19:16:26, Nirnimesh wrote: > LineEntry -> Prefs Done. http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:179: """Verify line entry when fullscreen is allowed.""" On 2012/04/23 19:16:26, Nirnimesh wrote: > it's not clear what you mean by 'line entry'. Change to: prefs? Done. http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:189: def testPrefsLineEntryForFullscreenExit(self): On 2012/04/23 19:16:26, Nirnimesh wrote: > LineEntry -> Prefs Done. http://codereview.chromium.org/10082033/diff/20001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:324: import time On 2012/04/23 19:16:26, Nirnimesh wrote: > move to the top of this file Done.
http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:19: //callback that the click has registered and the mouse lock state has changed. add a space right after the // in each of the above 2 lines http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:24: console.log("lock success") semicolon http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:30: console.log("lock failed") semicolon http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:116: If the helper is not successful then the tests will fail. nit: 'tests' --> 'test' http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:187: msg='Saved hostname pattern does not match expected pattern.') nit: might want to say what the expected and actual values were, if they're not already listed when this assertion fails http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:214: msg='Saved hostname and behavior patterns does not match expected.') 'does not' --> 'do not' http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:230: msg='Saved hostname and behavior patterns does not match expected.') 'does not' --> 'do not' http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:323: # Require manual intervention to send ESC key due to crbug.com/123930. Maybe turn this into a "TODO(dyu)" to remove once the associated bug is fixed. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:338: def F11KeyExistsTabAndBrowserFS(self): 'Exists' --> 'Exits'? http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:339: """Verify existing tab fullscreen exists all fullscreen modes. 'exists' --> 'exits'? http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:350: # Require manual intervention to send F11 key due to crbug.com/123930. same comment as line 323 above
http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:88: <button id="cancelFullscreen" onclick="cancelFullscreen()">cancelFullscreen()</button><br> This seems redundant with exitFullscreen(). http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:292: self.assertTrue(self.WaitUntil(lambda: not self.IsMouseLocked()), Similar to above tests (e.g. testNoMouseLockRequest), this should use the JS callback to detect the mouse lock result. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:304: self.AcceptCurrentFullscreenOrMouseLockRequest() Use a WaitUntil to confirm that we're fullscreen and mouse locked before continuing. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:323: # Require manual intervention to send ESC key due to crbug.com/123930. Do you need to skip this test in PYAUTO_TESTS?
http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:19: //callback that the click has registered and the mouse lock state has changed. On 2012/04/23 19:42:38, dennis_jeffrey wrote: > add a space right after the // in each of the above 2 lines Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:24: console.log("lock success") On 2012/04/23 19:42:38, dennis_jeffrey wrote: > semicolon Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:30: console.log("lock failed") On 2012/04/23 19:42:38, dennis_jeffrey wrote: > semicolon Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:88: <button id="cancelFullscreen" onclick="cancelFullscreen()">cancelFullscreen()</button><br> On 2012/04/23 20:08:20, scheib wrote: > This seems redundant with exitFullscreen(). Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:116: If the helper is not successful then the tests will fail. On 2012/04/23 19:42:38, dennis_jeffrey wrote: > nit: 'tests' --> 'test' Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:187: msg='Saved hostname pattern does not match expected pattern.') If the test fails the assert will display the two patterns that are not equal. On 2012/04/23 19:42:38, dennis_jeffrey wrote: > nit: might want to say what the expected and actual values were, if they're not > already listed when this assertion fails http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:214: msg='Saved hostname and behavior patterns does not match expected.') On 2012/04/23 19:42:38, dennis_jeffrey wrote: > 'does not' --> 'do not' Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:230: msg='Saved hostname and behavior patterns does not match expected.') On 2012/04/23 19:42:38, dennis_jeffrey wrote: > 'does not' --> 'do not' Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:292: self.assertTrue(self.WaitUntil(lambda: not self.IsMouseLocked()), On 2012/04/23 20:08:20, scheib wrote: > Similar to above tests (e.g. testNoMouseLockRequest), this should use the JS > callback to detect the mouse lock result. Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:304: self.AcceptCurrentFullscreenOrMouseLockRequest() On 2012/04/23 20:08:20, scheib wrote: > Use a WaitUntil to confirm that we're fullscreen and mouse locked before > continuing. Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:323: # Require manual intervention to send ESC key due to crbug.com/123930. No, since the continuous build will only run tests with the name "test" in front of it. These partial tests will be run on the command line individually. It will be faster than testing these manually. Until we can send keys to the browser I think this is the effective way to go. On 2012/04/23 20:08:20, scheib wrote: > Do you need to skip this test in PYAUTO_TESTS? http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:323: # Require manual intervention to send ESC key due to crbug.com/123930. On 2012/04/23 19:42:38, dennis_jeffrey wrote: > Maybe turn this into a "TODO(dyu)" to remove once the associated bug is fixed. Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:338: def F11KeyExistsTabAndBrowserFS(self): On 2012/04/23 19:42:38, dennis_jeffrey wrote: > 'Exists' --> 'Exits'? Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:339: """Verify existing tab fullscreen exists all fullscreen modes. On 2012/04/23 19:42:38, dennis_jeffrey wrote: > 'exists' --> 'exits'? Done. http://codereview.chromium.org/10082033/diff/22002/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:350: # Require manual intervention to send F11 key due to crbug.com/123930. On 2012/04/23 19:42:38, dennis_jeffrey wrote: > same comment as line 323 above Done.
LGTM with one minor nit to consider. http://codereview.chromium.org/10082033/diff/26001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/26001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:296: 'lockMouse1(arguments[arguments.length - 1])') should we add a semicolon after the javascript statement?
lgtm
http://codereview.chromium.org/10082033/diff/26001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10082033/diff/26001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:296: 'lockMouse1(arguments[arguments.length - 1])') On 2012/04/23 23:25:18, dennis_jeffrey wrote: > should we add a semicolon after the javascript statement? Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
