OS X: move from deprecated Unicode Utilities usage to CFStringTokenizer

RESOLVED FIXED in mozilla28

Status

()

Core
Internationalization
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
mozilla28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → joshmoz
(Assignee)

Comment 1

4 years ago
Created attachment 813973 [details] [diff] [review]
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.
Attachment #813973 - Flags: review?(smontagu)

Comment 2

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

Comment 3

4 years ago
Please provide detailed testing instructions on Mac OS.

Comment 4

4 years ago
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 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.
(Assignee)

Comment 6

4 years ago
Created attachment 828365 [details] [diff] [review]
Fix v1.1

Updated to current trunk.
Attachment #813973 - Attachment is obsolete: true
Attachment #813973 - Flags: review?(smontagu)
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
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
(Assignee)

Comment 9

4 years ago
Created attachment 828725 [details] [diff] [review]
Fix v1.2
(Assignee)

Comment 10

4 years ago
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.
Attachment #828725 - Attachment description: maclinebreak3.patch → Fix v1.2
(Assignee)

Comment 11

4 years ago
Updated build for testers:

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

Updated

4 years ago
Attachment #828365 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
All test failures resolved with this revision.

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

Comment 13

4 years ago
Created attachment 829055 [details] [diff] [review]
Fix v1.3
(Assignee)

Comment 14

4 years ago
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.
Attachment #829055 - Attachment description: maclinebreak4.patch → Fix v1.3
Attachment #829055 - Flags: review?(smontagu)
(Assignee)

Updated

4 years ago
Attachment #828725 - Attachment is obsolete: true
Attachment #829055 - Flags: review?(smontagu) → review+
https://hg.mozilla.org/mozilla-central/rev/ea78c491bc18
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
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?
(Assignee)

Comment 18

4 years ago
(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.
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.
You need to log in before you can comment on or make changes to this bug.