Exit tabbed fullscreen mode on navigation.

Exit tabbed fullscreen mode on navigation.


BUG=97553
TEST=Provided browser tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142918

[sky, 2012-05-08 23:44:46]

LGTM

[koz (OOO until 15th September), 2012-06-15 05:36:26]

This test was failing on linux before, but I've now fixed and updated the CL.
sky@ this could probably do with another look if you don't mind?

[sky, 2012-06-15 15:01:08]

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:160: fullscreened_tab_ =
TabContents::FromWebContents(web_contents);
Do we know when we get here fullscreened_tab_ is always NULL?

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:395: registrar_.Remove(this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
I think you should removeall here. Less error prone.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.h:44: // content::NotificationObserver
nit: in general we try to put overriden methods at the end of a section.

[scheib, 2012-06-15 16:12:31]

Thanks for this -- it's highly related to issues I was about to start for Mouse
Lock as well.  Also, CCing yzshen who is a fullscreen_controller master.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:879:
TestTabDoesntExitFullscreenOnSubFrameNavigation);
FullscreenControllerTest is already a friend, we shouldn't need line items for
every test. If there's any new use of private Browser members, consider placing
them in helper methods on FullscreenControllerTest. (Often simplifies those
tests for readability and likely get reused in other tests anyway).

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:170:
ExitCancelFullscreenOnNavigateMode();
Are you certain that we only want to be in CancelFullscreenOnNavigateMode if we
transition from non-fullscreen to tab-fullscreen? Why don't we want it if we
transition from browser-fullscreen to tab-fullscreen?

