Add fullscreen state support for chrome.windows

Add fullscreen state support for chrome.windows

BUG=112782
TEST=ExtensionApiTest.UpdateWindowShowState


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

[Mihai Parparita -not on Chrome, 2012-02-06 22:31:34]

Jenn: See the question below about the additional window state value and whether
that would break any existing users of the API.

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_util.cc (right):

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_util.cc:73: return
keys::kShowStateValueFullscreen;
Aaron: are we worried about this change not being backwards compatible (current
extensions would not be checking for the "fullscreen" enum value)? IMO, it seems
unlikely to break anything; the state property was added pretty recently
(http://crrev.com/111071) for Panels/Quasar, and we can check with them to see
if this breaks anything.

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:620:
browser->window()->EnterFullscreen(
Aaron: should the ability to go fullscreen be tied to a permission (even if we
don't prompt for it)?

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:621: GURL(),
FEB_TYPE_BROWSER_FULLSCREEN_EXIT_INSTRUCTION);
This ends up displaying the "You have gone full screen" message, which doesn't
seem quite right, since it wasn't necessarily the user that triggered the
extension calling chrome.windows.update. Perhaps there should be a separate
message that mentions that the extension/app triggered full screen?

[jennb, 2012-02-06 23:34:25]

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_util.cc (right):

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_util.cc:73: return
keys::kShowStateValueFullscreen;
On 2012/02/06 22:31:34, Mihai Parparita wrote:
> Aaron: are we worried about this change not being backwards compatible
(current
> extensions would not be checking for the "fullscreen" enum value)? IMO, it
seems
> unlikely to break anything; the state property was added pretty recently
> (http://crrev.com/111071) for Panels/Quasar, and we can check with them to see
> if this breaks anything.

Panels cannot go fullscreen so there's no backwards compatibility issues on our
end.

http://codereview.chromium.org/9328037/diff/6009/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/window_update/show_state/test.js
(right):

http://codereview.chromium.org/9328037/diff/6009/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/window_update/show_state/test.js:42:
pass(checkRestoreWithBounds));
Instead of having this special test path for panels, what about allowing the
test to always set fullscreen as the next step, even on a panel. Then, special
case the verification to check that panel does not become fullscreen?

[hashimoto, 2012-02-07 02:16:00]

Fixed the test and the message, PTAL.

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9328037/diff/6009/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:621: GURL(),
FEB_TYPE_BROWSER_FULLSCREEN_EXIT_INSTRUCTION);
On 2012/02/06 22:31:34, Mihai Parparita wrote:
> This ends up displaying the "You have gone full screen" message, which doesn't
> seem quite right, since it wasn't necessarily the user that triggered the
> extension calling chrome.windows.update. Perhaps there should be a separate
> message that mentions that the extension/app triggered full screen?

Done.
The message is now "<Extension Name> triggered full screen."

http://codereview.chromium.org/9328037/diff/6009/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/window_update/show_state/test.js
(right):

http://codereview.chromium.org/9328037/diff/6009/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/window_update/show_state/test.js:42:
pass(checkRestoreWithBounds));
On 2012/02/06 23:34:25, jennb wrote:
> Instead of having this special test path for panels, what about allowing the
> test to always set fullscreen as the next step, even on a panel. Then, special
> case the verification to check that panel does not become fullscreen?

Done.

I made no assertion for panels because Panel::Enter/ExitFullscreen is
NOTIMPLEMENTED now.

[hashimoto, 2012-02-07 02:18:34]

yzshen,

Could you review my changes for fullscreen_exit_bubble_type.h/cc?

[yzshen1, 2012-02-07 07:57:22]

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:621:
browser->window()->EnterFullscreen(
I am a little bit surprised that this doesn't need to talk with
fullscreen_controller. Are you sure that this is intended?

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble_type.cc (right):

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble_type.cc:70:
IDS_FULLSCREEN_SITE_ENTERED_MOUSELOCK, host);
Now that you are changing this file, would you please fix the indent of line 58,
61, 64, 70?

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble_type.h (right):

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble_type.h:32:
FEB_TYPE_BROWSER_EXTENSION_TRIGGERED_FULLSCREEN_EXIT_INSTRUCTION
nit: If possible, please pick a shorter name. :)

[hashimoto, 2012-02-07 11:53:17]

New patch set.
Separated FullscreenController related stuff to
http://codereview.chromium.org/9353010/

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:621:
browser->window()->EnterFullscreen(
On 2012/02/07 07:57:22, yzshen1 wrote:
> I am a little bit surprised that this doesn't need to talk with
> fullscreen_controller. Are you sure that this is intended?

I thought calling Enter/ExitFullscreen is enough but using FulscreenController
is more consistent with other code.
Made a separated patch, PTAL http://codereview.chromium.org/9353010/

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble_type.cc (right):

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble_type.cc:70:
IDS_FULLSCREEN_SITE_ENTERED_MOUSELOCK, host);
On 2012/02/07 07:57:22, yzshen1 wrote:
> Now that you are changing this file, would you please fix the indent of line
58,
> 61, 64, 70?

