Adds touch calibration methods to system_display.idl API

Adds touch calibration methods to system.idl API

Adds the three methods to system.idl file.
  - touchCalibrationStart
  - touchCalibrationSet
  - touchCalibrationReset

Also adds the required object type/structs to system.idl file.
  - Point
  - TouchCalibrationPair
  - TouchCalibrationPairQuad

Design Doc=go/cros-touch-calibration

BUG=667921
COMPONENT=System_display.idl, system display interface, display info provider

Committed: https://crrev.com/3495268d34006ced6ab8d55d3d55edd0f3469613
Cr-Commit-Position: refs/heads/master@{#435514}

[malaykeshav, 2016-11-29 00:10:39]

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

[commit-bot: I haz the power, 2016-11-29 00:11:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2016-11-29 00:22:35]

Patchset #1 (id:1) has been deleted

[malaykeshav, 2016-11-29 00:27:39]

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

[commit-bot: I haz the power, 2016-11-29 00:28:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2016-11-29 02:31:18]

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

[malaykeshav, 2016-11-29 02:31:18]

Added touch calibration methods to system_display.idl

[commit-bot: I haz the power, 2016-11-29 03:49:58]

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

[commit-bot: I haz the power, 2016-11-29 03:49:59]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2016-11-29 18:44:45]

PTAL

[stevenjb, 2016-11-29 18:53:29]

Please update system_display_apitest.cc also.

https://codereview.chromium.org/2538623002/diff/20001/extensions/browser/api/...
File extensions/browser/api/system_display/system_display_api.cc (right):

https://codereview.chromium.org/2538623002/diff/20001/extensions/browser/api/...
extensions/browser/api/system_display/system_display_api.cc:146: return
RespondNow(Error("Invalid display ID: " + params->id));
nit: It might be nice to explictly check for in-progress (vs. some other error)
if that makes sense.

https://codereview.chromium.org/2538623002/diff/20001/extensions/browser/api/...
extensions/browser/api/system_display/system_display_api.cc:157: return
RespondNow(Error("Invalid display ID: " + params->id));
{}

https://codereview.chromium.org/2538623002/diff/20001/extensions/common/api/s...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2538623002/diff/20001/extensions/common/api/s...
extensions/common/api/system_display.idl:278: // for display |id| is in progress
this will do nothing.
We should throw an error of calibration is in progress (or can not be started).

https://codereview.chromium.org/2538623002/diff/20001/extensions/common/api/s...
extensions/common/api/system_display.idl:288: //     |bounds.left| and
|bounds.top| values are no-op.
s/no-op/ignored/ ?

[malaykeshav, 2016-11-29 21:59:02]

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

[commit-bot: I haz the power, 2016-11-29 22:00:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2016-11-29 22:00:37]

Patchset #2 (id:40001) has been deleted

[malaykeshav, 2016-11-29 22:01:29]

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

[malaykeshav, 2016-11-29 22:02:33]

Description was changed from

==========
Adds touch calibration methods to system.idl API

Adds the three methods to system.idl file.
  - touchCalibrationStart
  - touchCalibrationSet
  - touchCalibrationReset

Also adds the required object type/structs to system.idl file.
  - Point
  - TouchCalibrationPair
  - TouchCalibrationPairQuad

Design Doc=go/cros-touch-calibration

BUG=667921
COMPONENT=System.idl, system display interface, display info provider
==========

to

==========
Adds touch calibration methods to system.idl API

Adds the three methods to system.idl file.
  - touchCalibrationStart
  - touchCalibrationSet
  - touchCalibrationReset

Also adds the required object type/structs to system.idl file.
  - Point
  - TouchCalibrationPair
  - TouchCalibrationPairQuad

Design Doc=go/cros-touch-calibration

BUG=667921
COMPONENT=System_display.idl, system display interface, display info provider
==========

[commit-bot: I haz the power, 2016-11-29 22:03:10]

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-11-30 00:06:20]

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

[commit-bot: I haz the power, 2016-11-30 00:06:21]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[malaykeshav, 2016-11-30 00:06:48]

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

