Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655003002:
Adds arrow icon and updates exit label color for touch calibration (Closed)
|
Created:
3 years, 11 months ago by malaykeshav Modified:
3 years, 10 months ago Reviewers:
oshima CC:
chromium-reviews, davemoore+watch_chromium.org, kalyank, oshima+watch_chromium.org, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds arrow icon and updates exit label color for touch calibration
- Updates text color for exit label, to make it brighter.
- Uses bubble border to add arrow icon.
Screenshot
https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1Yi1IOHNwbUpwSzQ/
Screenshot RTL
https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1RWNTM2VQQkVSS3c/
BUG=634166, 685338, 685339
COMPONENT=Touch Calibtation, Chrome OS
Review-Url: https://codereview.chromium.org/2655003002
Cr-Commit-Position: refs/heads/master@{#446878}
Committed: https://chromium.googlesource.com/chromium/src/+/6f2db51fbe14e281613fa054a650a314464125e1
Patch Set 1 : Adds arrow icon and updates exit label color for touch calibration #Patch Set 2 : Further adjustment after UI-Review #
Total comments: 4
Patch Set 3 : Adds arrow icon and updates exit label color for touch calibration #Patch Set 4 : Use bubble border to add arrow icon #Patch Set 5 : Better way to compute bounds of bubble #
Total comments: 1
Patch Set 6 : nit #Messages
Total messages: 50 (39 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
malaykeshav@chromium.org changed reviewers: + oshima@chromium.org
Some changes after UI review. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Adds an arrow icon to the hint box to match the UX. - Arrow icon also works in RTL mode. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1cmFLcklJX2JVNTI0d1p... BUG=634166 COMPONENT=Touch Calibtation, Chrome OS ========== to ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Adds an arrow icon to the hint box to match the UX. - Arrow icon also works in RTL mode. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1cmFLcklJX2JVNTI0d1p... BUG=634166, 685338 COMPONENT=Touch Calibtation, Chrome OS ==========
Description was changed from ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Adds an arrow icon to the hint box to match the UX. - Arrow icon also works in RTL mode. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1cmFLcklJX2JVNTI0d1p... BUG=634166, 685338 COMPONENT=Touch Calibtation, Chrome OS ========== to ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Adds an arrow icon to the hint box to match the UX. - Arrow icon also works in RTL mode. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1cmFLcklJX2JVNTI0d1p... BUG=634166, 685338, 685339 COMPONENT=Touch Calibtation, Chrome OS ==========
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.
Can you use bubble border (ui/views/bubble/bubble_border.h)? https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:317: bounds.height())) { can you use GetMirroredxxx api on views? https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:344: const int kMultiplier = base::i18n::IsRTL() ? 1 : -1; ditto
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...
Code now uses GetMirroredxxxx to set bounds for RTL mode. PTAL https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:317: bounds.height())) { On 2017/01/25 at 22:08:54, oshima wrote: > can you use GetMirroredxxx api on views? Done https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:344: const int kMultiplier = base::i18n::IsRTL() ? 1 : -1; On 2017/01/25 at 22:08:54, oshima wrote: > ditto Done
On 2017/01/25 22:08:54, oshima wrote: > Can you use bubble border (ui/views/bubble/bubble_border.h)? You probably missed this. Can you look into if this can be used here? > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc > (right): > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:317: > bounds.height())) { > can you use GetMirroredxxx api on views? > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:344: > const int kMultiplier = base::i18n::IsRTL() ? 1 : -1; > ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/26 at 00:37:39, oshima wrote: > On 2017/01/25 22:08:54, oshima wrote: > > Can you use bubble border (ui/views/bubble/bubble_border.h)? > You probably missed this. > > Can you look into if this can be used here? > > > > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > > File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc > > (right): > > > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > > chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:317: > > bounds.height())) { > > can you use GetMirroredxxx api on views? > > > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > > chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:344: > > const int kMultiplier = base::i18n::IsRTL() ? 1 : -1; > > ditto Bubble border is for a view. It cannot be used in this case, as here the bubble is drawn with `canvas->DrawRoundRect(bounds, corner_radius, paint);`. It is not a view. The border of the rounded rect do not match the view, because of rounded corners. It is also not possible to set the color of Arrow and bubble separately. (https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_border.h?rcl=0&l=171)
On 2017/01/26 at 00:50:44, malaykeshav wrote: > On 2017/01/26 at 00:37:39, oshima wrote: > > On 2017/01/25 22:08:54, oshima wrote: > > > Can you use bubble border (ui/views/bubble/bubble_border.h)? > > You probably missed this. > > > > Can you look into if this can be used here? > > > > > > > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc > > > (right): > > > > > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > > > chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:317: > > > bounds.height())) { > > > can you use GetMirroredxxx api on views? > > > > > > https://codereview.chromium.org/2655003002/diff/40001/chrome/browser/chromeos... > > > chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:344: > > > const int kMultiplier = base::i18n::IsRTL() ? 1 : -1; > > > ditto > > Bubble border is for a view. It cannot be used in this case, as here the bubble is drawn with `canvas->DrawRoundRect(bounds, corner_radius, paint);`. It is not a view. > The border of the rounded rect do not match the view, because of rounded corners. > It is also not possible to set the color of Arrow and bubble separately. (https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_border.h?rcl=0&l=171) ping
Patchset #4 (id:80001) has been deleted
Description was changed from ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Adds an arrow icon to the hint box to match the UX. - Arrow icon also works in RTL mode. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1cmFLcklJX2JVNTI0d1p... BUG=634166, 685338, 685339 COMPONENT=Touch Calibtation, Chrome OS ========== to ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Adds an arrow icon to the hint box to match the UX. - Arrow icon also works in RTL mode. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1Yi1IOHNwbUpwSzQ/ Screenshot RTL https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1RWNTM2VQQkVSS3c/ BUG=634166, 685338, 685339 COMPONENT=Touch Calibtation, Chrome OS ==========
Description was changed from ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Adds an arrow icon to the hint box to match the UX. - Arrow icon also works in RTL mode. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1Yi1IOHNwbUpwSzQ/ Screenshot RTL https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1RWNTM2VQQkVSS3c/ BUG=634166, 685338, 685339 COMPONENT=Touch Calibtation, Chrome OS ========== to ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Uses bubble border to add arrow icon. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1Yi1IOHNwbUpwSzQ/ Screenshot RTL https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1RWNTM2VQQkVSS3c/ BUG=634166, 685338, 685339 COMPONENT=Touch Calibtation, Chrome OS ==========
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Added bubble border with updated screenshots. PTAL
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
lgtm https://codereview.chromium.org/2655003002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2655003002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:408: views::View::OnPaint(canvas); nit: move this before other drawings.
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 oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2655003002/#ps180001 (title: "nit")
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": 180001, "attempt_start_ts": 1485568021359730,
"parent_rev": "f30619c261c576121ef5db19c1e4463beb53ef0e", "commit_rev":
"6f2db51fbe14e281613fa054a650a314464125e1"}
Message was sent while issue was closed.
Description was changed from ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Uses bubble border to add arrow icon. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1Yi1IOHNwbUpwSzQ/ Screenshot RTL https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1RWNTM2VQQkVSS3c/ BUG=634166, 685338, 685339 COMPONENT=Touch Calibtation, Chrome OS ========== to ========== Adds arrow icon and updates exit label color for touch calibration - Updates text color for exit label, to make it brighter. - Uses bubble border to add arrow icon. Screenshot https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1Yi1IOHNwbUpwSzQ/ Screenshot RTL https://drive.google.com/a/google.com/file/d/0B_WkX8bSkoT1RWNTM2VQQkVSS3c/ BUG=634166, 685338, 685339 COMPONENT=Touch Calibtation, Chrome OS Review-Url: https://codereview.chromium.org/2655003002 Cr-Commit-Position: refs/heads/master@{#446878} Committed: https://chromium.googlesource.com/chromium/src/+/6f2db51fbe14e281613fa054a650... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001) as https://chromium.googlesource.com/chromium/src/+/6f2db51fbe14e281613fa054a650... |
