Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(548)

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

Created:
7 years, 3 months ago by miu
Modified:
5 years, 8 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: ...
7 years, 3 months 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
7 years, 3 months 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, ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months ago (2013-09-10 18:38:33 UTC) #5
Avi (use Gerrit)
Robert last did fullscreen work, so he should look at this.
7 years, 3 months 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. ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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. ...
7 years, 3 months 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 ...
7 years, 3 months ago (2013-09-11 20:32:46 UTC) #11
sky
LGTM
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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 ...
7 years, 3 months 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
7 years, 3 months ago (2013-09-17 19:29:02 UTC) #28
yzshen1
lgtm
7 years, 3 months ago (2013-09-17 19:30:49 UTC) #29
commit-bot: I haz the power
Change committed as 223715
7 years, 3 months ago (2013-09-17 22:27:00 UTC) #30
tiffanioahugurl
5 years, 8 months ago (2015-04-20 00:00:42 UTC) #32
tiffanioahugurl
lgtm
5 years, 8 months ago (2015-04-20 00:00:46 UTC) #33
tiffanioahugurl
5 years, 8 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