Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10986092:
Transparent apps support (Closed)
|
Created:
8 years, 2 months ago by Bernie Modified:
7 years, 11 months ago CC:
chromium-reviews, jeremya+watch_chromium.org, Aaron Boodman, mihaip-chromium-reviews_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTransparent apps support
This is an initial implementation of an experimental extension which
enables apps using the appv2 API to request a transparent window
background through a new flag passed to chrome.app.window.create().
How to use:
chrome.app.window.create('xeyes.html', {
width: 500,
height: 309,
// Needs "experimental" permission in manifest.json
transparentBackground: true
});
Basic Demo: http://codewiz.org/pub/transparent-hello-world-app
Caveats:
- Transparent windows have a grey bar below the window title, about
30 pixels high. (http://crbug.com/152838)
BUG=125295
TEST=demo works
R=reveman@chromium.org,mpcomplete@chromium.org,sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175700
Patch Set 1 #Patch Set 2 : Fix author email #
Total comments: 18
Patch Set 3 : Address review comments #
Total comments: 2
Patch Set 4 : Address reviewer's comment #
Total comments: 4
Patch Set 5 : Rebase #Patch Set 6 : Address review comments. #Patch Set 7 : Rebase #
Messages
Total messages: 28 (0 generated)
Jeremy: how likely is it that we could implement this on Windows (I seem to recall translucent windows being more doable on Mac). I'm wary of introducing too much platform divergence in the APIs. https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... chrome/browser/extensions/api/app_window/app_window_api.cc:98: CommandLine::ForCurrentProcess()->HasSwitch( This should check if the extension has the "experimental" permission instead (the HTML frame option probably should too). https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... chrome/common/extensions/api/app_window.idl:51: // EXPERIMENTAL: Frame type: 'none' or 'chrome' (defaults to 'chrome'). This isn't an experimental option.
On 2012/09/28 19:15:34, Mihai Parparita wrote: > Jeremy: how likely is it that we could implement this on Windows (I seem to > recall translucent windows being more doable on Mac). I'm wary of introducing > too much platform divergence in the APIs. I don't think we can ever guarantee a transparent background. There might be cases where we just don't allow it (e.g. fullscreen) and it's going to be hard to support this on all platforms. I think it's fine to only support this in Ash under certain circumstances. We should probably add some events that will tell the app when transparent background is active or not so they can gracefully handle cases where it's not supported. > https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... > File chrome/browser/extensions/api/app_window/app_window_api.cc (right): > > https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... > chrome/browser/extensions/api/app_window/app_window_api.cc:98: > CommandLine::ForCurrentProcess()->HasSwitch( > This should check if the extension has the "experimental" permission instead > (the HTML frame option probably should too). > > https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... > File chrome/common/extensions/api/app_window.idl (right): > > https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... > chrome/common/extensions/api/app_window.idl:51: // EXPERIMENTAL: Frame type: > 'none' or 'chrome' (defaults to 'chrome'). > This isn't an experimental option.
code lgtm
http://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/shell_window_views.cc (right): http://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/shell_window_views.cc:328: params.remove_standard_frame = true; Don't you need to set params.transparent here? WebView creates a WebContentsViewAura. You may need to mark its window as not transparent too. Although I'm not sure since its' not drawn. Antoine could tell you if that matters. Does this work on windows? http://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/shell_window_views.cc:612: if (transparent_background_) { Does this result in flicker? Might it be possible for the page to attempt to paint before this is applied? http://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/shell_window_views.h (right): http://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/shell_window_views.h:136: bool transparent_background_; const
http://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/extens... File chrome/browser/ui/views/extensions/shell_window_views.cc (right): http://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/extens... chrome/browser/ui/views/extensions/shell_window_views.cc:328: params.remove_standard_frame = true; On 2012/09/28 22:29:13, sky wrote: > Don't you need to set params.transparent here? > > WebView creates a WebContentsViewAura. You may need to mark its window as not > transparent too. Although I'm not sure since its' not drawn. Antoine could tell > you if that matters. > > Does this work on windows? It works with Ash independent of the platform. I'm pretty sure this doesn't work on any platforms unless using Ash. Could be made to work without Ash on platforms with support for top-level windows with alpha channel but not sure that's a good idea. We should check what happens when using this without Ash so we don't end up drawing garbage..
https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... chrome/browser/extensions/api/app_window/app_window_api.cc:98: CommandLine::ForCurrentProcess()->HasSwitch( On 2012/09/28 19:15:34, Mihai Parparita wrote: > This should check if the extension has the "experimental" permission instead > (the HTML frame option probably should too). Ok. Could you point me at any existing code to perform this check?
https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... chrome/browser/extensions/api/app_window/app_window_api.cc:98: CommandLine::ForCurrentProcess()->HasSwitch( On 2012/10/01 18:47:24, Bernie wrote: > On 2012/09/28 19:15:34, Mihai Parparita wrote: > > This should check if the extension has the "experimental" permission instead > > (the HTML frame option probably should too). > > Ok. Could you point me at any existing code to perform this check? Something like: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
lgtm. This isn't possible on native Win32 until Aura-on-win32 is done (not for a while). https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/extensions/... File chrome/browser/ui/extensions/shell_window.h (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/extensions/... chrome/browser/ui/extensions/shell_window.h:58: bool transparent_background; Can you add a comment here about only being supported on ash? https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... chrome/common/extensions/api/app_window.idl:55: boolean? transparentBackground; Can you add that it's only supported on ash?
https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... File chrome/browser/extensions/api/app_window/app_window_api.cc (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/extensions/api... chrome/browser/extensions/api/app_window/app_window_api.cc:98: CommandLine::ForCurrentProcess()->HasSwitch( On 2012/10/01 20:04:58, Mihai Parparita wrote: > On 2012/10/01 18:47:24, Bernie wrote: > > On 2012/09/28 19:15:34, Mihai Parparita wrote: > > > This should check if the extension has the "experimental" permission instead > > > (the HTML frame option probably should too). > > > > Ok. Could you point me at any existing code to perform this check? > > Something like: > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/... Done. https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/extensions/... File chrome/browser/ui/extensions/shell_window.h (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/extensions/... chrome/browser/ui/extensions/shell_window.h:58: bool transparent_background; On 2012/10/04 22:12:56, jeremya wrote: > Can you add a comment here about only being supported on ash? Done. https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/exten... File chrome/browser/ui/views/extensions/shell_window_views.cc (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/exten... chrome/browser/ui/views/extensions/shell_window_views.cc:328: params.remove_standard_frame = true; On 2012/09/28 22:50:38, David Reveman wrote: > We should check what happens when using this without Ash so we don't end up drawing garbage.. I tried on Linux: setting the transparent background has no effect. https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/exten... chrome/browser/ui/views/extensions/shell_window_views.cc:612: if (transparent_background_) { On 2012/09/28 22:29:13, sky wrote: > Does this result in flicker? Might it be possible for the page to attempt to paint before this is applied? The window opens with a white background, then it becomes transparent, and the content is rendered last. I'm not sure where we could hook to make the background transparent earlier than this. https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/exten... File chrome/browser/ui/views/extensions/shell_window_views.h (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/exten... chrome/browser/ui/views/extensions/shell_window_views.h:136: bool transparent_background_; Done (also for frameless_). https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... chrome/common/extensions/api/app_window.idl:51: // EXPERIMENTAL: Frame type: 'none' or 'chrome' (defaults to 'chrome'). What I meant to say is that this option requires the experimental API permission (now fixed). https://codereview.chromium.org/10986092/diff/9/chrome/common/extensions/api/... chrome/common/extensions/api/app_window.idl:55: boolean? transparentBackground; On 2012/10/04 22:12:56, jeremya wrote: > Can you add that it's only supported on ash? Done.
lgtm https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/exten... File chrome/browser/ui/views/extensions/shell_window_views.cc (right): https://codereview.chromium.org/10986092/diff/9/chrome/browser/ui/views/exten... chrome/browser/ui/views/extensions/shell_window_views.cc:612: if (transparent_background_) { On 2012/10/10 21:05:01, Bernie wrote: > On 2012/09/28 22:29:13, sky wrote: > > Does this result in flicker? Might it be possible for the page to attempt to > paint before this is applied? > > The window opens with a white background, then it becomes transparent, and the > content is rendered last. > > I'm not sure where we could hook to make the background transparent earlier than > this. We'll need to fix this. But I think it's OK to address it in a follow up patch if you just file a bug for it and reference that bug in a TODO comment here. https://codereview.chromium.org/10986092/diff/21001/chrome/common/extensions/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10986092/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/app_window.idl:52: // Requires experimental API permission. It's only 'experimental-html' frame type that is experimental, right? Please update the comment so it doesn't seem like all frame types are experimental before landing.
https://codereview.chromium.org/10986092/diff/21001/chrome/common/extensions/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10986092/diff/21001/chrome/common/extensions/... chrome/common/extensions/api/app_window.idl:52: // Requires experimental API permission. On 2012/10/11 17:25:58, David Reveman wrote: > It's only 'experimental-html' frame type that is experimental, right? Please > update the comment so it doesn't seem like all frame types are experimental > before landing. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/codewiz@chromium.org/10986092/27001
Presubmit check for 10986092-27001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui/views
chrome/browser/ui/extensions
Presubmit checks took 5.6s to calculate.
Sky and mihaip, could you please take another look?
https://codereview.chromium.org/10986092/diff/27001/chrome/browser/ui/views/e... File chrome/browser/ui/views/extensions/shell_window_views.cc (right): https://codereview.chromium.org/10986092/diff/27001/chrome/browser/ui/views/e... chrome/browser/ui/views/extensions/shell_window_views.cc:404: params.use_system_default_icon = true; Don't you also need to set params.transparent? And similarly what about the native view created by WebContents?
lgtm https://codereview.chromium.org/10986092/diff/27001/chrome/common/extensions/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10986092/diff/27001/chrome/common/extensions/... chrome/common/extensions/api/app_window.idl:52: // The value 'experimental-html' requires experimental API permission. I'd rather we don't document the 'experimental-html' option here.
PTAL https://codereview.chromium.org/10986092/diff/27001/chrome/browser/ui/views/e... File chrome/browser/ui/views/extensions/shell_window_views.cc (right): https://codereview.chromium.org/10986092/diff/27001/chrome/browser/ui/views/e... chrome/browser/ui/views/extensions/shell_window_views.cc:404: params.use_system_default_icon = true; On 2012/11/08 16:29:10, sky wrote: > Don't you also need to set params.transparent? And similarly what about the native view created by WebContents? The helloworld demo app gets a transparent background without doing anything else. InitParams seems to infer transparency from UseTransparentWindows(), which always returns true on OS_CHROMEOS. https://codereview.chromium.org/10986092/diff/27001/chrome/common/extensions/... File chrome/common/extensions/api/app_window.idl (right): https://codereview.chromium.org/10986092/diff/27001/chrome/common/extensions/... chrome/common/extensions/api/app_window.idl:52: // The value 'experimental-html' requires experimental API permission. On 2012/11/11 23:22:43, jeremya wrote: > I'd rather we don't document the 'experimental-html' option here. Ok, reverted this chunk.
still lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/codewiz@chromium.org/10986092/38001
Presubmit check for 10986092-38001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui/views
Presubmit checks took 4.0s to calculate.
On 2013/01/02 22:46:53, jeremya wrote: > still lgtm. Thanks. Sky, could you please review the changes to chrome/browser/ui/views ?
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/codewiz@chromium.org/10986092/38001
Failed to apply patch for chrome/browser/ui/extensions/shell_window.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/extensions/shell_window.cc
Hunk #1 FAILED at 88.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/extensions/shell_window.cc.rej
Patch: chrome/browser/ui/extensions/shell_window.cc
Index: chrome/browser/ui/extensions/shell_window.cc
diff --git a/chrome/browser/ui/extensions/shell_window.cc
b/chrome/browser/ui/extensions/shell_window.cc
index
e7cfd21277f10859a23f81d79502a81bc0c9a64a..ba58238185bc1455727855c53019f2edb005a16f
100644
--- a/chrome/browser/ui/extensions/shell_window.cc
+++ b/chrome/browser/ui/extensions/shell_window.cc
@@ -88,6 +88,7 @@ void SuspendRenderViewHost(RenderViewHost* rvh) {
ShellWindow::CreateParams::CreateParams()
: window_type(ShellWindow::WINDOW_TYPE_DEFAULT),
frame(ShellWindow::FRAME_CHROME),
+ transparent_background(false),
bounds(INT_MIN, INT_MIN, INT_MIN, INT_MIN),
creator_process_id(0), hidden(false) {
}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/codewiz@chromium.org/10986092/49001
Message was sent while issue was closed.
Change committed as 175700 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
