Bug 42484

It would also be nice to display numbers in their localized form. Same probably applies to date/time fields.

Created attachment 80305 [details] Patch

I'm not sure how to use new ICU headers in Mac. Why we have Source/WebCore/icu/?

Created attachment 80307 [details] Patch 2

> We should accept localized number representations and convert them to an HTML5 number for form submission. Is that mandated by HTML5? The bug needs to explain where this "should" comes from. <http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values> in particular is not localization aware. > I'm not sure how to use new ICU headers in Mac. Why we have Source/WebCore/icu/? ICU is not an officially supported API on Mac OS X, so ICU headers aren't shipped with it. When really needed, we copy an appropriate version of a particular ICU header to our repository. I'm not sure if this is one of those cases, or we should just use CFNumberFormatter API.

This is a UI issue and is out of scope of the spec. You "should" accept localised forms if that's what's best for users.

(In reply to comment #5) > > We should accept localized number representations and convert them to an HTML5 number for form submission. > > Is that mandated by HTML5? The bug needs to explain where this "should" comes from. <http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values> in particular is not localization aware. It's not mandatory. I should have added more words to ChangeLog. This change is only for UI. HTML parsing and DOM property access are not affected. I saw several requests for localized numbers for <input type=number>, I haven't seen any objections, and I believe we should support them. > > I'm not sure how to use new ICU headers in Mac. Why we have Source/WebCore/icu/? > > ICU is not an officially supported API on Mac OS X, so ICU headers aren't shipped with it. When really needed, we copy an appropriate version of a particular ICU header to our repository. I'm not sure if this is one of those cases, or we should just use CFNumberFormatter API. CFNumberFormatter looks enough. Thank you.

> This change is only for UI. Will is affect VoiceOver?

(In reply to comment #8) > Will is affect VoiceOver? I don't know. How VoiceOver obtain text form an application?

(In reply to comment #9) > (In reply to comment #8) > > Will it affect VoiceOver? > > I don't know. How does VoiceOver obtain text form an application? It calls the functions defined in the WebCore/accessibility directory.

(In reply to comment #10) > > > Will it affect VoiceOver? > > > > I don't know. How does VoiceOver obtain text form an application? > > It calls the functions defined in the WebCore/accessibility directory. Thanks. I took a look at WebCore/accessibility/, and I couldn't get what would happen in this case. ok, I'll implement LocalizedNumberCF and try VoiceOver. I think VoiceOver should get localized text.

Created attachment 80733 [details] Patch 3 Add LocalizedNumberMac.mm and a test

(In reply to comment #11) > I think VoiceOver should get localized text. I confirmed that VoiceOver got a localized one with the patch.

Created attachment 80755 [details] Patch 4 Fix parsing error handling in LocalizedNumberMac.mm

Created attachment 80884 [details] Patch 5

Created attachment 84179 [details] Patch 6 Rebased

Comment on attachment 84179 [details] Patch 6 View in context: https://bugs.webkit.org/attachment.cgi?id=84179&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:5665 > + F5CC42DC12F801CA00D5F7E3 /* LocalizedNumberMac.mm in Sources */ = {isa = PBXBuildFile; fileRef = F5CC42DB12F801CA00D5F7E3 /* LocalizedNumberMac.mm */; }; Did you run sort-Xcode-project-file after your changes? This file looks out of order. > Source/WebCore/platform/text/mac/LocalizedNumberMac.mm:58 > + NSNumberFormatter* formatter = [[[NSNumberFormatter alloc] init] autorelease]; I don't know how this stuff works. Can you check with ObJC-heads before landing?

> Source/WebCore/platform/text/mac/LocalizedNumberMac.mm:58 > + NSNumberFormatter* formatter = [[[NSNumberFormatter alloc] init] autorelease]; Although autorelease works, we prefer explicit refcounting in WebCore. So, this should be: RetainPtr<NSNumberFormatter> formatter(AdoptNS, [[NSNumberFormatter alloc] init]); + NSNumber* num = [formatter numberFromString:numberString]; Please don't abbreviate. + if (num == nil) WebKit style is to not compare to zero, so this should just be "if (!num)" (with a better name for num). + NSNumber* num = [NSNumber numberWithDouble:number]; WebKit style is to put stars on the other side for Objective-C types. But it would be better to use alloc/init and RetainPtr here, too. The reasons I know of are: 1) It's faster to destroy temporaries right away, while they are in memory cache. 2) The autorelease pool won't be drained during micro-benchmarks that spin without returning to message loop, causing high memory usage.

Dimitri, Alexey, thank you for reviewing. I'll update the code and commit it.

> + NSNumber* num = [formatter numberFromString:numberString]; > + if (num == nil) Should we use RetainPtr<> here too?

(In reply to comment #20) > > + NSNumber* num = [formatter numberFromString:numberString]; > > + if (num == nil) > > Should we use RetainPtr<> here too? Probably, no. This object is already registered to an autorelease pool by NSNumberFormatter.

Landed: http://trac.webkit.org/changeset/80096

I think that it's more of a coding style issue in this case. If we were optimizing this function, we'd likely have to cache NSNumberFormatter instance at least.

(In reply to comment #23) > I think that it's more of a coding style issue in this case. If we were optimizing this function, we'd likely have to cache NSNumberFormatter instance at least. Indeed. These functions should be called only the main thread. It's easy to cache the number formatter.