Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 23477051: Embed Flash Fullscreen widget within browser window. (Closed)

Created:
6 years ago by miu
Modified:
4 years, 5 months ago
CC:
chromium-reviews, jbauman+watch_chromium.org, yusukes+watch_chromium.org, tfarina, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, scheib+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, miu+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 12

Patch Set 2 : Addressed review comments from sky@ and jam@. #

Patch Set 3 : Reverted adding Window::SetType(NORMAL) call in render_widget_host_view_aura.cc. Does not seem to … #

Patch Set 4 : Rolled WebContentsObserver into WebView. #

Total comments: 10

Patch Set 5 : Addressed rsesek's comments. #

Patch Set 6 : Add FullscreenController interactive UI testing. #

Patch Set 7 : Add caution comment to chrome_switches.cc. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+417 lines, -115 lines) Patch
M chrome/browser/ui/browser.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 chunk +13 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h View 1 2 3 4 3 chunks +19 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.mm View 1 2 3 4 5 chunks +68 lines, -8 lines 0 comments Download
M chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm View 2 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller.h View 1 2 3 4 5 5 chunks +26 lines, -12 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller.cc View 1 2 3 4 5 6 chunks +39 lines, -15 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller_interactive_browsertest.cc View 1 2 3 4 5 1 chunk +29 lines, -0 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller_test.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/fullscreen/fullscreen_controller_test.cc View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_view_host_impl.cc View 1 2 3 4 5 6 2 chunks +4 lines, -3 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 5 6 5 chunks +41 lines, -12 lines 0 comments Download
M content/public/browser/web_contents.h View 1 chunk +5 lines, -1 line 0 comments Download
M content/public/browser/web_contents_delegate.h View 1 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/browser/web_contents_delegate.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/controls/webview/webview.h View 1 2 3 5 chunks +25 lines, -5 lines 0 comments Download
M ui/views/controls/webview/webview.cc View 1 2 3 5 chunks +108 lines, -55 lines 0 comments Download

Messages

