Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2603463002:
Adds background fade in animation and exit label to touch calibrator view (Closed)
|
Created:
4 years ago by malaykeshav Modified:
4 years ago CC:
chromium-reviews, davemoore+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds background fade in animation and exit label to touch calibrator view
- Adds string resource for exit label
- Also updates unit tests.
Working protottpe(The video is the entire prototype. This change only
involves adding the fade in background animation.):
https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWdUVFNSM0F5WDF4YlBn/view
UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view
Design Doc=go/cros-touch-calibration
BUG=634166
COMPONENT=Touch Calibration, ChromeOS, Unittests
Committed: https://crrev.com/33a91601c079314e849291390e44fc93f6b05caf
Cr-Commit-Position: refs/heads/master@{#440571}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Resolving comments #
Depends on Patchset: Messages
Total messages: 25 (18 generated)
Description was changed from ========== Adds background fade n animation and exit label to touch calibrator view Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests ========== to ========== Adds background fade n animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests ==========
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 ========== Adds background fade n animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests ========== to ========== Adds background fade n animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests ==========
Description was changed from ========== Adds background fade n animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests ========== to ========== Adds background fade n animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests ==========
malaykeshav@chromium.org changed reviewers: + stevenjb@chromium.org, xiyuan@chromium.org
Adds a basic fade in animation, along with a text label informing the user how to exit the touch calibration view. Also updates the unit test. PTAL
Description was changed from ========== Adds background fade n animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests ========== to ========== Adds background fade in animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests ==========
https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:32: const float kBackgroundFinalOpacity = 0.75f; constexpr for all of these. https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:79: delete exit_label_; Views are owned by the views hierarchy, this shouldn't be necessary. https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:95: 3 * display_.bounds().height() / 4, kExitLabelWidth, nit: display_.bounds().height() * 3 / 4 is a little more readable. https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:113: state_ == BACKGROUND_FADING_OUT ? 0.f : kBackgroundFinalOpacity; nit: Don't initialize this, set it in an else at line 121 instead. https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:144: } if (state_ == BACKGROUND_FADING_IN) { } https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:164: } if (state_ == BACKGROUND_FADING_IN || state_ == UNKNOWN) { } In general, switch () is best used when handling all enum values with no default. There are exceptions, but for just 1-2 values, if () is simpler. https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:83: void SkipCurrentAnimation(); If this is only used for testing, just name it SkipCurrentAnimationForTest().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) 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 PTAL https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:32: const float kBackgroundFinalOpacity = 0.75f; On 2016/12/22 at 22:03:32, stevenjb wrote: > constexpr for all of these. done https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:79: delete exit_label_; On 2016/12/22 at 22:03:33, stevenjb wrote: > Views are owned by the views hierarchy, this shouldn't be necessary. Done https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:95: 3 * display_.bounds().height() / 4, kExitLabelWidth, On 2016/12/22 at 22:03:33, stevenjb wrote: > nit: display_.bounds().height() * 3 / 4 is a little more readable. Done https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:113: state_ == BACKGROUND_FADING_OUT ? 0.f : kBackgroundFinalOpacity; On 2016/12/22 at 22:03:32, stevenjb wrote: > nit: Don't initialize this, set it in an else at line 121 instead. Done https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:144: } On 2016/12/22 at 22:03:33, stevenjb wrote: > if (state_ == BACKGROUND_FADING_IN) { > } This will be handling multiple states in the subsequent CL. https://codereview.chromium.org/2502823002/diff/20001/chrome/browser/chromeos... https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:164: } On 2016/12/22 at 22:03:33, stevenjb wrote: > if (state_ == BACKGROUND_FADING_IN || state_ == UNKNOWN) { > } > > In general, switch () is best used when handling all enum values with no default. There are exceptions, but for just 1-2 values, if () is simpler. ditto https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:83: void SkipCurrentAnimation(); On 2016/12/22 at 22:03:33, stevenjb wrote: > If this is only used for testing, just name it SkipCurrentAnimationForTest(). Done
lgtm https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2603463002/diff/1/chrome/browser/chromeos/dis... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:144: } On 2016/12/23 00:09:06, malaykeshav wrote: > On 2016/12/22 at 22:03:33, stevenjb wrote: > > if (state_ == BACKGROUND_FADING_IN) { > > } > > This will be handling multiple states in the subsequent CL. > > https://codereview.chromium.org/2502823002/diff/20001/chrome/browser/chromeos... Normally I would suggest changing to a switch at that point, but it's fine.
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
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": 40001, "attempt_start_ts": 1482455506645300,
"parent_rev": "5bbf3b09ae2adf4c4b585d526d55fcff596440a5", "commit_rev":
"2d56efae16f43753f49c2314780cbd78ffe78b0a"}
Message was sent while issue was closed.
Description was changed from ========== Adds background fade in animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests ========== to ========== Adds background fade in animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests Review-Url: https://codereview.chromium.org/2603463002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Adds background fade in animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests Review-Url: https://codereview.chromium.org/2603463002 ========== to ========== Adds background fade in animation and exit label to touch calibrator view - Adds string resource for exit label - Also updates unit tests. Working protottpe(The video is the entire prototype. This change only involves adding the fade in background animation.): https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1QVR0WFJYdUY2SmZTdWd... UX Specs: https://drive.google.com/file/d/0B_2Uyb2Rhx2OU0FIbXUyMkhMZlE/view Design Doc=go/cros-touch-calibration BUG=634166 COMPONENT=Touch Calibration, ChromeOS, Unittests Committed: https://crrev.com/33a91601c079314e849291390e44fc93f6b05caf Cr-Commit-Position: refs/heads/master@{#440571} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/33a91601c079314e849291390e44fc93f6b05caf Cr-Commit-Position: refs/heads/master@{#440571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
