Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2603563002:
Plumbs touch calibration API to the display manager (Closed)
|
Created:
3 years, 12 months ago by malaykeshav Modified:
3 years, 11 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, extensions-reviews_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlumbs touch calibration API to the display manager
- touchCalibrationSet() and touchCalibrationReset() API now work.
- Fixes minor bug in TouchCalibratorController to allow graceful
destruction during tests.
- Updates DisplayInforProviderChromeOS unittests.
BUG=667921, 672990
COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration
Committed: https://crrev.com/795de01a5f04ab858c07e0af0ba56370eaae0aab
Cr-Commit-Position: refs/heads/master@{#441022}
Patch Set 1 : It works. The plumbing! #
Total comments: 12
Patch Set 2 : Resolve comments #Patch Set 3 : Restored is_calibrating() check #
Total comments: 3
Patch Set 4 : Nits #
Total comments: 8
Patch Set 5 : It works. The plumbing! #Patch Set 6 : It works. The plumbing! #
Messages
Total messages: 43 (26 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
malaykeshav@chromium.org changed reviewers: + stevenjb@chromium.org
PTAL
https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:391: *error = std::string("Display Id(" + id + ") is an invalid display ID"); nit: std::string() not needed. https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:693: // The display points must be withing the display bounds while the touch within https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:709: } Nit: invert comment above or logic order to match. https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.h (right): https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.h:59: chromeos::OverscanCalibrator* GetOverscanCalibrator(const std::string& id); nit: Order here should really be: private methods private members https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... File extensions/browser/api/system_display/system_display_api.cc (left): https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... extensions/browser/api/system_display/system_display_api.cc:276: } This seems like a useful error? (Although the logic appears to be backwards).
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Resolved comments. PTAL https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:391: *error = std::string("Display Id(" + id + ") is an invalid display ID"); On 2016/12/27 at 19:37:26, stevenjb wrote: > nit: std::string() not needed. Done https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:693: // The display points must be withing the display bounds while the touch On 2016/12/27 at 19:37:26, stevenjb wrote: > within done https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:709: } On 2016/12/27 at 19:37:26, stevenjb wrote: > Nit: invert comment above or logic order to match. Done https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.h (right): https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.h:59: chromeos::OverscanCalibrator* GetOverscanCalibrator(const std::string& id); On 2016/12/27 at 19:37:27, stevenjb wrote: > nit: Order here should really be: > > private methods > > private members Done https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... File extensions/browser/api/system_display/system_display_api.cc (left): https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... extensions/browser/api/system_display/system_display_api.cc:276: } On 2016/12/27 at 19:37:27, stevenjb wrote: > This seems like a useful error? (Although the logic appears to be backwards). This check has been moved to ValidateParamsForTouchCalibration() in https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... Will add it to TouchCalibrationStart() when this API is plumbed through after the UX is complete.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... File extensions/browser/api/system_display/system_display_api.cc (left): https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... extensions/browser/api/system_display/system_display_api.cc:276: } On 2016/12/27 20:07:01, malaykeshav wrote: > On 2016/12/27 at 19:37:27, stevenjb wrote: > > This seems like a useful error? (Although the logic appears to be backwards). > > This check has been moved to ValidateParamsForTouchCalibration() in > https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... > > Will add it to TouchCalibrationStart() when this API is plumbed through after > the UX is complete. Looking at that code, that seems like a strange place to do this test. The error really should be detected here. I am fine with moving the test to DisplayInfoProvider::TouchCalibrationStart and passing &error to that like in the other methods, but just moving that test, even temporarily, seems odd.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:40002) has been deleted
PTAL https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... File extensions/browser/api/system_display/system_display_api.cc (left): https://codereview.chromium.org/2603563002/diff/20001/extensions/browser/api/... extensions/browser/api/system_display/system_display_api.cc:276: } On 2016/12/27 at 21:58:12, stevenjb wrote: > On 2016/12/27 20:07:01, malaykeshav wrote: > > On 2016/12/27 at 19:37:27, stevenjb wrote: > > > This seems like a useful error? (Although the logic appears to be backwards). > > > > This check has been moved to ValidateParamsForTouchCalibration() in > > https://codereview.chromium.org/2603563002/diff/20001/chrome/browser/extensio... > > > > Will add it to TouchCalibrationStart() when this API is plumbed through after > > the UX is complete. > > Looking at that code, that seems like a strange place to do this test. The error really should be detected here. I am fine with moving the test to DisplayInfoProvider::TouchCalibrationStart and passing &error to that like in the other methods, but just moving that test, even temporarily, seems odd. Done
lgtm https://codereview.chromium.org/2603563002/diff/90001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2603563002/diff/90001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:729: (*error) = "Another touch calibration is already active."; nit: () not needed https://codereview.chromium.org/2603563002/diff/90001/extensions/browser/api/... File extensions/browser/api/system_display/display_info_provider.cc (right): https://codereview.chromium.org/2603563002/diff/90001/extensions/browser/api/... extensions/browser/api/system_display/display_info_provider.cc:142: NOTREACHED(); // Implemented on Chrome OS only in override. two spaces before //
The CQ bit was checked by malaykeshav@chromium.org
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/2603563002/#ps110001 (title: "Nits")
https://codereview.chromium.org/2603563002/diff/90001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2603563002/diff/90001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:729: (*error) = "Another touch calibration is already active."; On 2016/12/28 at 18:05:12, stevenjb wrote: > nit: () not needed done
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
malaykeshav@chromium.org changed reviewers: + reillyg@chromium.org
Adding reillyg@ for LGTM on DisplayInfoProviderChromeOS
lgtm with nits https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:397: "displays, cannot be calibrated for touch."; nit: No comma. https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:681: gfx::Size display_size(bounds.width, bounds.height); nit: move this down to right before you use it. It is odd to create a valid based on |bounds| before it is validated. https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:695: *error = std::string("Display points and touch points cannot have") + No need to explicitly construct an std::string. You can use a multiline string literal like: *error = "Display points and touch points cannot have" " negative coordinates"; Concatenation will be done at compile time. https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:702: *error = std::string("Display point coordinates cannot be more than") + Same here.
https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:397: "displays, cannot be calibrated for touch."; On 2016/12/29 at 19:04:36, Reilly Grant wrote: > nit: No comma. Done https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:681: gfx::Size display_size(bounds.width, bounds.height); On 2016/12/29 at 19:04:36, Reilly Grant wrote: > nit: move this down to right before you use it. It is odd to create a valid based on |bounds| before it is validated. Done https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:695: *error = std::string("Display points and touch points cannot have") + On 2016/12/29 at 19:04:36, Reilly Grant wrote: > No need to explicitly construct an std::string. You can use a multiline string literal like: > > *error = "Display points and touch points cannot have" > " negative coordinates"; > > Concatenation will be done at compile time. Ack. Done https://codereview.chromium.org/2603563002/diff/110001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:702: *error = std::string("Display point coordinates cannot be more than") + On 2016/12/29 at 19:04:36, Reilly Grant wrote: > Same here. Done
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2603563002/#ps130001 (title: "It works. The plumbing!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Plumbs touch calibration API to the display manager - touchCalibrationSet() and touchCalibrationReset() API now work. - Fixes minor bug in TouchCalibratorController to allow graceful destruction during tests. - Updates DisplayInforProviderChromeOS unittests. BUG=667921 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration ========== to ========== Plumbs touch calibration API to the display manager - touchCalibrationSet() and touchCalibrationReset() API now work. - Fixes minor bug in TouchCalibratorController to allow graceful destruction during tests. - Updates DisplayInforProviderChromeOS unittests. BUG=667921,672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2603563002/#ps150001 (title: "It works. The plumbing!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1483082360037170,
"parent_rev": "ae70e2036fdd47d42285cf842da3ffe956fc5e10", "commit_rev":
"2cc63a467a49bcc0b2ffd4ae9c1b1b548c403692"}
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1483082360037170,
"parent_rev": "2c9e12e54b31680bc8457fc6340dc9dd7044deb7", "commit_rev":
"c302bb19a62787e2e1a2b8305274f4660891a5ce"}
Message was sent while issue was closed.
Description was changed from ========== Plumbs touch calibration API to the display manager - touchCalibrationSet() and touchCalibrationReset() API now work. - Fixes minor bug in TouchCalibratorController to allow graceful destruction during tests. - Updates DisplayInforProviderChromeOS unittests. BUG=667921,672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration ========== to ========== Plumbs touch calibration API to the display manager - touchCalibrationSet() and touchCalibrationReset() API now work. - Fixes minor bug in TouchCalibratorController to allow graceful destruction during tests. - Updates DisplayInforProviderChromeOS unittests. BUG=667921,672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration Review-Url: https://codereview.chromium.org/2603563002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Plumbs touch calibration API to the display manager - touchCalibrationSet() and touchCalibrationReset() API now work. - Fixes minor bug in TouchCalibratorController to allow graceful destruction during tests. - Updates DisplayInforProviderChromeOS unittests. BUG=667921,672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration Review-Url: https://codereview.chromium.org/2603563002 ========== to ========== Plumbs touch calibration API to the display manager - touchCalibrationSet() and touchCalibrationReset() API now work. - Fixes minor bug in TouchCalibratorController to allow graceful destruction during tests. - Updates DisplayInforProviderChromeOS unittests. BUG=667921,672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration Committed: https://crrev.com/795de01a5f04ab858c07e0af0ba56370eaae0aab Cr-Commit-Position: refs/heads/master@{#441022} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/795de01a5f04ab858c07e0af0ba56370eaae0aab Cr-Commit-Position: refs/heads/master@{#441022} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
