Multi-monitor extension API.

Multi-monitor extension API.

Bring systemInfo.display out of experimental.

Rename function systemInfo.display.get() to systemInfo.display.getDisplayInfo(),
and implement this function for Chrome OS.

See design docs at 
https://code.google.com/p/chromium/issues/detail?id=168709#c13

For other platforms we plan to eventually implement the API but the
schedule is still TBD.

TODO: determine the display device DPI.
TODO: wire up notification of display changes.

BUG=168709
TEST=CQ, manual testing

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

[hshi1, 2013-01-24 01:58:50]

Please take a look. This implements systemInfo.display.getDisplayInfo() for
Chrome OS.

mihaip@, asargent@: owners of
chrome/browser/extensions, chrome/common/extensions, chrome/renderer/extensions,
chrome/renderer/resources/extensions

sky@, thakis@: owners of
chrome/chrome_browser_extensions.gypi,
chrome/renderer/resources/renderer_resources.grd

oshima@, mukai@: FYI

[hshi1, 2013-01-25 16:40:36]

On 2013/01/24 01:58:50, hshi1 wrote:
> Please take a look. This implements systemInfo.display.getDisplayInfo() for
> Chrome OS.
> 
> mihaip@, asargent@: owners of
> chrome/browser/extensions, chrome/common/extensions,
chrome/renderer/extensions,
> chrome/renderer/resources/extensions
> 
> sky@, thakis@: owners of
> chrome/chrome_browser_extensions.gypi,
> chrome/renderer/resources/renderer_resources.grd
> 
> oshima@, mukai@: FYI

Ping... Mihai or Antony, can you please review the extensions changes? It is
fairly straightforward. Thanks.

[oshima, 2013-01-25 17:28:41]

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

https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:19:
const float kDpi96 = 96.0;
no indent

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

https://codereview.chromium.org/11882009/diff/55009/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/11882009/diff/55009/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:34: Bounds bounds;
Don't you need work area bounds as well?

https://codereview.chromium.org/11882009/diff/55009/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_info_display_custom_bindings.js
(right):

https://codereview.chromium.org/11882009/diff/55009/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_info_display_custom_bindings.js:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
ditto

[hshi1, 2013-01-25 18:42:29]

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

https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/25 17:28:41, oshima wrote:
> 2013

Done.

https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:19:
const float kDpi96 = 96.0;
On 2013/01/25 17:28:41, oshima wrote:
> no indent

Done.

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

https://codereview.chromium.org/11882009/diff/55009/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/01/25 17:28:41, oshima wrote:
> 2013

Done.

https://codereview.chromium.org/11882009/diff/55009/chrome/common/extensions/...
chrome/common/extensions/api/system_info_display.idl:34: Bounds bounds;
On 2013/01/25 17:28:41, oshima wrote:
> Don't you need work area bounds as well?

Done.

https://codereview.chromium.org/11882009/diff/55009/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_info_display_custom_bindings.js
(right):

https://codereview.chromium.org/11882009/diff/55009/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_info_display_custom_bindings.js:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/25 17:28:41, oshima wrote:
> ditto

Done.

[hshi1, 2013-01-25 18:52:56]

Mike, can you help review the changes in chrome/.../extensions? I just realized
(told by Mukai) that mihaip@ is no longer with Google. Thanks!

[oshima, 2013-01-25 23:21:52]

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

https://codereview.chromium.org/11882009/diff/63006/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:47:
unit->dpi_y = kDpi96;
sorry i didn't catch it first time. You probably should multiply this with
Display::device_scale_factor()

[hshi1, 2013-01-25 23:46:07]

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

https://codereview.chromium.org/11882009/diff/63006/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:47:
unit->dpi_y = kDpi96;
On 2013/01/25 23:21:53, oshima wrote:
> sorry i didn't catch it first time. You probably should multiply this with
> Display::device_scale_factor()

Done.

