Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10535173:
Add three fullscreen tests and three mouse lock tests. (Closed)
|
Created:
8 years, 6 months ago by dyu1 Modified:
8 years, 5 months ago CC:
chromium-reviews, dennis_jeffrey, anantha, Nirnimesh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd three fullscreen tests and three mouse lock tests.
-testTabFSExitWhenNavBackToPrevPage
-testTabFSExitWhenNavToNewPage
-testTabFSDoesNotExitForAnchorLinks
-testMLExitWhenNavBackToPrevPage
-testMLExitWhenNavToNewPage
-testMLDoesNotExitForAnchorLinks
TEST=none
BUG=132665
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144771
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 15
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Total comments: 2
Patch Set 6 : Added a check to verify anchor is clicked for the test. #Patch Set 7 : #
Messages
Total messages: 17 (0 generated)
Thanks for more tests! https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:171: function goBack() { Init is the last function in this list of functions, just for tidiness consider moving this helper function above init? https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:179: <p><a href="#anchor" id="anchor">Click</a> to access the go back button.</p> This link doesn't need to link to a header by the back button. How about: <p><a href="#anchor" name="anchor" id="anchor">anchor link</a> navigates to an anchor on this page, should not exit fullscreen or mouse lock.</p> Also, place this link adjacent to the other navigation test.. keep the anchor and back navigation items next to each other. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:193: <h3><a name="anchor">Go Back Button</a></h3> Cut this line. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:194: <button id="goBack" onclick="goBack();">Back</button><br> s/Back/Navigate Back/; for clarity when manually viewing page. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... File chrome/test/functional/fullscreen_mouselock.py (right): https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... chrome/test/functional/fullscreen_mouselock.py:386: def testTabFSExitUponNavigation(self): Perhaps clarify that this navigation is "navigating backwards". Is it possible that opening a new URL with NavigateToURL would cause a different behavior? Perhaps have a test that starts with _InitiateTabFullscree() then calls NavigateToURL(...google) https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... chrome/test/functional/fullscreen_mouselock.py:393: msg='Tab fullscreen did not exist when navigating to a new page.') s/exist/exit/ -- in more than one place. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... chrome/test/functional/fullscreen_mouselock.py:395: def testTabFSDontExitForAnchorLinks(self): s/Dont/DoesNot/
Added two more additional tests. One to enter into tab FS mode first prior to navigating to another web page and a mouse lock test to verify the lock doesn't break when clicking on an anchor link on the same page. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:171: function goBack() { On 2012/06/14 22:51:12, scheib wrote: > Init is the last function in this list of functions, just for tidiness consider > moving this helper function above init? Done. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:179: <p><a href="#anchor" id="anchor">Click</a> to access the go back button.</p> On 2012/06/14 22:51:12, scheib wrote: > This link doesn't need to link to a header by the back button. How about: > > <p><a href="#anchor" name="anchor" id="anchor">anchor link</a> navigates to an > anchor on this page, should not exit fullscreen or mouse lock.</p> > > Also, place this link adjacent to the other navigation test.. keep the anchor > and back navigation items next to each other. Done. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:193: <h3><a name="anchor">Go Back Button</a></h3> On 2012/06/14 22:51:12, scheib wrote: > Cut this line. Done. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/data/fulls... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:194: <button id="goBack" onclick="goBack();">Back</button><br> On 2012/06/14 22:51:12, scheib wrote: > s/Back/Navigate Back/; for clarity when manually viewing page. Done. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... File chrome/test/functional/fullscreen_mouselock.py (right): https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... chrome/test/functional/fullscreen_mouselock.py:386: def testTabFSExitUponNavigation(self): On 2012/06/14 22:51:12, scheib wrote: > Perhaps clarify that this navigation is "navigating backwards". Is it possible > that opening a new URL with NavigateToURL would cause a different behavior? > Perhaps have a test that starts with _InitiateTabFullscree() then calls > NavigateToURL(...google) Done. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... chrome/test/functional/fullscreen_mouselock.py:393: msg='Tab fullscreen did not exist when navigating to a new page.') On 2012/06/14 22:51:12, scheib wrote: > s/exist/exit/ -- in more than one place. Done. https://chromiumcodereview.appspot.com/10535173/diff/1/chrome/test/functional... chrome/test/functional/fullscreen_mouselock.py:395: def testTabFSDontExitForAnchorLinks(self): On 2012/06/14 22:51:12, scheib wrote: > s/Dont/DoesNot/ Done.
http://codereview.chromium.org/10535173/diff/5001/chrome/test/data/fullscreen... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/5001/chrome/test/data/fullscreen... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:68: // In the pyAuto test when mouse lock is initiated and accepted, the pyAuto code remove |the| pyAuto code -> PyAuto test Remove 'In the pyauto test' at the beginning. it's repetitive. http://codereview.chromium.org/10535173/diff/5001/chrome/test/data/fullscreen... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:143: function goBack() { why are you using javascript to go back, instead of making pyauto calls? http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:395: self._driver.find_element_by_id('goBack').click() use pyauto calls instead of going back from js. http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:397: self.WaitUntil(lambda: not self.IsFullscreenForTab), Is IsFullscreenForTab missing ()? http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:397: self.WaitUntil(lambda: not self.IsFullscreenForTab), Is WaitUntil really required? http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:408: self.WaitUntil(lambda: not self.IsFullscreenForTab), is the waitUntil really required?
Please fix the CL description.
Looking good. Also, this makes me think that we could/should add variations for mouse lock as well. You do so for anchor, but not the navigation tests. http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:419: self.assertTrue( Some delay is needed here. Can we detect and wait for the navigation to #anchor to complete?
What is the correct behavior for mouse lock when navigating to another page? I'm assuming the lock should break? On 2012/06/15 16:49:00, scheib wrote: > Looking good. Also, this makes me think that we could/should add variations for > mouse lock as well. You do so for anchor, but not the navigation tests. > > http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... > File chrome/test/functional/fullscreen_mouselock.py (right): > > http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... > chrome/test/functional/fullscreen_mouselock.py:419: self.assertTrue( > Some delay is needed here. Can we detect and wait for the navigation to #anchor > to complete?
http://codereview.chromium.org/10535173/diff/5001/chrome/test/data/fullscreen... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/5001/chrome/test/data/fullscreen... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:68: // In the pyAuto test when mouse lock is initiated and accepted, the pyAuto code On 2012/06/15 05:40:15, Nirnimesh wrote: > remove |the| > pyAuto code -> PyAuto test > > Remove 'In the pyauto test' at the beginning. it's repetitive. Done. http://codereview.chromium.org/10535173/diff/5001/chrome/test/data/fullscreen... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:143: function goBack() { On 2012/06/15 05:40:15, Nirnimesh wrote: > why are you using javascript to go back, instead of making pyauto calls? Done. http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:395: self._driver.find_element_by_id('goBack').click() On 2012/06/15 05:40:15, Nirnimesh wrote: > use pyauto calls instead of going back from js. Done. http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:397: self.WaitUntil(lambda: not self.IsFullscreenForTab), On 2012/06/15 05:40:15, Nirnimesh wrote: > Is IsFullscreenForTab missing ()? Done. http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:397: self.WaitUntil(lambda: not self.IsFullscreenForTab), On 2012/06/15 05:40:15, Nirnimesh wrote: > Is WaitUntil really required? Done. http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:408: self.WaitUntil(lambda: not self.IsFullscreenForTab), On 2012/06/15 05:40:15, Nirnimesh wrote: > is the waitUntil really required? Done. http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:419: self.assertTrue( Researching how to go about doing this. On 2012/06/15 16:49:00, scheib wrote: > Some delay is needed here. Can we detect and wait for the navigation to #anchor > to complete?
lgtm http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:188: lock_result = self._driver.find_element_by_id('lockMouse2').click() Why is lock_result assigned here?
http://codereview.chromium.org/10535173/diff/13001/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/13001/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:68: // When mouse lock is initiated and accepted, PyAuto test early line break. Move some text from next line to this one. http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:190: self.WaitUntil(lambda: self.IsMouseLockPermissionRequested())) lambda: self.IsMouseLockPermissionRequested() is equivalent to: self.IsMouseLockPermissionRequested (ie no parens) http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:194: return lock_result merge with previous line
http://codereview.chromium.org/10535173/diff/13001/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/13001/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:68: // When mouse lock is initiated and accepted, PyAuto test On 2012/06/18 18:31:59, Nirnimesh wrote: > early line break. Move some text from next line to this one. Done. http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:188: lock_result = self._driver.find_element_by_id('lockMouse2').click() On 2012/06/18 16:26:24, scheib wrote: > Why is lock_result assigned here? Done. http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:190: self.WaitUntil(lambda: self.IsMouseLockPermissionRequested())) On 2012/06/18 18:31:59, Nirnimesh wrote: > lambda: self.IsMouseLockPermissionRequested() > > is equivalent to: > self.IsMouseLockPermissionRequested > > (ie no parens) Done. http://codereview.chromium.org/10535173/diff/13001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:194: return lock_result On 2012/06/18 18:31:59, Nirnimesh wrote: > merge with previous line Done.
http://codereview.chromium.org/10535173/diff/19001/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/19001/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:198: <p>The <a href="#anchor" name="anchor" id="anchor">anchor link</a> navigates to stick to 80 chars per line http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:152: self.assertTrue(self.WaitUntil(lambda: self.IsFullscreenForTab())) remove lambda and () http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:405: self.RunCommand(pyauto.IDC_BACK) self.GetBrowserWindow().GetTab().GoBack() Repeat elsewhere http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:406: self.assertTrue( change to assertFalse, and remove 'not' from the next line. Repeat for lines 417, 444, 454, ..
http://codereview.chromium.org/10535173/diff/19001/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/19001/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:198: <p>The <a href="#anchor" name="anchor" id="anchor">anchor link</a> navigates to Made all the changes to conform to 80 chars per line. On 2012/06/18 22:23:03, Nirnimesh wrote: > stick to 80 chars per line http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:152: self.assertTrue(self.WaitUntil(lambda: self.IsFullscreenForTab())) On 2012/06/18 22:23:03, Nirnimesh wrote: > remove lambda and () Done. http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:405: self.RunCommand(pyauto.IDC_BACK) On 2012/06/18 22:23:03, Nirnimesh wrote: > self.GetBrowserWindow().GetTab().GoBack() > > Repeat elsewhere Done. http://codereview.chromium.org/10535173/diff/19001/chrome/test/functional/ful... chrome/test/functional/fullscreen_mouselock.py:406: self.assertTrue( On 2012/06/18 22:23:03, Nirnimesh wrote: > change to assertFalse, and remove 'not' from the next line. > > Repeat for lines 417, 444, 454, .. Done. I was following the pattern of tests that scheib wrote. He used assertTrue and not.
LGTM. Thanks for the fixes. http://codereview.chromium.org/10535173/diff/23001/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/23001/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:51: // functions in the JS will be executed. The pyAuto code waits for lock_result pyAuto -> PyAuto
http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... File chrome/test/functional/fullscreen_mouselock.py (right): http://codereview.chromium.org/10535173/diff/5001/chrome/test/functional/full... chrome/test/functional/fullscreen_mouselock.py:419: self.assertTrue( I ran into two issues with the anchor link. Sometimes the link was not clicked at all in the test and on rare occasions a WebDriverEcxeption was being raised on the anchor link click. I managed to resolve the issue by adding an onlick event in the anchor link in the html and added a new function that checks if the link has been clicked. http://codereview.chromium.org/10535173/diff/23001/chrome/test/data/fullscree... File chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html (right): http://codereview.chromium.org/10535173/diff/23001/chrome/test/data/fullscree... chrome/test/data/fullscreen_mouselock/fullscreen_mouselock.html:51: // functions in the JS will be executed. The pyAuto code waits for lock_result On 2012/06/18 23:13:56, Nirnimesh wrote: > pyAuto -> PyAuto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dyu@chromium.org/10535173/35001
Change committed as 144771 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
