Wires up immersive mode for chrome and mash

Wires up immersive mode for chrome and mash

There is still a slew of things to do, but this is a good start (and
rather lengthy).

There are two distinct ways for immersive mode to work in mash:

1. Mash takes care of it all. In this mode a separate ui::Window is
   created for the reveal of the title area. HeaderView is used to
   render the title area of the reveal in the separate window. The
   client does nothing special here.
2. The client takes control of it all (as happens in chrome). Chrome
   too creates a separate window for the reveal. This window is a
   child of the chrome frame. Mash knows to paint window decorations
   by way of a special property set on the window. Chrome runs all the
   immersive logic, including positioning of the reveal window.

To get 2 to work I had to relax a constraint of mus, in particular
this allows the window manager to set the underlay of any
window. Previously only the owner could, but as 2 requires mash to set
the underlay I had to relax the constraint.

I also changed it so that events in the underlay always go to the
window manager, not the owner as previously.

BUG=634972, 640365
TEST=none
R=jamescook@chromium.org, tsepez@chromium.org

Committed: https://crrev.com/2d6a8e0fcb704248b64e7b65f4e76ace658a4bdf
Cr-Commit-Position: refs/heads/master@{#414655}

[sky, 2016-08-23 23:44:31]

tsepez: mojom and change to window_manager_access_policy.cc.
james: the rest.

[sky, 2016-08-23 23:44:34]

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

[commit-bot: I haz the power, 2016-08-23 23:45:03]

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 00:59:17]

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

[commit-bot: I haz the power, 2016-08-24 00:59:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-24 02:18:21]

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

[commit-bot: I haz the power, 2016-08-24 02:18:55]

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 03:21:15]

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

[commit-bot: I haz the power, 2016-08-24 03:21:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom Sepez, 2016-08-24 16:07:18]

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_manager.mojom:28: // Disables the window
manager from handling immersive fullscreen for the
WM still handles exit from immersive, right?  Otherwise, client could fake
returning out of fullscreen ...

[sky, 2016-08-24 16:22:17]

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_manager.mojom:28: // Disables the window
manager from handling immersive fullscreen for the
On 2016/08/24 16:07:18, Tom Sepez wrote:
> WM still handles exit from immersive, right?  Otherwise, client could fake
> returning out of fullscreen ...

When this is set all the windowmanager does is make the window fullscreen. The
window is restored when the client requests it to be restored (the windowmanager
could also trigger the restore).

[sky, 2016-08-24 16:33:27]

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

[commit-bot: I haz the power, 2016-08-24 16:34:05]

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

[Tom Sepez, 2016-08-24 16:48:03]

> When this is set all the windowmanager does is make the window fullscreen. The
> window is restored when the client requests it to be restored (the
windowmanager
> could also trigger the restore).

Ok, I guess I'm used to a UX where "Esc to exit fullscreen" is part of the
behaviour.  Who would handle the exit in that case?  If its the client, it could
refuse to release itself, no?

[sky, 2016-08-24 16:53:27]

You are correct. But the issue you mention isn't new to this patch,
it's true of fullscreen in general with architecture. The app is the
one that listens for escape to exit. If we didn't do it like this it
would mean games couldn't handle escape, or terminal emulators.

I do see the security issue though. Maybe file a separate issue as it
isn't specific to this?

  -Scott

On Wed, Aug 24, 2016 at 9:48 AM,  <tsepez@chromium.org> wrote:
>> When this is set all the windowmanager does is make the window fullscreen.
>> The
>> window is restored when the client requests it to be restored (the
> windowmanager
>> could also trigger the restore).
>
> Ok, I guess I'm used to a UX where "Esc to exit fullscreen" is part of the
> behaviour. Who would handle the exit in that case? If its the client, it
> could
> refuse to release itself, no?
>
>
> https://codereview.chromium.org/2277563002/

-- 
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.

[Tom Sepez, 2016-08-24 16:59:25]

Yes, OK, this is LGTM.  I just wanted to raise the larger issue. Can we get a
bug filed for it?

[James Cook, 2016-08-24 17:00:33]

Wow, epic CL. Is it possible to add test coverage for any of this behavior?

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

