Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557163002:
Implements computation of touch calibration transform using user provided data (Closed)
|
Created:
4 years ago by malaykeshav Modified:
4 years ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplements computation of touch calibration transform using user provided data
Implements the computation of touch transform used to transform all
touch input events coming from a touch device and maps them to their
corresponding locations on the screen/display device.
The design doc linked below offers an explanation on how all the
computation works.
- Additionally solves a minor bug related to Ozone.
- Adds test to to check for this minor bug.
Design Doc=go/cros-touch-calibration
BUG=667921, 672293
COMPONENT=Chrome OS, Touch, Ash
Committed: https://crrev.com/3e3486a3c4efe28a9e1c391af2fa8cc3f8b41ed8
Cr-Commit-Position: refs/heads/master@{#440515}
Patch Set 1 : Implements computation of touch calibration transform using user provided data #Patch Set 2 : Sync with ToT #Patch Set 3 : Added link to crbug in code #
Total comments: 47
Patch Set 4 : Resolving comments #Patch Set 5 : Sync with ToT #
Total comments: 18
Patch Set 6 : Resolving comments #
Total comments: 4
Patch Set 7 : Removed CHECK() and added test as friend class #
Total comments: 8
Patch Set 8 : Resolving comments #
Total comments: 12
Patch Set 9 : Resolved comments #Patch Set 10 : Fixed unused variable error #
Total comments: 1
Messages
Total messages: 81 (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: + oshima@chromium.org, sky@chromium.org
PTAL Updates touch transform controller to compute the transform using user provided calibration data. The computations are explained in the design-doc: go/cros-touch-calibration
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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 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...
Description was changed from ========== Implements computation of touch calibration transform using user provided data Implements the computation of touch transform used to transform all touch input events coming from a touch device and maps them to their corresponding locations on the screen/display device. The design doc linked below offers an explanation on how all the computation works. - Additionally solves a minor bug related to Ozone. - Adds test to to check for this minor bug. Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Chrome OS, Touch, Ash ========== to ========== Implements computation of touch calibration transform using user provided data Implements the computation of touch transform used to transform all touch input events coming from a touch device and maps them to their corresponding locations on the screen/display device. The design doc linked below offers an explanation on how all the computation works. - Additionally solves a minor bug related to Ozone. - Adds test to to check for this minor bug. Design Doc=go/cros-touch-calibration BUG=667921,672293 COMPONENT=Chrome OS, Touch, Ash ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
/cc +kylechar@: does this need some equivalent work for mus?
On 2016/12/08 02:13:27, sadrul wrote: > /cc +kylechar@: does this need some equivalent work for mus? Yes, we'll need something equivalent. We'll want to refactor most of touch_transformer_controllercc/h and move it from src/ash to src/ui. I've already moved some of the other the ash touchscreen code.
Ping for review.
sky@chromium.org changed reviewers: - sky@chromium.org
I'm not familiar at all with this code. I'm assuming Oshima or Sadrul knows more and am going removing myself. In the future when you add multiple reviewers please list what you expect them to review. https://codereview.chromium.org/2557163002/diff/60001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2557163002/diff/60001/ash/BUILD.gn#newcode1478 ash/BUILD.gn:1478: if (!(use_x11 || use_ozone) || !is_chromeos) { You can remove the is_chromeos check here as ash is now only chromeos.
@sadrul, @oshima - Ping for review This needs to be checked in for M57. Subsequent changes cannot be landed without this patch. Requesting for a faster review.
Found some typos in the patch. Do not have access to a corp device to make required changes till morning. Adding comments so there is no confusion in review till then. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:12: // #include "ui/gfx/geometry/point.h" Remove https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:45: point.x() + base::RandInt(-error.width() / 2, error.width()) / 2, point.x() + base::RandInt(-error.width() / 2, error.width() / 2), https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:549: // actually tapped (100,100) when asked to tap (100,100) with no human error. Update comment. This is not accurate user input. Its user input with some error.
kylechar@chromium.org changed reviewers: + kylechar@chromium.org
Was asked to review this by sadrul. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:58: SkMatrix44 touch_point_matrix; Can you move variables closer to where they are first used? https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:103: CHECK(product_matrix.invert(&product_matrix_inverse)); Is crashing the correct thing to do if the inverse can't be computed the best solution here? I'd think you would want to invalidate the calibration data and warn the user or something? https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:114: return std::make_pair(result_constants_x, result_constants_y); Why doesn't this just return the gfx::Transform you build in stored_ctm? https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:161: gfx::Transform stored_ctm; Define variable where you're going to use it. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:180: if (!touch_display.has_touch_calibration_data()) { Could you split this function into something like ComputeUncalibratedTransform() vs ComputeCalibratedTransform()? It would help with readability. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:208: auto touch_calib_size = gfx::SizeF touch_calib_size(...) https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:211: // Any additional tranfomration that needs to be applied to the display transformation https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:241: std::pair<SkVector4, SkVector4> result = SolveCalibrationEquation( Same as above, if this returned the gfx::Transform it would make more sense. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:39: // applied to the touch input's location. This also updates the transforms right? https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:43: FRIEND_TEST_ALL_PREFIXES(TouchTransformerControllerTest, Would it just be simpler to make TouchTransformerControllerTest a friend class at this point? https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:43: gfx::Point GetPointWithError(const gfx::Point& point, const gfx::Size& error) { Do you really want to use non-deterministic random numbers in a test? It doesn't seem strictly necessary here. It also increases the risk of a flakey test. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:53: void CheckPointsOfInterests(const int& touch_id, Don't pass int by const ref. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:404: TEST_F(TouchTransformerControllerTest, OzoneTrasnlation) { OzoneTranslation? https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:411: const gfx::Size DISPLAY_SIZE(1920, 1200), TOUCH_SIZE(1920, 1200); Constant names are supported be like kDisplaySize. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:413: const int DISPLAY_ID_1 = 1, DISPLAY_ID_2 = 2, TOUCH_ID_1 = 5, TOUCH_ID_2 = 6; One variable per line. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:415: display::ManagedDisplayInfo display1 = CreateDisplayInfo( optional: auto display1 = ...;
Patchset #5 (id:100001) 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 with the required changes. PTAL https://codereview.chromium.org/2557163002/diff/60001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2557163002/diff/60001/ash/BUILD.gn#newcode1478 ash/BUILD.gn:1478: if (!(use_x11 || use_ozone) || !is_chromeos) { On 2016/12/12 at 02:06:48, sky (OOO) wrote: > You can remove the is_chromeos check here as ash is now only chromeos. Done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:58: SkMatrix44 touch_point_matrix; On 2016/12/19 at 20:00:21, kylechar wrote: > Can you move variables closer to where they are first used? Done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:103: CHECK(product_matrix.invert(&product_matrix_inverse)); On 2016/12/19 at 20:00:21, kylechar wrote: > Is crashing the correct thing to do if the inverse can't be computed the best solution here? I'd think you would want to invalidate the calibration data and warn the user or something? This is an extremely rare case since we are using floating points. The inverse will almost always exist. The tradeoff to handling this case doesnt seem worth it. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:114: return std::make_pair(result_constants_x, result_constants_y); On 2016/12/19 20:00:21, kylechar wrote: > Why doesn't this just return the gfx::Transform you build in stored_ctm? Done. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:161: gfx::Transform stored_ctm; On 2016/12/19 20:00:21, kylechar wrote: > Define variable where you're going to use it. Done. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:180: if (!touch_display.has_touch_calibration_data()) { On 2016/12/19 at 20:00:21, kylechar wrote: > Could you split this function into something like ComputeUncalibratedTransform() vs ComputeCalibratedTransform()? It would help with readability. Splitting the uncalibrated transform and moving to a separate function. Leaving the calibrated transform as is. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:208: auto touch_calib_size = On 2016/12/19 at 20:00:21, kylechar wrote: > gfx::SizeF touch_calib_size(...) Done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:211: // Any additional tranfomration that needs to be applied to the display On 2016/12/19 at 20:00:21, kylechar wrote: > transformation done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.cc:241: std::pair<SkVector4, SkVector4> result = SolveCalibrationEquation( On 2016/12/19 at 20:00:21, kylechar wrote: > Same as above, if this returned the gfx::Transform it would make more sense. done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:39: // applied to the touch input's location. On 2016/12/19 at 20:00:21, kylechar wrote: > This also updates the transforms right? No. This only sets the TouchTransformerController in a state that returns an empty transform. This is required during calibration. We want raw touch inputs that are not transformed in their local screen space. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:43: FRIEND_TEST_ALL_PREFIXES(TouchTransformerControllerTest, On 2016/12/19 at 20:00:21, kylechar wrote: > Would it just be simpler to make TouchTransformerControllerTest a friend class at this point? Done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:12: // #include "ui/gfx/geometry/point.h" On 2016/12/19 at 19:12:01, malaykeshav wrote: > Remove Done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:43: gfx::Point GetPointWithError(const gfx::Point& point, const gfx::Size& error) { On 2016/12/19 at 20:00:22, kylechar wrote: > Do you really want to use non-deterministic random numbers in a test? It doesn't seem strictly necessary here. It also increases the risk of a flakey test. This is not completely non-deterministic. It produces a number within the expected range. The tests _cannot_ fail if the calibration is working as expected. This can be replaced with hardcoded extreme values by default, but that would add a lot of bp code. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:45: point.x() + base::RandInt(-error.width() / 2, error.width()) / 2, On 2016/12/19 at 19:12:01, malaykeshav wrote: > point.x() + base::RandInt(-error.width() / 2, error.width() / 2), done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:53: void CheckPointsOfInterests(const int& touch_id, On 2016/12/19 at 20:00:22, kylechar wrote: > Don't pass int by const ref. done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:404: TEST_F(TouchTransformerControllerTest, OzoneTrasnlation) { On 2016/12/19 at 20:00:22, kylechar wrote: > OzoneTranslation? done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:411: const gfx::Size DISPLAY_SIZE(1920, 1200), TOUCH_SIZE(1920, 1200); On 2016/12/19 at 20:00:21, kylechar wrote: > Constant names are supported be like kDisplaySize. Done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:413: const int DISPLAY_ID_1 = 1, DISPLAY_ID_2 = 2, TOUCH_ID_1 = 5, TOUCH_ID_2 = 6; On 2016/12/19 at 20:00:21, kylechar wrote: > One variable per line. Done https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:415: display::ManagedDisplayInfo display1 = CreateDisplayInfo( On 2016/12/19 at 20:00:21, kylechar wrote: > optional: auto display1 = ...; Ack https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:549: // actually tapped (100,100) when asked to tap (100,100) with no human error. On 2016/12/19 19:12:01, malaykeshav wrote: > Update comment. > This is not accurate user input. Its user input with some error. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:43: gfx::Point GetPointWithError(const gfx::Point& point, const gfx::Size& error) { On 2016/12/20 09:56:05, malaykeshav wrote: > On 2016/12/19 at 20:00:22, kylechar wrote: > > Do you really want to use non-deterministic random numbers in a test? It > doesn't seem strictly necessary here. It also increases the risk of a flakey > test. > > This is not completely non-deterministic. It produces a number within the > expected range. > The tests _cannot_ fail if the calibration is working as expected. > > This can be replaced with hardcoded extreme values by default, but that would > add a lot of bp code. Boilerplate code is a good trade-off to possibly flaky test. If you do keep the random points, make sure you print them out when the test expectation fails, so that it can be reproduced and tested for correctness. https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:49: std::array<std::pair<gfx::Point, gfx::Point>, 4> touch_point_pairs, const& https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:95: // #NOTE: If the determinent is zero then the inverse cannot be computed. The Just // NOTE: https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:97: CHECK(product_matrix.invert(&product_matrix_inverse)); How does a user recover from this error? i.e. how do you get different calibration values after the crash+restart? https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:119: gfx::Transform ctm, const & Make a copy in the function body that you then return. https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:124: auto current_size = gfx::SizeF(display.bounds_in_native().size()); gfx::SizeF current_size(display.bounds_in_native().size()) https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:207: display.bounds_in_native().y()); {} https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:40: void SetForCalibration(bool is_calibrating); What code is using this?
https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:39: // applied to the touch input's location. On 2016/12/20 09:56:04, malaykeshav wrote: > On 2016/12/19 at 20:00:21, kylechar wrote: > > This also updates the transforms right? > > No. This only sets the TouchTransformerController in a state that returns an > empty transform. > This is required during calibration. We want raw touch inputs that are not > transformed in their local screen space. What does the call to UpdateTouchTransformer() do then? It seems like it would set empty transforms if is_calibrating and properly sets transforms if !is_calibrating. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:43: FRIEND_TEST_ALL_PREFIXES(TouchTransformerControllerTest, On 2016/12/19 20:00:21, kylechar wrote: > Would it just be simpler to make TouchTransformerControllerTest a friend class > at this point? This wasn't done? https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:546: // The maximum user error rate is |ERROR|%. Since the calibration is performed The variable names weren't updated with the variables in a bunch of spots.
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. (Except for the friend class issue, which did not seem to work. Will try again in 8 hrs when I have access to corp network) PTAL https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:39: // applied to the touch input's location. On 2016/12/20 at 17:41:28, kylechar wrote: > On 2016/12/20 09:56:04, malaykeshav wrote: > > On 2016/12/19 at 20:00:21, kylechar wrote: > > > This also updates the transforms right? > > > > No. This only sets the TouchTransformerController in a state that returns an > > empty transform. > > This is required during calibration. We want raw touch inputs that are not > > transformed in their local screen space. > > What does the call to UpdateTouchTransformer() do then? It seems like it would set empty transforms if is_calibrating and properly sets transforms if !is_calibrating. Correct. When the calibration is under process (See https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... ) we want no transform to be applied to the touch inputs since we are trying to collect data that computes the new calibrated transform. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:43: FRIEND_TEST_ALL_PREFIXES(TouchTransformerControllerTest, On 2016/12/20 at 17:41:28, kylechar wrote: > On 2016/12/19 20:00:21, kylechar wrote: > > Would it just be simpler to make TouchTransformerControllerTest a friend class > > at this point? > > This wasn't done? I was unable to friend class TouchTransformerControllerTest for some reason. The compiler throws private function access error. I will try again when I have access to corp network tmrw morning. Resolving rest of the comments atm. https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller_unittest.cc:43: gfx::Point GetPointWithError(const gfx::Point& point, const gfx::Size& error) { On 2016/12/20 at 17:31:05, sadrul wrote: > On 2016/12/20 09:56:05, malaykeshav wrote: > > On 2016/12/19 at 20:00:22, kylechar wrote: > > > Do you really want to use non-deterministic random numbers in a test? It > > doesn't seem strictly necessary here. It also increases the risk of a flakey > > test. > > > > This is not completely non-deterministic. It produces a number within the > > expected range. > > The tests _cannot_ fail if the calibration is working as expected. > > > > This can be replaced with hardcoded extreme values by default, but that would > > add a lot of bp code. > > Boilerplate code is a good trade-off to possibly flaky test. > > If you do keep the random points, make sure you print them out when the test expectation fails, so that it can be reproduced and tested for correctness. Done Adding debug message that includes the point information. https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:49: std::array<std::pair<gfx::Point, gfx::Point>, 4> touch_point_pairs, On 2016/12/20 at 17:31:05, sadrul wrote: > const& They are being modified on line #57 https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:95: // #NOTE: If the determinent is zero then the inverse cannot be computed. The On 2016/12/20 at 17:31:05, sadrul wrote: > Just // NOTE: Done https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:97: CHECK(product_matrix.invert(&product_matrix_inverse)); On 2016/12/20 at 17:31:05, sadrul wrote: > How does a user recover from this error? i.e. how do you get different calibration values after the crash+restart? The user has to go to settings>display>touch calibration https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:119: gfx::Transform ctm, On 2016/12/20 at 17:31:05, sadrul wrote: > const & > > Make a copy in the function body that you then return. Done https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:124: auto current_size = gfx::SizeF(display.bounds_in_native().size()); On 2016/12/20 at 17:31:05, sadrul wrote: > gfx::SizeF current_size(display.bounds_in_native().size()) done https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:207: display.bounds_in_native().y()); On 2016/12/20 at 17:31:05, sadrul wrote: > {} done https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:40: void SetForCalibration(bool is_calibrating); On 2016/12/20 at 17:31:05, sadrul wrote: > What code is using this? That code hasnt been checked in yet. It is a subsequent patch that involves the UX for touch calibration. During touch calibration we do not want to apply any transform to the touch input. See https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:546: // The maximum user error rate is |ERROR|%. Since the calibration is performed On 2016/12/20 at 17:41:28, kylechar wrote: > The variable names weren't updated with the variables in a bunch of spots. Yes. I seemed to have missed updating the comments. Done
https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:97: CHECK(product_matrix.invert(&product_matrix_inverse)); A safer way to do this would be to add an if condition instead of a CHECK. It will reset the calibration data and return an empty transform. We can later on add a message for the user informing that the calibration was unsuccessful. Although the non existence of an inverse would be an extremely rare occurrence and hence I would defer this till testing and finding how often it happens.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:97: CHECK(product_matrix.invert(&product_matrix_inverse)); On 2016/12/20 19:34:56, malaykeshav wrote: > A safer way to do this would be to add an if condition instead of a CHECK. > It will reset the calibration data and return an empty transform. > > We can later on add a message for the user informing that the calibration was > unsuccessful. > Although the non existence of an inverse would be an extremely rare occurrence > and hence I would defer this till testing and finding how often it happens. > My understanding is, if this CHECK does trigger, the user has no way to recover from it (as in, every time chrome tries to start, it will crash. I don't know if even auto-update would work in that scenario). Is that not the case? Because if it is, then we cannot have this CHECK here, and need go with the alternate that you propose.
https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:97: CHECK(product_matrix.invert(&product_matrix_inverse)); On 2016/12/21 02:09:50, sadrul wrote: > On 2016/12/20 19:34:56, malaykeshav wrote: > > A safer way to do this would be to add an if condition instead of a CHECK. > > It will reset the calibration data and return an empty transform. > > > > We can later on add a message for the user informing that the calibration was > > unsuccessful. > > Although the non existence of an inverse would be an extremely rare occurrence > > and hence I would defer this till testing and finding how often it happens. > > > My understanding is, if this CHECK does trigger, the user has no way to recover > from it (as in, every time chrome tries to start, it will crash. I don't know if > even auto-update would work in that scenario). Is that not the case? Because if > it is, then we cannot have this CHECK here, and need go with the alternate that > you propose. Yes there would be no way to recover for the user. Will replace this eith a NOT_REACHED block along with the reset of touch calibration data.
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...
Removed CHECK and added test as friend class. PTAL https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/60001/ash/touch/touch_transfo... ash/touch/touch_transformer_controller.h:43: FRIEND_TEST_ALL_PREFIXES(TouchTransformerControllerTest, On 2016/12/20 at 19:16:55, malaykeshav wrote: > On 2016/12/20 at 17:41:28, kylechar wrote: > > On 2016/12/19 20:00:21, kylechar wrote: > > > Would it just be simpler to make TouchTransformerControllerTest a friend class > > > at this point? > > > > This wasn't done? > > I was unable to friend class TouchTransformerControllerTest for some reason. The compiler throws private function access error. I will try again when I have access to corp network tmrw morning. > Resolving rest of the comments atm. Done https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/140001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:97: CHECK(product_matrix.invert(&product_matrix_inverse)); On 2016/12/21 at 02:13:52, malaykeshav wrote: > On 2016/12/21 02:09:50, sadrul wrote: > > On 2016/12/20 19:34:56, malaykeshav wrote: > > > A safer way to do this would be to add an if condition instead of a CHECK. > > > It will reset the calibration data and return an empty transform. > > > > > > We can later on add a message for the user informing that the calibration was > > > unsuccessful. > > > Although the non existence of an inverse would be an extremely rare occurrence > > > and hence I would defer this till testing and finding how often it happens. > > > > > > My understanding is, if this CHECK does trigger, the user has no way to recover > > from it (as in, every time chrome tries to start, it will crash. I don't know if > > even auto-update would work in that scenario). Is that not the case? Because if > > it is, then we cannot have this CHECK here, and need go with the alternate that > > you propose. > > Yes there would be no way to recover for the user. Will replace this eith a NOT_REACHED block along with the reset of touch calibration data. Done
Patchset #7 (id:160001) 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.
Thanks. lgtm from me, but please wait for a final approval from kylechar@ too https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:40: void SetForCalibration(bool is_calibrating); On 2016/12/20 19:16:56, malaykeshav wrote: > On 2016/12/20 at 17:31:05, sadrul wrote: > > What code is using this? > > That code hasnt been checked in yet. It is a subsequent patch that involves the > UX for touch calibration. During touch calibration we do not want to apply any > transform to the touch input. See > https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Can you move this part into that CL then? https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:50: std::array<std::pair<gfx::Point, gfx::Point>, 4> touch_point_pairs, Make this a const-ref. Make a copy in the function body if you need to mutate it. https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:69: const std::string error_msg) { const & https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:128: // typedef test::AshTestBase TouchTransformerControllerTest; Remove. https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:517: #endif #endif // defined(USE_OZONE)
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. @kylechar PTAL https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/120001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:40: void SetForCalibration(bool is_calibrating); On 2016/12/22 at 02:25:43, sadrul wrote: > On 2016/12/20 19:16:56, malaykeshav wrote: > > On 2016/12/20 at 17:31:05, sadrul wrote: > > > What code is using this? > > > > That code hasnt been checked in yet. It is a subsequent patch that involves the > > UX for touch calibration. During touch calibration we do not want to apply any > > transform to the touch input. See > > https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... > > Can you move this part into that CL then? Done https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:50: std::array<std::pair<gfx::Point, gfx::Point>, 4> touch_point_pairs, On 2016/12/22 at 02:25:43, sadrul wrote: > Make this a const-ref. Make a copy in the function body if you need to mutate it. If we are making a copy again, isn't it better to pass by value? After a quick looking around, I found that post c++11, std vectors, strings, arrays and other internal data structures are better off to be passed as value. Am I missing something in the chromium style guide? https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:69: const std::string error_msg) { On 2016/12/22 at 02:25:43, sadrul wrote: > const & Done https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:128: // typedef test::AshTestBase TouchTransformerControllerTest; On 2016/12/22 at 02:25:43, sadrul wrote: > Remove. Done https://codereview.chromium.org/2557163002/diff/180001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:517: #endif On 2016/12/22 at 02:25:43, sadrul wrote: > #endif // defined(USE_OZONE) Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2557163002/diff/200001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2557163002/diff/200001/ash/BUILD.gn#newcode1406 ash/BUILD.gn:1406: if (!use_x11 && !use_ozone) { The top of this file has an assert(is_chromeos) so one of these two args have to be true now. I think you can remove this entirely. https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:117: (*ctm).ConcatTransform(gfx::Transform( optional nit: ctm->ConcatTransform(...) is probably clearer? https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:38: // During touch calibration we remove any kind of transform that is being You removed this from the cc file but not h file. https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:83: bool is_calibrating_ = false; Likewise, you seem to have removed the usage of this variable but not the variable. https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:439: const int kDisplayId1 = 1; nit: You use kDisplayId1, kDisplayId2, kTouchId1, kTouchId2 in many tests. You can declare them in the anonymous namespace at the top of the file once for all tests. namespace { constexpr int kDisplayId1 = 1; ... https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:560: TEST_F(TouchTransformerControllerTest, ErrorProneUserTouchCalibration) { optional: What exactly happens in this test if someone breaks the code in TouchTransformerController for only some of the range that GetPointWithError() can produce? I'm not a huge fan of this but not going to block on it. Maybe add a TODO here to change in a subsequent CL? Even if you tested a bunch of random calibration data (100 sets?) it would fail more often than not.
lgtm
Patchset #9 (id:220001) has been deleted
Patchset #9 (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...
https://codereview.chromium.org/2557163002/diff/200001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2557163002/diff/200001/ash/BUILD.gn#newcode1406 ash/BUILD.gn:1406: if (!use_x11 && !use_ozone) { On 2016/12/22 at 16:56:00, kylechar wrote: > The top of this file has an assert(is_chromeos) so one of these two args have to be true now. I think you can remove this entirely. Done https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:117: (*ctm).ConcatTransform(gfx::Transform( On 2016/12/22 at 16:56:00, kylechar wrote: > optional nit: ctm->ConcatTransform(...) is probably clearer? Done https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.h (right): https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:38: // During touch calibration we remove any kind of transform that is being On 2016/12/22 at 16:56:00, kylechar wrote: > You removed this from the cc file but not h file. Done. https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.h:83: bool is_calibrating_ = false; On 2016/12/22 at 16:56:00, kylechar wrote: > Likewise, you seem to have removed the usage of this variable but not the variable. Done https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller_unittest.cc (right): https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:439: const int kDisplayId1 = 1; On 2016/12/22 at 16:56:00, kylechar wrote: > nit: You use kDisplayId1, kDisplayId2, kTouchId1, kTouchId2 in many tests. You can declare them in the anonymous namespace at the top of the file once for all tests. > > namespace { > constexpr int kDisplayId1 = 1; > ... Done https://codereview.chromium.org/2557163002/diff/200001/ash/touch/touch_transf... ash/touch/touch_transformer_controller_unittest.cc:560: TEST_F(TouchTransformerControllerTest, ErrorProneUserTouchCalibration) { On 2016/12/22 16:56:00, kylechar wrote: > optional: What exactly happens in this test if someone breaks the code in > TouchTransformerController for only some of the range that GetPointWithError() > can produce? I'm not a huge fan of this but not going to block on it. Maybe add > a TODO here to change in a subsequent CL? Even if you tested a bunch of random > calibration data (100 sets?) it would fail more often than not. Changed random values to hardcoded ~5% error values.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, kylechar@chromium.org Link to the patchset: https://codereview.chromium.org/2557163002/#ps280001 (title: "Fixed unused variable error")
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...)
derat@chromium.org changed reviewers: + derat@chromium.org
lgtm for ash/ with a comment nit; feel free to address it in a later change if you're in a hurry to get this in. https://codereview.chromium.org/2557163002/diff/280001/ash/touch/touch_transf... File ash/touch/touch_transformer_controller.cc (right): https://codereview.chromium.org/2557163002/diff/280001/ash/touch/touch_transf... ash/touch/touch_transformer_controller.cc:40: // and returns the constants(defined below) using a least fit algorithm. nit: add space before '('
The CQ bit was checked by malaykeshav@chromium.org
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": 280001, "attempt_start_ts": 1482443176555540,
"parent_rev": "d7f450e68e78fcb47624d37f6aba3d98bf4ede64", "commit_rev":
"33407a4eee628084324144790228c809b5d61a1a"}
Message was sent while issue was closed.
Description was changed from ========== Implements computation of touch calibration transform using user provided data Implements the computation of touch transform used to transform all touch input events coming from a touch device and maps them to their corresponding locations on the screen/display device. The design doc linked below offers an explanation on how all the computation works. - Additionally solves a minor bug related to Ozone. - Adds test to to check for this minor bug. Design Doc=go/cros-touch-calibration BUG=667921,672293 COMPONENT=Chrome OS, Touch, Ash ========== to ========== Implements computation of touch calibration transform using user provided data Implements the computation of touch transform used to transform all touch input events coming from a touch device and maps them to their corresponding locations on the screen/display device. The design doc linked below offers an explanation on how all the computation works. - Additionally solves a minor bug related to Ozone. - Adds test to to check for this minor bug. Design Doc=go/cros-touch-calibration BUG=667921,672293 COMPONENT=Chrome OS, Touch, Ash Review-Url: https://codereview.chromium.org/2557163002 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Implements computation of touch calibration transform using user provided data Implements the computation of touch transform used to transform all touch input events coming from a touch device and maps them to their corresponding locations on the screen/display device. The design doc linked below offers an explanation on how all the computation works. - Additionally solves a minor bug related to Ozone. - Adds test to to check for this minor bug. Design Doc=go/cros-touch-calibration BUG=667921,672293 COMPONENT=Chrome OS, Touch, Ash Review-Url: https://codereview.chromium.org/2557163002 ========== to ========== Implements computation of touch calibration transform using user provided data Implements the computation of touch transform used to transform all touch input events coming from a touch device and maps them to their corresponding locations on the screen/display device. The design doc linked below offers an explanation on how all the computation works. - Additionally solves a minor bug related to Ozone. - Adds test to to check for this minor bug. Design Doc=go/cros-touch-calibration BUG=667921,672293 COMPONENT=Chrome OS, Touch, Ash Committed: https://crrev.com/3e3486a3c4efe28a9e1c391af2fa8cc3f8b41ed8 Cr-Commit-Position: refs/heads/master@{#440515} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3e3486a3c4efe28a9e1c391af2fa8cc3f8b41ed8 Cr-Commit-Position: refs/heads/master@{#440515} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
