Embed Flash Fullscreen widget within browser window.

Embed Flash Fullscreen widget within browser window.

This provides users with a single, consistent fullscreen-mode experience, with Flash fullscreen gaining the improved window/desktop management behaviors of HTML5 fullscreen.  This is accomplished by: 1) Modifying the browser window to insert Flash fullscreen widgets into the view hierarchy in response to "did show" notifications from WebContentsImpl; 2) Toggling browser window expansion (and user balloon pop-ups) via FullscreenController, the existing mechanism already used for HTML5 and F11 fullscreen.

Safety: This enhancement is disabled by default, requiring an --embed-flash-fullscreen command-line flag to be activated.

Testing: Confirmed Flash fullscreen works both the old way (as a separate, raw fullscreen window) and the new way (embedded within the browser window).  Tested all fullscreen transitions: in-and-out, between browser F11 mode and Flash fullscreen mode, using the keyboard vs. mouse, "ALT-Tab" by the user and other window/desktop management modes, exit from balloon pop-up vs. plugin, and so on.  For Flash compatibility, used IPC logging on Aura and Mac builds to confirm IPC messaging to the renderer process is identical to the old way.

Left to do: 1) GTK support.  2) Mouse-lock support.

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

[miu, 2013-09-10 03:15:58]

Hi all,

Please review my code.  Suggested focus areas:

hubbe: General code review, please.

jam: content/public/browser/web_contents* (WebContents interface changes)

