mash: improves browser frame decorations

mash: improves frame rendering in mash

BrowserFrameMus was created more than a year ago and had gotten a bit
out of sync with other ash changes. This change gets browser windows
looking better, but is really just a start. We really need to promote
BrowserHeaderPainter to ash, but that's a bigger project.

As I believe the non-client area should be rendered by ash we need a
way for clients to signal they want windows to look like browser
windows. This adds a WindowStyle that is meant to indicate that.

This also makes DesktopWindowTreeHostMus windows opacity honor that
set in the initparams, which is needed for browser windows as they are
transparent.

Lastly adds a couple of names to some of the apps, and makes the name
of a Widget be set on the top-level window.

BUG=697206
TEST=none

Review-Url: https://codereview.chromium.org/2724693002
Cr-Commit-Position: refs/heads/master@{#454938}
Committed: https://chromium.googlesource.com/chromium/src/+/b5e2126f04fe0ee4d9d418e2c2fa617bb7e37b97

[sky, 2017-03-02 18:36:09]

Description was changed from

==========
mash: improves browser frame decorations

The code drawing frame decorations was taken from more than a year ago
and has grown stale. This improves things somewhat. In particular:

. Windows are now transparent to match chromeos today.
. Title bar height matches classic chromeos and no more transparent area.

BUG=697206
TEST=none
==========

to

==========
mash: improves frame rendering in mash

BrowserFrameMus was created more than a year ago and had gotten a bit
out of sync with other ash changes. This change gets browser windows
looking better, but is really just a start. We really need to promote
BrowserHeaderPainter to ash, but that's a bigger project.

As I believe the non-client area should be rendered by ash we need a
way for clients to signal they want windows to look like browser
windows. This adds a WindowStyle that is meant to indicate that.

This also makes DesktopWindowTreeHostMus windows opacity honor that
set in the initparams, which is needed for browser windows as they are
transparent.

Lastly adds a couple of names to some of the apps, and makes the name
of a Widget be set on the top-level window.

BUG=697206
TEST=none
==========

[sky, 2017-03-02 18:36:10]

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

[sky, 2017-03-02 18:38:14]

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

[sky, 2017-03-02 18:38:26]

tsepez: mojom changes
jamescook: the rest

[sky, 2017-03-02 18:39:01]

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

[commit-bot: I haz the power, 2017-03-02 18:39:23]

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

[Tom Sepez, 2017-03-02 19:07:25]

mojom LGTM

[commit-bot: I haz the power, 2017-03-02 19:40:32]

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

[commit-bot: I haz the power, 2017-03-02 19:40:32]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-02 21:00:01]

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
File ash/common/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
ash/common/frame/custom_frame_view_ash.cc:140: base::Optional<int>
header_height_;
This makes sense as Optional.

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
File ash/common/frame/custom_frame_view_ash.h (right):

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
ash/common/frame/custom_frame_view_ash.h:65: void
SetHeaderHeight(base::Optional<int> height);
This is a little confusing to me. It seems like this should take a non-optional
int and only use the preferred size if this method is never called.

It seems weird to me that someone would call SetHeaderHeight(base::nullopt_t) or
other null-int.

https://codereview.chromium.org/2724693002/diff/20001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.cc (right):

https://codereview.chromium.org/2724693002/diff/20001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:226: CustomFrameViewMus*
custom_frame_view_ = nullptr;
nit: can you comment who owns this, since it's made with raw new?

https://codereview.chromium.org/2724693002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_bubble_views.cc (right):

https://codereview.chromium.org/2724693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/status_bubble_views.cc:656: params.name =
"StatusBubble";
Thanks for adding these.

https://codereview.chromium.org/2724693002/diff/20001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2724693002/diff/20001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:280: // window-manager draws the
non-client area.
Do we have to do it this way? I understand there's a pretty steep performance
cost to transparent windows. Or is this because we need rounded corners? If the
latter I'm confused by the comment.

[sky, 2017-03-02 21:28:42]

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
File ash/common/frame/custom_frame_view_ash.h (right):

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
ash/common/frame/custom_frame_view_ash.h:65: void
SetHeaderHeight(base::Optional<int> height);
On 2017/03/02 21:00:00, James Cook wrote:
> This is a little confusing to me. It seems like this should take a
non-optional
> int and only use the preferred size if this method is never called.
> 
> It seems weird to me that someone would call SetHeaderHeight(base::nullopt_t)
or
> other null-int.

If you go with the int, you have no way to unset. base::optional is designed for
this sort of thing. Otherwise you need an explicit ClearHeaderHeight(), which is
equally ick. We have started to use this pattern in other places, e.g.
https://chromium.googlesource.com/chromium/src/+/master/ui/views/controls/but...
.

https://codereview.chromium.org/2724693002/diff/20001/ash/mus/non_client_fram...
File ash/mus/non_client_frame_controller.cc (right):

https://codereview.chromium.org/2724693002/diff/20001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:226: CustomFrameViewMus*
custom_frame_view_ = nullptr;
On 2017/03/02 21:00:00, James Cook wrote:
> nit: can you comment who owns this, since it's made with raw new?