Perhaps this line should be after the } else { on line 188? (and the Enter call
on line 162 should be moved up?

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller_browsertest.cc (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_browsertest.cc:243:
tree_updated_observer1.Wait();
This block (and similar one in next test) can be replaced with:
ASSERT_NO_FATAL_FAILURE(ToggleTabFullscreen(fullscreen_tab, true));

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_browsertest.cc:251:
tree_updated_observer2.Wait();
Please refactor this block into a method on FullscreenControllerTest, similar to
ToggleTabFullscreen, to make this test easier to read and allow reuse of the
method. Now that you've done this the same should be done for mouse lock.
crbug.com/131702

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_browsertest.cc:276:
ASSERT_TRUE(browser()->window()->IsFullscreen());
If fullscreen were to change, it likely wouldn't have changed at this point. Can
we block waiting on something before ASSERTing? Perhaps on the navigation
notification. Hmm, really we need to wait even longer than that in the event
that the test receives it before the fullscreen controller.

[scheib, 2012-06-15 16:31:02]

CCing dyu as well, who has more tests in progress related to this
http://codereview.chromium.org/10535173/

[koz (OOO until 15th September), 2012-06-18 02:28:09]

Thanks for the reviews, guys!

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:879:
TestTabDoesntExitFullscreenOnSubFrameNavigation);
On 2012/06/15 16:12:31, scheib wrote:
> FullscreenControllerTest is already a friend, we shouldn't need line items for
> every test. If there's any new use of private Browser members, consider
placing
> them in helper methods on FullscreenControllerTest. (Often simplifies those
> tests for readability and likely get reused in other tests anyway).

Cool! Done.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:160: fullscreened_tab_ =
TabContents::FromWebContents(web_contents);
On 2012/06/15 15:01:08, sky wrote:
> Do we know when we get here fullscreened_tab_ is always NULL?

I'm not sure. scheib, or yzshen would probably be able to give a better answer.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:170:
ExitCancelFullscreenOnNavigateMode();
On 2012/06/15 16:12:31, scheib wrote:
> Are you certain that we only want to be in CancelFullscreenOnNavigateMode if
we
> transition from non-fullscreen to tab-fullscreen? Why don't we want it if we
> transition from browser-fullscreen to tab-fullscreen?
> 
> Perhaps this line should be after the } else { on line 188? (and the Enter
call
> on line 162 should be moved up?

Ah yes, good point. Wouldn't I want to move the
ExitCancelFullscreenOnNavigateMode() call to inside the } else { started on line
180? As it shouldn't matter if we are still in fullscreen mode or not, if we are
exiting tabbed fullscreen mode it should be cancelled?

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:395: registrar_.Remove(this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
On 2012/06/15 15:01:08, sky wrote:
> I think you should removeall here. Less error prone.

Done.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.h:44: // content::NotificationObserver
On 2012/06/15 15:01:08, sky wrote:
> nit: in general we try to put overriden methods at the end of a section.

Done.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller_browsertest.cc (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_browsertest.cc:243:
tree_updated_observer1.Wait();
On 2012/06/15 16:12:31, scheib wrote:
> This block (and similar one in next test) can be replaced with:
> ASSERT_NO_FATAL_FAILURE(ToggleTabFullscreen(fullscreen_tab, true));

Done.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_browsertest.cc:251:
tree_updated_observer2.Wait();
On 2012/06/15 16:12:31, scheib wrote:
> Please refactor this block into a method on FullscreenControllerTest, similar
to
> ToggleTabFullscreen, to make this test easier to read and allow reuse of the
> method. Now that you've done this the same should be done for mouse lock.
> crbug.com/131702

Done.

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_browsertest.cc:276:
ASSERT_TRUE(browser()->window()->IsFullscreen());
On 2012/06/15 16:12:31, scheib wrote:
> If fullscreen were to change, it likely wouldn't have changed at this point.
Can
> we block waiting on something before ASSERTing? Perhaps on the navigation
> notification. Hmm, really we need to wait even longer than that in the event
> that the test receives it before the fullscreen controller. 

NavigateToURL() blocks until the navigation is complete.

[scheib, 2012-06-18 03:58:09]

(will do a more detailed patch review Monday)

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:160: fullscreened_tab_ =
TabContents::FromWebContents(web_contents);
On 2012/06/18 02:28:10, koz wrote:
> On 2012/06/15 15:01:08, sky wrote:
> > Do we know when we get here fullscreened_tab_ is always NULL?
> 
> I'm not sure. scheib, or yzshen would probably be able to give a better
answer.

As far as I can tell by inspection last Friday: I wouldn't count on it. IIRC it
isn't cleared when exiting until at least the OS actually transitions the window
out of fullscreen and we get a notification of change later. Also, double check
the logic when exiting tab fullscreen when previously in browser fullscreen.

I don't follow why wky is concerned, though?

http://codereview.chromium.org/10378061/diff/13001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller.cc:170:
ExitCancelFullscreenOnNavigateMode();
On 2012/06/18 02:28:10, koz wrote:
> On 2012/06/15 16:12:31, scheib wrote:
> > Are you certain that we only want to be in CancelFullscreenOnNavigateMode if
> we
> > transition from non-fullscreen to tab-fullscreen? Why don't we want it if we
> > transition from browser-fullscreen to tab-fullscreen?
> > 
> > Perhaps this line should be after the } else { on line 188? (and the Enter
> call
> > on line 162 should be moved up?
> 
> Ah yes, good point. Wouldn't I want to move the
> ExitCancelFullscreenOnNavigateMode() call to inside the } else { started on
line
> 180? As it shouldn't matter if we are still in fullscreen mode or not, if we
are
> exiting tabbed fullscreen mode it should be cancelled?

Line 180 sounds right -- mis-reading of the } else { clauses wen writing my
previous comment.

[scheib, 2012-06-18 23:08:07]

lgtm

http://codereview.chromium.org/10378061/diff/24001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller_test.cc (right):

http://codereview.chromium.org/10378061/diff/24001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_test.cc:126:
observer.WaitForObservation(
Why not just observer.Wait()?

[koz (OOO until 15th September), 2012-06-19 00:45:55]

sky, still LGTY?

http://codereview.chromium.org/10378061/diff/24001/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen_controller_test.cc (right):

http://codereview.chromium.org/10378061/diff/24001/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen_controller_test.cc:126:
observer.WaitForObservation(
On 2012/06/18 23:08:07, scheib wrote:
> Why not just observer.Wait()?

Ah, that's much nicer! The dangers of cargo culting :-)

[sky, 2012-06-19 00:59:48]

yes, SLGTM

[commit-bot: I haz the power, 2012-06-19 01:01:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10378061/37001

[commit-bot: I haz the power, 2012-06-19 01:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10378061/37001

[M-A Ruel, 2012-06-19 01:37:39]

On 2012/06/19 01:35:10, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/koz%40chromium.org/10378061/37001

Sorry I confused the CQ, it should go properly. Ping me otherwise.

[scheib, 2012-06-19 02:41:20]

:( Reload test is flaky and cut? Can you put details onto
http://crbug.com/132669?

[koz (OOO until 15th September), 2012-06-19 03:07:39]

On 2012/06/19 02:41:20, scheib wrote:
> :( Reload test is flaky and cut? Can you put details onto
> http://crbug.com/132669?

Yep, done.

[commit-bot: I haz the power, 2012-06-19 03:54:52]

Try job failure for 10378061-37001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-19 03:58:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10378061/37001