Fix missing shadows for tooltip and menu

Fix missing shadows for tooltip and menu

When using a transparent visual the window manager does not draw any
shadow by default (ex: 'Compiz' WM). Note that this default behavior
is WM dependent.

So the solution is to not use a transparent visual for any window
type which is not TYPE_DRAG and not TYPE_WINDOW.

Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW
because of crbug.com/593256 and crbug.com/369209.

BUG=640170
R=piman@chromium.org, sadrul@chromium.org

[Julien Isorce Samsung, 2016-08-24 10:47:15]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 10:47:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 11:33:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 11:33:31]

Dry run: This issue passed the CQ dry run.

[Julien Isorce Samsung, 2016-08-24 12:01:06]

On 2016/08/24 11:33:31, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Hi, please have a look. Thx.

[piman, 2016-08-24 17:21:50]

https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1166:
ui::ChooseVisualForWindow(enable_transparent_visuals, &visual, &depth);
Ignoring the statics for a second, ChooseVisualForWindow would return the root
window's (i.e. the parent's) visual if enable_transparent_visuals is false.

So, could we fix ChooseVisualForWindow to cache separately the visuals for
transparent and opaque? I think it would make more sense: ChooseVisualForWindow
would choose the right visual based on enable_transparent_visuals, independent
of how it was called before. That way, we don't need to distribute the logic
between there and here.

[Julien Isorce Samsung, 2016-08-25 09:39:57]

On 2016/08/24 17:21:50, piman OOO back 2016-08-29 wrote:
>
https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
> File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):
> 
>
https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1166:
> ui::ChooseVisualForWindow(enable_transparent_visuals, &visual, &depth);
> Ignoring the statics for a second, ChooseVisualForWindow would return the root
> window's (i.e. the parent's) visual if enable_transparent_visuals is false.
> 
> So, could we fix ChooseVisualForWindow to cache separately the visuals for
> transparent and opaque? I think it would make more sense:
ChooseVisualForWindow
> would choose the right visual based on enable_transparent_visuals, independent
> of how it was called before. That way, we don't need to distribute the logic
> between there and here.

Hi Antoine, thx for the suggestion. Actually I thought about it as per comment
#4
https://bugs.chromium.org/p/chromium/issues/detail?id=640170#c4 but when I tried
it ends up to make ui::ChooseVisualForWindow over complicated and not easy to
read.
But I will have another go and I will upload the result in another CL so that we
can compare. Maybe I have not did it the exact way you are thinking. So let's
check. Thx.

[Julien Isorce Samsung, 2016-08-25 22:19:27]

On 2016/08/25 09:39:57, Julien Isorce wrote:
> On 2016/08/24 17:21:50, piman OOO back 2016-08-29 wrote:
> >
>
https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
> > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):
> > 
> >
>
https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
> > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1166:
> > ui::ChooseVisualForWindow(enable_transparent_visuals, &visual, &depth);
> > Ignoring the statics for a second, ChooseVisualForWindow would return the
root
> > window's (i.e. the parent's) visual if enable_transparent_visuals is false.
> > 
> > So, could we fix ChooseVisualForWindow to cache separately the visuals for
> > transparent and opaque? I think it would make more sense:
> ChooseVisualForWindow
> > would choose the right visual based on enable_transparent_visuals,
independent
> > of how it was called before. That way, we don't need to distribute the logic
> > between there and here.
> 
> Hi Antoine, thx for the suggestion. Actually I thought about it as per comment
> #4
> https://bugs.chromium.org/p/chromium/issues/detail?id=640170#c4 but when I
tried
> it ends up to make ui::ChooseVisualForWindow over complicated and not easy to
> read.
> But I will have another go and I will upload the result in another CL so that
we
> can compare. Maybe I have not did it the exact way you are thinking. So let's
> check. Thx.

Done here https://codereview.chromium.org/2268973004/ .
I still prefer the first version I uploaded.

[Julien Isorce Samsung, 2016-08-25 22:19:58]

On 2016/08/25 22:19:27, Julien Isorce wrote:
> On 2016/08/25 09:39:57, Julien Isorce wrote:
> > On 2016/08/24 17:21:50, piman OOO back 2016-08-29 wrote:
> > >
> >
>
https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
> > > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2268973004/diff/1/ui/views/widget/desktop_aur...
> > > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1166:
> > > ui::ChooseVisualForWindow(enable_transparent_visuals, &visual, &depth);
> > > Ignoring the statics for a second, ChooseVisualForWindow would return the
> root
> > > window's (i.e. the parent's) visual if enable_transparent_visuals is
false.
> > > 
> > > So, could we fix ChooseVisualForWindow to cache separately the visuals for
> > > transparent and opaque? I think it would make more sense:
> > ChooseVisualForWindow
> > > would choose the right visual based on enable_transparent_visuals,
> independent
> > > of how it was called before. That way, we don't need to distribute the
logic
> > > between there and here.
> > 
> > Hi Antoine, thx for the suggestion. Actually I thought about it as per
comment
> > #4
> > https://bugs.chromium.org/p/chromium/issues/detail?id=640170#c4 but when I
> tried
> > it ends up to make ui::ChooseVisualForWindow over complicated and not easy
to
> > read.
> > But I will have another go and I will upload the result in another CL so
that
> we
> > can compare. Maybe I have not did it the exact way you are thinking. So
let's
> > check. Thx.
> 
> Done here https://codereview.chromium.org/2268973004/ .
> I still prefer the first version I uploaded.

Oups here https://codereview.chromium.org/2280433004/

[Julien Isorce Samsung, 2016-08-26 15:26:06]

Description was changed from

==========
Fix missing shadows for tooltip and menu

When using a transparent visual the window manager does not draw any
shadow by default (ex: 'Compiz' WM). Note that this default behavior
is WM dependent.

So the solution is to not use a transparent visual for any window
type which is not TYPE_DRAG and not TYPE_WINDOW.

Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW
because of crbug.com/593256 and crbug.com/369209.

BUG=640170

R=piman@chromium.org, sadrul@chromium.org
==========

to

==========
Fix missing shadows for tooltip and menu

When using a transparent visual the window manager does not draw any
shadow by default (ex: 'Compiz' WM). Note that this default behavior
is WM dependent.

So the solution is to not use a transparent visual for any window
type which is not TYPE_DRAG and not TYPE_WINDOW.

Keep trying to use a transparent visual for type TYPE_DRAG or TYPE_WINDOW
because of crbug.com/593256 and crbug.com/369209.

BUG=640170

R=piman@chromium.org, sadrul@chromium.org
==========