Only enable mirroring for 2 displays

Only enable mirroring for 2 displays

BUG=597672
Committed: https://crrev.com/4c4c53ac6228ae9fca327c824d03f3c4c981b4af
Cr-Commit-Position: refs/heads/master@{#384138}

[stevenjb, 2016-03-30 01:02:52]

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

[oshima, 2016-03-30 18:51:22]

https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/display_options.js:491: allowMirroring
= true;
The original code was based on the assumption that there are at most 2 displays
connected.

It's proobably better to pass the boolean flag if mirroring is enabled rather
than having logic here. (DisplayManager::num_connected_displays() == 2)

[stevenjb, 2016-03-30 19:03:30]

https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/display_options.js:491: allowMirroring
= true;
On 2016/03/30 18:51:22, oshima wrote:
> The original code was based on the assumption that there are at most 2
displays
> connected.
> 
> It's proobably better to pass the boolean flag if mirroring is enabled rather
> than having logic here. (DisplayManager::num_connected_displays() == 2)

I don't understand your suggestion. Do you want me to move this logic to C++?
I'm not sure what the advantage to that would be.

[oshima, 2016-03-30 19:20:27]

On 2016/03/30 19:03:30, stevenjb wrote:
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> File chrome/browser/resources/options/chromeos/display_options.js (right):
> 
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> chrome/browser/resources/options/chromeos/display_options.js:491:
allowMirroring
> = true;
> On 2016/03/30 18:51:22, oshima wrote:
> > The original code was based on the assumption that there are at most 2
> displays
> > connected.
> > 
> > It's proobably better to pass the boolean flag if mirroring is enabled
rather
> > than having logic here. (DisplayManager::num_connected_displays() == 2)
> 
> I don't understand your suggestion. Do you want me to move this logic to C++?
> I'm not sure what the advantage to that would be.

so that we don't have to change JS when we enable mirroring on 3 displays.

I don't have strong opinion. I thought that if we ought to add new parameter,
(physically connected display), we can simply add boolean instead.

[stevenjb, 2016-03-30 19:54:56]

On 2016/03/30 19:20:27, oshima wrote:
> On 2016/03/30 19:03:30, stevenjb wrote:
> >
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> > File chrome/browser/resources/options/chromeos/display_options.js (right):
> > 
> >
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> > chrome/browser/resources/options/chromeos/display_options.js:491:
> allowMirroring
> > = true;
> > On 2016/03/30 18:51:22, oshima wrote:
> > > The original code was based on the assumption that there are at most 2
> > displays
> > > connected.
> > > 
> > > It's proobably better to pass the boolean flag if mirroring is enabled
> rather
> > > than having logic here. (DisplayManager::num_connected_displays() == 2)
> > 
> > I don't understand your suggestion. Do you want me to move this logic to
C++?
> > I'm not sure what the advantage to that would be.
> 
> so that we don't have to change JS when we enable mirroring on 3 displays.
> 
> I don't have strong opinion. I thought that if we ought to add new parameter,
> (physically connected display), we can simply add boolean instead.

We're probably going to have to change the UI anyway when we make that change.

Typically we do not attempt to provide all of the constraints of the model to
the UI, instead the UI matches the constraints of the model. For example, the UI
"knows" that mirroring does not apply for just 1 display, and it "knows" that
overscan is disabled for an internal display. I would rather have all of the
constraints in the JS code instead of in the interface.

Soon I will be switching the settings UI to use the system.display extension
API. I would like to have that API match the model as closely as possible, i.e.
possibly even more controller logic will be moved from C++ to JS.

[oshima, 2016-03-30 20:21:49]

On 2016/03/30 19:54:56, stevenjb wrote:
> On 2016/03/30 19:20:27, oshima wrote:
> > On 2016/03/30 19:03:30, stevenjb wrote:
> > >
> >
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> > > File chrome/browser/resources/options/chromeos/display_options.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> > > chrome/browser/resources/options/chromeos/display_options.js:491:
> > allowMirroring
> > > = true;
> > > On 2016/03/30 18:51:22, oshima wrote:
> > > > The original code was based on the assumption that there are at most 2
> > > displays
> > > > connected.
> > > > 
> > > > It's proobably better to pass the boolean flag if mirroring is enabled
> > rather
> > > > than having logic here. (DisplayManager::num_connected_displays() == 2)
> > > 
> > > I don't understand your suggestion. Do you want me to move this logic to
> C++?
> > > I'm not sure what the advantage to that would be.
> > 
> > so that we don't have to change JS when we enable mirroring on 3 displays.
> > 
> > I don't have strong opinion. I thought that if we ought to add new
parameter,
> > (physically connected display), we can simply add boolean instead.
> 
> We're probably going to have to change the UI anyway when we make that change.
> 
> Typically we do not attempt to provide all of the constraints of the model to
> the UI, instead the UI matches the constraints of the model. For example, the
UI
> "knows" that mirroring does not apply for just 1 display, and it "knows" that
> overscan is disabled for an internal display. I would rather have all of the
> constraints in the JS code instead of in the interface.
> 
> Soon I will be switching the settings UI to use the system.display extension
> API. I would like to have that API match the model as closely as possible,
i.e.
> possibly even more controller logic will be moved from C++ to JS.

I don't have strong opinion about it, so that is fine with me.

The real issue was trying to point out was that this will present "mirroring
option" even when it will not work (3 displays + unified desktop) If we want to
accept that case and just wait for new UI, it's fine with me too, but can you
update the comment?

[stevenjb, 2016-03-30 20:30:29]

On 2016/03/30 20:21:49, oshima wrote:
> On 2016/03/30 19:54:56, stevenjb wrote:
> > On 2016/03/30 19:20:27, oshima wrote:
> > > On 2016/03/30 19:03:30, stevenjb wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> > > > File chrome/browser/resources/options/chromeos/display_options.js
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1844783002/diff/1/chrome/browser/resources/op...
> > > > chrome/browser/resources/options/chromeos/display_options.js:491:
> > > allowMirroring
> > > > = true;
> > > > On 2016/03/30 18:51:22, oshima wrote:
> > > > > The original code was based on the assumption that there are at most 2
> > > > displays
> > > > > connected.
> > > > > 
> > > > > It's proobably better to pass the boolean flag if mirroring is enabled
> > > rather
> > > > > than having logic here. (DisplayManager::num_connected_displays() ==
2)
> > > > 
> > > > I don't understand your suggestion. Do you want me to move this logic to
> > C++?
> > > > I'm not sure what the advantage to that would be.
> > > 
> > > so that we don't have to change JS when we enable mirroring on 3 displays.
> > > 
> > > I don't have strong opinion. I thought that if we ought to add new
> parameter,
> > > (physically connected display), we can simply add boolean instead.
> > 
> > We're probably going to have to change the UI anyway when we make that
change.
> > 
> > Typically we do not attempt to provide all of the constraints of the model
to
> > the UI, instead the UI matches the constraints of the model. For example,
the
> UI
> > "knows" that mirroring does not apply for just 1 display, and it "knows"
that
> > overscan is disabled for an internal display. I would rather have all of the
> > constraints in the JS code instead of in the interface.
> > 
> > Soon I will be switching the settings UI to use the system.display extension
> > API. I would like to have that API match the model as closely as possible,
> i.e.
> > possibly even more controller logic will be moved from C++ to JS.
> 
> I don't have strong opinion about it, so that is fine with me.
> 
> The real issue was trying to point out was that this will present "mirroring
> option" even when it will not work (3 displays + unified desktop) If we want
to
> accept that case and just wait for new UI, it's fine with me too, but can you
> update the comment?

Ah, now I understand. We need to disable mirroring if the number of physical
displays != 2, independent of the unified setting, got it. Will fix.

[stevenjb, 2016-03-30 21:18:46]

PTAL

[oshima, 2016-03-30 21:46:02]

lgtm

[stevenjb, 2016-03-30 21:51:57]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-03-30 21:52:39]

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

[commit-bot: I haz the power, 2016-03-30 23:13:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-30 23:15:04]

Description was changed from

==========
Only enable mirroring for 2 displays

BUG=597672
==========

to

==========
Only enable mirroring for 2 displays

BUG=597672

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

[commit-bot: I haz the power, 2016-03-30 23:15:05]

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