Done.

https://codereview.chromium.org/2724693002/diff/20001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2724693002/diff/20001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:280: // window-manager draws the
non-client area.
On 2017/03/02 21:00:01, James Cook wrote:
> Do we have to do it this way? I understand there's a pretty steep performance
> cost to transparent windows. Or is this because we need rounded corners? If
the
> latter I'm confused by the comment.

I updated the comment. Any better?

As to performance cost, in classic ash we make the window's layer transparent as
well
(https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/vie...).
It's certainly the case that there are more windows (and more layers) in mash.
No doubt we will have to revisit this. Sadrul has been looking at performance,
so it is in people's minds.

[James Cook, 2017-03-02 21:44:24]

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
File ash/common/frame/custom_frame_view_ash.h (right):

https://codereview.chromium.org/2724693002/diff/20001/ash/common/frame/custom...
ash/common/frame/custom_frame_view_ash.h:65: void
SetHeaderHeight(base::Optional<int> height);
On 2017/03/02 21:28:42, sky wrote:
> On 2017/03/02 21:00:00, James Cook wrote:
> > This is a little confusing to me. It seems like this should take a
> non-optional
> > int and only use the preferred size if this method is never called.
> > 
> > It seems weird to me that someone would call
SetHeaderHeight(base::nullopt_t)
> or
> > other null-int.
> 
> If you go with the int, you have no way to unset. base::optional is designed
for
> this sort of thing. Otherwise you need an explicit ClearHeaderHeight(), which
is
> equally ick. We have started to use this pattern in other places, e.g.
>
https://chromium.googlesource.com/chromium/src/+/master/ui/views/controls/but...
> .

Yes, but I don't see you calling SetHeaderHeight() anywhere that unsets the
value.

https://codereview.chromium.org/2724693002/diff/20001/ui/views/mus/desktop_wi...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2724693002/diff/20001/ui/views/mus/desktop_wi...
ui/views/mus/desktop_window_tree_host_mus.cc:280: // window-manager draws the
non-client area.
On 2017/03/02 21:28:42, sky wrote:
> On 2017/03/02 21:00:01, James Cook wrote:
> > Do we have to do it this way? I understand there's a pretty steep
performance
> > cost to transparent windows. Or is this because we need rounded corners? If
> the
> > latter I'm confused by the comment.
> 
> I updated the comment. Any better?
> 
> As to performance cost, in classic ash we make the window's layer transparent
as
> well
>
(https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/vie...).
> It's certainly the case that there are more windows (and more layers) in mash.
> No doubt we will have to revisit this. Sadrul has been looking at performance,
> so it is in people's minds.

Better!

[James Cook, 2017-03-02 23:52:55]

LGTM per hallway discussion

[sky, 2017-03-06 19:11:18]

The CQ bit was checked by sky@chromium.org

[sky, 2017-03-06 19:11:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2724693002/#ps60001
(title: "merge")

[commit-bot: I haz the power, 2017-03-06 19:12:00]

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, 2017-03-06 20:25:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488827478901140,
"parent_rev": "ed3d60a6f09642595391b6678d46f548b649e832", "commit_rev":
"b5e2126f04fe0ee4d9d418e2c2fa617bb7e37b97"}

[commit-bot: I haz the power, 2017-03-06 20:26:01]

Description was changed from

==========
mash: improves frame rendering in mash

BrowserFrameMus was created more than a year ago and had gotten a bit
out of sync with other ash changes. This change gets browser windows
looking better, but is really just a start. We really need to promote
BrowserHeaderPainter to ash, but that's a bigger project.

As I believe the non-client area should be rendered by ash we need a
way for clients to signal they want windows to look like browser
windows. This adds a WindowStyle that is meant to indicate that.

This also makes DesktopWindowTreeHostMus windows opacity honor that
set in the initparams, which is needed for browser windows as they are
transparent.

Lastly adds a couple of names to some of the apps, and makes the name
of a Widget be set on the top-level window.

BUG=697206
TEST=none
==========

to

==========
mash: improves frame rendering in mash

BrowserFrameMus was created more than a year ago and had gotten a bit
out of sync with other ash changes. This change gets browser windows
looking better, but is really just a start. We really need to promote
BrowserHeaderPainter to ash, but that's a bigger project.

As I believe the non-client area should be rendered by ash we need a
way for clients to signal they want windows to look like browser
windows. This adds a WindowStyle that is meant to indicate that.

This also makes DesktopWindowTreeHostMus windows opacity honor that
set in the initparams, which is needed for browser windows as they are
transparent.

Lastly adds a couple of names to some of the apps, and makes the name
of a Widget be set on the top-level window.

BUG=697206
TEST=none

Review-Url: https://codereview.chromium.org/2724693002
Cr-Commit-Position: refs/heads/master@{#454938}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5e2126f04fe0ee4d9d418e2c2fa...
==========

[commit-bot: I haz the power, 2017-03-06 20:26:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b5e2126f04fe0ee4d9d418e2c2fa...