[webkit-reviews] review granted: [Bug 63209] Make line breaking obey the -webkit-locale property : [Attachment 98309] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted mitz at webkit.org's request for
review:
Bug 63209: Make line breaking obey the -webkit-locale property
https://bugs.webkit.org/show_bug.cgi?id=63209

Attachment 98309: Patch
https://bugs.webkit.org/attachment.cgi?id=98309&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98309&action=review

> Source/WebCore/platform/text/TextBreakIterator.h:48
> +    void releaseLineBreakIterator(TextBreakIterator*, const AtomicString&
locale = AtomicString());

I can see why this was easier to implement, but passing locale to the release
function is quite surprising and misleading. Perhaps it would be better to
maintain a reverse map of acquired TextBreakIterators to their locales?

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:78
> +    static LineBreakIteratorPool& sharedPool()
> +    {
> +	   DEFINE_STATIC_LOCAL(LineBreakIteratorPool, pool, ());

Perhaps ASSERT(isMainThread())?

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:93
> +	   return ubrk_open(UBRK_LINE, locale.isEmpty() ?
currentTextBreakLocaleID() : locale.string().ascii().data(), 0, 0,
&openStatus);

I would at least ASSERT the status.

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:123
> +    if (U_FAILURE(setTextStatus))
> +	   return 0;

I'm not sure what this error means. Is this a completely unexpected condition
that we should ASSERT against?