Makes WorkspaceLayoutManager use ash/wm/common types

Makes WorkspaceLayoutManager use ash/wm/common types

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/4dc7d0496343775c344899a2ad5742865f81a93b
Cr-Commit-Position: refs/heads/master@{#390297}

[sky, 2016-04-27 20:48:05]

https://codereview.chromium.org/1923983003/diff/1/ash/shelf/shelf_layout_mana...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1923983003/diff/1/ash/shelf/shelf_layout_mana...
ash/shelf/shelf_layout_manager.cc:191: // NOTE: OnShelfAlignmentChanged() is
implemented by way of
This is subtle and a bit unfortunate. But I think this class will die long term,
or at least be refactored in such a way that this code can be nuked.

https://codereview.chromium.org/1923983003/diff/1/ash/wm/always_on_top_contro...
File ash/wm/always_on_top_controller.cc (right):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/always_on_top_contro...
ash/wm/always_on_top_controller.cc:75: void
AlwaysOnTopController::OnWindowDestroying(wm::WmWindow* window) {
I'm hoping the order doesn't really matter here (before vs after). No tests
tripped over it, so I'm hoping I'm good.

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/switchable_wi...
File ash/wm/common/switchable_windows.cc (left):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/switchable_wi...
ash/wm/common/switchable_windows.cc:13: kShellWindowId_DefaultContainer,
I think this looks better, but I ran 'git cl format' and you can see the result.

[James Cook, 2016-04-27 23:24:24]

LGTM

https://codereview.chromium.org/1923983003/diff/1/ash/shelf/shelf_layout_mana...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1923983003/diff/1/ash/shelf/shelf_layout_mana...
ash/shelf/shelf_layout_manager.cc:194: // WmRootWindowControllerObservers may be
notified after ShellObservers.
The part that I missed in thinking about this was that some other layout manager
had been converted to WmRootWindowControllerObserver, and the ordering problem
was with the ones that have moved there. Is the comment below any clearer?

NOTE: Some other layout managers also observe for OnShelfAlignmentChanged() via
WmRootWindowControllerObserver instead of via ShellObserver. There are implicit
assumptions that these layout managers run in order. In order to preserve the
ordering, OnShelfAlignmentChanged() is implemented here in terms of a
WmRootWindowControllerObserver instead of a ShellObserver. This gives us a sane
ordering (or at least ordering as we've always had it in ash).

https://codereview.chromium.org/1923983003/diff/1/ash/wm/always_on_top_contro...
File ash/wm/always_on_top_controller.cc (right):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/always_on_top_contro...
ash/wm/always_on_top_controller.cc:75: void
AlwaysOnTopController::OnWindowDestroying(wm::WmWindow* window) {
On 2016/04/27 20:48:05, sky wrote:
> I'm hoping the order doesn't really matter here (before vs after). No tests
> tripped over it, so I'm hoping I'm good.

Acknowledged.

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/fullscreen_wi...
File ash/wm/common/fullscreen_window_finder.h (right):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/fullscreen_wi...
ash/wm/common/fullscreen_window_finder.h:8: #include "ash/ash_export.h"
not needed?

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/switchable_wi...
File ash/wm/common/switchable_windows.cc (left):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/switchable_wi...
ash/wm/common/switchable_windows.cc:13: kShellWindowId_DefaultContainer,
On 2016/04/27 20:48:05, sky wrote:
> I think this looks better, but I ran 'git cl format' and you can see the
result.

Yeah. I'm OK with the new formatting, and I'm also OK with changing it back. I
sometimes overrule 'git cl format' when it does egregious things, but that can
be a losing battle in the long run. Up to you.

https://codereview.chromium.org/1923983003/diff/1/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_layout_manager.h (right):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_layout_manager.h:55: // Overridden from
aura::LayoutManager:
nit: from wm::WmLayoutManager

[sky, 2016-04-27 23:33:28]

https://codereview.chromium.org/1923983003/diff/1/ash/shelf/shelf_layout_mana...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1923983003/diff/1/ash/shelf/shelf_layout_mana...
ash/shelf/shelf_layout_manager.cc:194: // WmRootWindowControllerObservers may be
notified after ShellObservers.
On 2016/04/27 23:24:24, James Cook wrote:
> The part that I missed in thinking about this was that some other layout
manager
> had been converted to WmRootWindowControllerObserver, and the ordering problem
> was with the ones that have moved there. Is the comment below any clearer?
> 
> NOTE: Some other layout managers also observe for OnShelfAlignmentChanged()
via
> WmRootWindowControllerObserver instead of via ShellObserver. There are
implicit
> assumptions that these layout managers run in order. In order to preserve the
> ordering, OnShelfAlignmentChanged() is implemented here in terms of a
> WmRootWindowControllerObserver instead of a ShellObserver. This gives us a
sane
> ordering (or at least ordering as we've always had it in ash).

I copied what you have here.

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/fullscreen_wi...
File ash/wm/common/fullscreen_window_finder.h (right):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/common/fullscreen_wi...
ash/wm/common/fullscreen_window_finder.h:8: #include "ash/ash_export.h"
On 2016/04/27 23:24:24, James Cook wrote:
> not needed?

I added ASH_EXPORT to the function below and kept it.

https://codereview.chromium.org/1923983003/diff/1/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_layout_manager.h (right):

https://codereview.chromium.org/1923983003/diff/1/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_layout_manager.h:55: // Overridden from
aura::LayoutManager:
On 2016/04/27 23:24:24, James Cook wrote:
> nit: from wm::WmLayoutManager

Done.

[sky, 2016-04-27 23:36:40]

The CQ bit was checked by sky@chromium.org

[sky, 2016-04-27 23:36:41]

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

[commit-bot: I haz the power, 2016-04-27 23:37:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923983003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923983003/40001

[commit-bot: I haz the power, 2016-04-28 00:17:10]

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

[commit-bot: I haz the power, 2016-04-28 00:17:11]

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

[sky, 2016-04-28 03:42:44]

The CQ bit was checked by sky@chromium.org

[sky, 2016-04-28 03:42:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/1923983003/#ps60001
(title: "fix always-on-top and remove mus changes")

[commit-bot: I haz the power, 2016-04-28 03:43:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923983003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923983003/60001

[sky, 2016-04-28 03:43:11]

I removed the mus changes from this patch as they appear to be causing problems.
Will investigate separately as they need not be part of this patch.

[James Cook, 2016-04-28 04:15:19]

On 2016/04/28 03:43:11, sky wrote:
> I removed the mus changes from this patch as they appear to be causing
problems.
> Will investigate separately as they need not be part of this patch.

SGTM

[commit-bot: I haz the power, 2016-04-28 04:15:36]

Committed patchset #4 (id:60001)

[sadrul, 2016-04-28 06:01:30]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2016-04-28 06:01:31]

https://codereview.chromium.org/1923983003/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1923983003/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:238:
->AddObserver(root_window_controller_observer_.get());
This causes a crash in chrome --mash, because this |root_window_| is not one of
the root-windows ash::Shell knows about, and so it doesn't have any
RootWindowController associated with it.

[sky, 2016-04-28 15:23:40]

https://codereview.chromium.org/1923983003/diff/60001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/1923983003/diff/60001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:238:
->AddObserver(root_window_controller_observer_.get());
On 2016/04/28 06:01:31, sadrul wrote:
> This causes a crash in chrome --mash, because this |root_window_| is not one
of
> the root-windows ash::Shell knows about, and so it doesn't have any
> RootWindowController associated with it.

I added a suggestion to your patch.

[commit-bot: I haz the power, 2016-04-30 17:16:08]

Patchset 4 (id:??) landed as
https://crrev.com/4dc7d0496343775c344899a2ad5742865f81a93b
Cr-Commit-Position: refs/heads/master@{#390297}

[commit-bot: I haz the power, 2016-04-30 17:54:08]

Description was changed from

==========
Makes WorkspaceLayoutManager use ash/wm/common types

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Makes WorkspaceLayoutManager use ash/wm/common types

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/4dc7d0496343775c344899a2ad5742865f81a93b
Cr-Commit-Position: refs/heads/master@{#390297}
==========

[msw, 2016-05-05 22:56:19]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2016-05-05 22:56:20]

I think this broke left/right shelf layout in mash...

It works at the prior ToT position #390296 (452ce11)
Mash crashes at this ToT position...
It's broken at ToT position #390541 (fa1ae58)

Scott/Sadrul, would you mind taking a quick look?
I can help if needed, but didn't look closely here.

[sky, 2016-05-06 15:40:07]

I believe Sadrul fixed that here: https://codereview.chromium.org/1916913007 .

I'll try tip of tree shortly and see what's what.

  -Scott

On Thu, May 5, 2016 at 3:56 PM,  <msw@chromium.org> wrote:
> I think this broke left/right shelf layout in mash...
>
> It works at the prior ToT position #390296 (452ce11)
> Mash crashes at this ToT position...
> It's broken at ToT position #390541 (fa1ae58)
>
> Scott/Sadrul, would you mind taking a quick look?
> I can help if needed, but didn't look closely here.
>
> https://codereview.chromium.org/1923983003/

-- 
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-05-06 15:52:48]

Indeed, I don't see a crash on tip of tree. Let me know if you are
seeing otherwise and I'll investigate.

  -Scott

On Fri, May 6, 2016 at 8:40 AM, Scott Violet <sky@chromium.org> wrote:
> I believe Sadrul fixed that here: https://codereview.chromium.org/1916913007
.
>
> I'll try tip of tree shortly and see what's what.
>
>   -Scott
>
> On Thu, May 5, 2016 at 3:56 PM,  <msw@chromium.org> wrote:
>> I think this broke left/right shelf layout in mash...
>>
>> It works at the prior ToT position #390296 (452ce11)
>> Mash crashes at this ToT position...
>> It's broken at ToT position #390541 (fa1ae58)
>>
>> Scott/Sadrul, would you mind taking a quick look?
>> I can help if needed, but didn't look closely here.
>>
>> https://codereview.chromium.org/1923983003/

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

[James Cook, 2016-05-06 17:24:01]

On 2016/05/06 15:52:48, sky wrote:
> Indeed, I don't see a crash on tip of tree. Let me know if you are
> seeing otherwise and I'll investigate.
> 
>   -Scott
> 
> On Fri, May 6, 2016 at 8:40 AM, Scott Violet <mailto:sky@chromium.org> wrote:
> > I believe Sadrul fixed that here: https://codereview.chromium.org/1916913007
> .
> >
> > I'll try tip of tree shortly and see what's what.
> >
> >   -Scott
> >
> > On Thu, May 5, 2016 at 3:56 PM,  <mailto:msw@chromium.org> wrote:
> >> I think this broke left/right shelf layout in mash...
> >>
> >> It works at the prior ToT position #390296 (452ce11)
> >> Mash crashes at this ToT position...
> >> It's broken at ToT position #390541 (fa1ae58)
> >>
> >> Scott/Sadrul, would you mind taking a quick look?
> >> I can help if needed, but didn't look closely here.
> >>
> >> https://codereview.chromium.org/1923983003/
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 

I think this issue is that setting the shelf alignment to left or right breaks
its layout. The shelf icons disappear. There's no crash at ToT, but I see the
layout issue.

[sky, 2016-05-06 17:26:18]

Ok, I'll look at that.

On Fri, May 6, 2016 at 10:24 AM,  <jamescook@chromium.org> wrote:
> On 2016/05/06 15:52:48, sky wrote:
>> Indeed, I don't see a crash on tip of tree. Let me know if you are
>> seeing otherwise and I'll investigate.
>>
>> -Scott
>>
>> On Fri, May 6, 2016 at 8:40 AM, Scott Violet <mailto:sky@chromium.org>
>> wrote:
>> > I believe Sadrul fixed that here:
>> > https://codereview.chromium.org/1916913007
>> .
>> >
>> > I'll try tip of tree shortly and see what's what.
>> >
>> > -Scott
>> >
>> > On Thu, May 5, 2016 at 3:56 PM, <mailto:msw@chromium.org> wrote:
>> >> I think this broke left/right shelf layout in mash...
>> >>
>> >> It works at the prior ToT position #390296 (452ce11)
>> >> Mash crashes at this ToT position...
>> >> It's broken at ToT position #390541 (fa1ae58)
>> >>
>> >> Scott/Sadrul, would you mind taking a quick look?
>> >> I can help if needed, but didn't look closely here.
>> >>
>> >> https://codereview.chromium.org/1923983003/
>>
>> --
>> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> I think this issue is that setting the shelf alignment to left or right
> breaks
> its layout. The shelf icons disappear. There's no crash at ToT, but I see
> the
> layout issue.
>
>
> https://codereview.chromium.org/1923983003/

-- 
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-05-09 14:21:50]

I couldn't readily find how to change the alignment on tip of tree,
and I'm about to head out on vacation. I believe what needs to happen
is ShelfLayoutManager should continue to override
OnShelfAlignmentChanged, but only call LayoutShelf() if in mus. If not
urgent wait until Thursday and I'll fix it first thing then.

Sorry for the hassle.

  -Scott

On Fri, May 6, 2016 at 10:26 AM, Scott Violet <sky@chromium.org> wrote:
> Ok, I'll look at that.
>
> On Fri, May 6, 2016 at 10:24 AM,  <jamescook@chromium.org> wrote:
>> On 2016/05/06 15:52:48, sky wrote:
>>> Indeed, I don't see a crash on tip of tree. Let me know if you are
>>> seeing otherwise and I'll investigate.
>>>
>>> -Scott
>>>
>>> On Fri, May 6, 2016 at 8:40 AM, Scott Violet <mailto:sky@chromium.org>
>>> wrote:
>>> > I believe Sadrul fixed that here:
>>> > https://codereview.chromium.org/1916913007
>>> .
>>> >
>>> > I'll try tip of tree shortly and see what's what.
>>> >
>>> > -Scott
>>> >
>>> > On Thu, May 5, 2016 at 3:56 PM, <mailto:msw@chromium.org> wrote:
>>> >> I think this broke left/right shelf layout in mash...
>>> >>
>>> >> It works at the prior ToT position #390296 (452ce11)
>>> >> Mash crashes at this ToT position...
>>> >> It's broken at ToT position #390541 (fa1ae58)
>>> >>
>>> >> Scott/Sadrul, would you mind taking a quick look?
>>> >> I can help if needed, but didn't look closely here.
>>> >>
>>> >> https://codereview.chromium.org/1923983003/
>>>
>>> --
>>> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>>>
>>
>> I think this issue is that setting the shelf alignment to left or right
>> breaks
>> its layout. The shelf icons disappear. There's no crash at ToT, but I see
>> the
>> layout issue.
>>
>>
>> https://codereview.chromium.org/1923983003/

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