mash: Remove WmRootWindowControllerObserver.

mash: Remove WmRootWindowControllerObserver.

WorkAreaChanged -> DisplayObserver::OnDisplayMetricsChanged.
ShelfAlignmentChanged -> ShellObserver::ShelfAlignmentChanged
TODO: Add checks for matching Display id? (didn't happen previously)

Notify ShelfLayoutManager of alignment changes before ShellObserver.
(to replace RootWindowControllerObserverImpl's apparent intent)

Add default empty DisplayObserver function impls.

BUG=NONE
TEST=No behavior changes.
R=sky@chromium.org

Committed: https://crrev.com/b2f3c132f2ab37ee750170dc3a9041dc8f726118
Cr-Commit-Position: refs/heads/master@{#415698}

[msw, 2016-08-31 00:49:05]

Description was changed from

==========
mash: Remove WmRootWindowControllerObserver.

BUG=NONE
TEST=No behavior changes.
R=
==========

to

==========
mash: Remove WmRootWindowControllerObserver.

WorkAreaChanged -> DisplayObserver::OnDisplayMetricsChanged.
ShelfAlignmentChanged -> ShellObserver::ShelfAlignmentChanged
TODO: Add checks for matching Display id? (didn't happen previously)

Notify ShelfLayoutManager of alignment changes before ShellObserver.
(to replace RootWindowControllerObserverImpl's apparent intent)

Add default empty DisplayObserver function impls.

BUG=NONE
TEST=No behavior changes.
R=sky@chromium.org
==========

[msw, 2016-08-31 00:49:05]

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

[msw, 2016-08-31 00:51:50]

Hey Scott, please take a look; thanks!

[msw, 2016-08-31 00:52:06]

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

[commit-bot: I haz the power, 2016-08-31 00:52:24]

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-31 01:57:05]

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

[commit-bot: I haz the power, 2016-08-31 01:57:06]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-31 16:04:31]

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
File ash/common/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:779: Relayout();
Should this early out if display id is not the same as the display id associated
with dock_container_?

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:279: const gfx::Rect
work_area(wm::GetDisplayWorkAreaBounds(window_));
Similar comment here about checking ids.

[msw, 2016-08-31 17:07:32]

Please take another look; thanks!

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
File ash/common/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:779: Relayout();
On 2016/08/31 16:04:30, sky wrote:
> Should this early out if display id is not the same as the display id
associated
> with dock_container_?

Done. I did in the prior patch set, but the old code didn't check.

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:279: const gfx::Rect
work_area(wm::GetDisplayWorkAreaBounds(window_));
On 2016/08/31 16:04:30, sky wrote:
> Similar comment here about checking ids.

Done.

[msw, 2016-08-31 17:07:40]

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

[commit-bot: I haz the power, 2016-08-31 17:07:56]

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

[sky, 2016-08-31 17:45:41]

LGTM

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
File ash/common/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:779: Relayout();
On 2016/08/31 17:07:32, msw wrote:
> On 2016/08/31 16:04:30, sky wrote:
> > Should this early out if display id is not the same as the display id
> associated
> > with dock_container_?
> 
> Done. I did in the prior patch set, but the old code didn't check.

The reason the old code didn't need to is because
WmRootWindowControllerObserver::OnWorkAreaChanged was only sent when the work
area of the display associated with the WmRootWindowController changed. That
isn't the case with display::DisplayObserver.

[msw, 2016-08-31 17:55:29]

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
File ash/common/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
ash/common/wm/dock/docked_window_layout_manager.cc:779: Relayout();
On 2016/08/31 17:45:41, sky wrote:
> On 2016/08/31 17:07:32, msw wrote:
> > On 2016/08/31 16:04:30, sky wrote:
> > > Should this early out if display id is not the same as the display id
> > associated
> > > with dock_container_?
> > 
> > Done. I did in the prior patch set, but the old code didn't check.
> 
> The reason the old code didn't need to is because
> WmRootWindowControllerObserver::OnWorkAreaChanged was only sent when the work
> area of the display associated with the WmRootWindowController changed. That
> isn't the case with display::DisplayObserver.

AFAICT, that's not true. Each WmRootWindowControllerAura instance (one per
display) would ping all of its observers anytime the metrics changed for *any*
display. ( see the removed OnDisplayMetricsChanged in
ash/aura/wm_root_window_controller_aura.cc ). It's possible this check will
avoid some unnecessary work, but it might also cause issues with unified mode or
something, I'm just not sure. I can land with the new checks and we can tackle
any issues as they surface.

[sky, 2016-08-31 18:04:34]

You are right. I like landing with new checks.

On Wed, Aug 31, 2016 at 10:55 AM,  <msw@chromium.org> wrote:
>
>
https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
> File ash/common/wm/dock/docked_window_layout_manager.cc (right):
>
>
https://codereview.chromium.org/2295003002/diff/20001/ash/common/wm/dock/dock...
> ash/common/wm/dock/docked_window_layout_manager.cc:779: Relayout();
> On 2016/08/31 17:45:41, sky wrote:
>> On 2016/08/31 17:07:32, msw wrote:
>> > On 2016/08/31 16:04:30, sky wrote:
>> > > Should this early out if display id is not the same as the display
> id
>> > associated
>> > > with dock_container_?
>> >
>> > Done. I did in the prior patch set, but the old code didn't check.
>>
>> The reason the old code didn't need to is because
>> WmRootWindowControllerObserver::OnWorkAreaChanged was only sent when
> the work
>> area of the display associated with the WmRootWindowController
> changed. That
>> isn't the case with display::DisplayObserver.
>
> AFAICT, that's not true. Each WmRootWindowControllerAura instance (one
> per display) would ping all of its observers anytime the metrics changed
> for *any* display. ( see the removed OnDisplayMetricsChanged in
> ash/aura/wm_root_window_controller_aura.cc ). It's possible this check
> will avoid some unnecessary work, but it might also cause issues with
> unified mode or something, I'm just not sure. I can land with the new
> checks and we can tackle any issues as they surface.
>
> https://codereview.chromium.org/2295003002/

-- 
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-31 18:19:59]

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