Total messages: 34 (1 generated)
miu
Hi all, Please review my code. Suggested focus areas: hubbe: General code review, please. jam: ...
6 years ago (2013-09-10 03:15:58 UTC) #1
miu
Link to chrome-eng-review@ proposal: https://docs.google.com/a/google.com/document/d/1JETVpLkkANZ0huHQAZfkhMWYntrIHm4ZIlfZ2GSXbpM/edit
6 years ago (2013-09-10 03:17:31 UTC) #2
sky
Stupid question, did eng-review approve the direction? On Mon, Sep 9, 2013 at 8:17 PM, ...
6 years ago (2013-09-10 16:56:15 UTC) #3
sail
+avi how might be better to review the cocoa code CC rsesek who's also working ...
6 years ago (2013-09-10 17:37:18 UTC) #4
miu
On 2013/09/10 16:56:15, sky wrote: > Stupid question, did eng-review approve the direction? Yes. Darin ...
6 years ago (2013-09-10 18:38:33 UTC) #5
Avi (use Gerrit)
Robert last did fullscreen work, so he should look at this.
6 years ago (2013-09-10 18:42:10 UTC) #6
sky
https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/webview.cc File ui/views/controls/webview/webview.cc (right): https://codereview.chromium.org/23477051/diff/1/ui/views/controls/webview/webview.cc#newcode33 ui/views/controls/webview/webview.cc:33: class WebView::FullscreenObserver : public content::WebContentsObserver { Add a description. ...
6 years ago (2013-09-10 19:38:57 UTC) #7
jam
content/public lgtm https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/web_contents_impl.cc#newcode1408 content/browser/web_contents/web_contents_impl.cc:1408: // "Tab/HTML Fullscreen" mode. Either way, make ...
6 years ago (2013-09-11 02:48:25 UTC) #8
miu
Thanks Scott and John. I took action on all your review comments: https://codereview.chromium.org/23477051/diff/1/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc ...
6 years ago (2013-09-11 03:57:03 UTC) #9
sky
On Tue, Sep 10, 2013 at 8:57 PM, <miu@chromium.org> wrote: > Thanks Scott and John. ...
6 years ago (2013-09-11 16:06:11 UTC) #10
miu
On 2013/09/12, sky wrote: > You can still register 'this' as the WebContentsObserver only when ...
6 years ago (2013-09-11 20:32:46 UTC) #11
sky
LGTM
6 years ago (2013-09-11 22:19:23 UTC) #12
Robert Sesek
So this will cause Flash fullscreen to use the same code path as HTML5 full ...
6 years ago (2013-09-12 15:11:35 UTC) #13
yzshen1
I have a few high-level questions: - It seems we also show the Chrome fullscreen ...
6 years ago (2013-09-12 18:39:43 UTC) #14
piman
On 2013/09/12 18:39:43, yzshen1 wrote: > I have a few high-level questions: > - It ...
6 years ago (2013-09-12 20:48:05 UTC) #15
miu
https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h File chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h (right): https://codereview.chromium.org/23477051/diff/30001/chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h#newcode41 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 ...
6 years ago (2013-09-12 23:16:43 UTC) #16
miu
On 2013/09/12 15:11:35, rsesek wrote: > So this will cause Flash fullscreen to use the ...
6 years ago (2013-09-12 23:34:23 UTC) #17
miu
On 2013/09/12 18:39:43, yzshen1 wrote: > I have a few high-level questions: > - It ...
6 years ago (2013-09-12 23:43:05 UTC) #18
miu
rsesek and yzshen: I took action on your code review comments, and hopefully addressed all ...
6 years ago (2013-09-12 23:44:48 UTC) #19
yzshen1
On 2013/09/12 23:44:48, Yuri wrote: > rsesek and yzshen: I took action on your code ...
6 years ago (2013-09-13 19:37:14 UTC) #20
Robert Sesek
On 2013/09/12 23:34:23, Yuri wrote: > On 2013/09/12 15:11:35, rsesek wrote: > > So this ...
6 years ago (2013-09-13 19:43:26 UTC) #21
miu
On 2013/09/13 19:37:14, yzshen1 wrote: > Have you tested interaction between JS fullscreen and Flash ...
6 years ago (2013-09-14 01:03:57 UTC) #22
miu
On 2013/09/13 19:43:26, rsesek wrote: > Thanks. I'd recommend you move your design doc to ...
6 years ago (2013-09-14 01:16:57 UTC) #23
yzshen1
On 2013/09/14 01:03:57, Yuri wrote: > On 2013/09/13 19:37:14, yzshen1 wrote: > > Have you ...
6 years ago (2013-09-16 16:55:58 UTC) #24
Robert Sesek
On 2013/09/14 01:16:57, Yuri wrote: > On 2013/09/13 19:43:26, rsesek wrote: > > Thanks. I'd ...
6 years ago (2013-09-17 15:15:10 UTC) #25
miu
On 2013/09/16 16:55:58, yzshen1 wrote: > Please have a bug report clearly documenting UI changes/concerns ...
6 years ago (2013-09-17 19:20:26 UTC) #26
miu
On 2013/09/17 15:15:10, rsesek wrote: > Some of the issues I outlined are not fixable ...
6 years ago (2013-09-17 19:24:59 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/23477051/60001
6 years ago (2013-09-17 19:29:02 UTC) #28
yzshen1
lgtm
6 years ago (2013-09-17 19:30:49 UTC) #29
commit-bot: I haz the power
Change committed as 223715
6 years ago (2013-09-17 22:27:00 UTC) #30
tiffanioahugurl
4 years, 5 months ago (2015-04-20 00:00:42 UTC) #32
tiffanioahugurl
lgtm
4 years, 5 months ago (2015-04-20 00:00:46 UTC) #33
tiffanioahugurl
4 years, 5 months ago (2015-04-20 00:00:49 UTC) #34
Message was sent while issue was closed.
lgtm

lgtm

Powered by Google App Engine
This is Rietveld 408576698