Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627483003:
Centering turn on Bluetooth message on the chooser (Closed)
DescriptionCentering turn on Bluetooth message on the chooser
Currently the message "Turn on Bluetooth to allow pairing" is aligned
left on the chooser. This CL moves it to the center of the chooser.
I uploaded some screenshots on the issue page.
BUG=664240
Review-Url: https://codereview.chromium.org/2627483003
Cr-Commit-Position: refs/heads/master@{#443015}
Committed: https://chromium.googlesource.com/chromium/src/+/2fa22cff92afbaa7bf994ea2a950c306f92e0749
Patch Set 1 : centering turn on Bluetooth message on the chooser #
Total comments: 6
Patch Set 2 : address comments #Patch Set 3 : added comment #
Messages
Total messages: 26 (17 generated)
The CQ bit was checked by juncai@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...
juncai@chromium.org changed reviewers: + rsesek@chromium.org, sky@chromium.org, tedchoc@chromium.org
tedchoc@chromium.org: Please review changes in //chrome/android/java/res/layout rsesek@chromium.org: Please review changes in //chrome/browser/ui/cocoa sky@chromium.org: Please review changes in //chrome/browser/ui/views
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/cocoa/dev... File chrome/browser/ui/cocoa/device_chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/cocoa/dev... chrome/browser/ui/cocoa/device_chooser_content_view_cocoa.mm:529: CGFloat adapterOffHelpButtonOriginX = Consider flooring this in case the division results in a factional pixel, which would result in fuzzy text.
On 2017/01/10 21:25:20, Robert Sesek wrote: > LGTM > > https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/cocoa/dev... > File chrome/browser/ui/cocoa/device_chooser_content_view_cocoa.mm (right): > > https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/cocoa/dev... > chrome/browser/ui/cocoa/device_chooser_content_view_cocoa.mm:529: CGFloat > adapterOffHelpButtonOriginX = > Consider flooring this in case the division results in a factional pixel, which > would result in fuzzy text. android - lgtm
https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... File chrome/browser/ui/views/device_chooser_content_view.cc (right): https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... chrome/browser/ui/views/device_chooser_content_view.cc:111: (rect.height() - turn_adapter_off_help_->height()) / 2)); Might this be on top of the throbber?
The CQ bit was checked by juncai@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/2627483003/diff/1/chrome/browser/ui/cocoa/dev... File chrome/browser/ui/cocoa/device_chooser_content_view_cocoa.mm (right): https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/cocoa/dev... chrome/browser/ui/cocoa/device_chooser_content_view_cocoa.mm:529: CGFloat adapterOffHelpButtonOriginX = On 2017/01/10 21:25:19, Robert Sesek wrote: > Consider flooring this in case the division results in a factional pixel, which > would result in fuzzy text. Done. https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... File chrome/browser/ui/views/device_chooser_content_view.cc (right): https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... chrome/browser/ui/views/device_chooser_content_view.cc:111: (rect.height() - turn_adapter_off_help_->height()) / 2)); On 2017/01/10 23:48:31, sky wrote: > Might this be on top of the throbber? No. The adapter off message will only be shown when the adapter is off, and in that case, the system won't be able to scan for devices, so the throbber won't be shown.
LGTM https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... File chrome/browser/ui/views/device_chooser_content_view.cc (right): https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... chrome/browser/ui/views/device_chooser_content_view.cc:111: (rect.height() - turn_adapter_off_help_->height()) / 2)); On 2017/01/11 00:12:50, juncai wrote: > On 2017/01/10 23:48:31, sky wrote: > > Might this be on top of the throbber? > > No. The adapter off message will only be shown when the adapter is off, and in > that case, the system won't be able to scan for devices, so the throbber won't > be shown. Ok, thanks. Could you add a comment here to that effect.
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_...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... File chrome/browser/ui/views/device_chooser_content_view.cc (right): https://codereview.chromium.org/2627483003/diff/1/chrome/browser/ui/views/dev... chrome/browser/ui/views/device_chooser_content_view.cc:111: (rect.height() - turn_adapter_off_help_->height()) / 2)); On 2017/01/11 00:36:14, sky wrote: > On 2017/01/11 00:12:50, juncai wrote: > > On 2017/01/10 23:48:31, sky wrote: > > > Might this be on top of the throbber? > > > > No. The adapter off message will only be shown when the adapter is off, and in > > that case, the system won't be able to scan for devices, so the throbber won't > > be shown. > > Ok, thanks. Could you add a comment here to that effect. Done.
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 juncai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, rsesek@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2627483003/#ps40001 (title: "added comment")
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": 1484172598512010,
"parent_rev": "a80bb6a60810ca919a1d01bc391716c040070b4f", "commit_rev":
"2fa22cff92afbaa7bf994ea2a950c306f92e0749"}
Message was sent while issue was closed.
Description was changed from ========== Centering turn on Bluetooth message on the chooser Currently the message "Turn on Bluetooth to allow pairing" is aligned left on the chooser. This CL moves it to the center of the chooser. I uploaded some screenshots on the issue page. BUG=664240 ========== to ========== Centering turn on Bluetooth message on the chooser Currently the message "Turn on Bluetooth to allow pairing" is aligned left on the chooser. This CL moves it to the center of the chooser. I uploaded some screenshots on the issue page. BUG=664240 Review-Url: https://codereview.chromium.org/2627483003 Cr-Commit-Position: refs/heads/master@{#443015} Committed: https://chromium.googlesource.com/chromium/src/+/2fa22cff92afbaa7bf994ea2a950... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2fa22cff92afbaa7bf994ea2a950... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
