Audit gfx::Elide*() for browser UI vs secondary UI |
||||||||||
Issue descriptionChrome Version : 65.0.3294.5 OS Version: OS X 10.12.6 On Mac, Secondary UI can be typeset differently to Browser UI. This can influence the width. On Mac, gfx::Elide methods need to be aware of how the text will eventually be typeset. The following methods are affected: gfx::ElideText gfx::ElideFilename gfx::ElideRectangleText gfx::GetStringWidth gfx::GetStringWidthF url_formatter::ElideUrl url_formatter::ElideHost url_formatter::ElideUrlSimple Also Canvas methods, e.g. Canvas::DrawStringRectWithFlags
,
Jan 4 2018
,
Jan 7 2018
,
Jan 9 2018
,
Jan 9 2018
CLs gfx::ElideFilename: https://chromium-review.googlesource.com/c/chromium/src/+/848695 gfx::ElideRectangleText, url_formatter::ElideHost, Canvas::DrawStringRectWithFlags: https://chromium-review.googlesource.com/c/chromium/src/+/856016 gfx::ElideText: https://chromium-review.googlesource.com/c/chromium/src/+/856059 (browser code) https://chromium-review.googlesource.com/c/chromium/src/+/855997 (autofill) url_formatter::ElideUrl: (only* used for the Mac status bubble...?) https://chromium-review.googlesource.com/c/chromium/src/+/856023 Haven't explored gfx::GetStringWidth[F] yet, but they've been getting incremental updates in the above CLs.
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15bd76a4f4ac3109352c1f4745e2f1541cc142f3 commit 15bd76a4f4ac3109352c1f4745e2f1541cc142f3 Author: Trent Apted <tapted@chromium.org> Date: Wed Jan 10 01:44:08 2018 Allow Mac UI to gfx::ElideFilename() with the correct typesetter. On Mac, Secondary UI can be typeset differently to Browser UI, since one is drawn by Views and the other by Cocoa. This can influence things like text width in subtle ways. On Mac, gfx::Elide methods need to be aware of how the text will eventually be typeset. This CL adds enum class gfx::Typesetter, which can be fed through to gfx:: methods used for eliding text or determining its width. For the first step, update callers of gfx::ElideFilename to use the correct typesetter. Bug: 798927 , 697407 Change-Id: I7eea58391475a68fefc469e7b87729664087777d Reviewed-on: https://chromium-review.googlesource.com/848695 Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#528199} [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/chrome/browser/download/download_item_model.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/chrome/browser/ui/cocoa/download/download_item_cell.mm [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/chrome/browser/ui/cocoa/download/md_download_item_view.mm [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/canvas.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/canvas.h [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/canvas_notimplemented.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/canvas_skia.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/render_text.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/render_text.h [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_constants.h [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_elider.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_elider.h [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_elider_unittest.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_utils.h [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_utils_android.cc [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_utils_ios.mm [modify] https://crrev.com/15bd76a4f4ac3109352c1f4745e2f1541cc142f3/ui/gfx/text_utils_skia.cc
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aa6cafdb584215e0158560cadec23b56a1751925 commit aa6cafdb584215e0158560cadec23b56a1751925 Author: Trent Apted <tapted@chromium.org> Date: Wed Jan 10 23:42:01 2018 Make gfx::ElideRectangleText() and url_formatter::ElideHost() typesetter-aware. gfx::ElideRectangleText() is used on Mac both for native (system- driven) UI and for Chrome secondary UI. The former is used for system notifications and will be typeset by CoreText. The latter is typeset by Harfbuzz. This CL introduces gfx::ElideRectangleTextForNativeUi(), whose only consumer at this stage is the Mac message center, which pops native system notifications. Nothing else but MacViews was relying on gfx::ElideRectangleText() on Mac, so switch it over to Harfbuzz. There *is* one other caller: ExtensionIconPlaceholder::Draw(), which draws a single letter to a canvas with DrawStringRectWithFlags(). gfx::Canvas::DrawStringRectWithFlags() may call ElideRectangleText() in some codepaths, so switch DrawStringRectWithFlags() over to Harfbuzz as well so it's consistent. (In practice, the single letter would could never elide, so this doesn't change much). Ensure the rest of cocoa/notification_controller.mm is using the correct typesetter. This requires url_formatter::ElideHost() to also be typesetter-aware. TBR=dtrainor@chromium.org Bug: 793184 , 798927 , 799300 Change-Id: Ibccb740f97a833c9701b25037d69b0369f732028 Reviewed-on: https://chromium-review.googlesource.com/856016 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#528483} [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/chrome/browser/download/download_item_model.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/chrome/browser/ui/cocoa/download/download_item_cell.mm [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/chrome/browser/ui/cocoa/download/md_download_item_view.mm [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/components/url_formatter/elide_url.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/components/url_formatter/elide_url.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/canvas_skia.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/render_text.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/render_text.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/text_constants.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/text_elider.cc [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/gfx/text_elider.h [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/message_center/cocoa/notification_controller.mm [modify] https://crrev.com/aa6cafdb584215e0158560cadec23b56a1751925/ui/views/controls/styled_label_unittest.cc
,
Jan 11 2018
,
Jan 11 2018
,
Jan 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/147e5f947fcda5f0698ea91734bc1c4b52216c0f commit 147e5f947fcda5f0698ea91734bc1c4b52216c0f Author: Trent Apted <tapted@chromium.org> Date: Fri Jan 12 06:02:43 2018 Mac: Make autofill UI typesetter aware. We want to switch autofill popups from Cocoa to Views. This can cause subtle typesetting changes and alter string widths. Currently, some eliding decisions are made in code shared between platforms. Make it aware of which typesetter is used to avoid eliding errors during the transition. This needs to be done now to ensure the Cocoa UI doesn't regress when the default typesetter changes. Bug: 798927 Change-Id: I009de07add316f117fecacf59adbe560b39b4dec Reviewed-on: https://chromium-review.googlesource.com/855997 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Mathieu Perreault <mathp@chromium.org> Cr-Commit-Position: refs/heads/master@{#528890} [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/autofill/autofill_popup_controller_impl.h [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/autofill/autofill_popup_view_delegate.h [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/autofill/password_generation_popup_controller_impl.h [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa_unittest.mm [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/views/autofill/autofill_popup_view_native_views_unittest.cc [modify] https://crrev.com/147e5f947fcda5f0698ea91734bc1c4b52216c0f/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc
,
Jan 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce8ecabefbd9cd68b48d708a8666433bf5257358 commit ce8ecabefbd9cd68b48d708a8666433bf5257358 Author: Trent Apted <tapted@chromium.org> Date: Sun Jan 14 23:50:25 2018 Make the Cocoa browser code typesetter aware. We are switching the default UI typesetter to Harfbuzz (matches Blink, is more optimised, shares code with other platforms, and is more feature-complete compared to RenderTextMac). But sublte typesetting changes can cause variations in string widths, so UI and eliding code must be aware of how the text will be typeset. r528199 added the API to do this. This CL ensures remaining code that will render (its) text using Cocoa is indicating that it will use the native typesetter. Typesetter::NATIVE is used for text that will always be native (e.g. menus, tooltips). Typesetter::BROWSER is used for everything else. Bug: 798927 Change-Id: I38e4cb4de6ff1c8a5cc814bd093f6f59ad62d021 Reviewed-on: https://chromium-review.googlesource.com/856059 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#529186} [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/chrome/browser/ui/cocoa/download/download_item_cell.mm [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/chrome/browser/ui/cocoa/history_menu_bridge.mm [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/chrome/browser/ui/cocoa/location_bar/page_info_bubble_decoration.mm [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/chrome/browser/ui/cocoa/screen_capture_notification_ui_cocoa.mm [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/chrome/browser/ui/cocoa/status_bubble_mac.mm [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/chrome/browser/ui/toolbar/back_forward_menu_model.cc [modify] https://crrev.com/ce8ecabefbd9cd68b48d708a8666433bf5257358/ui/base/cocoa/menu_controller.mm
,
Jan 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5c48792e949b9a6d2d9a0c2ce7a4f93fe286efe6 commit 5c48792e949b9a6d2d9a0c2ce7a4f93fe286efe6 Author: Trent Apted <tapted@chromium.org> Date: Wed Jan 17 04:25:47 2018 Use gfx::Typesetter::BROWSER for url_formatter::ElideUrl() StatusBarMac is the last remaining consumer of gfx:: elide and string width functions that draws Cocoa UI but is not passing info about the typesetter to use when eliding. It's also the only thing on Mac that uses url_formatter::ElideUrl(), so it's sufficient to pass gfx::Typesetter::BROWSER to ensure correct behavior. elide_url.cc is also the last remaining consumer of GetStringWidth that may draw to Cocoa UI but does not pass a typesetter. So, after this, gfx::Typesetter::DEFAULT can switch from BROWSER to HARFBUZZ to address subtle typesetting differences in Harmony UI, without adversely affecting any Cocoa browser UI. (gfx::Typesetter can disappear once mac_views_browser ships). Bug: 798927 Change-Id: I6f5ff9b6a325b3b4c3b766ae5d9aabc6470ca8b5 Reviewed-on: https://chromium-review.googlesource.com/856023 Reviewed-by: Peter Kasting <pkasting@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#529603} [modify] https://crrev.com/5c48792e949b9a6d2d9a0c2ce7a4f93fe286efe6/components/url_formatter/elide_url.cc
,
Jan 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8a2b8ef0a399f1ff0850181c0ee5f900a9425290 commit 8a2b8ef0a399f1ff0850181c0ee5f900a9425290 Author: Trent Apted <tapted@chromium.org> Date: Thu Jan 18 05:51:58 2018 Switch the default typesetter in gfx:: to HARFBUZZ. All Cocoa UI consumers now specify that they typeset with CoreText. The remaining consumers all use HARFBUZZ. This resolves some subtle problems in views UI around text eliding. Bug: 801094 , 801029 , 798927 , 454835 Change-Id: Iece3506938d83e214837491eec3ef097ac296d0c Reviewed-on: https://chromium-review.googlesource.com/869630 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#530064} [modify] https://crrev.com/8a2b8ef0a399f1ff0850181c0ee5f900a9425290/ui/gfx/canvas_skia.cc [modify] https://crrev.com/8a2b8ef0a399f1ff0850181c0ee5f900a9425290/ui/gfx/text_constants.h [modify] https://crrev.com/8a2b8ef0a399f1ff0850181c0ee5f900a9425290/ui/gfx/text_elider_unittest.cc
,
Jan 18 2018
,
Jan 22 2018
BTW, will this distinction disappear again some day with Mac Views Browser (at which point views will draw everything so Harfbuzz will typeset everything)?
,
Jan 22 2018
That's the plan - I want to scrap RenderTextMac - it's an unnecessary maintenance burden. That will will make these gfx::Typesetter args obsolete. A couple of UI surfaces may still want to target native UI -- tooltips, system notifications, menus. However, these few use cases should be easy to switch over to the system libraries that do the same thing, which are pretty good really and already used on iOS. E.g. text_utils_ios for GetStringWidth -- https://cs.chromium.org/chromium/src/ui/gfx/text_utils_ios.mm . There's also https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/util/core_text_util.h Or the eliding/width stuff can often be refactored so that it's left entirely up to the UI layer. That often does a better job (e.g. with caching/re-use of the typsetting results). Or is even a lot more correct -- e.g. eliding in the Mac bookmarks menu currently looks really silly and is full of truncation/mistakes/inconsistencies.
,
Jan 30 2018
Blocked issues all fixed. This is all done. There was threat of a revert in Issue 804884 , but it looks to be a compiler bug. Remaining steps for phasing out RenderTextMac and removing gfx::Typesetter:FOO tracked in Issue 454835 . |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by tapted@chromium.org
, Jan 4 2018