Exit tabbed fullscreen mode on navigation.

Exit tabbed fullscreen mode on navigation.


BUG=97553
TEST=browsertest provided.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104692

[Peter Kasting, 2011-09-23 19:20:06]

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:308: registrar_.Add(this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
Why is this here when you have the more-specific one below?

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3823: registrar_.Remove(this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
Is this codepath guaranteed to be reached in all cases (e.g. if a tab is
closed)?  If there are any unmatched cases, you'll slowly build up bad entries
in the registrar, which could have strange consequences.

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4154: ExitTabbedFullscreenModeIfNecessary();
Are you sure this is what you want to do?  It seems like this will force tabs
out of fullscreen when e.g. they have an AUTO_SUBFRAME navigation -- that
doesn't seem right.

[koz (OOO until 15th September), 2011-09-26 08:15:21]

Thanks, Peter!

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:308: registrar_.Add(this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
On 2011/09/23 19:20:06, Peter Kasting wrote:
> Why is this here when you have the more-specific one below?

Oops, this was left over from previous iterations.
Removed.

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3823: registrar_.Remove(this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
On 2011/09/23 19:20:06, Peter Kasting wrote:
> Is this codepath guaranteed to be reached in all cases (e.g. if a tab is
> closed)?  If there are any unmatched cases, you'll slowly build up bad entries
> in the registrar, which could have strange consequences.

I think it is. It is equivalent to exiting tabbed fullscreen mode, so this will
get cleaned up any time that tabbed fullscreen gets exited.

http://codereview.chromium.org/7966031/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4154: ExitTabbedFullscreenModeIfNecessary();
On 2011/09/23 19:20:06, Peter Kasting wrote:
> Are you sure this is what you want to do?  It seems like this will force tabs
> out of fullscreen when e.g. they have an AUTO_SUBFRAME navigation -- that
> doesn't seem right.

I've added a check for NavigationTypes that should not cause tabbed fullscreen
to exit. Do they seem okay to you?

[Peter Kasting, 2011-09-26 20:24:58]

http://codereview.chromium.org/7966031/diff/7001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7966031/diff/7001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4156: lcd.type != NavigationType::NAV_IGNORE) {
This still seems suspicious to me.  Why aren't you using
is_navigation_to_different_page() instead?  This kind of case is what it was
created for, I think.  Otherwise we'll still get things like fragment
navigations wrong.

[koz (OOO until 15th September), 2011-09-26 23:55:57]

http://codereview.chromium.org/7966031/diff/7001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7966031/diff/7001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4156: lcd.type != NavigationType::NAV_IGNORE) {
On 2011/09/26 20:24:58, Peter Kasting wrote:
> This still seems suspicious to me.  Why aren't you using
> is_navigation_to_different_page() instead?  This kind of case is what it was
> created for, I think.  Otherwise we'll still get things like fragment
> navigations wrong.

I figured I'd err on the side of caution with fragment navigations as they might
be to fragments outside the fullscreened element. It should still work either
way though.

[Peter Kasting, 2011-09-26 23:58:49]

LGTM with added test

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4151: case
content::NOTIFICATION_NAV_ENTRY_COMMITTED: {
Nit: Add blank line above

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4152: const content::LoadCommittedDetails& lcd =
Nit: I'd just inline this into the next statement

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4154: if (lcd.is_navigation_to_different_page()) {
Nit: No {} necessary

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:830:
IN_PROC_BROWSER_TEST_F(BrowserTest, TestTabExitsFullscreenOnNavigation) {
I think we should also have a test that fullscreen mode is not canceled when we
perform a subframe navigation or similar.

[koz (OOO until 15th September), 2011-09-27 03:49:30]

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4151: case
content::NOTIFICATION_NAV_ENTRY_COMMITTED: {
On 2011/09/26 23:58:50, Peter Kasting wrote:
> Nit: Add blank line above

Done.

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4152: const content::LoadCommittedDetails& lcd =
On 2011/09/26 23:58:50, Peter Kasting wrote:
> Nit: I'd just inline this into the next statement

Hm, doing so pushes it over 80 chars, so I think I'll leave it like this.

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4154: if (lcd.is_navigation_to_different_page()) {
On 2011/09/26 23:58:50, Peter Kasting wrote:
> Nit: No {} necessary

Done.

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:830:
IN_PROC_BROWSER_TEST_F(BrowserTest, TestTabExitsFullscreenOnNavigation) {
On 2011/09/26 23:58:50, Peter Kasting wrote:
> I think we should also have a test that fullscreen mode is not canceled when
we
> perform a subframe navigation or similar.

Done.

[Peter Kasting, 2011-09-27 17:04:14]

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4152: const content::LoadCommittedDetails& lcd =
On 2011/09/27 03:49:30, koz wrote:
> On 2011/09/26 23:58:50, Peter Kasting wrote:
> > Nit: I'd just inline this into the next statement
> 
> Hm, doing so pushes it over 80 chars, so I think I'll leave it like this.

One nice thing about inlining is that you remove the * and the .ptr() bits:

      if (Details<content::LoadCommittedDetails>(details)->
          is_navigation_to_different_page())
        ExitTabbedFullscreenModeIfNecessary();

I find this easier to read.

[koz (OOO until 15th September), 2011-09-28 01:40:24]

On 2011/09/27 17:04:14, Peter Kasting wrote:
> http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
http://codereview.chromium.org/7966031/diff/11001/chrome/browser/ui/browser.c...
> chrome/browser/ui/browser.cc:4152: const content::LoadCommittedDetails& lcd =
> On 2011/09/27 03:49:30, koz wrote:
> > On 2011/09/26 23:58:50, Peter Kasting wrote:
> > > Nit: I'd just inline this into the next statement
> > 
> > Hm, doing so pushes it over 80 chars, so I think I'll leave it like this.
> 
> One nice thing about inlining is that you remove the * and the .ptr() bits:
> 
>       if (Details<content::LoadCommittedDetails>(details)->
>           is_navigation_to_different_page())
>         ExitTabbedFullscreenModeIfNecessary();
> 
> I find this easier to read.

Ah, I didn't realise that. You're right, that is nicer, cheers!

I've added the test, so will submit with this change.

[commit-bot: I haz the power, 2011-09-30 03:15:51]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7966031/22001

[commit-bot: I haz the power, 2011-09-30 06:45:19]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 103424, job name
was 7966031-22001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-01 05:33:40]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7966031/22001

[commit-bot: I haz the power, 2011-10-01 05:33:41]

Can't apply patch for file chrome/browser/ui/browser.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/browser.cc
Hunk #1 succeeded at 139 (offset 2 lines).
Hunk #2 succeeded at 1692 (offset 22 lines).
Hunk #3 FAILED at 3773.
Hunk #4 succeeded at 4149 (offset 51 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/browser.cc.rej

[commit-bot: I haz the power, 2011-10-04 13:34:33]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7966031/30001

[commit-bot: I haz the power, 2011-10-04 15:51:18]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 103889, job name
was 7966031-30001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-07 02:38:32]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7966031/30001

[commit-bot: I haz the power, 2011-10-07 03:41:09]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 104433, job name
was 7966031-30001 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-10-10 02:07:27]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7966031/30001

[commit-bot: I haz the power, 2011-10-10 04:26:23]

Change committed as 104692