CrOS Window cycle ui - set widget bounds with initparams to avoid extra

CrOS Window cycle ui - set widget bounds with initparams to avoid extra
resizing.

This also requires changing Widget::SetInitialBoundsForFramelessWindows,
which inexplicably constrains the bounds to a rectangle that is smaller
than the work area.

BUG=646418
Committed: https://crrev.com/846a59b93f9911517c7aeb05b47af9b4c0cad175
Cr-Commit-Position: refs/heads/master@{#418446}

[Evan Stade, 2016-09-13 17:26:24]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-09-13 17:26:25]

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

https://codereview.chromium.org/2337213002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1459: SetBounds(bounds);
Does this make sense? An alternative would be to just remove the inset in
SetBoundsConstrained for frameless windows.

My best guess is that SetBoundsConstrained is designed to make sure normal
windows are within the work area and adds some "helpful" padding, so it might
not need to be applied to frameless windows.

[sky, 2016-09-13 19:44:02]

https://codereview.chromium.org/2337213002/diff/1/ash/common/wm/window_cycle_...
File ash/common/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/2337213002/diff/1/ash/common/wm/window_cycle_...
ash/common/wm/window_cycle_list.cc:623:
screen_observer_.Add(display::Screen::GetScreen());
optional: if you're moving this around, how about moving it to the end/beginning
of the function. It isn't inherently related to the creating the widget and
having it in the middle of creating/using the widget is a bit confusing.

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

https://codereview.chromium.org/2337213002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1459: SetBounds(bounds);
On 2016/09/13 17:26:25, Evan Stade wrote:
> Does this make sense? An alternative would be to just remove the inset in
> SetBoundsConstrained for frameless windows.
> 
> My best guess is that SetBoundsConstrained is designed to make sure normal
> windows are within the work area and adds some "helpful" padding, so it might
> not need to be applied to frameless windows.

This does indeed worry me. Did you look at the history to see if it was added
for a specific type? I'm inclined to think we should only do it for TYPE_WINDOW.

[Evan Stade, 2016-09-13 21:59:33]

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

https://codereview.chromium.org/2337213002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1459: SetBounds(bounds);
On 2016/09/13 19:44:02, sky wrote:
> On 2016/09/13 17:26:25, Evan Stade wrote:
> > Does this make sense? An alternative would be to just remove the inset in
> > SetBoundsConstrained for frameless windows.
> > 
> > My best guess is that SetBoundsConstrained is designed to make sure normal
> > windows are within the work area and adds some "helpful" padding, so it
might
> > not need to be applied to frameless windows.
> 
> This does indeed worry me. Did you look at the history to see if it was added
> for a specific type? I'm inclined to think we should only do it for
TYPE_WINDOW.

Do you mean TYPE_WINDOW_FRAMELESS? I don't really think TYPE_MENU or other
frameless types would want to be forced 10px inside the work area either. For
example, when you have a maximized browser window and you click the app menu,
the menu shouldn't be moved 10px to the left (and in fact it isn't, which is
because MenuHost can still call SetBounds() and circumvent this weird behavior
[1]). It appears that SetBoundsConstrained is only used from one place outside
of Widget: network_config_view.cc.

re: history --- for SetInitialBoundsForFramelessWindow, I suspect it was just
copy-pasted from SetInitialBounds[2]. That was added here[3].

[1]
https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_host.cc?rcl=...
[2] https://codereview.chromium.org/9250029
[3] https://codereview.chromium.org/8477019
[4] chrome/browser/chromeos/options/network_config_view.cc

[sky, 2016-09-13 23:36:23]

On Tue, Sep 13, 2016 at 2:59 PM,  <estade@chromium.org> wrote:
>
> https://codereview.chromium.org/2337213002/diff/1/ui/views/widget/widget.cc
> File ui/views/widget/widget.cc (right):
>
>
https://codereview.chromium.org/2337213002/diff/1/ui/views/widget/widget.cc#n...
> ui/views/widget/widget.cc:1459: SetBounds(bounds);
> On 2016/09/13 19:44:02, sky wrote:
>> On 2016/09/13 17:26:25, Evan Stade wrote:
>> > Does this make sense? An alternative would be to just remove the
> inset in
>> > SetBoundsConstrained for frameless windows.
>> >
>> > My best guess is that SetBoundsConstrained is designed to make sure
> normal
>> > windows are within the work area and adds some "helpful" padding, so
> it might
>> > not need to be applied to frameless windows.
>>
>> This does indeed worry me. Did you look at the history to see if it
> was added
>> for a specific type? I'm inclined to think we should only do it for
> TYPE_WINDOW.
>
> Do you mean TYPE_WINDOW_FRAMELESS? I don't really think TYPE_MENU or
> other frameless types would want to be forced 10px inside the work area
> either. For example, when you have a maximized browser window and you
> click the app menu, the menu shouldn't be moved 10px to the left (and in
> fact it isn't, which is because MenuHost can still call SetBounds() and
> circumvent this weird behavior [1]). It appears that
> SetBoundsConstrained is only used from one place outside of Widget:
> network_config_view.cc.

