Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(559)

Issue 9702055: Automated tests for full screen & mouse lock M16 features (Closed)

Created:
8 years, 9 months ago by scheib
Modified:
8 years, 8 months ago
CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, ajwong+watch_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

Automated tests for full screen & mouse lock M16 features Tests added to browsertest to improve coverage for fullscreen and mouse lock transitions. Several helper functions added to BrowserTest. Removes need to friend many tests and simplifies tests to increases readability. IsFullscreenForTab() removed globally leaving only IsFullscreenForTabOrPending. IsFullscreenForBrowser() added to fullscreen controller to enable testing. typedef BrowserWithTestWindowTest BrowserTest; removed due to name conflict. BUG=100678 TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129448 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130642

Patch Set 1 #

Total comments: 20

Patch Set 2 : Review issues addressed; merge tot #

Patch Set 3 : FileURL & BrowserTest compilation fixes #

Total comments: 12

Patch Set 4 : sky issues addressed; merged to r128813 #

Total comments: 1

Patch Set 5 : Valgrind error fix, merge to 130358 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+306 lines, -90 lines) Patch
M chrome/browser/ui/browser.h View 1 2 3 4 3 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 3 4 2 chunks +7 lines, -3 lines 0 comments Download
M chrome/browser/ui/browser_browsertest.cc View 1 2 3 4 12 chunks +257 lines, -55 lines 0 comments Download
M chrome/browser/ui/browser_unittest.cc View 3 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller.mm View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller_private.mm View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/fullscreen_controller.h View 1 2 3 1 chunk +13 lines, -2 lines 0 comments Download
M chrome/browser/ui/fullscreen_controller.cc View 1 2 3 4 4 chunks +10 lines, -10 lines 0 comments Download
M chrome/browser/ui/tests/browser_uitest.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/tab_contents/tab_contents.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M content/public/browser/web_contents_delegate.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M content/public/browser/web_contents_delegate.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 27 (0 generated)
scheib
8 years, 9 months ago (2012-03-15 00:27:48 UTC) #1
jeremya
after a brief overview, lgtm. Nice changes!
8 years, 9 months ago (2012-03-15 03:49:08 UTC) #2
koz (OOO until 15th September)
lgtm Nice change! http://codereview.chromium.org/9702055/diff/1/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (right): http://codereview.chromium.org/9702055/diff/1/chrome/browser/ui/browser_browsertest.cc#newcode1151 chrome/browser/ui/browser_browsertest.cc:1151: // Validate that going fullscreen for ...
8 years, 9 months ago (2012-03-16 04:31:22 UTC) #3
yzshen1
I am really glad to see this CL. THANK YOU! :) http://codereview.chromium.org/9702055/diff/1/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (right): ...
8 years, 9 months ago (2012-03-16 07:33:28 UTC) #4
scheib
Issues addressed. OWNERS, please take a look: sky: chrome/browser/ui/ brettw, API rename IsFullscreenForTab -> IsFullscreenForTabOrPending ...
8 years, 9 months ago (2012-03-16 21:20:03 UTC) #5
yzshen1
LGTM Thanks! http://codereview.chromium.org/9702055/diff/1/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (right): http://codereview.chromium.org/9702055/diff/1/chrome/browser/ui/browser_browsertest.cc#newcode230 chrome/browser/ui/browser_browsertest.cc:230: } else { // Not in browser ...
8 years, 9 months ago (2012-03-16 21:42:55 UTC) #6
scheib
http://codereview.chromium.org/9702055/diff/1/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (right): http://codereview.chromium.org/9702055/diff/1/chrome/browser/ui/browser_browsertest.cc#newcode230 chrome/browser/ui/browser_browsertest.cc:230: } else { // Not in browser fullscreen, expect ...
8 years, 9 months ago (2012-03-19 17:11:06 UTC) #7
scheib
OWNERS, please take a look: sky: chrome/browser/ui/ brettw, API rename IsFullscreenForTab -> IsFullscreenForTabOrPending impacting: chrome/browser/tab_contents/ ...
8 years, 9 months ago (2012-03-22 22:25:46 UTC) #8
sky
http://codereview.chromium.org/9702055/diff/18013/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9702055/diff/18013/chrome/browser/ui/browser.h#newcode893 chrome/browser/ui/browser.h:893: // True when the current tab is in fullscreen ...
8 years, 9 months ago (2012-03-22 22:48:09 UTC) #9
scheib
Sky items addressed, though I can't leave BrowserTest defined twice. http://codereview.chromium.org/9702055/diff/18013/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9702055/diff/18013/chrome/browser/ui/browser.h#newcode893 ...
8 years, 9 months ago (2012-03-26 23:30:46 UTC) #10
sky
On Mon, Mar 26, 2012 at 4:30 PM, <scheib@chromium.org> wrote: > Sky items addressed, though ...
8 years, 9 months ago (2012-03-26 23:32:57 UTC) #11
scheib
> Go with something more meaningful then, UITest is extremely generic. > How about BrowserUITest? ...
8 years, 9 months ago (2012-03-26 23:43:30 UTC) #12
sky
LGTM
8 years, 9 months ago (2012-03-27 00:12:41 UTC) #13
scheib
brettw, Owners review please: API rename IsFullscreenForTab -> IsFullscreenForTabOrPending impacting: chrome/browser/tab_contents/ content/
8 years, 9 months ago (2012-03-27 02:35:47 UTC) #14
brettw
content LGTM
8 years, 9 months ago (2012-03-27 03:01:02 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/9702055/31001
8 years, 9 months ago (2012-03-27 18:31:25 UTC) #16
commit-bot: I haz the power
Try job failure for 9702055-31001 (retry) on win for step "compile" (clobber build). It's a ...
8 years, 9 months ago (2012-03-27 22:57:00 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/9702055/31001
8 years, 9 months ago (2012-03-28 16:25:17 UTC) #18
commit-bot: I haz the power
Change committed as 129448
8 years, 9 months ago (2012-03-28 18:30:37 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/9702055/50001
8 years, 8 months ago (2012-04-03 20:04:49 UTC) #20
commit-bot: I haz the power
Try job failure for 9702055-50001 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=17298 Step "update" is always ...
8 years, 8 months ago (2012-04-03 20:33:37 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/9702055/50001
8 years, 8 months ago (2012-04-03 21:21:14 UTC) #22
commit-bot: I haz the power
Try job failure for 9702055-50001 (retry) on win_rel for steps "browser_tests, unit_tests". It's a second ...
8 years, 8 months ago (2012-04-04 01:05:39 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/9702055/50001
8 years, 8 months ago (2012-04-04 01:58:56 UTC) #24
commit-bot: I haz the power
Try job failure for 9702055-50001 (retry) on win_rel for step "browser_tests". It's a second try, ...
8 years, 8 months ago (2012-04-04 05:26:44 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/9702055/50001
8 years, 8 months ago (2012-04-04 14:44:05 UTC) #26
commit-bot: I haz the power
8 years, 8 months ago (2012-04-04 15:47:32 UTC) #27
Try job failure for 9702055-50001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Powered by Google App Engine
This is Rietveld 408576698