NEW 183149
Use ICU's ubidi facilities for canvas text
https://bugs.webkit.org/show_bug.cgi?id=183149
Summary Use ICU's ubidi facilities for canvas text
Myles C. Maxfield
Reported Monday, February 26, 2018 10:42:06 PM UTC
Use ICU's ubidi facilities for canvas text
Attachments
WIP (9.04 KB, patch)
2018-02-26 18:54 PST, Myles C. Maxfield
no flags
Patch (9.85 KB, patch)
2018-02-26 22:11 PST, Myles C. Maxfield
no flags
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.45 MB, application/zip)
2018-02-26 23:45 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews201 for win-future (11.60 MB, application/zip)
2018-02-26 23:53 PST, EWS Watchlist
no flags
Patch (11.96 KB, patch)
2018-02-27 19:34 PST, Myles C. Maxfield
no flags
Myles C. Maxfield
Comment 1 Tuesday, February 27, 2018 2:54:12 AM UTC
Myles C. Maxfield
Comment 2 Tuesday, February 27, 2018 6:11:27 AM UTC
Myles C. Maxfield
Comment 3 Tuesday, February 27, 2018 6:27:22 AM UTC
Comment on attachment 334670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334670&action=review > Source/WebCore/platform/graphics/GraphicsContext.cpp:631 > + UBiDi* line = ubidi_open(); Forgot to close.
Myles C. Maxfield
Comment 4 Tuesday, February 27, 2018 7:40:49 AM UTC
Comment on attachment 334670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334670&action=review > Source/WebCore/platform/graphics/GraphicsContext.cpp:642 > + ubidi_setPara(line, text, run.length(), run.direction() == LTR ? 0 : 1, nullptr, &errorCode); We may want to use UBIDI_DEFAULT_RTL or UBIDI_DEFAULT_LTR instead.
EWS Watchlist
Comment 5 Tuesday, February 27, 2018 7:45:54 AM UTC
Comment on attachment 334670 [details] Patch Attachment 334670 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6686428 New failing tests: fast/canvas/bidi.html
EWS Watchlist
Comment 6 Tuesday, February 27, 2018 7:45:55 AM UTC
Created attachment 334676 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 7 Tuesday, February 27, 2018 7:53:29 AM UTC
Comment on attachment 334670 [details] Patch Attachment 334670 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6686696 New failing tests: fast/canvas/bidi.html
EWS Watchlist
Comment 8 Tuesday, February 27, 2018 7:53:39 AM UTC
Created attachment 334677 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Myles C. Maxfield
Comment 9 Wednesday, February 28, 2018 3:34:52 AM UTC
Sam Weinig
Comment 10 Wednesday, March 21, 2018 10:44:43 PM UTC
Comment on attachment 334725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334725&action=review > Source/WebCore/ChangeLog:9 > + This is part of https://bugs.webkit.org/show_bug.cgi?id=183149 for using ICU's bidi facilities > + instead of our custom BidiResolver. I don't think this sentence adds much to to the introductory statement above. I think you should add a "why" clause here. For instance, why is using ICU's bidi facilities rather than our own a desirable thing? > Source/WebCore/platform/graphics/GraphicsContext.cpp:630 > + BidiCloser() > + { > + m_bidi = ubidi_open(); This can be in an initializer list. > Source/WebCore/platform/graphics/GraphicsContext.cpp:636 > + if (m_bidi) > + ubidi_close(m_bidi); In what cases is m_bidi null here? > Source/WebCore/platform/graphics/GraphicsContext.cpp:654 > + BidiCloser line; > + if (!line.bidi()) Can line.bidi() ever be null here?
Sam Weinig
Comment 11 Wednesday, March 21, 2018 10:46:18 PM UTC
Comment on attachment 334725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334725&action=review > LayoutTests/platform/ios/TestExpectations:3283 > +# This test requires specific metrics for the Times font. > +webkit.org/b/183149 fast/canvas/bidi.html [ ImageOnlyFailure ] Is there a way to use something like Ahem for this to avoid this issue?
Note You need to log in before you can comment on or make changes to this bug.