sail: chrome/browser/ui/cocoa/* (Mac browser UI changes)

sky: chrome/browser/ui/browser*, chrome/browser/ui/views/*, and ui/views/*
(non-Mac browser UI 
changes); and content/browser/* (RVH and WebContents changes)

yzshen: chrome/browser/ui/fullscreen/* (FullscreenController changes)

Thanks,
Yuri

[miu, 2013-09-10 03:17:31]

Link to chrome-eng-review@ proposal:
https://docs.google.com/a/google.com/document/d/1JETVpLkkANZ0huHQAZfkhMWYntrI...

[sky, 2013-09-10 16:56:15]

Stupid question, did eng-review approve the direction?

On Mon, Sep 9, 2013 at 8:17 PM,  <miu@chromium.org> wrote:
> Link to chrome-eng-review@ proposal:
>
https://docs.google.com/a/google.com/document/d/1JETVpLkkANZ0huHQAZfkhMWYntrI...
>
>
> https://codereview.chromium.org/23477051/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sail, 2013-09-10 17:37:18]

+avi how might be better to review the cocoa code
CC rsesek who's also working on fullscreen stuff

[miu, 2013-09-10 18:38:33]

On 2013/09/10 16:56:15, sky wrote:
> Stupid question, did eng-review approve the direction?

Yes.  Darin sent back an e-mail with the go-ahead last week.

[Avi (use Gerrit), 2013-09-10 18:42:10]

Robert last did fullscreen work, so he should look at this.

[sky, 2013-09-10 19:38:57]

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.cc:33: class WebView::FullscreenObserver :
public content::WebContentsObserver {
Add a description.

Is there a particular reason you're making WebView have a WebContentsObserver
rather than be a WebContentsObserver?

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.cc:50: WebView* const view_;
members at the end of a section.

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.cc:56: };
DISALLOW...

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
File ui/views/controls/webview/webview.h (right):

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.h:132: bool is_embedding_fullscreen_widget_;
Add a description.

[jam, 2013-09-11 02:48:25]

content/public lgtm

https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1408: // "Tab/HTML Fullscreen"
mode.  Either way, make sure "Flash Fullscreen" is
nit: layering violation to mention flash here

https://codereview.chromium.org/23477051/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/23477051/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_delegate.h:352: // render widget.  In this
case, ToggleFullscreenModeForTab() is used to enter
nit: don't mention what chrome code does in src/content, as that's a (comment)
layering violation

[miu, 2013-09-11 03:57:03]

Thanks Scott and John.  I took action on all your review comments:

https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1408: // "Tab/HTML Fullscreen"
mode.  Either way, make sure "Flash Fullscreen" is
On 2013/09/11 02:48:26, jam wrote:
> nit: layering violation to mention flash here

Done.

https://codereview.chromium.org/23477051/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/23477051/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_delegate.h:352: // render widget.  In this
case, ToggleFullscreenModeForTab() is used to enter
On 2013/09/11 02:48:26, jam wrote:
> nit: don't mention what chrome code does in src/content, as that's a (comment)
> layering violation

Done.

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.cc:33: class WebView::FullscreenObserver :
public content::WebContentsObserver {
On 2013/09/10 19:38:58, sky wrote:
> Add a description.

Done, and also for its "cousin" in the Mac cocoa UI code.  Also, good call here
since this is a good place to document some of the overall design in the code
for future readability.

> Is there a particular reason you're making WebView have a WebContentsObserver
> rather than be a WebContentsObserver?

Yes.  I chose to only allocate and register this observer when WebView is
configured to embed the fullscreen widgets.  This way, there won't be any
overhead in cases where the "embed" feature is not activated.

I was also concerned about muddying the WebView class itself since it already
implements WebContentsDelegate for the WebContents instances it creates and owns
internally.

All this said, I'm not strongly opposed to merging the WebContentsObserver into
WebView, if you feel that's the better course here.

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.cc:50: WebView* const view_;
On 2013/09/10 19:38:58, sky wrote:
> members at the end of a section.

Done.

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.cc:56: };
On 2013/09/10 19:38:58, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
File ui/views/controls/webview/webview.h (right):

https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
ui/views/controls/webview/webview.h:132: bool is_embedding_fullscreen_widget_;
On 2013/09/10 19:38:58, sky wrote:
> Add a description.

Done.

[sky, 2013-09-11 16:06:11]

On Tue, Sep 10, 2013 at 8:57 PM,  <miu@chromium.org> wrote:
> Thanks Scott and John.  I took action on all your review comments:
>
>
>
>
https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/web_contents_impl.cc (right):
>
>
https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_impl.cc:1408: // "Tab/HTML
> Fullscreen" mode.  Either way, make sure "Flash Fullscreen" is
> On 2013/09/11 02:48:26, jam wrote:
>>
>> nit: layering violation to mention flash here
>
>
> Done.
>
>
>
https://codereview.chromium.org/23477051/diff/1/content/public/browser/web_co...
> File content/public/browser/web_contents_delegate.h (right):
>
>
https://codereview.chromium.org/23477051/diff/1/content/public/browser/web_co...
> content/public/browser/web_contents_delegate.h:352: // render widget.
> In this case, ToggleFullscreenModeForTab() is used to enter
> On 2013/09/11 02:48:26, jam wrote:
>>
>> nit: don't mention what chrome code does in src/content, as that's a
>
> (comment)
>>
>> layering violation
>
>
> Done.
>
>
>
https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
> File ui/views/controls/webview/webview.cc (right):
>
>
https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
> ui/views/controls/webview/webview.cc:33: class
> WebView::FullscreenObserver : public content::WebContentsObserver {
> On 2013/09/10 19:38:58, sky wrote:
>>
>> Add a description.
>
>
> Done, and also for its "cousin" in the Mac cocoa UI code.  Also, good
> call here since this is a good place to document some of the overall
> design in the code for future readability.
>
>
>> Is there a particular reason you're making WebView have a
>
> WebContentsObserver
>>
>> rather than be a WebContentsObserver?
>
>
> Yes.  I chose to only allocate and register this observer when WebView
> is configured to embed the fullscreen widgets.  This way, there won't be
> any overhead in cases where the "embed" feature is not activated.
>
> I was also concerned about muddying the WebView class itself since it
> already implements WebContentsDelegate for the WebContents instances it
> creates and owns internally.
>
> All this said, I'm not strongly opposed to merging the
> WebContentsObserver into WebView, if you feel that's the better course
> here.

You can still register 'this' as the WebContentsObserver only when
necessary. With your current patch set it feels weird that WebView
implements some things but not all.

  -Scott

>
>
https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
> ui/views/controls/webview/webview.cc:50: WebView* const view_;
> On 2013/09/10 19:38:58, sky wrote:
>>
>> members at the end of a section.
>
>
> Done.
>
>
https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
> ui/views/controls/webview/webview.cc:56: };
> On 2013/09/10 19:38:58, sky wrote:
>>
>> DISALLOW...
>
>
> Done.
>
>
>
https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
> File ui/views/controls/webview/webview.h (right):
>
>
https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/web...
> ui/views/controls/webview/webview.h:132: bool
> is_embedding_fullscreen_widget_;
> On 2013/09/10 19:38:58, sky wrote:
>>
>> Add a description.
>
>
> Done.
>
> https://codereview.chromium.org/23477051/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2013-09-11 20:32:46]

On 2013/09/12, sky wrote:
> You can still register 'this' as the WebContentsObserver only when
> necessary. With your current patch set it feels weird that WebView
> implements some things but not all.

Done.  Had to remove a couple of non-virtual private methods in WebView to avoid
conflicts with methods of the same name in WebContentsObserver.  PTAL (patch set
4).

[sky, 2013-09-11 22:19:23]

LGTM

[Robert Sesek, 2013-09-12 15:11:35]

So this will cause Flash fullscreen to use the same code path as HTML5 full
screen? What's the motivation for this? I think this will probably be a worse
user experience on Mac, to be honest, because our fullscreen story there is very
weak.

Also, please set the BUG= to attach this CL to, along with other relevant
materials.

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h:41:
andAutoEmbedFullscreen:(BOOL)enableEmbeddedFullscreen;
nit: indent 4 spaces because aligning colons doesn't result in the correct
indentation

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:55:
andAutoEmbedFullscreen:(BOOL)enableEmbeddedFullscreen {
nit: indent 4

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:94:
content::RenderWidgetHostView* const fullscreen_view =
naming: fullscreenView

[yzshen1, 2013-09-12 18:39:43]

I have a few high-level questions:
- It seems we also show the Chrome fullscreen bubble for Flash after this
change. Have we gone through UI review for this? Flash already has its own
fullscreen UI ("Press Esc to exit full screen mode").

- If I read the code correctly, we will still use the special fullscreen widget
for Flash after this change, right? (We don't want to use the regular JS
fullscreen for Flash. It is less efficient because we would have to composite on
top of the web page.)

Adding Antoine who knows Flash fullscreen well.

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:113: } else if
(window_->IsFullscreenWithChrome()) {
It is better not to put part of the if...else if...else struct inside #if. #if
should enclose complete statements.

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.h (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.h:25: // "browser
fullscreen". "Tab fullscreen" refers to a renderer-initiated
Please add tests for the changes to fullscreen_controller

[piman, 2013-09-12 20:48:05]

On 2013/09/12 18:39:43, yzshen1 wrote:
> I have a few high-level questions:
> - It seems we also show the Chrome fullscreen bubble for Flash after this
> change. Have we gone through UI review for this? Flash already has its own
> fullscreen UI ("Press Esc to exit full screen mode").

Yeah, we certainly don't want that. This should probably got through launch / UI
review.

> - If I read the code correctly, we will still use the special fullscreen
widget
> for Flash after this change, right? (We don't want to use the regular JS
> fullscreen for Flash. It is less efficient because we would have to composite
on
> top of the web page.)

From the look of it, that's the case, yes.

> 
> Adding Antoine who knows Flash fullscreen well.
> 
>
https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
> File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):
> 
>
https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
> chrome/browser/ui/fullscreen/fullscreen_controller.cc:113: } else if
> (window_->IsFullscreenWithChrome()) {
> It is better not to put part of the if...else if...else struct inside #if. #if
> should enclose complete statements.
> 
>
https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
> File chrome/browser/ui/fullscreen/fullscreen_controller.h (right):
> 
>
https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
> chrome/browser/ui/fullscreen/fullscreen_controller.h:25: // "browser
> fullscreen". "Tab fullscreen" refers to a renderer-initiated
> Please add tests for the changes to fullscreen_controller

[miu, 2013-09-12 23:16:43]

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h:41:
andAutoEmbedFullscreen:(BOOL)enableEmbeddedFullscreen;
On 2013/09/12 15:11:36, rsesek wrote:
> nit: indent 4 spaces because aligning colons doesn't result in the correct
> indentation

Done.

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:55:
andAutoEmbedFullscreen:(BOOL)enableEmbeddedFullscreen {
On 2013/09/12 15:11:36, rsesek wrote:
> nit: indent 4

Done.

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm:94:
content::RenderWidgetHostView* const fullscreen_view =
On 2013/09/12 15:11:36, rsesek wrote:
> naming: fullscreenView

Done.

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:113: } else if
(window_->IsFullscreenWithChrome()) {
On 2013/09/12 18:39:43, yzshen1 wrote:
> It is better not to put part of the if...else if...else struct inside #if. #if
> should enclose complete statements.

Done.  I was attempting to clean things up to be more readable.

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.h (right):

https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.h:25: // "browser
fullscreen". "Tab fullscreen" refers to a renderer-initiated
On 2013/09/12 18:39:43, yzshen1 wrote:
> Please add tests for the changes to fullscreen_controller

Done.

[miu, 2013-09-12 23:34:23]

On 2013/09/12 15:11:35, rsesek wrote:
> So this will cause Flash fullscreen to use the same code path as HTML5 full
> screen?

Sort of.  It only shares the FullscreenController, which is responsible for
controlling the browser window and balloon pop-ups.  The design doc explains
this better (see link from the bug, or message #2 on this review).

> What's the motivation for this?

Explained in proposal/design doc.  Put simply: 1) consistent user experience for
all fullscreen modes in Chrome; 2) prerequisite for improving tab capture
experience when fullscreening.

> I think this will probably be a worse
> user experience on Mac, to be honest, because our fullscreen story there is
very
> weak.

Are you sure?  Everyone I've talked with, and shown prototypes to, has said they
feel the fullscreen experience is much improved on Mac with this change,
especially with regards to window/desktop switching behavior.  Try patching this
in on a Mac and see for yourself.  ;-)

> Also, please set the BUG= to attach this CL to, along with other relevant
> materials.

Done.

[miu, 2013-09-12 23:43:05]

On 2013/09/12 18:39:43, yzshen1 wrote:
> I have a few high-level questions:
> - It seems we also show the Chrome fullscreen bubble for Flash after this
> change. Have we gone through UI review for this? Flash already has its own
> fullscreen UI ("Press Esc to exit full screen mode").

Esc message: Yes, that is a concern, and mentioned in the design doc, and on my
to-do list.

UI review: The plan is to implement this change, leaving it disabled by default
behind a command-line flag.  Then, via Chrome Canary builds, people will be able
to review it much easier by interacting with a working feature.  I don't plan on
enabling this without going through UI review and until all issues are
addressed.

> - If I read the code correctly, we will still use the special fullscreen
widget
> for Flash after this change, right?

Yes.  It's both the simplest solution and the one that is least-likely to break
compatibility.  All discussed in the design doc.

[miu, 2013-09-12 23:44:48]

rsesek and yzshen: I took action on your code review comments, and hopefully
addressed all your high-level concerns.  PTAL.

Thanks,
Yuri

[yzshen1, 2013-09-13 19:37:14]

On 2013/09/12 23:44:48, Yuri wrote:
> rsesek and yzshen: I took action on your code review comments, and hopefully
> addressed all your high-level concerns.  PTAL.
> 
> Thanks,
> Yuri

Have you tested interaction between JS fullscreen and Flash fullscreen? For
example:
- get into tab fullscreen via JS and then request to enter Flash fullscreen.
(Use JS to make a page element which contains, say, a Youtube video to become
fullscreen and then click the fullscreen button of Youtube.)
- get into Flash fullsceen and then request to exit tab fullscreen via JS.
- etc.

[Robert Sesek, 2013-09-13 19:43:26]

On 2013/09/12 23:34:23, Yuri wrote:
> On 2013/09/12 15:11:35, rsesek wrote:
> > So this will cause Flash fullscreen to use the same code path as HTML5 full
> > screen?
> 
> Sort of.  It only shares the FullscreenController, which is responsible for
> controlling the browser window and balloon pop-ups.  The design doc explains
> this better (see link from the bug, or message #2 on this review).
> 
> > What's the motivation for this?
> 
> Explained in proposal/design doc.  Put simply: 1) consistent user experience
for
> all fullscreen modes in Chrome; 2) prerequisite for improving tab capture
> experience when fullscreening.

Thanks. I'd recommend you move your design doc to chromium.org at some point.
This is an open source project after all :).

> > I think this will probably be a worse
> > user experience on Mac, to be honest, because our fullscreen story there is
> very
> > weak.
> 
> Are you sure?  Everyone I've talked with, and shown prototypes to, has said
they
> feel the fullscreen experience is much improved on Mac with this change,
> especially with regards to window/desktop switching behavior.  Try patching
this
> in on a Mac and see for yourself.  ;-)

This is a bit of a tradeoff. You're gaining the ability to switch apps, but it
also means a long/slow transition to the fullscreen space on 10.7+. And if you
have more than one monitor it makes it impossible to do something else on the
other monitor(s). (These are both issues with HTML5 fullscreen currently, though
users have noted they prefer the way Flash does fullscreen). I'm especially
concerned about the space-switching animation/transition because it is slow and
it freezes/snapshots the web contents, which could be annoying when taking a
playing video fullscreen.

[miu, 2013-09-14 01:03:57]

On 2013/09/13 19:37:14, yzshen1 wrote:
> Have you tested interaction between JS fullscreen and Flash fullscreen? For
> example:
> - get into tab fullscreen via JS and then request to enter Flash fullscreen.
> (Use JS to make a page element which contains, say, a Youtube video to become
> fullscreen and then click the fullscreen button of Youtube.)
> - get into Flash fullsceen and then request to exit tab fullscreen via JS.
> - etc.

No.  AFAICT, it's not possible: Both the JS fullscreen API and Flash widget
require a user action (mouse click, etc.) to engage fullscreen.  For example,
let's say you fullscreen an HTML5 video and then how would you click a button in
a Flash widget to fullscreen it as well?

However, note that the code change I made in
WebContentsImpl::ToggleFullscreenMode() makes it impossible for both fullscreen
modes to be active at the same time.  (It shuts down whatever's running to
engage the later-requested one.)

Would you mind if we moved this conversation over to the crbug, or e-mail?  I'm
eager to submit this change so that we have a Canary build to interact with,
test, and discuss things like this.  Remember that this is disabled behind a
flag; so, nothing is being launched here.

[miu, 2013-09-14 01:16:57]

On 2013/09/13 19:43:26, rsesek wrote:
> Thanks. I'd recommend you move your design doc to http://chromium.org at some
point.
> This is an open source project after all :).

Good idea, and done:
https://sites.google.com/a/chromium.org/dev/developers/design-documents/embed...

> > > I think this will probably be a worse
> > > user experience on Mac, to be honest, because our fullscreen story there
is
> > very
> > > weak.
>
> This is a bit of a tradeoff. You're gaining the ability to switch apps, but it
> also means a long/slow transition to the fullscreen space on 10.7+. And if you

Okay.  I agree that sounds like there's work to do for UX on Mac for both
Fullscreen modes: For Flash, better window management.  For HTML5, multi-monitor
and animation issues.

It looks like my solution here presents a good opportunity to unify.  Efforts
can be focused on a single fullscreen window implementation to benefit all
fullscreen modes.  All will get the best, consistent behavior.  SGTY?

[yzshen1, 2013-09-16 16:55:58]

On 2013/09/14 01:03:57, Yuri wrote:
> On 2013/09/13 19:37:14, yzshen1 wrote:
> > Have you tested interaction between JS fullscreen and Flash fullscreen? For
> > example:
> > - get into tab fullscreen via JS and then request to enter Flash fullscreen.
> > (Use JS to make a page element which contains, say, a Youtube video to
become
> > fullscreen and then click the fullscreen button of Youtube.)
> > - get into Flash fullsceen and then request to exit tab fullscreen via JS.
> > - etc.
> 
> No.  AFAICT, it's not possible: Both the JS fullscreen API and Flash widget
> require a user action (mouse click, etc.) to engage fullscreen.  For example,
> let's say you fullscreen an HTML5 video and then how would you click a button
in
> a Flash widget to fullscreen it as well?
(Flash fullscreen Pepper API doesn't require user action, but I think Flash will
ensure that a user action to trigger fullscreen.)
You could have user action for both. For example, a user action results in a
call to JS fullscreen API, making part of the web page (which contains a Flash
Youtube video) enter fullscreen. And then the user clicks the fullscreen button
in the Flash video.

And exiting fullscreen doesn't require user action. So with this change, a JS
exiting fullscreen request will also exit Flash fullscreen (and vice versa),
right?

> 
> However, note that the code change I made in
> WebContentsImpl::ToggleFullscreenMode() makes it impossible for both
fullscreen
> modes to be active at the same time.  (It shuts down whatever's running to
> engage the later-requested one.)
> 
> Would you mind if we moved this conversation over to the crbug, or e-mail? 
I'm
> eager to submit this change so that we have a Canary build to interact with,
> test, and discuss things like this.  Remember that this is disabled behind a
> flag; so, nothing is being launched here.

Please have a bug report clearly documenting UI changes/concerns mentioned in
this review. And add a comment to the commandline switch that unless all things
listed in that bug are reviewed and approved, the commandline switch cannot be
removed.

Thanks!

[Robert Sesek, 2013-09-17 15:15:10]

On 2013/09/14 01:16:57, Yuri wrote:
> On 2013/09/13 19:43:26, rsesek wrote:
> > Thanks. I'd recommend you move your design doc to http://chromium.org at
some
> point.
> > This is an open source project after all :).
> 
> Good idea, and done:
>
https://sites.google.com/a/chromium.org/dev/developers/design-documents/embed...
> 
> > > > I think this will probably be a worse
> > > > user experience on Mac, to be honest, because our fullscreen story there
> is
> > > very
> > > > weak.
> >
> > This is a bit of a tradeoff. You're gaining the ability to switch apps, but
it
> > also means a long/slow transition to the fullscreen space on 10.7+. And if
you
> 
> Okay.  I agree that sounds like there's work to do for UX on Mac for both
> Fullscreen modes: For Flash, better window management.  For HTML5,
multi-monitor
> and animation issues.
> 
> It looks like my solution here presents a good opportunity to unify.  Efforts
> can be focused on a single fullscreen window implementation to benefit all
> fullscreen modes.  All will get the best, consistent behavior.  SGTY?

Some of the issues I outlined are not fixable because they're controlled by
system behavior. I'm still not convinced that this will be a better experience
on Mac (it may be on other platforms), but this code is fine/LGTM and I'll
reserve final judgement until I can play with it more in Canary. Please include
me on any reviews to change Mac fullscreen behavior or to flip this switch on by
default.

[miu, 2013-09-17 19:20:26]

On 2013/09/16 16:55:58, yzshen1 wrote:
> Please have a bug report clearly documenting UI changes/concerns mentioned in
> this review. And add a comment to the commandline switch that unless all
things
> listed in that bug are reviewed and approved, the commandline switch cannot be
> removed.

Done: http://crbug.com/290403  (I cc'ed you on it.)

> Thanks!

Thank you also for the quick turnaround on review comments.  Please be assured
nothing here is just being slapped together, and the goal is certainly not to
turn on this feature until all UX issues are fully addressed.  It is meant to be
a UX improvement, after all.  ;-)

[miu, 2013-09-17 19:24:59]

On 2013/09/17 15:15:10, rsesek wrote:
> Some of the issues I outlined are not fixable because they're controlled by
> system behavior. I'm still not convinced that this will be a better experience
> on Mac (it may be on other platforms), but this code is fine/LGTM and I'll
> reserve final judgement until I can play with it more in Canary. Please
include
> me on any reviews to change Mac fullscreen behavior or to flip this switch on
by
> default.

Thanks for keeping an open mind on this.  My prototypes have already gone
through a couple rounds of peer review/discussion, and so far most everyone sees
this as a good direction to go in.  Now that I know, I'll definitely keep you in
the loop on those discussions.  I think once we can all play with it in Canary,
it'll be a lot easier to move forward.

[commit-bot: I haz the power, 2013-09-17 19:29:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/23477051/60001

[yzshen1, 2013-09-17 19:30:49]

lgtm

[commit-bot: I haz the power, 2013-09-17 22:27:00]

Change committed as 223715

[tiffanioahugurl, 2015-04-20 00:00:41]

tiffanioahugurl@gmail.com changed reviewers:
+ tiffanioahugurl@gmail.com

[tiffanioahugurl, 2015-04-20 00:00:46]

lgtm

[tiffanioahugurl, 2015-04-20 00:00:49]

lgtm

lgtm