WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(9.85 KB, patch)
2018-02-26 22:11 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(11.96 KB, patch)
2018-02-27 19:34 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
Tuesday, February 27, 2018 2:54:12 AM UTC
Created
attachment 334663
[details]
WIP
Myles C. Maxfield
Comment 2
Tuesday, February 27, 2018 6:11:27 AM UTC
Created
attachment 334670
[details]
Patch
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
Created
attachment 334725
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug