Bug 183149 – Use ICU's ubidi facilities for canvas text

• Component: Canvas
• Status: NEW

[Myles C. Maxfield]

Use ICU's ubidi facilities for canvas text

[Myles C. Maxfield]

Created attachment 334663 [details]
WIP

[Myles C. Maxfield]

Created attachment 334670 [details]
Patch

[Myles C. Maxfield]

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 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 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]

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 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]

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]

Created attachment 334725 [details]
Patch

[Sam Weinig]

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 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?