Summary: | Use ICU's ubidi facilities for canvas text | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||
Component: | Canvas | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||
Status: | NEW --- | ||||||||||||||
Severity: | Normal | CC: | dino, ews-watchlist, jonlee, sam, simon.fraser, thorton, zalan | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2018-02-26 14:42:06 PST
Created attachment 334663 [details]
WIP
Created attachment 334670 [details]
Patch
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. 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. 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 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
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 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
Created attachment 334725 [details]
Patch
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? 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? |