[oshima, 2013-01-25 23:55:51]

lgtm

[hshi1, 2013-01-26 01:40:58]

Matt, would you be able to review the extensions part? I couldn't get a response
from Antony or Mike. Thanks!

[Matt Perry, 2013-01-26 01:52:29]

https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_display/display_info_provider.h
(right):

https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider.h:18:
static DisplayInfoProvider* GetDisplayInfo();
Why this rename? DisplayInfoProvider::GetDisplayInfo() is redundant.

https://codereview.chromium.org/11882009/diff/78001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/11882009/diff/78001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:330: "extension_types":
["platform_app"]
Let's let extensions have access to this as well.

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_info_display_custom_bindings.js
(right):

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_info_display_custom_bindings.js:17:
new chrome.Event("systemInfo.display.onDisplayChanged");
nit: indent + 2

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_info_display_custom_bindings.js:17:
new chrome.Event("systemInfo.display.onDisplayChanged");
How does this event work? Nothing ever adds a listener to it, and it doesn't
seem to be dispatched ever.

[hshi1, 2013-01-26 02:02:16]

https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_display/display_info_provider.h
(right):

https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider.h:18:
static DisplayInfoProvider* GetDisplayInfo();
On 2013/01/26 01:52:29, Matt Perry wrote:
> Why this rename? DisplayInfoProvider::GetDisplayInfo() is redundant.

Unfortunately this name must match the API definition (system_info_display.idl),
due to the current design of the SystemInfoProvider class (which is shared by
all systemInfo namespace).

The API definition originally used the simple "Get" name but the latest spec
decides to use GetDisplayInfo instead.

https://codereview.chromium.org/11882009/diff/78001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/11882009/diff/78001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:330: "extension_types":
["platform_app"]
On 2013/01/26 01:52:29, Matt Perry wrote:
> Let's let extensions have access to this as well.

Done. I actually believe we need all 3, the use case for GVC is likely a
packaged app.

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_info_display_custom_bindings.js
(right):

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_info_display_custom_bindings.js:17:
new chrome.Event("systemInfo.display.onDisplayChanged");
On 2013/01/26 01:52:29, Matt Perry wrote:
> nit: indent + 2

Done.

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_info_display_custom_bindings.js:17:
new chrome.Event("systemInfo.display.onDisplayChanged");
On 2013/01/26 01:52:29, Matt Perry wrote:
> How does this event work? Nothing ever adds a listener to it, and it doesn't
> seem to be dispatched ever.

See the CL description -- the event is currently a TODO item, I'm leaving this
to a separate CL.

[Matt Perry, 2013-01-26 02:05:03]

lgtm

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_info_display_custom_bindings.js
(right):

https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_info_display_custom_bindings.js:17:
new chrome.Event("systemInfo.display.onDisplayChanged");
On 2013/01/26 02:02:17, hshi1 wrote:
> On 2013/01/26 01:52:29, Matt Perry wrote:
> > How does this event work? Nothing ever adds a listener to it, and it doesn't
> > seem to be dispatched ever.
> 
> See the CL description -- the event is currently a TODO item, I'm leaving this
> to a separate CL.
> 

I would leave this whole file out of this CL then, since it doesn't seem to be
needed until you hook up the event. Not a big deal though.

[hshi1, 2013-01-26 02:12:19]

Adding Nico (somehow I forgot to add you for OWNERS review). Need your review
for chrome/chrome_browser_extensions.gypi and renderer_resources.grd. Thanks.

[Nico, 2013-01-26 04:41:30]

grd and gypi lgtm

[commit-bot: I haz the power, 2013-01-26 04:44:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/11882009/86004

[commit-bot: I haz the power, 2013-01-26 04:44:50]

Presubmit check for 11882009-86004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/extensions/api/system_info_display/system_info_display_api.cc,
line 13, 85 chars

Presubmit checks took 4.7s to calculate.