Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2685273002:
Fix regression where font size too small for IME short name (Closed)
|
Created:
3 years, 10 months ago by Azure Wei Modified:
3 years, 10 months ago Reviewers:
tdanderson CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFor IMEs whose short name are more than 2 characters (INTL, EXTD, etc.),
we shrink the font size until fits within the bounds of |kMenuIconSize|.
But when performing the shrinking operation, we should use the text size
without insects for comparing.
Verified with both opt-in IME menu and system menu.
BUG=690563
TEST=Verified on local build.
Review-Url: https://codereview.chromium.org/2685273002
Cr-Commit-Position: refs/heads/master@{#450230}
Committed: https://chromium.googlesource.com/chromium/src/+/436c4bebb1f31ef7246eb7747eccd9a812f09cf7
Patch Set 1 #Patch Set 2 #
Total comments: 3
Patch Set 3 : sync #Messages
Total messages: 18 (9 generated)
Description was changed from ========== Use font size. BUG=690563 TEST=Verified on local build. ========== to ========== For IMEs whose short name are more than 2 characters (INTL, EXTD, etc.), we shrink the font size until fits within the bounds of |kMenuIconSize|. But when performing the shrinking operation, we should use the text size without insects for comparing. Verified with both opt-in IME menu and system menu. BUG=690563 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + tdanderson@chromium.org
Please review this CL. Thanks!
https://codereview.chromium.org/2685273002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2685273002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:109: id_label->GetInsets().width()) > kMenuIconSize && Can you please post a screenshot to the bug of what various IME short names look like with this CL applied (be sure to include INTL, EXTD, and the ones that are present in the screenshots attached to #0 on the bug). I'm confused about what caused this to regress between m-57 and m-58, do you have any ideas?
https://codereview.chromium.org/2685273002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2685273002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:109: id_label->GetInsets().width()) > kMenuIconSize && On 2017/02/10 16:16:45, tdanderson wrote: > Can you please post a screenshot to the bug of what various IME short names look > like with this CL applied (be sure to include INTL, EXTD, and the ones that are > present in the screenshots attached to #0 on the bug). > > I'm confused about what caused this to regress between m-57 and m-58, do you > have any ideas? Sure, attached screenshot to crbug.com/690563. Please take a look. I've tried some recent CLs on this file, but failed to find the one causing the regression. I suspected it may caused by some CL related to theme. But anyway, I think we could use Labels::GetTextSize() to avoid such regressing. Since GetTextSize() is protected in Labels, I used GetPreferredSize()-GetInsets() to get it.
LGTM https://codereview.chromium.org/2685273002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_list_view.cc (right): https://codereview.chromium.org/2685273002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_list_view.cc:109: id_label->GetInsets().width()) > kMenuIconSize && On 2017/02/13 01:07:29, Azure Wei wrote: > On 2017/02/10 16:16:45, tdanderson wrote: > > Can you please post a screenshot to the bug of what various IME short names > look > > like with this CL applied (be sure to include INTL, EXTD, and the ones that > are > > present in the screenshots attached to #0 on the bug). > > > > I'm confused about what caused this to regress between m-57 and m-58, do you > > have any ideas? > > Sure, attached screenshot to crbug.com/690563. Please take a look. Thanks a lot, and sgabriel@ has signed off on this. > > I've tried some recent CLs on this file, but failed to find the one causing the > regression. I suspected it may caused by some CL related to theme. > > But anyway, I think we could use Labels::GetTextSize() to avoid such regressing. > Since GetTextSize() is protected in Labels, I used > GetPreferredSize()-GetInsets() to get it. Acknowledged.
The CQ bit was checked by azurewei@chromium.org
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: 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 azurewei@chromium.org
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 azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2685273002/#ps40001 (title: "sync")
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": 1487039279325840,
"parent_rev": "58e0b3fed0864eaa09a5d34ad332b1508f9ec222", "commit_rev":
"436c4bebb1f31ef7246eb7747eccd9a812f09cf7"}
Message was sent while issue was closed.
Description was changed from ========== For IMEs whose short name are more than 2 characters (INTL, EXTD, etc.), we shrink the font size until fits within the bounds of |kMenuIconSize|. But when performing the shrinking operation, we should use the text size without insects for comparing. Verified with both opt-in IME menu and system menu. BUG=690563 TEST=Verified on local build. ========== to ========== For IMEs whose short name are more than 2 characters (INTL, EXTD, etc.), we shrink the font size until fits within the bounds of |kMenuIconSize|. But when performing the shrinking operation, we should use the text size without insects for comparing. Verified with both opt-in IME menu and system menu. BUG=690563 TEST=Verified on local build. Review-Url: https://codereview.chromium.org/2685273002 Cr-Commit-Position: refs/heads/master@{#450230} Committed: https://chromium.googlesource.com/chromium/src/+/436c4bebb1f31ef7246eb7747ecc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/436c4bebb1f31ef7246eb7747ecc... |
