Add additional properties to display system info on ChromeOS

Add additional properties to display system info on ChromeOS

Also, move the info querying logic to the UI thread (as DisplayManager is
not thread safe and lives on UI thread).

added properties (only for ChromeOS):
rotation -> the screen rotation
mirroring_source_id -> the id of mirroring display (if any)
visible_area -> the display bounds withing its sceen (calculated from overscan)

BUG=237314
TEST=Changed insents/rotation/mirroring on an external monitor and
validated the values returned by the getDisplayInfo are correct

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206298

[tbarzic, 2013-06-10 18:10:54]

oshima: display_info_provider_chromeos.cc, and to check comments in
system_info_display.idl make sense.
asargent: owner for the extensions code
gspencer: who wrote up the current api proposal

for reference:
https://docs.google.com/a/google.com/document/d/1Kh6doO_zABOm3IejjU-NEn--jABG...

[Greg Spencer (Chromium), 2013-06-10 19:17:42]

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
(right):

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:70:
unit->rotation = static_cast<int>(display->rotation()) * kBaseRotation;
Should this be "% 360"?  Just to prevent invalid values: the only valid values
should be 0, 90, 180 and 270.

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/system_info_display_apitest.cc
(right):

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/system_info_display_apitest.cc:21:
for (int i = 0; i < 2; i++) {
Don't you want to go to i < 4 to catch all the rotations and to future proof the
code for when we support more than two displays?

[tbarzic, 2013-06-10 19:53:50]

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
(right):

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:70:
unit->rotation = static_cast<int>(display->rotation()) * kBaseRotation;
On 2013/06/10 19:17:42, Greg Spencer (Chromium) wrote:
> Should this be "% 360"?  Just to prevent invalid values: the only valid values
> should be 0, 90, 180 and 270.

I think it is safe to assume rotation returned from display->rotation is < 4 (it
returns enum with 4 values).
Hm, when I think of it, it may be better not to rely on enum values
corresponding to rotation_degrees / 90. Added utility method for conversion..

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/system_info_display_apitest.cc
(right):

https://codereview.chromium.org/16687002/diff/19001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/system_info_display_apitest.cc:21:
for (int i = 0; i < 2; i++) {
On 2013/06/10 19:17:42, Greg Spencer (Chromium) wrote:
> Don't you want to go to i < 4 to catch all the rotations and to future proof
the
> code for when we support more than two displays?

done, but I don't think that this adds any test coverage (as we get values
directly from a mock).

[oshima, 2013-06-10 22:52:24]

https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
(right):

https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:90:
unit->visible_area.top = overscan_insets.top();
Overscan shouldn't really be visible to app or chrome.
How this is going to be used?

[tbarzic, 2013-06-11 01:24:05]

https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
(right):

https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:90:
unit->visible_area.top = overscan_insets.top();
On 2013/06/10 22:52:24, oshima wrote:
> Overscan shouldn't really be visible to app or chrome.
> How this is going to be used?

this is going to be used to set overscan insets from an app in kiosk mode.
the app would change the overscan insets by setting new visible area value (and
this value would be used as a reference)

[asargent_no_longer_on_chrome, 2013-06-11 18:22:01]

extensions changes lgtm

https://codereview.chromium.org/16687002/diff/29001/chrome/common/extensions/...
File chrome/common/extensions/api/system_info_display.idl (right):

https://codereview.chromium.org/16687002/diff/29001/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:24: // If mirroring is not
in progress, set to an empty string.
You could also make the field optional (just add a question mark after the type
name), and then it would be undefined if mirroring is not in progress. I think
the code people would write would be the same either way, e.g.

chrome.systemInfo.display.getDisplayInfo(function(info) {
  if (info.mirroringSourceId) {
    // do something, etc.
    ...
  }
  ...
});

so feel free to leave it as you currently have if you want.

https://codereview.chromium.org/16687002/diff/29001/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:52: Bounds workArea;
style nit: it would be a little easier to read these if you introduced a leading
newline above each starting comment block, e.g.

// Whatever
long foo;

// Something
DOMString bar;

[oshima, 2013-06-11 18:37:54]

https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
(right):

https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:90:
unit->visible_area.top = overscan_insets.top();
On 2013/06/11 01:24:05, tbarzic wrote:
> On 2013/06/10 22:52:24, oshima wrote:
> > Overscan shouldn't really be visible to app or chrome.
> > How this is going to be used?
> 
> this is going to be used to set overscan insets from an app in kiosk mode.
> the app would change the overscan insets by setting new visible area value
(and
> this value would be used as a reference)

I didn't fully understand the usage scenario.
Do we want the operator of management console adjust the value based on
value of the other display?

[tbarzic, 2013-06-11 20:01:29]

On 2013/06/11 18:37:54, oshima wrote:
>
https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
> (right):
> 
>
https://codereview.chromium.org/16687002/diff/29001/chrome/browser/extensions...
>
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:90:
> unit->visible_area.top = overscan_insets.top();
> On 2013/06/11 01:24:05, tbarzic wrote:
> > On 2013/06/10 22:52:24, oshima wrote:
> > > Overscan shouldn't really be visible to app or chrome.
> > > How this is going to be used?
> > 
> > this is going to be used to set overscan insets from an app in kiosk mode.
> > the app would change the overscan insets by setting new visible area value
> (and
> > this value would be used as a reference)
> 
> I didn't fully understand the usage scenario.
> Do we want the operator of management console adjust the value based on
> value of the other display?

well, basically, the point of passing visible area here is to give the extension
the initial insets so it can be shown to the user and give the user options to
tweak the insets (something like what we have in display settings page)(also so
the extension can undo changes by setting the initial insets).
Greg may have some more insight into this.

when I think about it, it may be better to pass insets directly than visible
area, as we may not determine right/bottom insets from the visible area because
we don't really know the screen width/height; I'd prefer this than changing the
current meaning of bounds property
(I think that Greg had an assumption that bounds property will have the screen
size, and not the logical display size, when he designed these changes).

[tbarzic, 2013-06-11 23:56:52]

https://codereview.chromium.org/16687002/diff/29001/chrome/common/extensions/...
File chrome/common/extensions/api/system_info_display.idl (right):

https://codereview.chromium.org/16687002/diff/29001/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:24: // If mirroring is not
in progress, set to an empty string.
On 2013/06/11 18:22:01, Antony Sargent wrote:
> You could also make the field optional (just add a question mark after the
type
> name), and then it would be undefined if mirroring is not in progress. I think
> the code people would write would be the same either way, e.g.
> 
> chrome.systemInfo.display.getDisplayInfo(function(info) {
>   if (info.mirroringSourceId) {
>     // do something, etc.
>     ...
>   }
>   ...
> });
> 
> so feel free to leave it as you currently have if you want.

ok, I'll leave it as it is then

https://codereview.chromium.org/16687002/diff/29001/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:52: Bounds workArea;
On 2013/06/11 18:22:01, Antony Sargent wrote:
> style nit: it would be a little easier to read these if you introduced a
leading
> newline above each starting comment block, e.g.
> 
> // Whatever
> long foo;
> 
> // Something
> DOMString bar;
> 

Done.

[oshima, 2013-06-12 00:08:53]

display_info_provider_chromeos lgtm

[Greg Spencer (Chromium), 2013-06-13 17:02:58]

lgtm

[commit-bot: I haz the power, 2013-06-13 20:19:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/16687002/45001

[commit-bot: I haz the power, 2013-06-13 22:32:53]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-13 23:12:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/16687002/56001

[commit-bot: I haz the power, 2013-06-14 00:26:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-14 01:58:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/16687002/63001

[commit-bot: I haz the power, 2013-06-14 04:08:57]

Change committed as 206298