Refactor menu dependency on aura/wm SetShadowType

Refactor menu dependency on aura/wm SetShadowType

We want to run menus without aura for the mac port of views.
Add the abstract function widget::SetHasActivationShadow so
that SetShadowType invocations can be changed to no longer
depend directly on aura/wm.

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

[sky, 2014-05-15 22:37:18]

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

https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h...
ui/views/widget/widget.h:203: bool has_dropshadow;
I believe menus are the only one using this, and it overlaps with what you're
trying to do. Can you extend this to cover what you care about?

[Andre, 2014-05-15 22:49:36]

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

https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h...
ui/views/widget/widget.h:203: bool has_dropshadow;
On 2014/05/15 22:37:19, sky wrote:
> I believe menus are the only one using this, and it overlaps with what you're
> trying to do. Can you extend this to cover what you care about?

It is true that menus are the only one using this.
I was extending this at one point, but Trent had a concern that they are
different kinds of shadows.
See
https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w...

Also, the SetShadowType invocation in dialog_delegate.cc is not as part of
Widget::Init, so we won't be able to set up has_dropshadow there.

[sky, 2014-05-15 22:55:10]

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

https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h...
ui/views/widget/widget.h:203: bool has_dropshadow;
On 2014/05/15 22:49:37, Andre wrote:
> On 2014/05/15 22:37:19, sky wrote:
> > I believe menus are the only one using this, and it overlaps with what
you're
> > trying to do. Can you extend this to cover what you care about?
> 
> It is true that menus are the only one using this.
> I was extending this at one point, but Trent had a concern that they are
> different kinds of shadows.

They are all shadows though. Create an enum if you need to.

> See
>
https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w...
> 
> Also, the SetShadowType invocation in dialog_delegate.cc is not as part of
> Widget::Init, so we won't be able to set up has_dropshadow there.

Can the places that create a dialog set it in the initparams?

[Andre, 2014-05-16 01:36:58]

On 2014/05/15 22:55:10, sky wrote:
> https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h
> File ui/views/widget/widget.h (right):
> 
>
https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h...
> ui/views/widget/widget.h:203: bool has_dropshadow;
> On 2014/05/15 22:49:37, Andre wrote:
> > On 2014/05/15 22:37:19, sky wrote:
> > > I believe menus are the only one using this, and it overlaps with what
> you're
> > > trying to do. Can you extend this to cover what you care about?
> > 
> > It is true that menus are the only one using this.
> > I was extending this at one point, but Trent had a concern that they are
> > different kinds of shadows.
> 
> They are all shadows though. Create an enum if you need to.
> 
> > See
> >
>
https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w...
> > 
> > Also, the SetShadowType invocation in dialog_delegate.cc is not as part of
> > Widget::Init, so we won't be able to set up has_dropshadow there.
> 
> Can the places that create a dialog set it in the initparams?

Yes, seems like we should be able to set the initparams in
DialogDelegate::CreateDialogWidget.
But creating an enum might be tricky. From my reading of the code, menus that
doesn't have
a bubble border appear to be set to have both kinds of shadows.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/shadow_...
So the enum might have to be {none, drop, activation, both}, and the default
value of the enum
will be either activation or none depending on the window type?

[sky, 2014-05-16 15:55:47]

I believe shadow type set in initparams takes precedence over that of
GetShadowTypeFromWindow. I'm ok if the enum needs have a number of
values.

  -Scott

On Thu, May 15, 2014 at 6:36 PM,  <andresantoso@chromium.org> wrote:
> On 2014/05/15 22:55:10, sky wrote:
>>
>>
>>
https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h
>> File ui/views/widget/widget.h (right):
>
>
>
>
https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h...
>>
>> ui/views/widget/widget.h:203: bool has_dropshadow;
>> On 2014/05/15 22:49:37, Andre wrote:
>> > On 2014/05/15 22:37:19, sky wrote:
>> > > I believe menus are the only one using this, and it overlaps with what
>> you're
>> > > trying to do. Can you extend this to cover what you care about?
>> >
>> > It is true that menus are the only one using this.
>> > I was extending this at one point, but Trent had a concern that they are
>> > different kinds of shadows.
>
>
>> They are all shadows though. Create an enum if you need to.
>
>
>> > See
>> >
>
>
>
https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w...
>>
>> >
>> > Also, the SetShadowType invocation in dialog_delegate.cc is not as part
>> > of
>> > Widget::Init, so we won't be able to set up has_dropshadow there.
>
>
>> Can the places that create a dialog set it in the initparams?
>
>
> Yes, seems like we should be able to set the initparams in
> DialogDelegate::CreateDialogWidget.
> But creating an enum might be tricky. From my reading of the code, menus
> that
> doesn't have
> a bubble border appear to be set to have both kinds of shadows.
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/shadow_...
> So the enum might have to be {none, drop, activation, both}, and the default
> value of the enum
> will be either activation or none depending on the window type?
>
>
> https://codereview.chromium.org/290553002/

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

