New issue
Advanced search Search tips

Issue 798927 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocked on:
issue 803354

Blocking:
issue 793184
issue 799300
issue 801029
issue 801094



Sign in to add a comment

Audit gfx::Elide*() for browser UI vs secondary UI

Project Member Reported by tapted@chromium.org, Jan 4 2018

Issue description

Chrome 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

 
Components: UI
Description: Show this description
Blocking: 799300
Description: Show this description
Status: Started (was: Assigned)
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.


Project Member

Comment 6 by bugdroid1@chromium.org, 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

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Comment 8 by tapted@chromium.org, Jan 11 2018

Blocking: 801094

Comment 9 by tapted@chromium.org, Jan 11 2018

Blocking: 801029
Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Project Member

Comment 13 by bugdroid1@chromium.org, 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

Blockedon: 803354
Cc: pkasting@chromium.org
BTW, will this distinction disappear again some day with Mac Views Browser (at which point views will draw everything so Harfbuzz will typeset everything)?
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.
Status: Fixed (was: Started)
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