Add FullscreenController::ToggleFullscreenModeWithExtension

Add FullscreenController::ToggleFullscreenModeWithExtension

Add a new method
Remove unused argument |for_tab|, it is only used inside fullscreen_controller.cc

BUG=112782
TEST=build success


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

[hashimoto, 2012-02-07 11:39:44]

This patch is separated from http://codereview.chromium.org/9328037/

[hashimoto, 2012-02-07 11:40:54]

+koz

[scheib, 2012-02-07 18:55:09]

http://codereview.chromium.org/9353010/diff/31/chrome/browser/ui/fullscreen_c...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/31/chrome/browser/ui/fullscreen_c...
chrome/browser/ui/fullscreen_controller.cc:204: GetFullscreenExitBubbleType();
Can this logic be kept inside GetFullscreenExitBubbleType()?

[koz (OOO until 15th September), 2012-02-08 00:06:22]

lgtm

[hashimoto, 2012-02-08 02:43:18]

http://codereview.chromium.org/9353010/diff/31/chrome/browser/ui/fullscreen_c...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/31/chrome/browser/ui/fullscreen_c...
chrome/browser/ui/fullscreen_controller.cc:204: GetFullscreenExitBubbleType();
On 2012/02/07 18:55:10, scheib wrote:
> Can this logic be kept inside GetFullscreenExitBubbleType()?

Done.

[scheib, 2012-02-08 03:44:11]

lgtm

[hashimoto, 2012-02-08 03:57:46]

Ben, could you review this change as an OWNER of browser/ui/ ?

Evan, could you review this change as an OWNER of browser/ui/gtk?

[yzshen1, 2012-02-08 18:05:26]

http://codereview.chromium.org/9353010/diff/6003/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/6003/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:116:
ToggleFullscreenModeInternal(true, NULL);
I think the following scenario doesn't work correctly:
1) an extension triggers fullscreen (Show: "some extension triggered full
screen.")
2) enter tab full screen (Show: "some website is now full screen.")
3) exit tab full screen (Note: at this point, it should show the same text as
step 1, instead of the text for user-triggered fullscreen.)

You need to remember whether an extension triggers the fullscreen, and show the
correct text. (With that information, you don't need to pass in a parameter to
GetFullscreenExitBubbleType.)

This page describes the state transition of the fullscreen exit bubble:
http://www.chromium.org/developers/design-documents/mouse-lock

[hashimoto, 2012-02-09 09:10:17]

http://codereview.chromium.org/9353010/diff/6003/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/6003/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_controller.cc:116:
ToggleFullscreenModeInternal(true, NULL);
On 2012/02/08 18:05:27, yzshen1 wrote:
> I think the following scenario doesn't work correctly:
> 1) an extension triggers fullscreen (Show: "some extension triggered full
> screen.")
> 2) enter tab full screen (Show: "some website is now full screen.")
> 3) exit tab full screen (Note: at this point, it should show the same text as
> step 1, instead of the text for user-triggered fullscreen.)
> 
> You need to remember whether an extension triggers the fullscreen, and show
the
> correct text. (With that information, you don't need to pass in a parameter to
> GetFullscreenExitBubbleType.)
> 
> This page describes the state transition of the fullscreen exit bubble:
> http://www.chromium.org/developers/design-documents/mouse-lock
Thank you for finding this.
Fixed by adding |extension_caused_fullscreen_|.

