Use the current primary display to update output/configure.

Use the current primary display to update output/configure.

BUG=633666
TEST=tested on samus:
Connect external display  and switch the primary to external using alt-fullscreen. Arc app can move within the external display.

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

[oshima, 2016-08-02 19:53:35]

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

[oshima, 2016-08-02 19:54:13]

Description was changed from

==========
Use the current primary display to update output/configure.

BUG=633666
==========

to

==========
Use the current primary display to update output/configure.

BUG=633666
==========

[oshima, 2016-08-02 19:54:13]

oshima@chromium.org changed reviewers:
+ reveman@chromium.org

[commit-bot: I haz the power, 2016-08-02 19:54:29]

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-02 20:34:59]

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

[commit-bot: I haz the power, 2016-08-02 20:35:05]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-08-02 21:05:24]

do we also need to send events when primary display changes?

[oshima, 2016-08-02 21:08:50]

On 2016/08/02 21:05:24, reveman wrote:
> do we also need to send events when primary display changes?

This is to catch the case when primary changed without bounds change. (between
exactly same displays)
I thought this isn't important for arc++, but I can add it if necessary. Please
let me know.

[reveman, 2016-08-02 21:19:43]

On 2016/08/02 at 21:08:50, oshima wrote:
> On 2016/08/02 21:05:24, reveman wrote:
> > do we also need to send events when primary display changes?
> 
> This is to catch the case when primary changed without bounds change. (between
exactly same displays)
> I thought this isn't important for arc++, but I can add it if necessary.
Please let me know.

If changing primary display to a new display with different metrics generate
metrics changed events then current code lg. It's not clear from the
DisplayObserver interface that this would be the case.

[oshima, 2016-08-02 21:43:24]

On 2016/08/02 21:19:43, reveman wrote:
> On 2016/08/02 at 21:08:50, oshima wrote:
> > On 2016/08/02 21:05:24, reveman wrote:
> > > do we also need to send events when primary display changes?
> > 
> > This is to catch the case when primary changed without bounds change.
(between
> exactly same displays)
> > I thought this isn't important for arc++, but I can add it if necessary.
> Please let me know.
> 
> If changing primary display to a new display with different metrics generate
> metrics changed events then current code lg. It's not clear from the
> DisplayObserver interface that this would be the case.

Yes, if the new primary has different size/workspace/dsf, then these changes
will be caught by flags.

[oshima, 2016-08-02 21:43:26]

On 2016/08/02 21:19:43, reveman wrote:
> On 2016/08/02 at 21:08:50, oshima wrote:
> > On 2016/08/02 21:05:24, reveman wrote:
> > > do we also need to send events when primary display changes?
> > 
> > This is to catch the case when primary changed without bounds change.
(between
> exactly same displays)
> > I thought this isn't important for arc++, but I can add it if necessary.
> Please let me know.
> 
> If changing primary display to a new display with different metrics generate
> metrics changed events then current code lg. It's not clear from the
> DisplayObserver interface that this would be the case.

Yes, if the new primary has different size/workspace/dsf, then these changes
will be caught by flags.

[reveman, 2016-08-02 22:12:35]

lgtm % nits

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1041: class WaylandDisplayObserver : public
display::DisplayObserver {
nit: can you change the name of this to WaylandPrimaryDisplayObserver or add a
comment above to make it clear that it observes the primary display

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1060: if (changed_metrics &
nit: can you add a short comment here that makes it clear that a primary display
change will also result in a change to other metrics even if old primary display
metrics and new primary display metrics have not actually changed. I did not
expect that from this observer api

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1068: void SendPrimaryDisplayMetrics() {
nit: keep name as SendDisplayMetrics if you change the class name as suggested
above

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1069: // TODO(reveman): Multi-display support.
nit: move this TODO to where we instantiate the class as that's where we
currently limit us to only primary display

[oshima, 2016-08-02 23:45:22]

Description was changed from

==========
Use the current primary display to update output/configure.

BUG=633666
==========

to

==========
Use the current primary display to update output/configure.

BUG=633666
TEST=tested on samus:
Connect external display  and switch the primary to external using
alt-fullscreen. Arc app can move within the external display.
==========

[oshima, 2016-08-03 00:05:10]

Patchset #2 (id:20001) has been deleted

[oshima, 2016-08-03 00:22:56]

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1041: class WaylandDisplayObserver : public
display::DisplayObserver {
On 2016/08/02 22:12:35, reveman wrote:
> nit: can you change the name of this to WaylandPrimaryDisplayObserver or add a
> comment above to make it clear that it observes the primary display

Done.

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1060: if (changed_metrics &
On 2016/08/02 22:12:35, reveman wrote:
> nit: can you add a short comment here that makes it clear that a primary
display
> change will also result in a change to other metrics even if old primary
display
> metrics and new primary display metrics have not actually changed. I did not
> expect that from this observer api

Done.

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1068: void SendPrimaryDisplayMetrics() {
On 2016/08/02 22:12:35, reveman wrote:
> nit: keep name as SendDisplayMetrics if you change the class name as suggested
> above

Done.

https://codereview.chromium.org/2202343003/diff/1/components/exo/wayland/serv...
components/exo/wayland/server.cc:1069: // TODO(reveman): Multi-display support.
On 2016/08/02 22:12:35, reveman wrote:
> nit: move this TODO to where we instantiate the class as that's where we
> currently limit us to only primary display

Done.

[oshima, 2016-08-03 00:23:17]

The CQ bit was checked by oshima@chromium.org

[oshima, 2016-08-03 00:23:17]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2202343003/#ps60001
(title: "updated comments")

[commit-bot: I haz the power, 2016-08-03 00:24: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-03 00:55:20]

Description was changed from

==========
Use the current primary display to update output/configure.

BUG=633666
TEST=tested on samus:
Connect external display  and switch the primary to external using
alt-fullscreen. Arc app can move within the external display.
==========

to

==========
Use the current primary display to update output/configure.

BUG=633666
TEST=tested on samus:
Connect external display  and switch the primary to external using
alt-fullscreen. Arc app can move within the external display.
==========

[commit-bot: I haz the power, 2016-08-03 00:55:21]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-03 00:57:09]

Description was changed from

==========
Use the current primary display to update output/configure.

BUG=633666
TEST=tested on samus:
Connect external display  and switch the primary to external using
alt-fullscreen. Arc app can move within the external display.
==========

to

==========
Use the current primary display to update output/configure.

BUG=633666
TEST=tested on samus:
Connect external display  and switch the primary to external using
alt-fullscreen. Arc app can move within the external display.

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

[commit-bot: I haz the power, 2016-08-03 00:57:10]

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