Chromium Code Reviews
Help | Chromium Project | Sign in
(1076)

Issue 11882009: Multi-monitor extension API. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 3 months ago by hshi1
Modified:
1 year, 2 months ago
CC:
chromium-reviews_chromium.org, Aaron Boodman, darin-cc_chromium.org, mp3geek
Visibility:
Public.

Description

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

Patch Set 1 : Use the systemInfo.display namespace and remove the experimental one. #

Patch Set 2 : Add permission message enum. #

Patch Set 3 : Fix unit_test failure in PermissionsTest.PermissionMessages. #

Patch Set 4 : Add chromeos specific display info provider. #

Patch Set 5 : Add the mock implementation for chromeos so as to test from the app side. #

Patch Set 6 : Wire up the getDisplayInfo API with ash display manager. #

Patch Set 7 : Rebase to 177802 and resolve conflicts with histogram related changes. #

Patch Set 8 : Fix a style warning. #

Patch Set 9 : Rebased to 178436. #

Patch Set 10 : Leave a TODO for the hard-coded DPI value. #

Total comments: 10

Patch Set 11 : Add work area parameters and update copyright year. #

Total comments: 2

Patch Set 12 : Use display's device scale factor in DPI calculation. #

Total comments: 9

Patch Set 13 : Address mpcomplete's comments. #

Patch Set 14 : Rebase to 179043. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+198 lines, -56 lines) Lint Patch
M chrome/browser/extensions/api/system_info_display/display_info_provider.h View 1 chunk +3 lines, -3 lines 0 comments 0 errors Download
M chrome/browser/extensions/api/system_info_display/display_info_provider.cc View 1 chunk +1 line, -1 line 0 comments 0 errors Download
A chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +63 lines, -0 lines 0 comments 2 errors Download
M chrome/browser/extensions/api/system_info_display/system_info_display_api.h View 1 2 3 4 5 6 1 chunk +4 lines, -4 lines 0 comments 0 errors Download
M chrome/browser/extensions/api/system_info_display/system_info_display_api.cc View 1 2 3 4 5 6 7 1 chunk +7 lines, -9 lines 0 comments 1 errors Download
M chrome/browser/extensions/api/system_info_display/system_info_display_apitest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +15 lines, -14 lines 0 comments 0 errors Download
M chrome/browser/extensions/extension_function_histogram_value.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments ? errors Download
M chrome/chrome_browser_extensions.gypi View 1 2 3 4 5 6 7 8 9 10 12 13 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/common/extensions/api/_permission_features.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M chrome/common/extensions/api/api.gyp View 1 2 3 4 5 6 7 8 9 10 12 13 2 chunks +1 line, -1 line 0 comments ? errors Download
A chrome/common/extensions/api/system_info_display.idl View 1 2 3 4 5 6 7 8 9 10 1 chunk +50 lines, -0 lines 0 comments ? errors Download
M chrome/common/extensions/extension_manifest_constants.cc View 1 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/common/extensions/permissions/api_permission.h View 1 12 13 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/common/extensions/permissions/api_permission.cc View 1 12 13 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/common/extensions/permissions/permission_message.h View 1 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/common/extensions/permissions/permission_set_unittest.cc View 1 2 12 13 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/renderer/extensions/dispatcher.cc View 1 1 chunk +2 lines, -0 lines 0 comments ? errors Download
A chrome/renderer/resources/extensions/system_info_display_custom_bindings.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +18 lines, -0 lines 0 comments ? errors Download
M chrome/renderer/resources/renderer_resources.grd View 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/test/data/extensions/api_test/systeminfo/display/manifest.json View 1 chunk +2 lines, -2 lines 0 comments ? errors Download
M chrome/test/data/extensions/api_test/systeminfo/display/test_display_api.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +20 lines, -21 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 16
hshi1
Please take a look. This implements systemInfo.display.getDisplayInfo() for Chrome OS. mihaip@, asargent@: owners of chrome/browser/extensions, ...
1 year, 2 months ago #1
hshi1
On 2013/01/24 01:58:50, hshi1 wrote: > Please take a look. This implements systemInfo.display.getDisplayInfo() for > ...
1 year, 2 months ago #2
oshima
https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc File chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc#newcode1 chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...
1 year, 2 months ago #3
hshi1
https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc File chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/11882009/diff/55009/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc#newcode1 chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...
1 year, 2 months ago #4
hshi1
Mike, can you help review the changes in chrome/.../extensions? I just realized (told by Mukai) ...
1 year, 2 months ago #5
oshima
https://codereview.chromium.org/11882009/diff/63006/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc File chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/11882009/diff/63006/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc#newcode47 chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:47: unit->dpi_y = kDpi96; sorry i didn't catch it first ...
1 year, 2 months ago #6
hshi1
https://codereview.chromium.org/11882009/diff/63006/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc File chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/11882009/diff/63006/chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc#newcode47 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: > ...
1 year, 2 months ago #7
oshima
lgtm
1 year, 2 months ago #8
hshi1
Matt, would you be able to review the extensions part? I couldn't get a response ...
1 year, 2 months ago #9
Matt Perry
https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions/api/system_info_display/display_info_provider.h File chrome/browser/extensions/api/system_info_display/display_info_provider.h (right): https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions/api/system_info_display/display_info_provider.h#newcode18 chrome/browser/extensions/api/system_info_display/display_info_provider.h:18: static DisplayInfoProvider* GetDisplayInfo(); Why this rename? DisplayInfoProvider::GetDisplayInfo() is redundant. ...
1 year, 2 months ago #10
hshi1
https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions/api/system_info_display/display_info_provider.h File chrome/browser/extensions/api/system_info_display/display_info_provider.h (right): https://codereview.chromium.org/11882009/diff/78001/chrome/browser/extensions/api/system_info_display/display_info_provider.h#newcode18 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: ...
1 year, 2 months ago #11
Matt Perry
lgtm https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources/extensions/system_info_display_custom_bindings.js File chrome/renderer/resources/extensions/system_info_display_custom_bindings.js (right): https://codereview.chromium.org/11882009/diff/78001/chrome/renderer/resources/extensions/system_info_display_custom_bindings.js#newcode17 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: > ...
1 year, 2 months ago #12
hshi1
Adding Nico (somehow I forgot to add you for OWNERS review). Need your review for ...
1 year, 2 months ago #13
Nico
grd and gypi lgtm
1 year, 2 months ago #14
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/11882009/86004
1 year, 2 months ago #15
I haz the power (commit-bot)
1 year, 2 months ago #16
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6