Add app.window.alphaEnabled() and onAlphaEnabledChanged.

Add app.window.alphaEnabled() and onAlphaEnabledChanged.

This allows an app to determine whether a window created with
"transparent_background" will work as expected.

This also allows the app to detect when "transparent_background" might
stop working, e.g. when Windows changes from Aero to Classic.

This also fixes the bug where "transparent_background" windows
created in Classic render as black rectangles.

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

[jackhou1, 2014-07-09 07:43:13]

wez, would you mind reviewing this while benwells is OOO? 

I'll ask Ben to review it when he gets back on Monday.

[Wez, 2014-07-09 20:14:45]

On 2014/07/09 07:43:13, jackhou1 wrote:
> wez, would you mind reviewing this while benwells is OOO? 
> 
> I'll ask Ben to review it when he gets back on Monday.

Yup, will take a look today - thanks!

[Wez, 2014-07-09 22:03:30]

https://codereview.chromium.org/375183002/diff/40001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/40001/apps/app_window.cc#newco...
apps/app_window.cc:763: native_app_window_->CanHaveTransparentBackground());
Why do we not want to have alphaEnabled=false for
non-requested-transparent-background windows?

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_window.h
File apps/ui/native_app_window.h (right):

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_windo...
apps/ui/native_app_window.h:90: virtual bool CanHaveTransparentBackground()
const = 0;
Add comment e.g. "Returns false if the NativeAppWindow ignores alpha
transparency."

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_windo...
apps/ui/native_app_window.h:90: virtual bool CanHaveTransparentBackground()
const = 0;
Do we want this to be called "transparent background", if the API is becoming
"alphaEnabled"?

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:401: return true;
So by default we're assuming that Views can always support transparency, unless
the platform-specific impl overrides this?

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:405:
app_window_->OnNativeWindowChanged();
Does this fix the Classic->Aero transitions?

https://codereview.chromium.org/375183002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/375183002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:236:
create_params.frame == AppWindow::FRAME_NONE &&
If requesting transparent background requires frame=none then rather than
silently fail, perhaps we should actually return an API error - WDYT?

https://codereview.chromium.org/375183002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/375183002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:354: // Whether alpha is enabled for
this window.
nit: Suggest "Does the window have access to alpha transparency?" or "Can the
window use alpha transparency?"to fit the style of comments on similar getters.

https://codereview.chromium.org/375183002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:439: // Fired when
window.alphaEnabled changes.
nit: This is obvious from the name; suggest "Fired when the window's ability to
use alpha transparency changes."

[jackhou1, 2014-07-10 03:04:18]

https://codereview.chromium.org/375183002/diff/40001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/40001/apps/app_window.cc#newco...
apps/app_window.cc:763: native_app_window_->CanHaveTransparentBackground());
On 2014/07/09 22:03:29, Wez wrote:
> Why do we not want to have alphaEnabled=false for
> non-requested-transparent-background windows?

No particular reason. It just needs to be constant when
requested_transparent_background_ is false.

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_window.h
File apps/ui/native_app_window.h (right):

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_windo...
apps/ui/native_app_window.h:90: virtual bool CanHaveTransparentBackground()
const = 0;
On 2014/07/09 22:03:29, Wez wrote:
> Add comment e.g. "Returns false if the NativeAppWindow ignores alpha
> transparency."

Oops, done.

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_windo...
apps/ui/native_app_window.h:90: virtual bool CanHaveTransparentBackground()
const = 0;
On 2014/07/09 22:03:29, Wez wrote:
> Do we want this to be called "transparent background", if the API is becoming
> "alphaEnabled"?

Done.

This CL still conflicts a fair bit with
https://codereview.chromium.org/190533006

I'll probably have to rebase, so I'll go over these names again at that point.

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:401: return true;
On 2014/07/09 22:03:30, Wez wrote:
> So by default we're assuming that Views can always support transparency,
unless
> the platform-specific impl overrides this?

It looks like we use ChromeNativeAppWindowViews on CrOS and Linux. This logic
should probably live in the respective *TreeHost.

I'm not really sure what would be the idiomatic way to do this in views. Maybe
something like:
NativeAppWindowViews::CanHaveAlphaEnabled()
  calls views::Widget::CanHaveAlphaEnabled()
    calls WindowTreeHost::CanHaveAlphaEnabled()
      implemented by AppWindowDesktopWindowTreeHostWin?

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:405:
app_window_->OnNativeWindowChanged();
On 2014/07/09 22:03:30, Wez wrote:
> Does this fix the Classic->Aero transitions?