https://codereview.chromium.org/2277563002/diff/40001/ash/common/frame/custom...
ash/common/frame/custom_frame_view_ash.h:43: explicit CustomFrameViewAsh(
I think this would be clearer with two constructors:

CustomFrameViewAsh(frame)
CustomFrameViewAsh(frame, immersive_delegate, enable_immersive)

and have the first one use C++11 constructor delegation to call the second.

(Or change all the call sites, there are only 6 existing ones.)

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
File ash/mus/frame/custom_frame_view_mus.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
ash/mus/frame/custom_frame_view_mus.cc:28: // we only see the client apps client
area.
super nit: apps -> app's

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
ash/mus/frame/custom_frame_view_mus.cc:29:
canvas->FillRect(GetBoundsForClientView(), SK_ColorBLACK,
Dumb question: Why is this SK_ColorBLACK? How does that end up in the client
area being transparent? (and below)

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
File ash/mus/frame/custom_frame_view_mus.h (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
ash/mus/frame/custom_frame_view_mus.h:25: ui::PaintCache paint_cache_;
nit: #include for this file

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
File ash/mus/frame/detached_title_area_renderer.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
ash/mus/frame/detached_title_area_renderer.cc:25: params.name =
"DetachedTitleAreaRenderer";
Hooray for every widget having a name!

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
File ash/mus/frame/detached_title_area_renderer.h (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
ash/mus/frame/detached_title_area_renderer.h:5: #ifndef
ASH_MUS_FRAME_DETATCHED_TITLE_AREA_RENDERER_H_
DETATCHED -> DETACHED

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
ash/mus/frame/detached_title_area_renderer.h:40: // to |window|.
nit: reflow

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

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:132: void OnImmersiveRevealStarted()
override { SetVisibleFraction(0); }
I found this a little confusing. I expected it to create the title area window,
and Ended/Exited to explicitly destroy it. What do you think about making this
logic a little more like HeaderView::OnImmersiveRevealStarted/Ended/Exited?

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:156: visible_bounds -=
title_area_window_->bounds().origin().OffsetFromOrigin();
Do you need this translation? I'm having a hard time figuring this out.
|title_area_window_| can be partially offscreen, but once the intersect is done
the remaining area is all onscreen on top of |frame_window_|, right?

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:427:
ShouldRenderParentTitleArea(params.target)) {
optional nit: early return?

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/property_util.cc
File ash/mus/property_util.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/property_util.c...
ash/mus/property_util.cc:250: return window->HasSharedProperty(
Also check the value of the property, like above.

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:236:
[ui::mojom::WindowManager::kRendererParentTitleArea_Property] =
optional: "using ui::mojom::WindowManager;" might make this wrap better

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:240:
std::string("ChromeImmersiveRevealWindow"));
I think you can just set init_params.name for this.

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:19: class
ImmersiveModeControllerAsh
I think the commentary from your CL description should live somewhere in the
code as comments. This feature is tricky. This might be a good place for it, or
in ImmersiveModeController.

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_manager.mojom:30: // themselves.
nit: Type: bool

https://codereview.chromium.org/2277563002/diff/40001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:506: // If the event is in the non-client
area the event goes to the owner of
nit: Remove this comment

[sky, 2016-08-24 17:30:29]

I filed http://crbug.com/640671 for the fullscreen issue.

  -Scott

On Wed, Aug 24, 2016 at 9:48 AM,  <tsepez@chromium.org> wrote:
>> When this is set all the windowmanager does is make the window fullscreen.
>> The
>> window is restored when the client requests it to be restored (the
> windowmanager
>> could also trigger the restore).
>
> Ok, I guess I'm used to a UX where "Esc to exit fullscreen" is part of the
> behaviour. Who would handle the exit in that case? If its the client, it
> could
> refuse to release itself, no?
>
>
> https://codereview.chromium.org/2277563002/

-- 
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.

[commit-bot: I haz the power, 2016-08-24 18:40:58]

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

[commit-bot: I haz the power, 2016-08-24 18:40:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sky, 2016-08-24 19:23:29]

I'm hoping for test coverage in a future patch. Sorry for ginormo patch.

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

https://codereview.chromium.org/2277563002/diff/40001/ash/common/frame/custom...
ash/common/frame/custom_frame_view_ash.h:43: explicit CustomFrameViewAsh(
On 2016/08/24 17:00:32, James Cook wrote:
> I think this would be clearer with two constructors:
> 
> CustomFrameViewAsh(frame)
> CustomFrameViewAsh(frame, immersive_delegate, enable_immersive)
> 
> and have the first one use C++11 constructor delegation to call the second.
> 
> (Or change all the call sites, there are only 6 existing ones.)

Can you outline why you feel having two constructors is clearer? It seems like
all you get with two constructors is the opportunity for different
documentation, but that's covered here. One benefit of a single constructor like
this is it's easy to see the defaults. Two constructors means you have to look
to the definition to see the behavior.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
File ash/mus/frame/custom_frame_view_mus.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
ash/mus/frame/custom_frame_view_mus.cc:28: // we only see the client apps client
area.
On 2016/08/24 17:00:32, James Cook wrote:
> super nit: apps -> app's

Done.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
ash/mus/frame/custom_frame_view_mus.cc:29:
canvas->FillRect(GetBoundsForClientView(), SK_ColorBLACK,
On 2016/08/24 17:00:32, James Cook wrote:
> Dumb question: Why is this SK_ColorBLACK? How does that end up in the client
> area being transparent? (and below)

Not a dumb question. This should be kClear_Mode. Fixed.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
File ash/mus/frame/custom_frame_view_mus.h (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/custom_fr...
ash/mus/frame/custom_frame_view_mus.h:25: ui::PaintCache paint_cache_;
On 2016/08/24 17:00:32, James Cook wrote:
> nit: #include for this file

Done.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
File ash/mus/frame/detached_title_area_renderer.h (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
ash/mus/frame/detached_title_area_renderer.h:5: #ifndef
ASH_MUS_FRAME_DETATCHED_TITLE_AREA_RENDERER_H_
On 2016/08/24 17:00:32, James Cook wrote:
> DETATCHED -> DETACHED

Done.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/frame/detached_...
ash/mus/frame/detached_title_area_renderer.h:40: // to |window|.
On 2016/08/24 17:00:32, James Cook wrote:
> nit: reflow

I thought it was separate to warrant it's own paragraph, but merged.

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

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:132: void OnImmersiveRevealStarted()
override { SetVisibleFraction(0); }
On 2016/08/24 17:00:32, James Cook wrote:
> I found this a little confusing. I expected it to create the title area
window,
> and Ended/Exited to explicitly destroy it. What do you think about making this
> logic a little more like HeaderView::OnImmersiveRevealStarted/Ended/Exited?

Done.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:156: visible_bounds -=
title_area_window_->bounds().origin().OffsetFromOrigin();
On 2016/08/24 17:00:32, James Cook wrote:
> Do you need this translation? I'm having a hard time figuring this out.
> |title_area_window_| can be partially offscreen, but once the intersect is
done
> the remaining area is all onscreen on top of |frame_window_|, right?

visible_bounds is in the coordinates of the parent, the offset converts to that
of title_area_window_ so that it can then convert to screen. I updated the
comment and added a DCHECK as code needs to be updated when parent changed.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/non_client_fram...
ash/mus/non_client_frame_controller.cc:427:
ShouldRenderParentTitleArea(params.target)) {
On 2016/08/24 17:00:32, James Cook wrote:
> optional nit: early return?

Done.

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/property_util.cc
File ash/mus/property_util.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/ash/mus/property_util.c...
ash/mus/property_util.cc:250: return window->HasSharedProperty(
On 2016/08/24 17:00:32, James Cook wrote:
> Also check the value of the property, like above.

Done.

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:236:
[ui::mojom::WindowManager::kRendererParentTitleArea_Property] =
On 2016/08/24 17:00:32, James Cook wrote:
> optional: "using ui::mojom::WindowManager;" might make this wrap better

That doesn't work for constants.

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:240:
std::string("ChromeImmersiveRevealWindow"));
On 2016/08/24 17:00:32, James Cook wrote:
> I think you can just set init_params.name for this.

That only works if WindowManagerConnection creates the window, because the
window is not created through WindowManagerConnection it won't work.

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:19: class
ImmersiveModeControllerAsh
On 2016/08/24 17:00:32, James Cook wrote:
> I think the commentary from your CL description should live somewhere in the
> code as comments. This feature is tricky. This might be a good place for it,
or
> in ImmersiveModeController.

I went with a markdown file in ash/mus/frame for this. WDYT?

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2277563002/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_manager.mojom:30: // themselves.
On 2016/08/24 17:00:33, James Cook wrote:
> nit: Type: bool

Done.

https://codereview.chromium.org/2277563002/diff/40001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.cc (right):

https://codereview.chromium.org/2277563002/diff/40001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.cc:506: // If the event is in the non-client
area the event goes to the owner of
On 2016/08/24 17:00:33, James Cook wrote:
> nit: Remove this comment

Done. In removing this comment I realized the code could be better structured.
Sorry for more change.

[sky, 2016-08-24 19:23:31]

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

[commit-bot: I haz the power, 2016-08-24 19:24:50]

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

[James Cook, 2016-08-24 19:56:30]

LGTM

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

https://codereview.chromium.org/2277563002/diff/40001/ash/common/frame/custom...
ash/common/frame/custom_frame_view_ash.h:43: explicit CustomFrameViewAsh(
On 2016/08/24 19:23:28, sky wrote:
> On 2016/08/24 17:00:32, James Cook wrote:
> > I think this would be clearer with two constructors:
> > 
> > CustomFrameViewAsh(frame)
> > CustomFrameViewAsh(frame, immersive_delegate, enable_immersive)
> > 
> > and have the first one use C++11 constructor delegation to call the second.
> > 
> > (Or change all the call sites, there are only 6 existing ones.)
> 
> Can you outline why you feel having two constructors is clearer? It seems like
> all you get with two constructors is the opportunity for different
> documentation, but that's covered here. One benefit of a single constructor
like
> this is it's easy to see the defaults. Two constructors means you have to look
> to the definition to see the behavior.

This allows "new CustomFrameViewAsh(frame, delegate)" without the
enable_immersive parameter, which felt like a case you might not want. I don't
feel that strongly, though.

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/2277563002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:19: class
ImmersiveModeControllerAsh
On 2016/08/24 19:23:29, sky wrote:
> On 2016/08/24 17:00:32, James Cook wrote:
> > I think the commentary from your CL description should live somewhere in the
> > code as comments. This feature is tricky. This might be a good place for it,
> or
> > in ImmersiveModeController.
> 
> I went with a markdown file in ash/mus/frame for this. WDYT?

I like it.

https://codereview.chromium.org/2277563002/diff/80001/ash/mus/frame/README.md
File ash/mus/frame/README.md (right):

https://codereview.chromium.org/2277563002/diff/80001/ash/mus/frame/README.md...
ash/mus/frame/README.md:2: frames decorations in mash.
nit: frames -> frame

https://codereview.chromium.org/2277563002/diff/80001/ash/mus/frame/README.md...
ash/mus/frame/README.md:4: ### Immersive mode
I would change this to "immersive fullscreen" and add a sort description of the
feature, something like:

Immersive fullscreen is a fullscreen mode similar to Mac OS presentation mode.
In it the user can move the mouse to the top of the screen and the window header
will slide down on top of the fullscreen window contents.

(And I like the idea of more README.md files!)

https://codereview.chromium.org/2277563002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/2277563002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:23: // See
ash/mus/frame/README.me for description of how immersive mode works in
.me -> .md

[sky, 2016-08-24 20:11:14]

https://codereview.chromium.org/2277563002/diff/80001/ash/mus/frame/README.md
File ash/mus/frame/README.md (right):

https://codereview.chromium.org/2277563002/diff/80001/ash/mus/frame/README.md...
ash/mus/frame/README.md:2: frames decorations in mash.
On 2016/08/24 19:56:29, James Cook wrote:
> nit: frames -> frame

Done.

https://codereview.chromium.org/2277563002/diff/80001/ash/mus/frame/README.md...
ash/mus/frame/README.md:4: ### Immersive mode
On 2016/08/24 19:56:29, James Cook wrote:
> I would change this to "immersive fullscreen" and add a sort description of
the
> feature, something like:
> 
> Immersive fullscreen is a fullscreen mode similar to Mac OS presentation mode.
> In it the user can move the mouse to the top of the screen and the window
header
> will slide down on top of the fullscreen window contents.
> 
> (And I like the idea of more README.md files!)

I modified this slightly, but mostly kept it.

https://codereview.chromium.org/2277563002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/2277563002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:23: // See
ash/mus/frame/README.me for description of how immersive mode works in
On 2016/08/24 19:56:30, James Cook wrote:
> .me -> .md

Done.

[sky, 2016-08-24 20:11:39]

The CQ bit was checked by sky@chromium.org

[sky, 2016-08-24 20:11:39]

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/2277563002/#ps100001
(title: "feedback")

[commit-bot: I haz the power, 2016-08-24 20:11:50]

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 22:03:54]

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

[commit-bot: I haz the power, 2016-08-24 22:03:56]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-08-24 22:13:48]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-08-24 22:15:24]

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-25 00:10:44]

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

[commit-bot: I haz the power, 2016-08-25 00:10:45]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sky, 2016-08-25 16:22:40]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-08-25 16:23:08]

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-25 19:22:34]

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

[commit-bot: I haz the power, 2016-08-25 19:22:35]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sky, 2016-08-25 20:23:24]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-08-25 20:24:47]

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-25 22:31:03]

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

