Migrate toolkit-views to RenderTextHarfbuzz only; Move gfx::Elide*,GetStringWidth* methods to gfx::RenderText. |
||||||||||
Issue descriptionChrome Version : m65 RenderTextMac is not feature complete (e.g. no support for multiline or cursors/navigation/selection). toolkit-views has a bunch of workarounds to deal with this, but they are slow and increase complexity. Many issues are blocked on either adding more workarounds, or phasing out RenderTextMac. We should phase out RenderTextMac from toolkit-views. This is not easy, mainly because there are many callers to gfx::GetStringWidth() and related functions that come from cross-platform code, which does not whether that text will be rendered by toolkit-views or by AppKit. As much as possible, calls to ElideText and GetStringWidth need to be pushed down so that they are closer to the text-drawing code (e.g. views::Label). ElideText, GetStringWidth, and related functions tend to be slow/wasteful in any case, since they create a gfx::RenderText, typeset all the text to find the width, and then throw all that computation away. This usually leaves something like views::Label to typeset the text again when drawing. ElideRectangleText in particular is wasteful since it is not taking advantage of RenderText multiline support when it is available. Plan: Move these methods to RenderText to encourage reuse of the render-text object. Then, if a caller can not move away from gfx::Elide/GetStringWidth then at least it has a chance to cache the RenderText instance, and create the right instance.
,
Dec 4 2017
pre-yak: https://chromium-review.googlesource.com/c/chromium/src/+/805516 cl: https://chromium-review.googlesource.com/c/chromium/src/+/805534 many post-yaks to come, I need to - Audit Cocoa browser UI (and cross platform things like bookmark model), ensure nothing is sending a string drawn by AppKit to gfx::ElideText and friends. - Remove gfx::RenderText::CreateInstanceDeprecated()
,
Dec 4 2017
,
Dec 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046 commit a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046 Author: Trent Apted <tapted@chromium.org> Date: Thu Dec 07 23:39:48 2017 MacViews: Make views::Label RenderTextHarfBuzz only. RenderTextMac does not support multiline (nor text selection). Lack of multiline support results in some workarounds that are very inefficient in practice. This requires Label to temporarily use a different RenderText implementation to the one used by methods such as gfx::GetStringWidth(), gfx::ElideText() and url_formatter::ElideUrl(). This is a problem, since these methods may be used by UI code before it is known whether the text will be drawn by Cocoa or by Views. This can result in text being drawn clipped or with padding. Next step: adjust calls to gfx::GetStringWidth(),etc. that may eventually be used to render to Cocoa UI, and ensure they specify RenderTextMac. Then, change the default render text instance used for these gfx:: utility methods (or phase out those methods entirely since caching a RenderText can often achieve much better performance). TBR=bajones@chromium.org, sadrul@chromium.org Bug: 791391 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ie5eb3f5b9a3592bf60e3bc10ce1f79f0c1f139ee Reviewed-on: https://chromium-review.googlesource.com/805534 Commit-Queue: Trent Apted <tapted@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#522617} [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ash/system/user/user_card_view.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/chrome/browser/ui/views/frame/hosted_app_frame_header_ash.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/chrome/browser/ui/views/omnibox/omnibox_result_view.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/chrome/browser/vr/elements/ui_texture.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/components/ui_devtools/views/dom_agent.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/app_list/views/search_result_view.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/gfx/canvas_skia.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/gfx/render_text.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/gfx/render_text.h [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/gfx/text_elider.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/views/bubble/bubble_frame_view_unittest.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/views/controls/label.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/views/controls/label.h [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/views/controls/label_unittest.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/views/controls/textfield/textfield_model.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/views/corewm/tooltip_aura.cc [modify] https://crrev.com/a2e6ffe492fa09ab3a55f1ba11e99d09cc24b046/ui/views/examples/multiline_example.cc
,
Jan 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf commit cb80f1dfa0af3b4623394da9199e0ea5f3e962bf Author: Trent Apted <tapted@chromium.org> Date: Wed Jan 03 20:24:03 2018 Adds LabelTest.FadedString for views::Label. A Pixel Test. There's an existing pixel test, LayerWithRealCompositorTest's CanvasDrawFadedString. However, - Nothing calls Canvas::DrawFadedText() except this test - The test is only enabled on Windows ("pixel tests are hard") - The test's reference image is too big.. - ..which means it doesn't actually need to elide/fade the string. - It requires a lot of plumbing (DrawFadedStringLayerDelegate) - Is uses cc::FuzzyPixelComparator with some magic numbers. http://crrev.com/c/828220 would delete Canvas::DrawFadedText() since nothing uses it, along with the existing pixel test. However, faded text still exists and is used in views::Label. This CL moves the pixel test to views::Label and "seeds" a pixel testing framework under ui/views for testing UI. Native UI often, intentionally differs across platforms and OS versions. The approach for the pixel testing framework attempts to cater for that without creating a maintenance burden. Bug: 791391 Change-Id: I6c71f2c98cf7feca07e2a4a10cf37926710b9d9f Reviewed-on: https://chromium-review.googlesource.com/805516 Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#526787} [modify] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/BUILD.gn [modify] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/controls/label_unittest.cc [modify] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/DEPS [add] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/data/linux/string_faded.png [add] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/data/mac-mac10.12/string_faded.png [add] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/data/mac-mac10.9/string_faded.png [add] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/data/win10/string_faded.png [add] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/data/win7/string_faded.png [add] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/views_pixel_test.cc [add] https://crrev.com/cb80f1dfa0af3b4623394da9199e0ea5f3e962bf/ui/views/test/views_pixel_test.h
,
Jan 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/427d196382504575ce3ebf6b68430c6813f74310 commit 427d196382504575ce3ebf6b68430c6813f74310 Author: Xida Chen <xidachen@chromium.org> Date: Wed Jan 03 21:17:16 2018 Revert "Adds LabelTest.FadedString for views::Label. A Pixel Test." This reverts commit cb80f1dfa0af3b4623394da9199e0ea5f3e962bf. Reason for revert: <INSERT REASONING HERE> Caused test failure: https://ci.chromium.org/buildbot/chromium.win/Win%207%20Tests%20x64%20%281%29/32933 Original change's description: > Adds LabelTest.FadedString for views::Label. A Pixel Test. > > There's an existing pixel test, LayerWithRealCompositorTest's > CanvasDrawFadedString. However, > - Nothing calls Canvas::DrawFadedText() except this test > - The test is only enabled on Windows ("pixel tests are hard") > - The test's reference image is too big.. > - ..which means it doesn't actually need to elide/fade the string. > - It requires a lot of plumbing (DrawFadedStringLayerDelegate) > - Is uses cc::FuzzyPixelComparator with some magic numbers. > > http://crrev.com/c/828220 would delete Canvas::DrawFadedText() since > nothing uses it, along with the existing pixel test. However, faded > text still exists and is used in views::Label. > > This CL moves the pixel test to views::Label and "seeds" a pixel > testing framework under ui/views for testing UI. Native UI often, > intentionally differs across platforms and OS versions. The approach > for the pixel testing framework attempts to cater for that without > creating a maintenance burden. > > Bug: 791391 > Change-Id: I6c71f2c98cf7feca07e2a4a10cf37926710b9d9f > Reviewed-on: https://chromium-review.googlesource.com/805516 > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Michael Wasserman <msw@chromium.org> > Commit-Queue: Trent Apted <tapted@chromium.org> > Cr-Commit-Position: refs/heads/master@{#526787} TBR=danakj@chromium.org,msw@chromium.org,tapted@chromium.org,dpranke@chromium.org Change-Id: Ie633a0da4bb33234c5be723a8aed13d63cdca403 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 791391 Reviewed-on: https://chromium-review.googlesource.com/848059 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#526816} [modify] https://crrev.com/427d196382504575ce3ebf6b68430c6813f74310/ui/views/BUILD.gn [modify] https://crrev.com/427d196382504575ce3ebf6b68430c6813f74310/ui/views/controls/label_unittest.cc [modify] https://crrev.com/427d196382504575ce3ebf6b68430c6813f74310/ui/views/test/DEPS [delete] https://crrev.com/7a82cda4f10516c1da1fcb990a6a33146660448b/ui/views/test/data/linux/string_faded.png [delete] https://crrev.com/7a82cda4f10516c1da1fcb990a6a33146660448b/ui/views/test/data/mac-mac10.12/string_faded.png [delete] https://crrev.com/7a82cda4f10516c1da1fcb990a6a33146660448b/ui/views/test/data/mac-mac10.9/string_faded.png [delete] https://crrev.com/7a82cda4f10516c1da1fcb990a6a33146660448b/ui/views/test/data/win10/string_faded.png [delete] https://crrev.com/7a82cda4f10516c1da1fcb990a6a33146660448b/ui/views/test/data/win7/string_faded.png [delete] https://crrev.com/7a82cda4f10516c1da1fcb990a6a33146660448b/ui/views/test/views_pixel_test.cc [delete] https://crrev.com/7a82cda4f10516c1da1fcb990a6a33146660448b/ui/views/test/views_pixel_test.h
,
Jan 23 2018
,
Mar 26 2018
remaining work here: reland https://chromium-review.googlesource.com/805516 I suspect the workaround for the failure is already in a comment on the CL (use absolute paths). _Ideally_ the isolate stuff just works properly with relative paths.. or it spits out this error more reliably rather than waiting for CLs to hit the waterfall.
,
Mar 26 2018
tapted: I'm leaving this one assigned to you, targeted at M-68 - if that doesn't seem feasible please kick it back.
,
Mar 26 2018
,
Mar 27 2018
,
Mar 29 2018
** Bulk Edit ** FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.
,
Apr 25 2018
Pls mark the bug as fixed if CL is landed in trunk and nothing else is pending. Thank you.
,
Jun 20 2018
,
Jul 12
,
Jul 12
,
Jul 26
,
Aug 13
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by tapted@chromium.org
, Dec 4 2017