923967 - OS X: move from deprecated Unicode Utilities usage to CFStringTokenizer

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Josh Aas]

We should move the following code from the old Carbon APIs to Core Foundation.

 0:01.61 nsCarbonBreaker.o
 0:02.03 Warning: -Wdeprecated-declarations in /Users/josh/src/mozilla/ff_trunk_debug/intl/lwbrk/src/nsCarbonBreaker.cpp: 'UCCreateTextBreakLocator' is deprecated: first deprecated in OS X 10.6
 0:02.03 /Users/josh/src/mozilla/ff_trunk_debug/intl/lwbrk/src/nsCarbonBreaker.cpp:20:21: warning: 'UCCreateTextBreakLocator' is deprecated: first deprecated in OS X 10.6 [-Wdeprecated-declarations]
 0:02.03   OSStatus status = UCCreateTextBreakLocator(nullptr,
 0:02.03                     ^
 0:02.03 /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/UnicodeUtilities.h:702:1: note: 'UCCreateTextBreakLocator' declared here
 0:02.03 UCCreateTextBreakLocator(
 0:02.03 ^
 0:02.03 Warning: -Wdeprecated-declarations in /Users/josh/src/mozilla/ff_trunk_debug/intl/lwbrk/src/nsCarbonBreaker.cpp: 'UCFindTextBreak' is deprecated: first deprecated in OS X 10.6
 0:02.03 /Users/josh/src/mozilla/ff_trunk_debug/intl/lwbrk/src/nsCarbonBreaker.cpp:30:14: warning: 'UCFindTextBreak' is deprecated: first deprecated in OS X 10.6 [-Wdeprecated-declarations]
 0:02.03     status = UCFindTextBreak(breakLocator,
 0:02.03              ^
 0:02.03 /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/UnicodeUtilities.h:723:1: note: 'UCFindTextBreak' declared here
 0:02.03 UCFindTextBreak(
 0:02.03 ^
 0:02.03 Warning: -Wdeprecated-declarations in /Users/josh/src/mozilla/ff_trunk_debug/intl/lwbrk/src/nsCarbonBreaker.cpp: 'UCDisposeTextBreakLocator' is deprecated: first deprecated in OS X 10.6
 0:02.03 /Users/josh/src/mozilla/ff_trunk_debug/intl/lwbrk/src/nsCarbonBreaker.cpp:44:3: warning: 'UCDisposeTextBreakLocator' is deprecated: first deprecated in OS X 10.6 [-Wdeprecated-declarations]
 0:02.03   UCDisposeTextBreakLocator(&breakLocator);
 0:02.03   ^
 0:02.03 /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/UnicodeUtilities.h:747:1: note: 'UCDisposeTextBreakLocator' declared here
 0:02.03 UCDisposeTextBreakLocator(TextBreakLocatorRef * breakRef)     __OSX_AVAILABLE_BUT_DEPRECATED(__MAC_10_0, __MAC_10_6, __IPHONE_NA, __IPHONE_NA);
 0:02.03 ^
 0:02.03 3 warnings generated.

Comment 1

[Josh Aas]

Attachment: Fix v1.0

Note that I don't know what this code is used for and I haven't tested my patch at all. I wrote what seems correct, it compiles, and the browser runs without any noticeable issues during a few minutes of surfing around.

Someone who knows what they're doing (Simon) should make sure this is correct before we take a patch.

Comment 2

[Gen Kanai [:gen]]

Adding Keng, Cheng, Chantra, Vannak for line break testing on Thai and Khmer. Adding Arky for Lao and Tibetan.

Comment 3

[Gen Kanai [:gen]]

Please provide detailed testing instructions on Mac OS.

Comment 4

[arky [:arky]]

Thanks @gen. 

It would be great to have some screenshots of test content. That would be easy to review. Not everyone I know have a Mac

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 813973 [details] [diff] [review]
Fix v1.0

Review of attachment 813973 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/lwbrk/src/nsCarbonBreaker.cpp
@@ +16,2 @@
>  
> +  CFLocaleRef lr = CFLocaleCopyCurrent();

I'm not sure we want to be using the user's current locale here; if there are potential variations in line-breaking behavior, shouldn't the choice be determined primarily by the language of the page content?

Ah, never mind - for kCFStringTokenizerUnitLineBreak, the doc says that "the locale parameter of CFStringTokenizerCreate is ignored". So it won't make any difference, at least for the time being. (The doc also says that "the locale sensitivity of the tokenization unit options may change in a future release".)

Might be worth at least leaving a comment here, if we don't want to actually implement anything more sophisticated for now.

Comment 6

[Josh Aas]

Attachment: Fix v1.1

Updated to current trunk.

Comment 7

[Josh Aas]

Build to test is here:

http://people.mozilla.org/~josh/firefox-923967-2.dmg

There were a couple of test failures I need to investigate.

Comment 8

[Josh Aas]

This test appears to exercise the code in question. My patch causes the test to crash.

./mach mochitest-chrome layout/generic/test/test_backspace_delete.xul

Comment 9

[Josh Aas]

Attachment: Fix v1.2

Comment 10

[Josh Aas]

Comment on attachment 828725 [details] [diff] [review]
Fix v1.2

This fixes the crash in the test by adding back a missing memset. Also gets rid of the ultimately-unused locale variable.

Comment 11

[Josh Aas]

Updated build for testers:

http://people.mozilla.org/~josh/firefox-923967-3.dmg

Comment 12

[Josh Aas]

All test failures resolved with this revision.

https://tbpl.mozilla.org/?tree=Try&rev=9bce784023e7

Comment 13

[Josh Aas]

Attachment: Fix v1.3

Comment 14

[Josh Aas]

Comment on attachment 829055 [details] [diff] [review]
Fix v1.3

I ran the test that had previously failed and recorded the results every time the function in question ran, both with and without my patch. I discovered that the results with my patch differed only in that they always included the leading edge (index 0) as a break. Skipping the leading edge makes my patched version match the un-patched version output perfectly.

I still can't claim to know exactly what this does (though I have some idea). However, I think between the passing tests and the output comparison we have a good case to go ahead and land this. That way it'll get tested early in this release cycle and we can back out if people see problems.

Comment 15

[Josh Aas]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ea78c491bc18

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea78c491bc18

Comment 17

[Cameron Kaiser [:spectre]]

Josh, I've asked politely before: can you cc me on bugs that remove older Mac code? I don't care about the removal, but it's nice to know; we're still maintaining a Tier-3 port out there. Is there some reason you'd prefer not to?

Comment 18

[Josh Aas]

(In reply to Cameron Kaiser [:spectre] from comment #17)
> Josh, I've asked politely before: can you cc me on bugs that remove older
> Mac code? I don't care about the removal, but it's nice to know; we're still
> maintaining a Tier-3 port out there. Is there some reason you'd prefer not
> to?

I just don't remember to remind you because I don't do this stuff often and there is no system in place for me to be reminded to tell you. I am happy to help you when I can remember but, unfortunately, dealing with this sort of thing is inherent in the task you have taken up.

Comment 19

[Cameron Kaiser [:spectre]]

I fully get that, and I'm really only interested in big switches like this that drop deprecated APIs entirely. Would that be a reasonable request? It's just handy to know where the bug was in case it's either 1) merely a matter of backing it out or 2) backing it out *and* conforming to a new method, like in the ColorSync Manager changes where I had to hack qcms a little.

I appreciate it, and thanks.