[commit-bot: I haz the power, 2016-08-25 22:31:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2016-08-25 22:43:05]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-08-25 22:44:08]

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-26 01:18:26]

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

[commit-bot: I haz the power, 2016-08-26 01:18:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2016-08-26 03:28:03]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-08-26 03:28:31]

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-26 05:27:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-26 05:30:57]

Description was changed from

==========
Wires up immersive mode for chrome and mash

There is still a slew of things to do, but this is a good start (and
rather lengthy).

There are two distinct ways for immersive mode to work in mash:

1. Mash takes care of it all. In this mode a separate ui::Window is
   created for the reveal of the title area. HeaderView is used to
   render the title area of the reveal in the separate window. The
   client does nothing special here.
2. The client takes control of it all (as happens in chrome). Chrome
   too creates a separate window for the reveal. This window is a
   child of the chrome frame. Mash knows to paint window decorations
   by way of a special property set on the window. Chrome runs all the
   immersive logic, including positioning of the reveal window.

To get 2 to work I had to relax a constraint of mus, in particular
this allows the window manager to set the underlay of any
window. Previously only the owner could, but as 2 requires mash to set
the underlay I had to relax the constraint.

I also changed it so that events in the underlay always go to the
window manager, not the owner as previously.

BUG=634972, 640365
TEST=none
R=jamescook@chromium.org, tsepez@chromium.org
==========

