Fixing opacity for browser frame window on ASH/Windows.

Fixing opacity for browser frame window on ASH/Windows.

Reviewers=scottmg, sky, cpu
TBR=pfeldman
BUG=324103
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241535

[scottmg, 2013-12-06 20:52:00]

https://codereview.chromium.org/108193005/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/108193005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_frame.cc:101: // If this window is under
ASH on Windows. We need it to be Translucent.
nit; "If this window is under ASH on Windows. We need it to be Translucent."

to

"If this window is under ASH on Windows, we need it to be translucent."

https://codereview.chromium.org/108193005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_frame.cc:104: params.opacity =
views::Widget::InitParams::TRANSLUCENT_WINDOW;
It should ask ChromeViewsDelegate::UseTransparentWindows, right? Can that not be
made to return the right thing?

[Shrikant Kelkar, 2013-12-06 21:46:50]

ptal.

https://codereview.chromium.org/108193005/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/108193005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_frame.cc:101: // If this window is under
ASH on Windows. We need it to be Translucent.
On 2013/12/06 20:52:00, scottmg wrote:
> nit; "If this window is under ASH on Windows. We need it to be Translucent."
> 
> to
> 
> "If this window is under ASH on Windows, we need it to be translucent."

Done.

https://codereview.chromium.org/108193005/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_frame.cc:104: params.opacity =
views::Widget::InitParams::TRANSLUCENT_WINDOW;
On 2013/12/06 20:52:00, scottmg wrote:
> It should ask ChromeViewsDelegate::UseTransparentWindows, right? Can that not
be
> made to return the right thing?

It doesn't do the right thing as of now for ASH windows (on Windows). We could
place it there but then there is no context and though GetActiveDesktop() is
currently being used in multiple places to derive context, it could potentially
be a problem in future with instances on both sides, Native and ASH.

[scottmg, 2013-12-06 22:00:17]

The #if seems OK, though in general it's a bit confusing when sometimes
transparency comes in and sometimes it derived via the delegate (this isn't new
here though).

I'm not owners though, see what sky thinks. :)

https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:102: // TODO(shrikant): Instead
of #if, this logic can possibly be moved to
I think just delete this TODO.

[Shrikant Kelkar, 2013-12-10 19:57:37]

On 2013/12/06 22:00:17, scottmg wrote:
> The #if seems OK, though in general it's a bit confusing when sometimes
> transparency comes in and sometimes it derived via the delegate (this isn't
new
> here though).
> 
> I'm not owners though, see what sky thinks. :)
> 
>
https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/frame/browser_frame.cc (right):
> 
>
https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/frame/browser_frame.cc:102: // TODO(shrikant): Instead
> of #if, this logic can possibly be moved to
> I think just delete this TODO.

sky, ping

[sky, 2013-12-10 21:10:26]

Why do we need this? The chromeos side doesn't have this. What's different for
win-ash?

[Shrikant Kelkar, 2013-12-11 19:35:19]

On 2013/12/10 21:10:26, sky wrote:
> Why do we need this? The chromeos side doesn't have this. What's different for
> win-ash?

I believe for chromeos we return true from
ChromeViewsDelegate::UseTransparentWindows 
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...).
As of now, this function doesn't seem to have browser/desktop context.

[sky, 2013-12-11 21:17:27]

https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:102: // TODO(shrikant): Instead
of #if, this logic can possibly be moved to
On 2013/12/06 22:00:17, scottmg wrote:
> I think just delete this TODO.

Ah, that's right. In that case isn't the better fix to pass context as the TODO
says in that function so that you don't need an ifdef here?

[Shrikant Kelkar, 2013-12-11 21:58:36]

https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/108193005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_frame.cc:102: // TODO(shrikant): Instead
of #if, this logic can possibly be moved to
On 2013/12/11 21:17:28, sky wrote:
> On 2013/12/06 22:00:17, scottmg wrote:
> > I think just delete this TODO.
> 
> Ah, that's right. In that case isn't the better fix to pass context as the
TODO
> says in that function so that you don't need an ifdef here?

Please correct me, but as I understand host_desktop_type is not easily
accessible or comes late in the picture in terms of Widget initialization or
creation. In this particular case of ASH/Windows host_desktop_type is required
to determine if we want translucent or opaque frame window.

[Shrikant Kelkar, 2013-12-12 20:55:24]

ptal.

[sky, 2013-12-12 21:07:22]

https://codereview.chromium.org/108193005/diff/40001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (left):

https://codereview.chromium.org/108193005/diff/40001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:366:
ViewsDelegate::views_delegate->OnBeforeWidgetInit(&params, this);
Sorry, I just remembered this function. Can all the logic be moved to
OnBeforeWidgetInit? You'll probably want to handle the else case (for tests).