BTW, I found a bug where entering and exiting tab-fullscreen mode inside
browser-fullscreen mode does not work well. (filed in http://crbug.com/113349)
Is this a known bug?

[scheib, 2012-02-09 18:05:28]

lgtm

[Ben Goodger (Google), 2012-02-09 19:32:49]

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

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:540: void ToggleFullscreenModeWithExtension(const
Extension& extension);
Add a comment here that refers to the comment in fullscreen_controller that I
have asked you to write.

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:169: void
FullscreenController::ToggleFullscreenMode() {
In adding these functions you've made the order of functions in the .cc not
match the .h. Fix this, and make sure never to do this again in any CL that you
write in Chrome code from now on.

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:57: void
ToggleFullscreenModeWithExtension(const Extension& extension);
It's not clear to me what it means to toggle fullscreen mode "with an
extension". When something is non-obvious you should always add documentation.
You should note why someone would call this function.

[hashimoto, 2012-02-10 05:09:32]

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

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:540: void ToggleFullscreenModeWithExtension(const
Extension& extension);
On 2012/02/09 19:32:49, Ben Goodger (Google) wrote:
> Add a comment here that refers to the comment in fullscreen_controller that I
> have asked you to write.

Done.

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:169: void
FullscreenController::ToggleFullscreenMode() {
On 2012/02/09 19:32:49, Ben Goodger (Google) wrote:
> In adding these functions you've made the order of functions in the .cc not
> match the .h. Fix this, and make sure never to do this again in any CL that
you
> write in Chrome code from now on.

Sorry, fixed.

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/9353010/diff/16005/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:57: void
ToggleFullscreenModeWithExtension(const Extension& extension);
On 2012/02/09 19:32:49, Ben Goodger (Google) wrote:
> It's not clear to me what it means to toggle fullscreen mode "with an
> extension". When something is non-obvious you should always add documentation.
> You should note why someone would call this function.

Done.

[yzshen1, 2012-02-10 21:24:49]

Mostly looks good. Thanks!

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:161: extension_caused_fullscreen_ =
&extension;
[nit, optional]
Please comment that if this causes fullscreen to exit, we will reset
extension_caused_fullscreen_ to NULL in ToggleFullscreenModeInternal().

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:291: url =
extension_caused_fullscreen_->url();
What is the lifespan of extension_caused_fullscreen_? Can we guarantee that it
won't die while FullscreenController holds a pointer to it?

[hashimoto, 2012-02-13 07:43:44]

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:161: extension_caused_fullscreen_ =
&extension;
On 2012/02/10 21:24:49, yzshen1 wrote:
> [nit, optional]
> Please comment that if this causes fullscreen to exit, we will reset
> extension_caused_fullscreen_ to NULL in ToggleFullscreenModeInternal().

Done.

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:291: url =
extension_caused_fullscreen_->url();
On 2012/02/10 21:24:49, yzshen1 wrote:
> What is the lifespan of extension_caused_fullscreen_? Can we guarantee that it
> won't die while FullscreenController holds a pointer to it?

Good point, thank you for pointing this out.
If the user uninstalls an extension when the browser is in fullscreen caused by
the extension, it crashes on UpdateFullscreenExitBubbleContent.

To avoid crash, I changed the type of |extension_caused_fullscreen_| from
Extension* to GURL.
By doing this, "<Extension ID> triggered full screen" is shown when the
extension is already uninstalled. (<Extension ID> is a string like
"lcigcdppmkbhecgefjelmcfkcgfonkdk")

Showing cryptic message is not a good thing, but I think this is acceptable
because this use case will not be so common.
If this is not OK, maybe we should show something like "You are still in full
screen" after exiting from tab-fullscreen?

[yzshen1, 2012-02-13 18:38:48]

lgtm

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.cc (right):

http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.cc:291: url =
extension_caused_fullscreen_->url();
On 2012/02/13 07:43:44, hashimoto wrote:
> On 2012/02/10 21:24:49, yzshen1 wrote:
> > What is the lifespan of extension_caused_fullscreen_? Can we guarantee that
it
> > won't die while FullscreenController holds a pointer to it?
> 
> Good point, thank you for pointing this out.
> If the user uninstalls an extension when the browser is in fullscreen caused
by
> the extension, it crashes on UpdateFullscreenExitBubbleContent.
> 
> To avoid crash, I changed the type of |extension_caused_fullscreen_| from
> Extension* to GURL.
> By doing this, "<Extension ID> triggered full screen" is shown when the
> extension is already uninstalled. (<Extension ID> is a string like
> "lcigcdppmkbhecgefjelmcfkcgfonkdk")
It seems fine to me.

Just out of curiosity: According to your description, if the extension doesn't
get uninstalled while in the fullscreen mode, the text displayed __won't__ be
"<Extension ID> triggered full screen". Is that what you mean? But I cannot tell
from the code why the text displayed will be different. Am I missing anything?


> 
> Showing cryptic message is not a good thing, but I think this is acceptable
> because this use case will not be so common.
> If this is not OK, maybe we should show something like "You are still in full
> screen" after exiting from tab-fullscreen?

[Evan Stade, 2012-02-13 20:08:00]

i'm guessing you only want me to review the gtk change. That lgtm

http://codereview.chromium.org/9353010/diff/30001/chrome/browser/ui/gtk/fulls...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/9353010/diff/30001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:62:
FEB_TYPE_BROWSER_EXTENSION_FULLSCREEN_EXIT_INSTRUCTION) {
4 more space of indent

[hashimoto, 2012-02-14 00:59:55]

Ben, can I have your OWNER LGTM for browser/ui?

http://codereview.chromium.org/9353010/diff/30001/chrome/browser/ui/gtk/fulls...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/9353010/diff/30001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:62:
FEB_TYPE_BROWSER_EXTENSION_FULLSCREEN_EXIT_INSTRUCTION) {
On 2012/02/13 20:08:00, Evan Stade wrote:
> 4 more space of indent

Done.

[hashimoto, 2012-02-14 01:04:36]

On 2012/02/13 18:38:48, yzshen1 wrote:
> lgtm
> 
>
http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
> File chrome/browser/ui/fullscreen_controller.cc (right):
> 
>
http://codereview.chromium.org/9353010/diff/15024/chrome/browser/ui/fullscree...
> chrome/browser/ui/fullscreen_controller.cc:291: url =
> extension_caused_fullscreen_->url();
> On 2012/02/13 07:43:44, hashimoto wrote:
> > On 2012/02/10 21:24:49, yzshen1 wrote:
> > > What is the lifespan of extension_caused_fullscreen_? Can we guarantee
that
> it
> > > won't die while FullscreenController holds a pointer to it?
> > 
> > Good point, thank you for pointing this out.
> > If the user uninstalls an extension when the browser is in fullscreen caused
> by
> > the extension, it crashes on UpdateFullscreenExitBubbleContent.
> > 
> > To avoid crash, I changed the type of |extension_caused_fullscreen_| from
> > Extension* to GURL.
> > By doing this, "<Extension ID> triggered full screen" is shown when the
> > extension is already uninstalled. (<Extension ID> is a string like
> > "lcigcdppmkbhecgefjelmcfkcgfonkdk")
> It seems fine to me.
> 
> Just out of curiosity: According to your description, if the extension doesn't
> get uninstalled while in the fullscreen mode, the text displayed __won't__ be
> "<Extension ID> triggered full screen". Is that what you mean? 
Right.

> But I cannot tell
> from the code why the text displayed will be different. Am I missing anything?
GetLabelTextForType in fullscreen_exit_bubble_type.cc uses the extension's name
as |host|, but when it cannot find the extension, it uses the host of
extension's URL instead (i.e. extension ID).
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/fullscree...
> 
> 
> > 
> > Showing cryptic message is not a good thing, but I think this is acceptable
> > because this use case will not be so common.
> > If this is not OK, maybe we should show something like "You are still in
full
> > screen" after exiting from tab-fullscreen?

[yzshen1, 2012-02-14 01:09:48]

LGTM

> GetLabelTextForType in fullscreen_exit_bubble_type.cc uses the extension's
name
> as |host|, but when it cannot find the extension, it uses the host of
> extension's URL instead (i.e. extension ID).
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/fullscree...

I see. Thanks!

[Ben Goodger (Google), 2012-02-14 19:01:15]

Can you please email me a screenshot of what the full screen exit bubble looks
like when it shows up for an extension? Thanks.

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/med...
File chrome/browser/chromeos/media/media_player.cc (right):

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/med...
chrome/browser/chromeos/media/media_player.cc:179: if (mediaplayer_browser_) {
nit, while you're here, no braces around single line if.

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:57: // Extension API implementation
uses this method to toggle fullscreen mode.
Add:
// The extension's URL is displayed in the full screen bubble UI to attribute
the cause of the full screen state change.

[scheib, 2012-02-14 19:08:13]

Please update http://www.chromium.org/developers/design-documents/mouse-lock
which lists the permutations of strings displayed.

[hashimoto, 2012-02-15 00:30:01]

On Wed, Feb 15, 2012 at 4:01 AM, <ben@chromium.org> wrote:

> Can you please email me a screenshot of what the full screen exit bubble
> looks
> like when it shows up for an extension? Thanks.
>
Attached are the images, normal.png is when entering 'browser full screen'
with extension API.
deleted.png is when exiting 'tab full screen' after entering 'browser full
screen' with extension API and deleting the extension (it seems to be very
rare use case).

Sorry for 'exit full screen' shown in Japanese language because of my UI
settings.


>
> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>
browser/chromeos/media/media_**player.cc<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc>
> File chrome/browser/chromeos/media/**media_player.cc (right):
>
> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>
browser/chromeos/media/media_**player.cc#newcode179<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc#newcode179>
> chrome/browser/chromeos/media/**media_player.cc:179: if
> (mediaplayer_browser_) {
> nit, while you're here, no braces around single line if.
>
> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>
browser/ui/fullscreen_**controller.h<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h>
> File chrome/browser/ui/fullscreen_**controller.h (right):
>
> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>
browser/ui/fullscreen_**controller.h#newcode57<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h#newcode57>
> chrome/browser/ui/fullscreen_**controller.h:57: // Extension API
> implementation uses this method to toggle fullscreen mode.
> Add:
> // The extension's URL is displayed in the full screen bubble UI to
> attribute the cause of the full screen state change.
>
>
http://codereview.chromium.**org/9353010/<http://codereview.chromium.org/9353...
>

[hashimoto, 2012-02-15 01:40:42]

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/med...
File chrome/browser/chromeos/media/media_player.cc (right):

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/med...
chrome/browser/chromeos/media/media_player.cc:179: if (mediaplayer_browser_) {
On 2012/02/14 19:01:16, Ben Goodger (Google) wrote:
> nit, while you're here, no braces around single line if.

Done.

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_controller.h (right):

http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_controller.h:57: // Extension API implementation
uses this method to toggle fullscreen mode.
On 2012/02/14 19:01:16, Ben Goodger (Google) wrote:
> Add:
> // The extension's URL is displayed in the full screen bubble UI to attribute
> the cause of the full screen state change.

Done.

[hashimoto, 2012-02-15 01:42:24]

On 2012/02/14 19:08:13, scheib wrote:
> Please update http://www.chromium.org/developers/design-documents/mouse-lock
> which lists the permutations of strings displayed.

Looks like the spreadsheet cannot be edited by my chromium account.

[hashimoto, 2012-02-15 01:46:49]

thakis,
Can I have your OWNER LGTM about tiny change on
chrome/browser/ui/cocoa/browser_window_cocoa.mm?

[Nico, 2012-02-15 01:47:58]

cocoa lgtm stamp

[scheib, 2012-02-15 05:29:29]

On 2012/02/15 01:42:24, hashimoto wrote:
> On 2012/02/14 19:08:13, scheib wrote:
> > Please update http://www.chromium.org/developers/design-documents/mouse-lock
> > which lists the permutations of strings displayed.
> 
> Looks like the spreadsheet cannot be edited by my chromium account.

I think it would be valuable to document on that page the new type of string
that can be displayed, and how it may appear if the extension is deleted.

The spreadsheet was good for initial implementation planning, but at this point
I wonder if we should update it -- since it's structure varies from the
implementation and also doesn't capture the events that happen on state
transitions. I've given your account edit permission and leave it to you to
decide to update it or to mark it as out of date (ok to strip from that page).

[hashimoto, 2012-02-15 10:41:10]

On 2012/02/15 05:29:29, scheib wrote:
> On 2012/02/15 01:42:24, hashimoto wrote:
> > On 2012/02/14 19:08:13, scheib wrote:
> > > Please update
http://www.chromium.org/developers/design-documents/mouse-lock
> > > which lists the permutations of strings displayed.
> > 
> > Looks like the spreadsheet cannot be edited by my chromium account.
> 
> I think it would be valuable to document on that page the new type of string
> that can be displayed, and how it may appear if the extension is deleted.
> 
> The spreadsheet was good for initial implementation planning, but at this
point
> I wonder if we should update it -- since it's structure varies from the
> implementation and also doesn't capture the events that happen on state
> transitions. I've given your account edit permission and leave it to you to
> decide to update it or to mark it as out of date (ok to strip from that page).

Added strings to the page and marked the sheet as out of date.

[Ben Goodger (Google), 2012-02-15 22:19:40]

Can you show something more useful than the extension id?

jklsdfjklsdfjklsdfjklsdfjklsdfjklsdfjklsdf triggered fullscreen is a
nonsensical message.

-Ben

On Tue, Feb 14, 2012 at 4:29 PM, Ryo Hashimoto <hashimoto@chromium.org>wrote:

> On Wed, Feb 15, 2012 at 4:01 AM, <ben@chromium.org> wrote:
>
>> Can you please email me a screenshot of what the full screen exit bubble
>> looks
>> like when it shows up for an extension? Thanks.
>>
> Attached are the images, normal.png is when entering 'browser full screen'
> with extension API.
> deleted.png is when exiting 'tab full screen' after entering 'browser full
> screen' with extension API and deleting the extension (it seems to be very
> rare use case).
>
> Sorry for 'exit full screen' shown in Japanese language because of my UI
> settings.
>
>
>>
>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>
browser/chromeos/media/media_**player.cc<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc>
>> File chrome/browser/chromeos/media/**media_player.cc (right):
>>
>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>
browser/chromeos/media/media_**player.cc#newcode179<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc#newcode179>
>> chrome/browser/chromeos/media/**media_player.cc:179: if
>> (mediaplayer_browser_) {
>> nit, while you're here, no braces around single line if.
>>
>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>
browser/ui/fullscreen_**controller.h<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h>
>> File chrome/browser/ui/fullscreen_**controller.h (right):
>>
>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>
browser/ui/fullscreen_**controller.h#newcode57<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h#newcode57>
>> chrome/browser/ui/fullscreen_**controller.h:57: // Extension API
>> implementation uses this method to toggle fullscreen mode.
>> Add:
>> // The extension's URL is displayed in the full screen bubble UI to
>> attribute the cause of the full screen state change.
>>
>>
http://codereview.chromium.**org/9353010/<http://codereview.chromium.org/9353...
>>
>
>

[hashimoto, 2012-02-15 23:08:55]

Extension IDs are shown only when
1. An extension triggers fullscreen.
2. User uninstalls the extension.
3. A tab enters fullscreen (e.g. fullscreen <video>).
4. A tab exits fullscreen.
to tell the user that the browser window is still fullscreen even after the
tab fullscreen exits.

We can show something like "You are still in full screen" instead of the
message with extension ID, but I think this case happens very rarely.
 2012/02/16 7:19 "Ben Goodger (Google)" <ben@chromium.org>:

> Can you show something more useful than the extension id?
>
> jklsdfjklsdfjklsdfjklsdfjklsdfjklsdfjklsdf triggered fullscreen is a
> nonsensical message.
>
> -Ben
>
> On Tue, Feb 14, 2012 at 4:29 PM, Ryo Hashimoto <hashimoto@chromium.org>wrote:
>
>> On Wed, Feb 15, 2012 at 4:01 AM, <ben@chromium.org> wrote:
>>
>>> Can you please email me a screenshot of what the full screen exit bubble
>>> looks
>>> like when it shows up for an extension? Thanks.
>>>
>> Attached are the images, normal.png is when entering 'browser full
>> screen' with extension API.
>> deleted.png is when exiting 'tab full screen' after entering 'browser
>> full screen' with extension API and deleting the extension (it seems to be
>> very rare use case).
>>
>> Sorry for 'exit full screen' shown in Japanese language because of my UI
>> settings.
>>
>>
>>>
>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>
browser/chromeos/media/media_**player.cc<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc>
>>> File chrome/browser/chromeos/media/**media_player.cc (right):
>>>
>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>
browser/chromeos/media/media_**player.cc#newcode179<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc#newcode179>
>>> chrome/browser/chromeos/media/**media_player.cc:179: if
>>> (mediaplayer_browser_) {
>>> nit, while you're here, no braces around single line if.
>>>
>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>
browser/ui/fullscreen_**controller.h<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h>
>>> File chrome/browser/ui/fullscreen_**controller.h (right):
>>>
>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>
browser/ui/fullscreen_**controller.h#newcode57<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h#newcode57>
>>> chrome/browser/ui/fullscreen_**controller.h:57: // Extension API
>>> implementation uses this method to toggle fullscreen mode.
>>> Add:
>>> // The extension's URL is displayed in the full screen bubble UI to
>>> attribute the cause of the full screen state change.
>>>
>>>
http://codereview.chromium.**org/9353010/<http://codereview.chromium.org/9353...
>>>
>>
>>
>

[Ben Goodger (Google), 2012-02-15 23:11:59]

I want to repeat, you are showing this:

sdfjkljkljklfjklsdfjklsdfsdfjkl triggered full screen.

This is never acceptable under any circumstances. How am I supposed to know
what jkl;jkl;fjklofjklsdfjklsdf is?

Since you have the id, you should be able to fish out the name of the
extension and use that instead:

"GoatTeleporter Pro triggered full screen"

if that is not possible then

"An extension triggered full screen"

would be better than what you have.

-Ben

On Wed, Feb 15, 2012 at 3:08 PM, Ryo Hashimoto <hashimoto@chromium.org>wrote:

> Extension IDs are shown only when
> 1. An extension triggers fullscreen.
> 2. User uninstalls the extension.
> 3. A tab enters fullscreen (e.g. fullscreen <video>).
> 4. A tab exits fullscreen.
> to tell the user that the browser window is still fullscreen even after
> the tab fullscreen exits.
>
> We can show something like "You are still in full screen" instead of the
> message with extension ID, but I think this case happens very rarely.
>  2012/02/16 7:19 "Ben Goodger (Google)" <ben@chromium.org>:
>
> Can you show something more useful than the extension id?
>>
>> jklsdfjklsdfjklsdfjklsdfjklsdfjklsdfjklsdf triggered fullscreen is a
>> nonsensical message.
>>
>> -Ben
>>
>> On Tue, Feb 14, 2012 at 4:29 PM, Ryo Hashimoto <hashimoto@chromium.org>wrote:
>>
>>> On Wed, Feb 15, 2012 at 4:01 AM, <ben@chromium.org> wrote:
>>>
>>>> Can you please email me a screenshot of what the full screen exit
>>>> bubble looks
>>>> like when it shows up for an extension? Thanks.
>>>>
>>> Attached are the images, normal.png is when entering 'browser full
>>> screen' with extension API.
>>> deleted.png is when exiting 'tab full screen' after entering 'browser
>>> full screen' with extension API and deleting the extension (it seems to be
>>> very rare use case).
>>>
>>> Sorry for 'exit full screen' shown in Japanese language because of my UI
>>> settings.
>>>
>>>
>>>>
>>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>>
browser/chromeos/media/media_**player.cc<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc>
>>>> File chrome/browser/chromeos/media/**media_player.cc (right):
>>>>
>>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>>
browser/chromeos/media/media_**player.cc#newcode179<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/chromeos/media/media_player.cc#newcode179>
>>>> chrome/browser/chromeos/media/**media_player.cc:179: if
>>>> (mediaplayer_browser_) {
>>>> nit, while you're here, no braces around single line if.
>>>>
>>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>>
browser/ui/fullscreen_**controller.h<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h>
>>>> File chrome/browser/ui/fullscreen_**controller.h (right):
>>>>
>>>> http://codereview.chromium.**org/9353010/diff/35002/chrome/**
>>>>
browser/ui/fullscreen_**controller.h#newcode57<http://codereview.chromium.org/9353010/diff/35002/chrome/browser/ui/fullscreen_controller.h#newcode57>
>>>> chrome/browser/ui/fullscreen_**controller.h:57: // Extension API
>>>> implementation uses this method to toggle fullscreen mode.
>>>> Add:
>>>> // The extension's URL is displayed in the full screen bubble UI to
>>>> attribute the cause of the full screen state change.
>>>>
>>>>
http://codereview.chromium.**org/9353010/<http://codereview.chromium.org/9353...
>>>>
>>>
>>>
>>

[hashimoto, 2012-02-20 09:27:30]

New patch set, PTAL.
Cryptic extension ID is no longer shown.

[scheib, 2012-02-20 23:47:17]

On 2012/02/20 09:27:30, hashimoto wrote:
> New patch set, PTAL.
> Cryptic extension ID is no longer shown.

lgtm, please update design doc page.

[hashimoto, 2012-02-21 02:24:49]

On 2012/02/20 23:47:17, scheib wrote:
> On 2012/02/20 09:27:30, hashimoto wrote:
> > New patch set, PTAL.
> > Cryptic extension ID is no longer shown.
> 
> lgtm, please update design doc page.

Done.

beng, could you take a look on this change?

[Ben Goodger (Google), 2012-02-21 15:36:11]

What do you show now?

-Ben

On Mon, Feb 20, 2012 at 6:24 PM, <hashimoto@chromium.org> wrote:

> On 2012/02/20 23:47:17, scheib wrote:
>
>> On 2012/02/20 09:27:30, hashimoto wrote:
>> > New patch set, PTAL.
>> > Cryptic extension ID is no longer shown.
>>
>
>  lgtm, please update design doc page.
>>
>
> Done.
>
> beng, could you take a look on this change?
>
>
http://codereview.chromium.**org/9353010/<http://codereview.chromium.org/9353...
>

[Evan Stade, 2012-02-21 22:00:06]

On 2012/02/21 15:36:11, Ben Goodger (Google) wrote:
> What do you show now?

"An extension triggered full screen."

> 
> -Ben
> 
> On Mon, Feb 20, 2012 at 6:24 PM, <mailto:hashimoto@chromium.org> wrote:
> 
> > On 2012/02/20 23:47:17, scheib wrote:
> >
> >> On 2012/02/20 09:27:30, hashimoto wrote:
> >> > New patch set, PTAL.
> >> > Cryptic extension ID is no longer shown.
> >>
> >
> >  lgtm, please update design doc page.
> >>
> >
> > Done.
> >
> > beng, could you take a look on this change?
> >
> >
>
http://codereview.chromium.**org/9353010/%3Chttp://codereview.chromium.org/93...>
> >

[Ben Goodger (Google), 2012-02-21 22:40:58]

lgtm

[commit-bot: I haz the power, 2012-02-22 02:19:40]

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

[commit-bot: I haz the power, 2012-02-22 04:46:19]

Change committed as 122969