to

==========
Wires up immersive mode for chrome and mash

There is still a slew of things to do, but this is a good start (and
rather lengthy).

There are two distinct ways for immersive mode to work in mash:

1. Mash takes care of it all. In this mode a separate ui::Window is
   created for the reveal of the title area. HeaderView is used to
   render the title area of the reveal in the separate window. The
   client does nothing special here.
2. The client takes control of it all (as happens in chrome). Chrome
   too creates a separate window for the reveal. This window is a
   child of the chrome frame. Mash knows to paint window decorations
   by way of a special property set on the window. Chrome runs all the
   immersive logic, including positioning of the reveal window.

To get 2 to work I had to relax a constraint of mus, in particular
this allows the window manager to set the underlay of any
window. Previously only the owner could, but as 2 requires mash to set
the underlay I had to relax the constraint.

I also changed it so that events in the underlay always go to the
window manager, not the owner as previously.

BUG=634972, 640365
TEST=none
R=jamescook@chromium.org, tsepez@chromium.org

Committed: https://crrev.com/2d6a8e0fcb704248b64e7b65f4e76ace658a4bdf
Cr-Commit-Position: refs/heads/master@{#414655}
==========

[commit-bot: I haz the power, 2016-08-26 05:30:58]

Patchset 6 (id:??) landed as
https://crrev.com/2d6a8e0fcb704248b64e7b65f4e76ace658a4bdf
Cr-Commit-Position: refs/heads/master@{#414655}