[sky, 2013-12-12 21:07:41]

And to clear, I'm suggesting nuking UseTransparentWindow entirely.

[Shrikant Kelkar, 2013-12-12 21:11:37]

Ohh, cool. I thought about nuking that function but wanted to confirm with
you. Will send another review shortly.
On Dec 12, 2013 1:07 PM, <sky@chromium.org> wrote:

> And to clear, I'm suggesting nuking UseTransparentWindow entirely.
>
> https://codereview.chromium.org/108193005/
>

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

[sky, 2013-12-12 23:58:14]

https://codereview.chromium.org/108193005/diff/60001/ash/shell/content_client...
File ash/shell/content_client/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/108193005/diff/60001/ash/shell/content_client...
ash/shell/content_client/shell_browser_main_parts.cc:67: if
(use_transparent_windows_)
Where does use_transperent_windows_ come from? Seems like you want this to be
true (based on code your nuking). Also, shouldn't this be before the early
return on 64?

https://codereview.chromium.org/108193005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/chrome_views_delegate.cc:194: // We would need to
determine opacity if it's not already specified.
You're making a method that is already unreadable even more so. Here's what you
should do:

Create function called GetOpacityForNewlyCreatedWindows().
. chrome_views_delegate_chromeos.cc: GetOpacityForNewlyCreatedWindows always
returns TRANSLUCENT
. chrome_views_delegate_aura: if starting on 205 in
GetOpacityForNewlyCreatedWindows (it isn't specific to windows).
. and in this file you'll have GetOpacityForNewlyCreatedWindows definition, but
only if !aura and !chromeos.

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
File ui/views/test/test_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
ui/views/test/test_views_delegate.cc:71: if (!params)
This if shouldn't be necessary.

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
ui/views/test/test_views_delegate.h:62: virtual bool
UseTransparentWindows(Widget::InitParams* params) const OVERRIDE;
Remove this entirely.

https://codereview.chromium.org/108193005/diff/60001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (left):

https://codereview.chromium.org/108193005/diff/60001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:367: 
On 367 handle the case of opacity still being INFER_OPACITY.

[Shrikant Kelkar, 2013-12-17 07:06:21]

ptal.

https://codereview.chromium.org/108193005/diff/60001/ash/shell/content_client...
File ash/shell/content_client/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/108193005/diff/60001/ash/shell/content_client...
ash/shell/content_client/shell_browser_main_parts.cc:67: if
(use_transparent_windows_)
On 2013/12/12 23:58:14, sky wrote:
> Where does use_transperent_windows_ come from? Seems like you want this to be
> true (based on code your nuking). Also, shouldn't this be before the early
> return on 64?

TestViewsDelegate defines a metod SetUseTransparentWindows for unit testing,
that's where this variable gets set.

https://codereview.chromium.org/108193005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/chrome_views_delegate.cc:194: // We would need to
determine opacity if it's not already specified.
On 2013/12/12 23:58:14, sky wrote:
> You're making a method that is already unreadable even more so. Here's what
you
> should do:
> 
> Create function called GetOpacityForNewlyCreatedWindows().
> . chrome_views_delegate_chromeos.cc: GetOpacityForNewlyCreatedWindows always
> returns TRANSLUCENT
> . chrome_views_delegate_aura: if starting on 205 in
> GetOpacityForNewlyCreatedWindows (it isn't specific to windows).
> . and in this file you'll have GetOpacityForNewlyCreatedWindows definition,
but
> only if !aura and !chromeos.

Done.

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
File ui/views/test/test_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
ui/views/test/test_views_delegate.cc:71: if (!params)
On 2013/12/12 23:58:14, sky wrote:
> This if shouldn't be necessary.

Done.

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/108193005/diff/60001/ui/views/test/test_views...
ui/views/test/test_views_delegate.h:62: virtual bool
UseTransparentWindows(Widget::InitParams* params) const OVERRIDE;
On 2013/12/12 23:58:14, sky wrote:
> Remove this entirely.

Done.

https://codereview.chromium.org/108193005/diff/60001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (left):

https://codereview.chromium.org/108193005/diff/60001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:367: 
On 2013/12/12 23:58:14, sky wrote:
> On 367 handle the case of opacity still being INFER_OPACITY.

Done.

https://codereview.chromium.org/108193005/diff/60001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:367: 
On 2013/12/12 23:58:14, sky wrote:
> On 367 handle the case of opacity still being INFER_OPACITY.

Done.

https://codereview.chromium.org/108193005/diff/60001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:367: 
On 2013/12/12 23:58:14, sky wrote:
> On 367 handle the case of opacity still being INFER_OPACITY.

Done.

[sky, 2013-12-17 15:58:59]

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:196: if (params->opacity ==
views::Widget::InitParams::INFER_OPACITY) {
nit: no {}

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.h:57: views::Widget::InitParams*
params);
How about naming this GetWindowOpacity and make it take a const InitParams&.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.h:58:
DISALLOW_COPY_AND_ASSIGN(ChromeViewsDelegate);
nit: newline between 57/58

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:9: #if defined(USE_AURA)
You shouldn't need this ifdef here.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:14: // All other windows
except TYPE_WINDOW AND TYPE_PANEL should be
This comment isn't helpful since it just documents what the code does. Comments
should describe why the code is the way it is.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:20: if
(chrome::IsNativeViewInAsh(params->context) ||
You want GetContext(), and it make be NULL.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_chromeos.cc (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_chromeos.cc:7: #if
defined(USE_CHROMEOS)
You shouldn't need an ifdef here.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_chromeos.cc:12: // All other
windows except TYPE_WINDOW AND TYPE_PANEL should be
This comment isn't helpful since it just documents what the code does. Comments
should describe why the code is the way it is.

https://codereview.chromium.org/108193005/diff/100001/content/shell/browser/s...
File content/shell/browser/shell_views.cc (right):

https://codereview.chromium.org/108193005/diff/100001/content/shell/browser/s...
content/shell/browser/shell_views.cc:60: virtual bool UseTransparentWindows(
Didn't you remove this function from TestViewDelegate/ViewsDelegate?

[Shrikant Kelkar, 2013-12-17 18:00:59]

ptal

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:196: if (params->opacity ==
views::Widget::InitParams::INFER_OPACITY) {
On 2013/12/17 15:59:00, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.h:57: views::Widget::InitParams*
params);
On 2013/12/17 15:59:00, sky wrote:
> How about naming this GetWindowOpacity and make it take a const InitParams&.

Done.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.h:58:
DISALLOW_COPY_AND_ASSIGN(ChromeViewsDelegate);
On 2013/12/17 15:59:00, sky wrote:
> nit: newline between 57/58

Done.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:9: #if defined(USE_AURA)
On 2013/12/17 15:59:00, sky wrote:
> You shouldn't need this ifdef here.

Done.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:14: // All other windows
except TYPE_WINDOW AND TYPE_PANEL should be
On 2013/12/17 15:59:00, sky wrote:
> This comment isn't helpful since it just documents what the code does.
Comments
> should describe why the code is the way it is.

Done.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:20: if
(chrome::IsNativeViewInAsh(params->context) ||
On 2013/12/17 15:59:00, sky wrote:
> You want GetContext(), and it make be NULL.

Didn't see GetContext in widget.cc, only found in header file. If required will
define it later, most of the places seem to use context directly.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_chromeos.cc (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_chromeos.cc:7: #if
defined(USE_CHROMEOS)
On 2013/12/17 15:59:00, sky wrote:
> You shouldn't need an ifdef here.

Done.

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_chromeos.cc:12: // All other
windows except TYPE_WINDOW AND TYPE_PANEL should be
On 2013/12/17 15:59:00, sky wrote:
> This comment isn't helpful since it just documents what the code does.
Comments
> should describe why the code is the way it is.

Done.

https://codereview.chromium.org/108193005/diff/100001/content/shell/browser/s...
File content/shell/browser/shell_views.cc (right):

https://codereview.chromium.org/108193005/diff/100001/content/shell/browser/s...
content/shell/browser/shell_views.cc:60: virtual bool UseTransparentWindows(
On 2013/12/17 15:59:00, sky wrote:
> Didn't you remove this function from TestViewDelegate/ViewsDelegate?

Done.

[sky, 2013-12-17 19:14:22]

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/108193005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:20: if
(chrome::IsNativeViewInAsh(params->context) ||
On 2013/12/17 18:01:00, Shrikant Kelkar wrote:
> On 2013/12/17 15:59:00, sky wrote:
> > You want GetContext(), and it make be NULL.
> 
> Didn't see GetContext in widget.cc, only found in header file. If required
will
> define it later, most of the places seem to use context directly.

HA! Nuke the method then. I believe you should prefer parent, and if null use
context.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:195: // We would need to
determine opacity if it's not already specified.
nit: remove 'would'.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.h:56:
views::Widget::InitParams::WindowOpacity GetOpacityForInitParams(
Add documentation, especially as to when it is invoked.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:12: if (params.type !=
views::Widget::InitParams::TYPE_WINDOW &&
This is the same everywhere, right? Can't we move it back to Widget?

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:19: (params.top_level &&
params.type !=
params.top_level isn't quit the same as the old code.

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

https://codereview.chromium.org/108193005/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.cc:351: if (params.opacity ==
views::Widget::InitParams::INFER_OPACITY) {
nit: no {}

[Shrikant Kelkar, 2013-12-17 22:43:33]

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:195: // We would need to
determine opacity if it's not already specified.
On 2013/12/17 19:14:23, sky wrote:
> nit: remove 'would'.

Done.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.h:56:
views::Widget::InitParams::WindowOpacity GetOpacityForInitParams(
On 2013/12/17 19:14:23, sky wrote:
> Add documentation, especially as to when it is invoked.

Done.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:12: if (params.type !=
views::Widget::InitParams::TYPE_WINDOW &&
On 2013/12/17 19:14:23, sky wrote:
> This is the same everywhere, right? Can't we move it back to Widget?

Yes we can move back, but then opacity determination logic will get again
sprinkled at multiple places.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:19: (params.top_level &&
params.type !=
On 2013/12/17 19:14:23, sky wrote:
> params.top_level isn't quit the same as the old code.

sorry, didn't get it?

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

https://codereview.chromium.org/108193005/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.cc:351: if (params.opacity ==
views::Widget::InitParams::INFER_OPACITY) {
On 2013/12/17 19:14:23, sky wrote:
> nit: no {}

Done.

[sky, 2013-12-17 22:58:54]

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:12: if (params.type !=
views::Widget::InitParams::TYPE_WINDOW &&
On 2013/12/17 22:43:34, Shrikant Kelkar wrote:
> On 2013/12/17 19:14:23, sky wrote:
> > This is the same everywhere, right? Can't we move it back to Widget?
> 
> Yes we can move back, but then opacity determination logic will get again
> sprinkled at multiple places.

Why would it be wrong to centralize the exact same code? I'm only talking about
lines 12-14, not all of it.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:19: (params.top_level &&
params.type !=
On 2013/12/17 22:43:34, Shrikant Kelkar wrote:
> On 2013/12/17 19:14:23, sky wrote:
> > params.top_level isn't quit the same as the old code.
> 
> sorry, didn't get it?

The old code considered top_level as:

    params.top_level ||
      (!params.child &&
       params.type != InitParams::TYPE_CONTROL &&
       params.type != InitParams::TYPE_TOOLTIP);

Which isn't quite the same as what you have here.

[Shrikant Kelkar, 2013-12-17 23:33:28]

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:12: if (params.type !=
views::Widget::InitParams::TYPE_WINDOW &&
On 2013/12/17 22:58:55, sky wrote:
> On 2013/12/17 22:43:34, Shrikant Kelkar wrote:
> > On 2013/12/17 19:14:23, sky wrote:
> > > This is the same everywhere, right? Can't we move it back to Widget?
> > 
> > Yes we can move back, but then opacity determination logic will get again
> > sprinkled at multiple places.
> 
> Why would it be wrong to centralize the exact same code? I'm only talking
about
> lines 12-14, not all of it.

Done.

https://codereview.chromium.org/108193005/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:19: (params.top_level &&
params.type !=
On 2013/12/17 22:58:55, sky wrote:
> On 2013/12/17 22:43:34, Shrikant Kelkar wrote:
> > On 2013/12/17 19:14:23, sky wrote:
> > > params.top_level isn't quit the same as the old code.
> > 
> > sorry, didn't get it?
> 
> The old code considered top_level as:
> 
>     params.top_level ||
>       (!params.child &&
>        params.type != InitParams::TYPE_CONTROL &&
>        params.type != InitParams::TYPE_TOOLTIP);
> 
> Which isn't quite the same as what you have here.

params.top_level is getting reassigned immediately following this statement in
old & new code (Widget::Init) and OnBeforeWidgetInit() being called after that.
I understand that it would make GetOpacityForInitParams dependent on the fact
that params.top_level being corrected/reassigned before calling this function.
Wondering what's your suggestion? Recheck with similar condition as old one?

[sky, 2013-12-17 23:57:08]

LGTM - thanks

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

https://codereview.chromium.org/108193005/diff/160001/ui/views/widget/widget....
ui/views/widget/widget.cc:348: if (params.opacity ==
views::Widget::InitParams::INFER_OPACITY)
nit: combine into single if.

[Shrikant Kelkar, 2013-12-18 00:05:06]

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

https://codereview.chromium.org/108193005/diff/160001/ui/views/widget/widget....
ui/views/widget/widget.cc:348: if (params.opacity ==
views::Widget::InitParams::INFER_OPACITY)
On 2013/12/17 23:57:08, sky wrote:
> nit: combine into single if.

Done.

[commit-bot: I haz the power, 2013-12-18 00:09:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shrikant@chromium.org/108193005/180001

[commit-bot: I haz the power, 2013-12-18 01:20:19]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-18 07:48:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shrikant@chromium.org/108193005/240002

[commit-bot: I haz the power, 2013-12-18 10:18:41]

Change committed as 241535