[Andre, 2014-05-19 16:02:34]

On 2014/05/16 15:55:47, sky wrote:
> I believe shadow type set in initparams takes precedence over that of
> GetShadowTypeFromWindow. I'm ok if the enum needs have a number of
> values.
> 
>   -Scott
> 
> On Thu, May 15, 2014 at 6:36 PM,  <mailto:andresantoso@chromium.org> wrote:
> > On 2014/05/15 22:55:10, sky wrote:
> >>
> >>
> >>
> https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h
> >> File ui/views/widget/widget.h (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/290553002/diff/20001/ui/views/widget/widget.h...
> >>
> >> ui/views/widget/widget.h:203: bool has_dropshadow;
> >> On 2014/05/15 22:49:37, Andre wrote:
> >> > On 2014/05/15 22:37:19, sky wrote:
> >> > > I believe menus are the only one using this, and it overlaps with what
> >> you're
> >> > > trying to do. Can you extend this to cover what you care about?
> >> >
> >> > It is true that menus are the only one using this.
> >> > I was extending this at one point, but Trent had a concern that they are
> >> > different kinds of shadows.
> >
> >
> >> They are all shadows though. Create an enum if you need to.
> >
> >
> >> > See
> >> >
> >
> >
> >
>
https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w...
> >>
> >> >
> >> > Also, the SetShadowType invocation in dialog_delegate.cc is not as part
> >> > of
> >> > Widget::Init, so we won't be able to set up has_dropshadow there.
> >
> >
> >> Can the places that create a dialog set it in the initparams?
> >
> >
> > Yes, seems like we should be able to set the initparams in
> > DialogDelegate::CreateDialogWidget.
> > But creating an enum might be tricky. From my reading of the code, menus
> > that
> > doesn't have
> > a bubble border appear to be set to have both kinds of shadows.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/shadow_...
> > So the enum might have to be {none, drop, activation, both}, and the default
> > value of the enum
> > will be either activation or none depending on the window type?
> >
> >
> > https://codereview.chromium.org/290553002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks Scott, please take a look at update patch #2.

[sky, 2014-05-20 13:30:42]

LGTM with a name change.

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

https://codereview.chromium.org/290553002/diff/40001/ui/views/widget/widget.h...
ui/views/widget/widget.h:184: DEFAULT_SHADOW,   // Use default shadow setting.
SHADOW_TYPE_DEFAULT, SHADOW_TYPE_NONE, SHADOW_TYPE_DROP (this is the common
naming style as used by content and is preferable). Also, better document what
default means.

[Andre, 2014-05-21 02:07:38]

On 2014/05/20 13:30:42, sky wrote:
> LGTM with a name change.
> 
> https://codereview.chromium.org/290553002/diff/40001/ui/views/widget/widget.h
> File ui/views/widget/widget.h (right):
> 
>
https://codereview.chromium.org/290553002/diff/40001/ui/views/widget/widget.h...
> ui/views/widget/widget.h:184: DEFAULT_SHADOW,   // Use default shadow setting.
> SHADOW_TYPE_DEFAULT, SHADOW_TYPE_NONE, SHADOW_TYPE_DROP (this is the common
> naming style as used by content and is preferable). Also, better document what
> default means.

Done (Patch Set #3).

[Andre, 2014-05-21 06:12:39]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-05-21 06:13:55]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/andresantoso@chromium.org/290553002/70001

[commit-bot: I haz the power, 2014-05-21 20:02:52]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-21 23:40:21]

Change committed as 272016