Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2638903003:
Updates touch calibration API and plumbs through the native touch calibration method (Closed)
|
Created:
3 years, 11 months ago by malaykeshav Modified:
3 years, 11 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description- Plumbs through native touch calibration from MD settings to
display manager via the system display API.
Modifications done to System Display API:
- Renames API method.
- Splits touchCalibrationSet() method into 2 separate methods.
(Why? https://docs.google.com/document/d/1fFvSk35wWRoKspJRR1hYMqlfeyhYO4l7s92g-5m9vPo/)
- Updates SystemDisplayAPI Test
BUG=672990
COMPONENT=Display Info Provider, Chrome OS, System Display API, Touch Calibration
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2638903003
Cr-Commit-Position: refs/heads/master@{#444950}
Committed: https://chromium.googlesource.com/chromium/src/+/5f64df43034f52aea554ca764633c58a72f44831
Patch Set 1 #
Total comments: 14
Patch Set 2 : Adds a callback and renames StartNativeTouchCalibration to ShowNativeTouchCalibration #Patch Set 3 : Resolving comments #
Total comments: 4
Patch Set 4 : Rename files #Patch Set 5 : Resolving Comments #
Total comments: 23
Patch Set 6 : Resolve comments #
Total comments: 3
Patch Set 7 : Resolve comments #Patch Set 8 : Update histogram.xml #Messages
Total messages: 74 (54 generated)
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...
malaykeshav@chromium.org changed reviewers: + stevenjb@chromium.org
This change plumbs through the API that initiates the native touch calibration UX. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Plumbs through native touch calibration from MD settings to display manager Native touch calibration can now be started from MD settings. The md setting for this is only visible if the correct switch is enabled. BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration ========== to ========== Plumbs through native touch calibration from MD settings to display manager Native touch calibration can now be started from MD settings. The md setting for this is only visible if the correct switch is enabled. BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from
==========
Plumbs through native touch calibration from MD settings to display manager
Native touch calibration can now be started from MD settings.
The md setting for this is only visible if the correct switch is
enabled.
BUG=672990
COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
- Plumbs through native touch calibration from MD settings to
display manager via the system display API.
- Renames API method that starts native touch calibration.
- Splits touchCalibrationSet() API method into 2 separate methods. This is
required
since custom touch calibration UX will require an un-transformed touch input
to
collect calibration data.
BUG=672990
COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
Description was changed from
==========
- Plumbs through native touch calibration from MD settings to
display manager via the system display API.
- Renames API method that starts native touch calibration.
- Splits touchCalibrationSet() API method into 2 separate methods. This is
required
since custom touch calibration UX will require an un-transformed touch input
to
collect calibration data.
BUG=672990
COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
- Plumbs through native touch calibration from MD settings to
display manager via the system display API.
- Renames API method that starts native touch calibration.
- Splits touchCalibrationSet() API method into 2 separate methods. This is
required since custom touch calibration UX will require an un-transformed
touch input to collect calibration data.
BUG=672990
COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
Description was changed from ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. - Renames API method that starts native touch calibration. - Splits touchCalibrationSet() API method into 2 separate methods. This is required since custom touch calibration UX will require an un-transformed touch input to collect calibration data. BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method that starts native touch calibration. - Splits touchCalibrationSet() method into 2 separate methods. This is required since custom touch calibration UX will require an un-transformed touch input to collect calibration data. BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method that starts native touch calibration. - Splits touchCalibrationSet() method into 2 separate methods. This is required since custom touch calibration UX will require an un-transformed touch input to collect calibration data. BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method. - Splits touchCalibrationSet() method into 2 separate methods. BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:60001) 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.
Description was changed from ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method. - Splits touchCalibrationSet() method into 2 separate methods. BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method. - Splits touchCalibrationSet() method into 2 separate methods. (Why? https://docs.google.com/document/d/1fFvSk35wWRoKspJRR1hYMqlfeyhYO4l7s92g-5m9vPo/) BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
malaykeshav@chromium.org changed reviewers: + michaelpg@chromium.org, reillyg@chromium.org
Adding @reillyg to review DisplayInfoProviderChromeOS Adding @michaelpg to review JS files
I have some questions with the API, but I'm not really familiar with this feature. Can you add oshima@ or someone else more familiar with how this is intended to work? https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... File extensions/common/api/system_display.idl (right): https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:289: static void startCustomTouchCalibration(DOMString id); I find having multiple 'start' methods confusing. It is unclear what happens if both are called. Also, what is the expected flow with startNativeTouchCalibration? There is no equivalent 'complete'. What triggers removal of the overlay? Does it make sense to make 'native/custom' a parameter to startTouchCalibarion?
https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:718: *error = "Please call StartCustomTouchCalibration() before calling this " Since this error is passed back to the extension the function name should match what is defined in the IDL. https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.h (right): https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.h:66: bool is_custom_touch_calibration_active_ = false; Remove "is_". https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:1208: bool success = false; Declare |success| where it is assigned.
On 2017/01/18 at 21:01:36, stevenjb wrote: > I have some questions with the API, but I'm not really familiar with this feature. Can you add oshima@ or someone else more familiar with how this is intended to work? > > https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... > File extensions/common/api/system_display.idl (right): > > https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... > extensions/common/api/system_display.idl:289: static void startCustomTouchCalibration(DOMString id); > I find having multiple 'start' methods confusing. It is unclear what happens if both are called. > > Also, what is the expected flow with startNativeTouchCalibration? There is no equivalent 'complete'. What triggers removal of the overlay? > > Does it make sense to make 'native/custom' a parameter to startTouchCalibarion? The two start methods cannot be started simultaneously. Only one can be active at a time. Start Native Touch Calibration - This involves starting Chrome OS's native touch calibration UX that is based on the UX specs https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OU05FR01rUGV3Zms The UX collects touch calibration associated data and stores it on device for later use. This does not require any complete() call as the UX itself handles graceful exit. Start Custom UX - This is used by businesses, enterprises, kiosk mode or any chrome app to have touch calibration with their own UX. This API informs the device that a custom touch calibration UX is going to be started up to collect touch calibration data. Once the custom UX completes collection of data, it can store/commit the same using the complete() API call. (Why we need two separate API methods for Custom UX calibration is explained here: https://docs.google.com/document/d/1fFvSk35wWRoKspJRR1hYMqlfeyhYO4l7s92g-5m9vPo/ )
https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... File extensions/common/api/system_display.idl (right): https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:283: static void startNativeTouchCalibration(DOMString id); If startNativeTouchCalibration doesn't require a complete() call, we should name this something like showNativeTouchCalibration() and clarify the comment. It would also be helpful to provide a completion callback when the native UI is dismissed, is that possible? https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:305: static void resetTouchCalibration(DOMString id); resetCustomTouchCalibration?
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...
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
malaykeshav@chromium.org changed reviewers: + oshima@chromium.org
Resolved comments. PTAL https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.cc:718: *error = "Please call StartCustomTouchCalibration() before calling this " On 2017/01/18 at 21:35:56, Reilly Grant wrote: > Since this error is passed back to the extension the function name should match what is defined in the IDL. Done https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos.h (right): https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos.h:66: bool is_custom_touch_calibration_active_ = false; On 2017/01/18 at 21:35:56, Reilly Grant wrote: > Remove "is_". Done https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/2638903003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:1208: bool success = false; On 2017/01/18 at 21:35:56, Reilly Grant wrote: > Declare |success| where it is assigned. Done https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... File extensions/common/api/system_display.idl (right): https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:283: static void startNativeTouchCalibration(DOMString id); On 2017/01/18 21:58:05, stevenjb wrote: > If startNativeTouchCalibration doesn't require a complete() call, we should name > this something like showNativeTouchCalibration() and clarify the comment. > > It would also be helpful to provide a completion callback when the native UI is > dismissed, is that possible? Renamed to showNativeTouchCalibration() and added a callback when the native UI is dismissed. https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:289: static void startCustomTouchCalibration(DOMString id); On 2017/01/18 21:01:35, stevenjb wrote: > I find having multiple 'start' methods confusing. It is unclear what happens if > both are called. > > Also, what is the expected flow with startNativeTouchCalibration? There is no > equivalent 'complete'. What triggers removal of the overlay? > > Does it make sense to make 'native/custom' a parameter to startTouchCalibarion? Acknowledged. https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:305: static void resetTouchCalibration(DOMString id); On 2017/01/18 21:58:05, stevenjb wrote: > resetCustomTouchCalibration? This resets any stored touch calibration data and has nothing to do with the UX.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2638903003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h (right): https://codereview.chromium.org/2638903003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h:53: void AddCallback(const TouchCalibrationCallback& callback); Why not pass this callback to StartCalibration()? https://codereview.chromium.org/2638903003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h:88: bool has_callback_; This field is unnecessary. callback_.is_null() will be true if a callback has not been set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 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.
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.
PTAL https://codereview.chromium.org/2638903003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h (right): https://codereview.chromium.org/2638903003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h:53: void AddCallback(const TouchCalibrationCallback& callback); On 2017/01/19 at 02:41:46, Reilly Grant wrote: > Why not pass this callback to StartCalibration()? I was thinking of making the callback optional in the IDL file, but got lost in the documentation on to use it. Moving the callback to StartCalibration() https://codereview.chromium.org/2638903003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h:88: bool has_callback_; On 2017/01/19 at 02:41:46, Reilly Grant wrote: > This field is unnecessary. callback_.is_null() will be true if a callback has not been set. Done
We also need to update system_display_apitest.cc. Be sure to test that |callback| is invoked for showNativeTouchCalibration() for success and failure conditions. https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... File extensions/common/api/system_display.idl (right): https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:305: static void resetTouchCalibration(DOMString id); On 2017/01/19 01:30:51, malaykeshav wrote: > On 2017/01/18 21:58:05, stevenjb wrote: > > resetCustomTouchCalibration? > > This resets any stored touch calibration data and has nothing to do with the UX. Hmm. Unfortunately this is different behavior from overscanCalibrationReset, which resets to the last saved value during calibration. That one should probably be named 'cancel', but changing that is risky. Maybe name this 'clearTouchCalibration' since it is removing all saved calibration data? https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:85: callback_.Reset(); Run callback_ with 'false' here instead of line 94? https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:47: TouchCalibratorController::TouchCalibrationCallback callback; nit: 'do_nothing' or 'empty_callback' https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:672: "calling this method."; Declare string constants at top of file. Also, untranslated error strings like this should be as short and straightforward as possible, e,g, "Calibration in progress". (The id is known since it was passed to this). https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:715: touch_calibration_target_id_ = ""; nit: touch_calibration_target_id_.clear() https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:719: "calling this method."; Declare string constants at top of file and sumplify (e.g. completeCustomTouchCalibration called before startCustomTouchCalibration). https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:1228: "this method."; This string should be made public and declared in the header so it can be used here. Then if someone changes the string the unit test won't fail unexpectedly. https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:1295: error = ""; nit: error.clear() https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/display.js (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/display.js:319: this.selectedDisplay.id, function(success) {}); Ideally we should disable the button while touch calibration is in progress and enable it when it completes. In order to do that robustly however we would also need to be able to query whether calibration is in progress when we load the UI, so maybe add a TODO here instead and file a bug for that. In the meanwhile, if we make the callback optional, no need to pass an empty function. https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... File extensions/browser/api/system_display/display_info_provider.h (right): https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... extensions/browser/api/system_display/display_info_provider.h:91: virtual bool IsNativeTouchCalibrationActive(std::string* error); All of these should be documented. In particular, document when and how |error| will be set. https://codereview.chromium.org/2638903003/diff/180001/extensions/common/api/... File extensions/common/api/system_display.idl (right): https://codereview.chromium.org/2638903003/diff/180001/extensions/common/api/... extensions/common/api/system_display.idl:284: // progress this will throw an error. Indicate whether the call back will be invoked on error (e.g. calibration is in progress). It's probably easiest for developers if the callback is always invoked, but I don't feel strongly so long as it is documented. https://codereview.chromium.org/2638903003/diff/180001/extensions/common/api/... extensions/common/api/system_display.idl:290: DOMString id, NativeTouchCalibrationCallback callback); If we make the callback optional, we don't need to provide it in places where we don't care about the result.
do you still need me on this? if it's just for display.js it looks like stevenjb has that covered.
Description was changed from ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method. - Splits touchCalibrationSet() method into 2 separate methods. (Why? https://docs.google.com/document/d/1fFvSk35wWRoKspJRR1hYMqlfeyhYO4l7s92g-5m9vPo/) BUG=672990 COMPONENT=Display Info Provider, Chrome OS, API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method. - Splits touchCalibrationSet() method into 2 separate methods. (Why? https://docs.google.com/document/d/1fFvSk35wWRoKspJRR1hYMqlfeyhYO4l7s92g-5m9vPo/) - Updates SystemDisplayAPI Test BUG=672990 COMPONENT=Display Info Provider, Chrome OS, System Display API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
The CQ bit was unchecked by malaykeshav@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm from my perspective once stevenjb's comments are resolved.
Patchset #6 (id:240001) 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...
Resolved comments and added system display api tests. PTAL https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... File extensions/common/api/system_display.idl (right): https://codereview.chromium.org/2638903003/diff/80001/extensions/common/api/s... extensions/common/api/system_display.idl:305: static void resetTouchCalibration(DOMString id); On 2017/01/19 at 17:19:01, stevenjb wrote: > On 2017/01/19 01:30:51, malaykeshav wrote: > > On 2017/01/18 21:58:05, stevenjb wrote: > > > resetCustomTouchCalibration? > > > > This resets any stored touch calibration data and has nothing to do with the UX. > > Hmm. Unfortunately this is different behavior from overscanCalibrationReset, which resets to the last saved value during calibration. That one should probably be named 'cancel', but changing that is risky. > > Maybe name this 'clearTouchCalibration' since it is removing all saved calibration data? Done https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:85: callback_.Reset(); On 2017/01/19 at 17:19:01, stevenjb wrote: > Run callback_ with 'false' here instead of line 94? Done https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:47: TouchCalibratorController::TouchCalibrationCallback callback; On 2017/01/19 at 17:19:01, stevenjb wrote: > nit: 'do_nothing' or 'empty_callback' Done https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:672: "calling this method."; On 2017/01/19 at 17:19:01, stevenjb wrote: > Declare string constants at top of file. > > Also, untranslated error strings like this should be as short and straightforward as possible, e,g, "Calibration in progress". (The id is known since it was passed to this). Reducing string length and moving to top of file. https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:715: touch_calibration_target_id_ = ""; On 2017/01/19 at 17:19:01, stevenjb wrote: > nit: touch_calibration_target_id_.clear() Done https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:719: "calling this method."; On 2017/01/19 at 17:19:01, stevenjb wrote: > Declare string constants at top of file and sumplify (e.g. completeCustomTouchCalibration called before startCustomTouchCalibration). Done https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:1228: "this method."; On 2017/01/19 at 17:19:01, stevenjb wrote: > This string should be made public and declared in the header so it can be used here. Then if someone changes the string the unit test won't fail unexpectedly. Done https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:1295: error = ""; On 2017/01/19 at 17:19:01, stevenjb wrote: > nit: error.clear() Done https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/display.js (right): https://codereview.chromium.org/2638903003/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/display.js:319: this.selectedDisplay.id, function(success) {}); On 2017/01/19 at 17:19:01, stevenjb wrote: > Ideally we should disable the button while touch calibration is in progress and enable it when it completes. In order to do that robustly however we would also need to be able to query whether calibration is in progress when we load the UI, so maybe add a TODO here instead and file a bug for that. > > In the meanwhile, if we make the callback optional, no need to pass an empty function. This would be a safety measure since the calibration UX overlays the entire screen. And the user cannot interact with anything other than the touch calibration UX while it is in progress. https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... File extensions/browser/api/system_display/display_info_provider.h (right): https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... extensions/browser/api/system_display/display_info_provider.h:91: virtual bool IsNativeTouchCalibrationActive(std::string* error); On 2017/01/19 at 17:19:01, stevenjb wrote: > All of these should be documented. In particular, document when and how |error| will be set. Adding comments where the error is being set. https://codereview.chromium.org/2638903003/diff/180001/extensions/common/api/... File extensions/common/api/system_display.idl (right): https://codereview.chromium.org/2638903003/diff/180001/extensions/common/api/... extensions/common/api/system_display.idl:284: // progress this will throw an error. On 2017/01/19 at 17:19:01, stevenjb wrote: > Indicate whether the call back will be invoked on error (e.g. calibration is in progress). It's probably easiest for developers if the callback is always invoked, but I don't feel strongly so long as it is documented. Done https://codereview.chromium.org/2638903003/diff/180001/extensions/common/api/... extensions/common/api/system_display.idl:290: DOMString id, NativeTouchCalibrationCallback callback); On 2017/01/19 at 17:19:01, stevenjb wrote: > If we make the callback optional, we don't need to provide it in places where we don't care about the result. Done
lgtm https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... File extensions/browser/api/system_display/display_info_provider.h (right): https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... extensions/browser/api/system_display/display_info_provider.h:91: virtual bool IsNativeTouchCalibrationActive(std::string* error); On 2017/01/19 23:49:07, malaykeshav wrote: > On 2017/01/19 at 17:19:01, stevenjb wrote: > > All of these should be documented. In particular, document when and how > |error| will be set. > > Adding comments where the error is being set. Public methods should *always* be documented, especially virtual methods (but not the overrides). We are obviously not great about that in this class, but we really should improve that. https://codereview.chromium.org/2638903003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/display_info_provider_chromeos.cc (right): https://codereview.chromium.org/2638903003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:415: "Please complete the custom touch calibration that is under progress."; nit: simplify https://codereview.chromium.org/2638903003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/display_info_provider_chromeos.cc:437: "Another touch calibration is already active."; Thanks!!! https://codereview.chromium.org/2638903003/diff/260001/extensions/browser/api... File extensions/browser/api/system_display/system_display_apitest.cc (right): https://codereview.chromium.org/2638903003/diff/260001/extensions/browser/api... extensions/browser/api/system_display/system_display_apitest.cc:479: Maybe add a TODO to add tests for custom calibration? (Unless I missed them).
On 2017/01/20 at 00:07:07, stevenjb wrote: > lgtm > > https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... > File extensions/browser/api/system_display/display_info_provider.h (right): > > https://codereview.chromium.org/2638903003/diff/180001/extensions/browser/api... > extensions/browser/api/system_display/display_info_provider.h:91: virtual bool IsNativeTouchCalibrationActive(std::string* error); > On 2017/01/19 23:49:07, malaykeshav wrote: > > On 2017/01/19 at 17:19:01, stevenjb wrote: > > > All of these should be documented. In particular, document when and how > > |error| will be set. > > > > Adding comments where the error is being set. > > Public methods should *always* be documented, especially virtual methods (but not the overrides). We are obviously not great about that in this class, but we really should improve that. In this case it would mostly be a repeat of the description in system_display.idl which has already been referred to in the comment above the declaration. Also the error being set depends on the impl of the interface, so I am not sure if it should be defined here. It does however mention that if an error occurs, then the |error| string will be set by the impl. > > https://codereview.chromium.org/2638903003/diff/260001/chrome/browser/extensi... > File chrome/browser/extensions/display_info_provider_chromeos.cc (right): > > https://codereview.chromium.org/2638903003/diff/260001/chrome/browser/extensi... > chrome/browser/extensions/display_info_provider_chromeos.cc:415: "Please complete the custom touch calibration that is under progress."; > nit: simplify > Done > https://codereview.chromium.org/2638903003/diff/260001/chrome/browser/extensi... > chrome/browser/extensions/display_info_provider_chromeos.cc:437: "Another touch calibration is already active."; > Thanks!!! > > https://codereview.chromium.org/2638903003/diff/260001/extensions/browser/api... > File extensions/browser/api/system_display/system_display_apitest.cc (right): > > https://codereview.chromium.org/2638903003/diff/260001/extensions/browser/api... > extensions/browser/api/system_display/system_display_apitest.cc:479: > Maybe add a TODO to add tests for custom calibration? (Unless I missed them). This has been thoroughly tested in display_info_provider_chromeos_unittest. system_display_api has minimal code and only forwards the call. Would it be useful to write tests for it?
Adding @isherman for extension_function_histogram_value.h
malaykeshav@chromium.org changed reviewers: + isherman@chromium.org
histograms.xml lgtm
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.
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2638903003/#ps300001 (title: "Update histogram.xml")
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": 300001, "attempt_start_ts": 1484877813656860,
"parent_rev": "cec3fb2dbc4433165fa180f5c33062edac1b0135", "commit_rev":
"5f64df43034f52aea554ca764633c58a72f44831"}
Message was sent while issue was closed.
Description was changed from ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method. - Splits touchCalibrationSet() method into 2 separate methods. (Why? https://docs.google.com/document/d/1fFvSk35wWRoKspJRR1hYMqlfeyhYO4l7s92g-5m9vPo/) - Updates SystemDisplayAPI Test BUG=672990 COMPONENT=Display Info Provider, Chrome OS, System Display API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== - Plumbs through native touch calibration from MD settings to display manager via the system display API. Modifications done to System Display API: - Renames API method. - Splits touchCalibrationSet() method into 2 separate methods. (Why? https://docs.google.com/document/d/1fFvSk35wWRoKspJRR1hYMqlfeyhYO4l7s92g-5m9vPo/) - Updates SystemDisplayAPI Test BUG=672990 COMPONENT=Display Info Provider, Chrome OS, System Display API, Touch Calibration CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2638903003 Cr-Commit-Position: refs/heads/master@{#444950} Committed: https://chromium.googlesource.com/chromium/src/+/5f64df43034f52aea554ca764633... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:300001) as https://chromium.googlesource.com/chromium/src/+/5f64df43034f52aea554ca764633... |