I mean:

if (type == TYPE_WINDOW)
  SetBoundsContrained();
else
  SetBounds().


>
> re: history --- for SetInitialBoundsForFramelessWindow, I suspect it was
> just copy-pasted from SetInitialBounds[2]. That was added here[3].
>
> [1]
>
https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_host.cc?rcl=...
> [2] https://codereview.chromium.org/9250029
> [3] https://codereview.chromium.org/8477019
> [4] chrome/browser/chromeos/options/network_config_view.cc
>
> https://codereview.chromium.org/2337213002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-09-13 23:43:19]

On Tue, Sep 13, 2016 at 5:36 PM, Scott Violet <sky@chromium.org> wrote:

> On Tue, Sep 13, 2016 at 2:59 PM,  <estade@chromium.org> wrote:
> >
> > https://codereview.chromium.org/2337213002/diff/1/ui/
> views/widget/widget.cc
> > File ui/views/widget/widget.cc (right):
> >
> > https://codereview.chromium.org/2337213002/diff/1/ui/
> views/widget/widget.cc#newcode1459
> > ui/views/widget/widget.cc:1459: SetBounds(bounds);
> > On 2016/09/13 19:44:02, sky wrote:
> >> On 2016/09/13 17:26:25, Evan Stade wrote:
> >> > Does this make sense? An alternative would be to just remove the
> > inset in
> >> > SetBoundsConstrained for frameless windows.
> >> >
> >> > My best guess is that SetBoundsConstrained is designed to make sure
> > normal
> >> > windows are within the work area and adds some "helpful" padding, so
> > it might
> >> > not need to be applied to frameless windows.
> >>
> >> This does indeed worry me. Did you look at the history to see if it
> > was added
> >> for a specific type? I'm inclined to think we should only do it for
> > TYPE_WINDOW.
> >
> > Do you mean TYPE_WINDOW_FRAMELESS? I don't really think TYPE_MENU or
> > other frameless types would want to be forced 10px inside the work area
> > either. For example, when you have a maximized browser window and you
> > click the app menu, the menu shouldn't be moved 10px to the left (and in
> > fact it isn't, which is because MenuHost can still call SetBounds() and
> > circumvent this weird behavior [1]). It appears that
> > SetBoundsConstrained is only used from one place outside of Widget:
> > network_config_view.cc.
>
> I mean:
>
> if (type == TYPE_WINDOW)
>   SetBoundsContrained();
> else
>   SetBounds().
>

I don't think we can get type == TYPE_WINDOW
in SetInitialBoundsForFramelessWindow().


>
>
> >
> > re: history --- for SetInitialBoundsForFramelessWindow, I suspect it was
> > just copy-pasted from SetInitialBounds[2]. That was added here[3].
> >
> > [1]
> > https://cs.chromium.org/chromium/src/ui/views/
> controls/menu/menu_host.cc?rcl=1473779955&l=169
> > [2] https://codereview.chromium.org/9250029
> > [3] https://codereview.chromium.org/8477019
> > [4] chrome/browser/chromeos/options/network_config_view.cc
> >
> > https://codereview.chromium.org/2337213002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-09-13 23:56:52]

Turns out SetInitialBoundsForFramelessWindow is only called for non-windows. So,
LGTM

[Evan Stade, 2016-09-13 23:59:05]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 23:59:34]

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

[Evan Stade, 2016-09-14 00:05:07]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-09-14 00:05:13]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-14 00:05:53]

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-09-14 01:21:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-14 01:23:31]

Description was changed from

==========
CrOS Window cycle ui - set widget bounds with initparams to avoid extra
resizing.

This also requires changing Widget::SetInitialBoundsForFramelessWindows,
which inexplicably constrains the bounds to a rectangle that is smaller
than the work area.

BUG=646418
==========

to

==========
CrOS Window cycle ui - set widget bounds with initparams to avoid extra
resizing.

This also requires changing Widget::SetInitialBoundsForFramelessWindows,
which inexplicably constrains the bounds to a rectangle that is smaller
than the work area.

BUG=646418

Committed: https://crrev.com/846a59b93f9911517c7aeb05b47af9b4c0cad175
Cr-Commit-Position: refs/heads/master@{#418446}
==========

[commit-bot: I haz the power, 2016-09-14 01:23:33]

Patchset 1 (id:??) landed as
https://crrev.com/846a59b93f9911517c7aeb05b47af9b4c0cad175
Cr-Commit-Position: refs/heads/master@{#418446}