This just updates alphaEnabled in the javascript context.

https://codereview.chromium.org/375183002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/375183002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:236:
create_params.frame == AppWindow::FRAME_NONE &&
On 2014/07/09 22:03:30, Wez wrote:
> If requesting transparent background requires frame=none then rather than
> silently fail, perhaps we should actually return an API error - WDYT?

Done.

https://codereview.chromium.org/375183002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/375183002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:354: // Whether alpha is enabled for
this window.
On 2014/07/09 22:03:30, Wez wrote:
> nit: Suggest "Does the window have access to alpha transparency?" or "Can the
> window use alpha transparency?"to fit the style of comments on similar
getters.

Done.

https://codereview.chromium.org/375183002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:439: // Fired when
window.alphaEnabled changes.
On 2014/07/09 22:03:30, Wez wrote:
> nit: This is obvious from the name; suggest "Fired when the window's ability
to
> use alpha transparency changes."

Done.

[Wez, 2014-07-11 17:14:10]

LGTM, though as noted, I'm no Aura/views expert; experts may prefer plumbing
things all the way down to the tree host.

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_window.h
File apps/ui/native_app_window.h (right):

https://codereview.chromium.org/375183002/diff/40001/apps/ui/native_app_windo...
apps/ui/native_app_window.h:90: virtual bool CanHaveTransparentBackground()
const = 0;
On 2014/07/10 03:04:18, jackhou1 wrote:
> On 2014/07/09 22:03:29, Wez wrote:
> > Do we want this to be called "transparent background", if the API is
becoming
> > "alphaEnabled"?
> 
> Done.
> 
> This CL still conflicts a fair bit with
> https://codereview.chromium.org/190533006
> 
> I'll probably have to rebase, so I'll go over these names again at that point.

Acknowledged.

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:401: return true;
On 2014/07/10 03:04:18, jackhou1 wrote:
> On 2014/07/09 22:03:30, Wez wrote:
> > So by default we're assuming that Views can always support transparency,
> unless
> > the platform-specific impl overrides this?
> 
> It looks like we use ChromeNativeAppWindowViews on CrOS and Linux. This logic
> should probably live in the respective *TreeHost.
> 
> I'm not really sure what would be the idiomatic way to do this in views. Maybe
> something like:
> NativeAppWindowViews::CanHaveAlphaEnabled()
>   calls views::Widget::CanHaveAlphaEnabled()
>     calls WindowTreeHost::CanHaveAlphaEnabled()
>       implemented by AppWindowDesktopWindowTreeHostWin?

Looks plausible, but I'm no expect on views/Aura.

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:405:
app_window_->OnNativeWindowChanged();
On 2014/07/10 03:04:18, jackhou1 wrote:
> On 2014/07/09 22:03:30, Wez wrote:
> > Does this fix the Classic->Aero transitions?
> 
> This just updates alphaEnabled in the javascript context.

Acknowledged.

https://codereview.chromium.org/375183002/diff/60001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/60001/apps/app_window.cc#newco...
apps/app_window.cc:763: : false);
nit: Or simply |requested_transparent_background_ &&
native_app_window_->CanHaveAlphaEnabled()| ?

https://codereview.chromium.org/375183002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/375183002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:237: if
(options->transparent_background.get()) {
Not necessarily for this CL:

Can we re-name this option to alpha_background or similar, and have the JS API
accept backgroundAlpha or transparentBackground options (so we don't break our
existing test app :)?

[jackhou1, 2014-07-14 03:05:24]

Thanks for reviewing!

https://codereview.chromium.org/375183002/diff/60001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/60001/apps/app_window.cc#newco...
apps/app_window.cc:763: : false);
On 2014/07/11 17:14:10, Wez wrote:
> nit: Or simply |requested_transparent_background_ &&
> native_app_window_->CanHaveAlphaEnabled()| ?

Done.

[jackhou1, 2014-07-14 03:08:12]

benwells, could you take a look at this?