[commit-bot: I haz the power, 2016-08-31 18:20:00]

Dry run: This issue passed the CQ dry run.

[msw, 2016-08-31 18:41:58]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-08-31 18:42:40]

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-31 18:47:50]

Description was changed from

==========
mash: Remove WmRootWindowControllerObserver.

WorkAreaChanged -> DisplayObserver::OnDisplayMetricsChanged.
ShelfAlignmentChanged -> ShellObserver::ShelfAlignmentChanged
TODO: Add checks for matching Display id? (didn't happen previously)

Notify ShelfLayoutManager of alignment changes before ShellObserver.
(to replace RootWindowControllerObserverImpl's apparent intent)

Add default empty DisplayObserver function impls.

BUG=NONE
TEST=No behavior changes.
R=sky@chromium.org
==========

to

==========
mash: Remove WmRootWindowControllerObserver.

WorkAreaChanged -> DisplayObserver::OnDisplayMetricsChanged.
ShelfAlignmentChanged -> ShellObserver::ShelfAlignmentChanged
TODO: Add checks for matching Display id? (didn't happen previously)

Notify ShelfLayoutManager of alignment changes before ShellObserver.
(to replace RootWindowControllerObserverImpl's apparent intent)

Add default empty DisplayObserver function impls.

BUG=NONE
TEST=No behavior changes.
R=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-08-31 18:47:51]

Committed patchset #3 (id:40001)

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

Description was changed from

==========
mash: Remove WmRootWindowControllerObserver.

WorkAreaChanged -> DisplayObserver::OnDisplayMetricsChanged.
ShelfAlignmentChanged -> ShellObserver::ShelfAlignmentChanged
TODO: Add checks for matching Display id? (didn't happen previously)

Notify ShelfLayoutManager of alignment changes before ShellObserver.
(to replace RootWindowControllerObserverImpl's apparent intent)

Add default empty DisplayObserver function impls.

BUG=NONE
TEST=No behavior changes.
R=sky@chromium.org
==========

to

==========
mash: Remove WmRootWindowControllerObserver.

WorkAreaChanged -> DisplayObserver::OnDisplayMetricsChanged.
ShelfAlignmentChanged -> ShellObserver::ShelfAlignmentChanged
TODO: Add checks for matching Display id? (didn't happen previously)

Notify ShelfLayoutManager of alignment changes before ShellObserver.
(to replace RootWindowControllerObserverImpl's apparent intent)

Add default empty DisplayObserver function impls.

BUG=NONE
TEST=No behavior changes.
R=sky@chromium.org

Committed: https://crrev.com/b2f3c132f2ab37ee750170dc3a9041dc8f726118
Cr-Commit-Position: refs/heads/master@{#415698}
==========

[commit-bot: I haz the power, 2016-08-31 18:50:21]

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