[commit-bot: I haz the power, 2016-11-30 00:07:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[malaykeshav, 2016-11-30 00:53:16]

PTAL. Updated code based on previous comments.

No tests to update in system_display_api_unittest as of now. Will require an
update if there is a change to GetDisplayInfo in the future.

https://codereview.chromium.org/2538623002/diff/20001/extensions/browser/api/...
File extensions/browser/api/system_display/system_display_api.cc (right):

https://codereview.chromium.org/2538623002/diff/20001/extensions/browser/api/...
extensions/browser/api/system_display/system_display_api.cc:146: return
RespondNow(Error("Invalid display ID: " + params->id));
On 2016/11/29 at 18:53:29, stevenjb wrote:
> nit: It might be nice to explictly check for in-progress (vs. some other
error) if that makes sense.

Done

https://codereview.chromium.org/2538623002/diff/20001/extensions/browser/api/...
extensions/browser/api/system_display/system_display_api.cc:157: return
RespondNow(Error("Invalid display ID: " + params->id));
On 2016/11/29 at 18:53:29, stevenjb wrote:
> {}

Done

https://codereview.chromium.org/2538623002/diff/20001/extensions/common/api/s...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2538623002/diff/20001/extensions/common/api/s...
extensions/common/api/system_display.idl:278: // for display |id| is in progress
this will do nothing.
On 2016/11/29 at 18:53:29, stevenjb wrote:
> We should throw an error of calibration is in progress (or can not be
started).

Done

https://codereview.chromium.org/2538623002/diff/20001/extensions/common/api/s...
extensions/common/api/system_display.idl:288: //     |bounds.left| and
|bounds.top| values are no-op.
On 2016/11/29 at 18:53:29, stevenjb wrote:
> s/no-op/ignored/ ?

Done

[commit-bot: I haz the power, 2016-11-30 03:09:38]

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

[commit-bot: I haz the power, 2016-11-30 03:09:39]

Dry run: This issue passed the CQ dry run.

[malaykeshav, 2016-11-30 19:18:37]

Ping

[stevenjb, 2016-11-30 21:42:04]

Hm, we really should have a test that verifies that passing |pairs| to
touchCalibrationSet() correctly sets values in a fake DisplayInfoProvider
implementation, but as long as we test the actual implementaion when we add it,
I guess that's OK.

Also, I would be more comfortable with a DisplayInfoProviderChromeOS
implementation that does nothing instead of NOTREACHED() since currently using
this API can only trigger NOTREACHED(), even on Chrome OS.

That said, the code LGTM.

https://codereview.chromium.org/2538623002/diff/60001/extensions/browser/api/...
File extensions/browser/api/system_display/display_info_provider.cc (right):

https://codereview.chromium.org/2538623002/diff/60001/extensions/browser/api/...
extensions/browser/api/system_display/display_info_provider.cc:141: return true;
This really should return false by default.

[malaykeshav, 2016-11-30 21:53:39]

https://codereview.chromium.org/2538623002/diff/60001/extensions/browser/api/...
File extensions/browser/api/system_display/display_info_provider.cc (right):

https://codereview.chromium.org/2538623002/diff/60001/extensions/browser/api/...
extensions/browser/api/system_display/display_info_provider.cc:141: return true;
On 2016/11/30 at 21:42:04, stevenjb wrote:
> This really should return false by default.

Done

[malaykeshav, 2016-11-30 22:06:03]

malaykeshav@chromium.org changed reviewers:
+ asargent@chromium.org, mpearson@chromium.org

[malaykeshav, 2016-11-30 22:06:04]

I've touched more components:

- mpearson@, could you PTAL at /extension_function_histogram_value.h ?

- asargent,@ could you PTAL at /extensions/common/api/system_display.idl ?

[malaykeshav, 2016-11-30 22:06:46]

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

[commit-bot: I haz the power, 2016-11-30 22:07:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2016-11-30 23:05:39]

histograms.xml lgtm

[commit-bot: I haz the power, 2016-11-30 23:27:27]

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

[commit-bot: I haz the power, 2016-11-30 23:27:28]

Dry run: This issue passed the CQ dry run.

[asargent_no_longer_on_chrome, 2016-12-01 00:38:36]

lgtm

https://codereview.chromium.org/2538623002/diff/80001/extensions/common/api/s...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2538623002/diff/80001/extensions/common/api/s...
extensions/common/api/system_display.idl:278: // for display |id| is already in
progress this will throw an error.
Would the correct thing be for a developer to call touchCalibrationReset if
calibration is already in progress and this returned an error? If so that might
be worth mentioning in the comment.

[malaykeshav, 2016-12-01 00:42:22]

https://codereview.chromium.org/2538623002/diff/80001/extensions/common/api/s...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2538623002/diff/80001/extensions/common/api/s...
extensions/common/api/system_display.idl:278: // for display |id| is already in
progress this will throw an error.
On 2016/12/01 at 00:38:36, Antony Sargent wrote:
> Would the correct thing be for a developer to call touchCalibrationReset if
calibration is already in progress and this returned an error? If so that might
be worth mentioning in the comment.

The only way to quit the calibration is by some kind of user input, that is,
either by pressing the escape key or by completing the calibration process that
is under progress.

[malaykeshav, 2016-12-01 01:06:13]

The CQ bit was checked by malaykeshav@chromium.org

[malaykeshav, 2016-12-01 01:06:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2538623002/#ps80001
(title: "Adds touch calibration methods to system.idl API")

[commit-bot: I haz the power, 2016-12-01 01:07: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-12-01 01:16:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480554373119440,
"parent_rev": "9c43c2789a6472e9471ffdd59ea3a4e968ab8bbf", "commit_rev":
"9f452a1dbe8610ae53020ffa836118600a835f64"}

[commit-bot: I haz the power, 2016-12-01 01:17:21]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-12-01 01:21:11]

Description was changed from

==========
Adds touch calibration methods to system.idl API

Adds the three methods to system.idl file.
  - touchCalibrationStart
  - touchCalibrationSet
  - touchCalibrationReset

Also adds the required object type/structs to system.idl file.
  - Point
  - TouchCalibrationPair
  - TouchCalibrationPairQuad

Design Doc=go/cros-touch-calibration

BUG=667921
COMPONENT=System_display.idl, system display interface, display info provider
==========

to

==========
Adds touch calibration methods to system.idl API

Adds the three methods to system.idl file.
  - touchCalibrationStart
  - touchCalibrationSet
  - touchCalibrationReset

Also adds the required object type/structs to system.idl file.
  - Point
  - TouchCalibrationPair
  - TouchCalibrationPairQuad

Design Doc=go/cros-touch-calibration

BUG=667921
COMPONENT=System_display.idl, system display interface, display info provider

Committed: https://crrev.com/3495268d34006ced6ab8d55d3d55edd0f3469613
Cr-Commit-Position: refs/heads/master@{#435514}
==========

[commit-bot: I haz the power, 2016-12-01 01:21:12]

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