I've highlighting some remaining questions below.

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/40001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:401: return true;
On 2014/07/11 17:14:10, Wez wrote:
> On 2014/07/10 03:04:18, jackhou1 wrote:
> > On 2014/07/09 22:03:30, Wez wrote:
> > > So by default we're assuming that Views can always support transparency,
> > unless
> > > the platform-specific impl overrides this?
> > 
> > It looks like we use ChromeNativeAppWindowViews on CrOS and Linux. This
logic
> > should probably live in the respective *TreeHost.
> > 
> > I'm not really sure what would be the idiomatic way to do this in views.
Maybe
> > something like:
> > NativeAppWindowViews::CanHaveAlphaEnabled()
> >   calls views::Widget::CanHaveAlphaEnabled()
> >     calls WindowTreeHost::CanHaveAlphaEnabled()
> >       implemented by AppWindowDesktopWindowTreeHostWin?
> 
> Looks plausible, but I'm no expect on views/Aura.

benwells, any advice on this?

https://codereview.chromium.org/375183002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/375183002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:237: if
(options->transparent_background.get()) {
On 2014/07/11 17:14:10, Wez wrote:
> Not necessarily for this CL:
> 
> Can we re-name this option to alpha_background or similar, and have the JS API
> accept backgroundAlpha or transparentBackground options (so we don't break our
> existing test app :)?

benwells, what do you think of this?
Maybe it should be implemented in the other CL
(https://codereview.chromium.org/190533006/).

[tapted, 2014-07-14 03:30:24]

drive-by: (I had a peek to see if there was anything yet for mac-views :)

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.cc#newco...
apps/app_window.cc:168: always_on_top(false) {}
initialize requested_transparent_background_?

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.h
File apps/app_window.h (right):

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.h#newcod...
apps/app_window.h:366: bool RequestedTransparentBackground() const;
should this be inline/hacker_style()?

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:400: return true;
does this work fine on desktop-linux as well as ChromeOS? I thought we were
still missing things for the general case there..

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.h (right):

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.h:56: void CanHaveAlphaEnabledChanged();
maybe prefix with 'On'? reads a bit like it should return a value

https://codereview.chromium.org/375183002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (right):

https://codereview.chromium.org/375183002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:199: return
ui::win::IsAeroGlassEnabled();
You could also use widget()->ShouldWindowContentsBeTransparent();

Sadly (I think) it returns `false` on ChromeOS

https://codereview.chromium.org/375183002/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/375183002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:355: [nodoc] static boolean
alphaEnabled();
For things not yet in stable channel, convention seems to be also to add a TODO
in addition to the [nodoc]. Same below.

[jackhou1, 2014-07-14 04:51:03]

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.cc#newco...
apps/app_window.cc:168: always_on_top(false) {}
On 2014/07/14 03:30:24, tapted wrote:
> initialize requested_transparent_background_?

Done.

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.h
File apps/app_window.h (right):

https://codereview.chromium.org/375183002/diff/80001/apps/app_window.h#newcod...
apps/app_window.h:366: bool RequestedTransparentBackground() const;
On 2014/07/14 03:30:24, tapted wrote:
> should this be inline/hacker_style()?

Done.

But this will probably change when merged with
https://codereview.chromium.org/190533006/

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.cc:400: return true;
On 2014/07/14 03:30:24, tapted wrote:
> does this work fine on desktop-linux as well as ChromeOS? I thought we were
> still missing things for the general case there..

This doesn't work on Linux. 

I think this logic should live in the tree host, but I'm not sure what's the
right way to pipe it back up to the NativeAppWindow. What's in this patch set
obviously doesn't work though, so I'll implement it in a way that works (add
CanHaveAlphaEnabled to Widget and WindowTreeHost) and wait for benwells'
comments.

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
File apps/ui/views/native_app_window_views.h (right):

https://codereview.chromium.org/375183002/diff/80001/apps/ui/views/native_app...
apps/ui/views/native_app_window_views.h:56: void CanHaveAlphaEnabledChanged();
On 2014/07/14 03:30:24, tapted wrote:
> maybe prefix with 'On'? reads a bit like it should return a value

Done.

https://codereview.chromium.org/375183002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc (right):

https://codereview.chromium.org/375183002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.cc:199: return
ui::win::IsAeroGlassEnabled();
On 2014/07/14 03:30:24, tapted wrote:
> You could also use widget()->ShouldWindowContentsBeTransparent();
> 
> Sadly (I think) it returns `false` on ChromeOS

ShouldWindowContentsBeTransparent is meant for native frames that have
Aero-style transparency, e.g. browser windows where the tabs are mildly
transparent. I think it's the same mechanism used to get transparent backgrounds
on Aero, but the logic isn't the same, so it's probably better to have a
different method for CanHaveAlphaEnabled.

https://codereview.chromium.org/375183002/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/375183002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:355: [nodoc] static boolean
alphaEnabled();
On 2014/07/14 03:30:24, tapted wrote:
> For things not yet in stable channel, convention seems to be also to add a
TODO
> in addition to the [nodoc]. Same below.

Done.

[levin, 2014-07-15 20:40:05]

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:247: }
Changes in this file seem unrelated to the rest of the goals.

This change seems more about making the api more bulletproof which I applaud,
but it could this be in a separate cl (since isn't part of the core changes that
are necessary to make things work). 

imo, the smaller the cl the less risky (and the more possible to get merge
approval).

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.h (right):

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.h:24: }
Please consider removing this part of the change from this cl.

