Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839873003:
PlatformFontMac: better guess for average width (Closed)
|
Created:
3 years, 8 months ago by Elly Fong-Jones Modified:
3 years, 7 months ago CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlatformFontMac: better guess for average width
PlatformFontMac::GetExpectedTextWidth() was wildly over-estimating text widths,
causing dialogs that try to size using Widget::GetLocalizedContentsWidth to be
extremely wide on Mac. This change replaces the guess in PlatformFontMac's
CalculateMetricsAndInitRenderParams() with a more empirically accurate guess.
BUG=654128
Review-Url: https://codereview.chromium.org/2839873003
Cr-Commit-Position: refs/heads/master@{#469336}
Committed: https://chromium.googlesource.com/chromium/src/+/3ff0361da27135ec64b787f2630af4ae960399b0
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixes #
Total comments: 4
Patch Set 3 : add a bit of safety checking #
Total comments: 1
Patch Set 4 : as -> attr_string #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== PlatformFontMac: better guess for average width PlatformFontMac::GetExpectedTextWidth() was wildly over-estimating text widths, causing dialogs that try to size using Widget::GetLocalizedContentsWidth to be extremely wide on Mac. This change replaces the guess in PlatformFontMac's CalculateMetricsAndInitRenderParams() with a more empirically accurate guess. BUG=654128 ========== to ========== PlatformFontMac: better guess for average width PlatformFontMac::GetExpectedTextWidth() was wildly over-estimating text widths, causing dialogs that try to size using Widget::GetLocalizedContentsWidth to be extremely wide on Mac. This change replaces the guess in PlatformFontMac's CalculateMetricsAndInitRenderParams() with a more empirically accurate guess. BUG=654128 ==========
ellyjones@chromium.org changed reviewers: + tapted@chromium.org
tapted: ptal? :)
this seems reasonable - just some suggestions for possible improvements https://codereview.chromium.org/2839873003/diff/1/ui/gfx/platform_font_mac.mm File ui/gfx/platform_font_mac.mm (right): https://codereview.chromium.org/2839873003/diff/1/ui/gfx/platform_font_mac.mm... ui/gfx/platform_font_mac.mm:225: NSAttributedString* as = [[[NSAttributedString alloc] since this is a bit heavyweight, and barely anything calls GetExpectedTextWidth(..), can we calculate this inside GetExpectedTextWidth? the data members should have initializers in the .h file (some code above becomes redundant), then GetExpectedTextWidth can check whether average_width_ has been changed from 0. Also we like to avoid sending objects to the autorelease pool if it can be avoided. We can use scoped_nsobject rather than autorelease to do that. https://codereview.chromium.org/2839873003/diff/1/ui/gfx/platform_font_mac.mm... ui/gfx/platform_font_mac.mm:228: average_width_ = [as size].width / [as length]; rounding is occurring here, and GetExpectedTextWidth() takes a "length" argument that will magnify the rounding. I think we should just make average_width_ a float, and explicitly do ceil(average_width_ * length) in GetExpectedTextWidth()
tapted: ptal? :) https://codereview.chromium.org/2839873003/diff/1/ui/gfx/platform_font_mac.mm File ui/gfx/platform_font_mac.mm (right): https://codereview.chromium.org/2839873003/diff/1/ui/gfx/platform_font_mac.mm... ui/gfx/platform_font_mac.mm:225: NSAttributedString* as = [[[NSAttributedString alloc] On 2017/05/01 06:48:05, tapted wrote: > since this is a bit heavyweight, and barely anything calls > GetExpectedTextWidth(..), can we calculate this inside GetExpectedTextWidth? > > the data members should have initializers in the .h file (some code above > becomes redundant), then GetExpectedTextWidth can check whether average_width_ > has been changed from 0. > > Also we like to avoid sending objects to the autorelease pool if it can be > avoided. We can use scoped_nsobject rather than autorelease to do that. Done. https://codereview.chromium.org/2839873003/diff/1/ui/gfx/platform_font_mac.mm... ui/gfx/platform_font_mac.mm:228: average_width_ = [as size].width / [as length]; On 2017/05/01 06:48:05, tapted wrote: > rounding is occurring here, and GetExpectedTextWidth() takes a "length" argument > that will magnify the rounding. I think we should just make average_width_ a > float, and explicitly do ceil(average_width_ * length) in GetExpectedTextWidth() Done.
lgtm with the following https://codereview.chromium.org/2839873003/diff/20001/ui/gfx/platform_font_ma... File ui/gfx/platform_font_mac.mm (right): https://codereview.chromium.org/2839873003/diff/20001/ui/gfx/platform_font_ma... ui/gfx/platform_font_mac.mm:136: if (!average_width_) { I think we need a `&& native_font_` in this condition -- there are weird cases where it's null for invalid font names and I'm not sure whether NSAttributedString will behave in that case https://codereview.chromium.org/2839873003/diff/20001/ui/gfx/platform_font_ma... ui/gfx/platform_font_mac.mm:145: average_width_ = [as size].width / [as length]; nit: DCHECK(!!average_width_) ?
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2839873003/#ps40001 (title: "add a bit of safety checking")
https://codereview.chromium.org/2839873003/diff/20001/ui/gfx/platform_font_ma... File ui/gfx/platform_font_mac.mm (right): https://codereview.chromium.org/2839873003/diff/20001/ui/gfx/platform_font_ma... ui/gfx/platform_font_mac.mm:136: if (!average_width_) { On 2017/05/03 00:43:35, tapted wrote: > I think we need a `&& native_font_` in this condition -- there are weird cases > where it's null for invalid font names and I'm not sure whether > NSAttributedString will behave in that case Done. https://codereview.chromium.org/2839873003/diff/20001/ui/gfx/platform_font_ma... ui/gfx/platform_font_mac.mm:145: average_width_ = [as size].width / [as length]; On 2017/05/03 00:43:35, tapted wrote: > nit: DCHECK(!!average_width_) ? Done.
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...)
ellyjones@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: ptal as ui/font owner? :)
lgtm https://codereview.chromium.org/2839873003/diff/40001/ui/gfx/platform_font_ma... File ui/gfx/platform_font_mac.mm (right): https://codereview.chromium.org/2839873003/diff/40001/ui/gfx/platform_font_ma... ui/gfx/platform_font_mac.mm:142: base::scoped_nsobject<NSAttributedString> as([[NSAttributedString alloc] Nit: |as| is too short a name and acronyms are discouraged. Suggest attr_string.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2839873003/#ps60001 (title: "as -> attr_string")
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": 60001, "attempt_start_ts": 1493909116276190,
"parent_rev": "7e3aaf256e6d56290701b3d4b7d982139d8441da", "commit_rev":
"3ff0361da27135ec64b787f2630af4ae960399b0"}
Message was sent while issue was closed.
Description was changed from ========== PlatformFontMac: better guess for average width PlatformFontMac::GetExpectedTextWidth() was wildly over-estimating text widths, causing dialogs that try to size using Widget::GetLocalizedContentsWidth to be extremely wide on Mac. This change replaces the guess in PlatformFontMac's CalculateMetricsAndInitRenderParams() with a more empirically accurate guess. BUG=654128 ========== to ========== PlatformFontMac: better guess for average width PlatformFontMac::GetExpectedTextWidth() was wildly over-estimating text widths, causing dialogs that try to size using Widget::GetLocalizedContentsWidth to be extremely wide on Mac. This change replaces the guess in PlatformFontMac's CalculateMetricsAndInitRenderParams() with a more empirically accurate guess. BUG=654128 Review-Url: https://codereview.chromium.org/2839873003 Cr-Commit-Position: refs/heads/master@{#469336} Committed: https://chromium.googlesource.com/chromium/src/+/3ff0361da27135ec64b787f2630a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3ff0361da27135ec64b787f2630a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