Done in http://codereview.chromium.org/9353010/

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble_type.h (right):

http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble_type.h:32:
FEB_TYPE_BROWSER_EXTENSION_TRIGGERED_FULLSCREEN_EXIT_INSTRUCTION
On 2012/02/07 07:57:22, yzshen1 wrote:
> nit: If possible, please pick a shorter name. :)

Done in http://codereview.chromium.org/9353010/

http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/window_update/show_state/test.js
(right):

http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/window_update/show_state/test.js:22:
chrome.test.assertEq('fullscreen', theWindow.state);
Not checking size here because BrowserWindowCocoa returns wrong size when full
screen (filed in crbug.com/112984)

[yzshen1, 2012-02-07 18:32:46]

Thanks!
Since you created a new CL and moved things that I am interested in there, I
will leave this CL to other reviewers.

On 2012/02/07 11:53:17, hashimoto wrote:
> New patch set.
> Separated FullscreenController related stuff to
> http://codereview.chromium.org/9353010/
> 
>
http://codereview.chromium.org/9328037/diff/12002/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_tabs_module.cc (right):
> 
>
http://codereview.chromium.org/9328037/diff/12002/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_tabs_module.cc:621:
> browser->window()->EnterFullscreen(
> On 2012/02/07 07:57:22, yzshen1 wrote:
> > I am a little bit surprised that this doesn't need to talk with
> > fullscreen_controller. Are you sure that this is intended?
> 
> I thought calling Enter/ExitFullscreen is enough but using FulscreenController
> is more consistent with other code.
> Made a separated patch, PTAL http://codereview.chromium.org/9353010/
> 
>
http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
> File chrome/browser/ui/fullscreen_exit_bubble_type.cc (right):
> 
>
http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
> chrome/browser/ui/fullscreen_exit_bubble_type.cc:70:
> IDS_FULLSCREEN_SITE_ENTERED_MOUSELOCK, host);
> On 2012/02/07 07:57:22, yzshen1 wrote:
> > Now that you are changing this file, would you please fix the indent of line
> 58,
> > 61, 64, 70?
> 
> Done in http://codereview.chromium.org/9353010/
> 
>
http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
> File chrome/browser/ui/fullscreen_exit_bubble_type.h (right):
> 
>
http://codereview.chromium.org/9328037/diff/12002/chrome/browser/ui/fullscree...
> chrome/browser/ui/fullscreen_exit_bubble_type.h:32:
> FEB_TYPE_BROWSER_EXTENSION_TRIGGERED_FULLSCREEN_EXIT_INSTRUCTION
> On 2012/02/07 07:57:22, yzshen1 wrote:
> > nit: If possible, please pick a shorter name. :)
> 
> Done in http://codereview.chromium.org/9353010/
> 
>
http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
> File chrome/test/data/extensions/api_test/window_update/show_state/test.js
> (right):
> 
>
http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
> chrome/test/data/extensions/api_test/window_update/show_state/test.js:22:
> chrome.test.assertEq('fullscreen', theWindow.state);
> Not checking size here because BrowserWindowCocoa returns wrong size when full
> screen (filed in crbug.com/112984)

[jennb, 2012-02-07 18:41:37]

http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/window_update/show_state/test.js
(right):

http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/window_update/show_state/test.js:21: if
(theWindow.type != 'panel')
Can you set the panel case to check that the state is still 'normal' ? That's a
safe assertion for now and will be a good reminder to update this test if we
ever change the panel code to allow fullscreen change.

[hashimoto, 2012-02-08 04:22:35]

http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/window_update/show_state/test.js
(right):

http://codereview.chromium.org/9328037/diff/16002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/window_update/show_state/test.js:21: if
(theWindow.type != 'panel')
On 2012/02/07 18:41:38, jennb wrote:
> Can you set the panel case to check that the state is still 'normal' ? That's
a
> safe assertion for now and will be a good reminder to update this test if we
> ever change the panel code to allow fullscreen change.

Done.

[Mihai Parparita -not on Chrome, 2012-02-08 05:59:26]

LGTM, but please wait for Aaron's thoughts on permissions.

[hashimoto, 2012-02-10 14:23:26]

On 2012/02/08 05:59:26, Mihai Parparita wrote:
> LGTM, but please wait for Aaron's thoughts on permissions.

Any update?

[Aaron Boodman, 2012-02-10 23:38:19]

On 2012/02/10 14:23:26, hashimoto wrote:
> On 2012/02/08 05:59:26, Mihai Parparita wrote:
> > LGTM, but please wait for Aaron's thoughts on permissions.
> 
> Any update?

No permission is required for this.

LGTM

[commit-bot: I haz the power, 2012-02-22 06:06:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9328037/29010

[commit-bot: I haz the power, 2012-02-22 07:22:33]

Change committed as 122995