[jackhou1, 2014-07-16 01:26:29]

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:247: }
On 2014/07/15 20:40:05, levin wrote:
> Changes in this file seem unrelated to the rest of the goals.
> 
> This change seems more about making the api more bulletproof which I applaud,
> but it could this be in a separate cl (since isn't part of the core changes
that
> are necessary to make things work). 
> 
> imo, the smaller the cl the less risky (and the more possible to get merge
> approval).
> 

Seems fair, the feature is still behind a flag after all. I'll put these in
another CL.

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_win.h (right):

https://codereview.chromium.org/375183002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_win.h:24: }
On 2014/07/15 20:40:05, levin wrote:
> Please consider removing this part of the change from this cl.

Done.

[jackhou1, 2014-07-16 01:27:21]

tapted, would you mind reviewing this for OWNERS in apps/ since benwells is
away?

[tapted, 2014-07-16 03:01:01]

mostly nits. You might still need an owner for
chrome/{common,browser,renderer*/extensions/* (and sky might have a better idea
for the NativeWidgetFoo change, but it seems OK to me).

https://codereview.chromium.org/375183002/diff/120001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/120001/apps/app_window.cc#newc...
apps/app_window.cc:749: return requested_transparent_background_;
nit: inline this in the header? (ignoring names, this looks more correct for the
follow-up in https://codereview.chromium.org/190533006/ too).

https://codereview.chromium.org/375183002/diff/120001/apps/ui/views/native_ap...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/120001/apps/ui/views/native_ap...
apps/ui/views/native_app_window_views.cc:404: void
NativeAppWindowViews::OnCanHaveAlphaEnabledChanged() {
nit: follow header declaration order - this should be between Init and
InitializeWindow (probably after the destructor, but that should be before Init,
if you feel like moving it).

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:181: virtual bool
CanHaveAlphaEnabled() const OVERRIDE;
can you put this in NativeWidgetMac too?

and I think it should be added to InvokeWidgetMethods(..) in widget_unittest.cc

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.cc:987: bool Widget::CanHaveAlphaEnabled() const {
nit: move up

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.h:736: bool CanHaveAlphaEnabled() const;
nit: move up ap bit, to come after OnRootViewLayout()? -- keeps it with the
other things-that-are-delegated-to-NativeWidgetPrivate

[jackhou1, 2014-07-16 05:54:05]

> mostly nits. You might still need an owner for
> chrome/{common,browser,renderer*/extensions/* (and sky might have a better
idea
> for the NativeWidgetFoo change, but it seems OK to me).

Yeah, I'll ask sky to review too.

https://codereview.chromium.org/375183002/diff/120001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/375183002/diff/120001/apps/app_window.cc#newc...
apps/app_window.cc:749: return requested_transparent_background_;
On 2014/07/16 03:01:01, tapted wrote:
> nit: inline this in the header? (ignoring names, this looks more correct for
the
> follow-up in https://codereview.chromium.org/190533006/ too).

Done.

https://codereview.chromium.org/375183002/diff/120001/apps/ui/views/native_ap...
File apps/ui/views/native_app_window_views.cc (right):

https://codereview.chromium.org/375183002/diff/120001/apps/ui/views/native_ap...
apps/ui/views/native_app_window_views.cc:404: void
NativeAppWindowViews::OnCanHaveAlphaEnabledChanged() {
On 2014/07/16 03:01:01, tapted wrote:
> nit: follow header declaration order - this should be between Init and
> InitializeWindow (probably after the destructor, but that should be before
Init,
> if you feel like moving it).

Done.

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:181: virtual bool
CanHaveAlphaEnabled() const OVERRIDE;
On 2014/07/16 03:01:01, tapted wrote:
> can you put this in NativeWidgetMac too?
> 
> and I think it should be added to InvokeWidgetMethods(..) in
widget_unittest.cc

Done.

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.cc:987: bool Widget::CanHaveAlphaEnabled() const {
On 2014/07/16 03:01:01, tapted wrote:
> nit: move up

Done.

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/375183002/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.h:736: bool CanHaveAlphaEnabled() const;
On 2014/07/16 03:01:01, tapted wrote:
> nit: move up ap bit, to come after OnRootViewLayout()? -- keeps it with the
> other things-that-are-delegated-to-NativeWidgetPrivate

Done.

[jackhou1, 2014-07-16 07:45:13]

sky, could you please review this for OWNERS in:

chrome/
ui/views/

[tapted, 2014-07-16 07:49:44]

apps/ lgtm

[sky, 2014-07-16 15:14:54]

You also need to update the x11 side.

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:368: return
ui::win::IsAeroGlassEnabled();
This should call your new function.

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/widget....
ui/views/widget/widget.h:730: bool CanHaveAlphaEnabled() const;
This name is confusing. No where in widget do we have anything named 'alpha'. I
think you want IsTranslucentWindowOpacity supported, which better matches what
you're tracking. You'll want to update the comments above WindowOpacity to see
this method.

[jackhou1, 2014-07-17 03:07:23]

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right):

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:368: return
ui::win::IsAeroGlassEnabled();
On 2014/07/16 15:14:54, sky wrote:
> This should call your new function.

Done.

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/375183002/diff/140001/ui/views/widget/widget....
ui/views/widget/widget.h:730: bool CanHaveAlphaEnabled() const;
On 2014/07/16 15:14:54, sky wrote:
> This name is confusing. No where in widget do we have anything named 'alpha'.
I
> think you want IsTranslucentWindowOpacity supported, which better matches what
> you're tracking. You'll want to update the comments above WindowOpacity to see
> this method.

Done.

[sky, 2014-07-17 15:52:32]

LGTM

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host.h (right):

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host.h:157: // Returns true if
the Widget can have a transparent background.
supports translucency.

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:425: bool
NativeWidgetMac::IsTranslucentWindowOpacitySupported() const {
Does mac really not support translucent windows?

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/widget....
ui/views/widget/widget.h:201: // or not at all if
IsTranslucentWindowOpacitySupported() is false.
That first sentence is meant to indicate what the consumer of widget may do
where as what you're saying applies to whether translucency is supported. Break
this up leaving the first line as one sentence with something like: 'Translucent
windows may not always be supported. Use IsTranslucentWindowOpacitySupported to
determine if translucent windows are supported.'

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/widget....
ui/views/widget/widget.h:730: // Whether the widget can have a transparent
background.
Whether the widget supports translucency.

[tapted, 2014-07-18 00:22:28]

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:425: bool
NativeWidgetMac::IsTranslucentWindowOpacitySupported() const {
On 2014/07/17 15:52:32, sky wrote:
> Does mac really not support translucent windows?

It does, but the ubercompositor setup that will be used on mac currently doesn't
- I'm still tracking down where the alpha channel is getting lost [WIP:
crrev.com/283053002]. We will also need to ensure that the way transparent-pixel
hit-testing is accomplished on Mac ticks all the security boxes before allowing
packaged AppWindows to use it.

[jackhou1, 2014-07-18 00:41:16]

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host.h (right):

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host.h:157: // Returns true if
the Widget can have a transparent background.
On 2014/07/17 15:52:32, sky wrote:
> supports translucency.

Done.

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/widget....
ui/views/widget/widget.h:201: // or not at all if
IsTranslucentWindowOpacitySupported() is false.
On 2014/07/17 15:52:32, sky wrote:
> That first sentence is meant to indicate what the consumer of widget may do
> where as what you're saying applies to whether translucency is supported.
Break
> this up leaving the first line as one sentence with something like:
'Translucent
> windows may not always be supported. Use IsTranslucentWindowOpacitySupported
to
> determine if translucent windows are supported.'

Done.

https://codereview.chromium.org/375183002/diff/180001/ui/views/widget/widget....
ui/views/widget/widget.h:730: // Whether the widget can have a transparent
background.
On 2014/07/17 15:52:32, sky wrote:
> Whether the widget supports translucency.

Done.

[jackhou1, 2014-07-18 04:28:11]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-07-18 04:30:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/375183002/220001

[commit-bot: I haz the power, 2014-07-18 07:21:29]

Change committed as 284026