Bug 147605 - [INTL] Implement Number Format Functions
Summary: [INTL] Implement Number Format Functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 154160 154530 154533
Blocks: 90906 152033
  Show dependency treegraph
 
Reported: 2015-08-03 17:21 PDT by Andy VanWagoner
Modified: 2016-02-22 05:08 PST (History)
12 users (show)

See Also:


Attachments
WIP (8.56 KB, patch)
2016-02-02 17:46 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (26.59 KB, patch)
2016-02-12 18:25 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (766.76 KB, application/zip)
2016-02-12 19:12 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (809.49 KB, application/zip)
2016-02-12 19:16 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (830.68 KB, application/zip)
2016-02-12 19:19 PST, Build Bot
no flags Details
Patch (27.07 KB, patch)
2016-02-12 19:23 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (26.83 KB, patch)
2016-02-14 16:19 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (26.99 KB, patch)
2016-02-14 16:22 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (27.44 KB, patch)
2016-02-15 16:41 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch for landing (27.38 KB, patch)
2016-02-19 17:05 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch for landing (27.37 KB, patch)
2016-02-19 17:10 PST, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy VanWagoner 2015-08-03 17:21:58 PDT
Implement ECMA-402 2.0 11.3.4 Number Format Functions
http://ecma-international.org/publications/standards/Ecma-402.htm

This makes the function returned by Intl.NumberFormat.prototype.format locale-aware and sensitive to the options passed to the constructor.
Comment 1 Sukolsak Sakshuwong 2016-02-02 17:46:21 PST
Created attachment 270536 [details]
WIP
Comment 2 Sukolsak Sakshuwong 2016-02-12 18:25:50 PST
Created attachment 271257 [details]
Patch
Comment 3 Build Bot 2016-02-12 19:12:22 PST
Comment on attachment 271257 [details]
Patch

Attachment 271257 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/822892

New failing tests:
js/intl-numberformat.html
Comment 4 Build Bot 2016-02-12 19:12:24 PST
Created attachment 271260 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-02-12 19:16:35 PST
Comment on attachment 271257 [details]
Patch

Attachment 271257 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/822904

New failing tests:
js/intl-numberformat.html
Comment 6 Build Bot 2016-02-12 19:16:38 PST
Created attachment 271261 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-02-12 19:19:56 PST
Comment on attachment 271257 [details]
Patch

Attachment 271257 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/822888

New failing tests:
js/intl-numberformat.html
Comment 8 Build Bot 2016-02-12 19:19:59 PST
Created attachment 271262 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Sukolsak Sakshuwong 2016-02-12 19:23:02 PST
Created attachment 271263 [details]
Patch
Comment 10 Darin Adler 2016-02-14 14:57:27 PST
Comment on attachment 271263 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271263&action=review

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:70
> +IntlNumberFormat::~IntlNumberFormat()
> +{
> +    if (m_numberFormat)
> +        unum_close(m_numberFormat);
> +}

I would have slightly preferred that we use std::unique_ptr instead.

    struct UNumberFormatDeleter {
        void operator()(UNumberFormat* numberFormat) const
        {
            if (numberFormat)
                unum_close(numberFormat);
        }
    };

    std::unique_ptr<UNumberFormat, UNumberFormatDeleter> m_numberFormat;

If we did that, we would not need to define ~IntlNumberFormat() and other code would be a bit simpler as well.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:401
> +    UNumberFormat* numberFormat = unum_open(style, nullptr, 0, m_locale.utf8().data(), nullptr, &status);

If we used std::unique_ptr this would be:

    auto numberFormat = std::unique_ptr<UNumberFormat, UNumberFormatDeleter>(unum_open(style, nullptr, 0, m_locale.utf8().data(), nullptr, &status));

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:406
> +        unum_setTextAttribute(numberFormat, UNUM_CURRENCY_CODE, StringView(m_currency).upconvertedCharacters(), 3, &status);

If we used std::unique_ptr, these lines would need to use get().

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:419
> +        unum_close(numberFormat);

We would not need this if we used std::unique_ptr.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:423
> +    m_numberFormat = numberFormat;

This line would need to be:

    m_numberFormat = WTFMove(numberFormat);

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:436
> +    if (bitwise_cast<uint64_t>(number) == bitwise_cast<uint64_t>(-0.0))
> +        number = 0.0;

How is the bitwise_cast code better than:

    // Map negative zero to positive zero.
    if (!number)
        number = 0;

I don’t think it is.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:447
> +        return state.vm().throwException(&state, createError(&state, ASCIILiteral("Failed to format a number.")));

Where is the test coverage for this? When can this fail?
Comment 11 Sukolsak Sakshuwong 2016-02-14 16:19:12 PST
Created attachment 271314 [details]
Patch
Comment 12 Sukolsak Sakshuwong 2016-02-14 16:22:57 PST
Created attachment 271315 [details]
Patch
Comment 13 Sukolsak Sakshuwong 2016-02-14 16:35:48 PST
(In reply to comment #10)
> Comment on attachment 271263 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271263&action=review
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:70
> > +IntlNumberFormat::~IntlNumberFormat()
> > +{
> > +    if (m_numberFormat)
> > +        unum_close(m_numberFormat);
> > +}
> 
> I would have slightly preferred that we use std::unique_ptr instead.
> 
>     struct UNumberFormatDeleter {
>         void operator()(UNumberFormat* numberFormat) const
>         {
>             if (numberFormat)
>                 unum_close(numberFormat);
>         }
>     };
> 
>     std::unique_ptr<UNumberFormat, UNumberFormatDeleter> m_numberFormat;
> 
> If we did that, we would not need to define ~IntlNumberFormat() and other
> code would be a bit simpler as well.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:401
> > +    UNumberFormat* numberFormat = unum_open(style, nullptr, 0, m_locale.utf8().data(), nullptr, &status);
> 
> If we used std::unique_ptr this would be:
> 
>     auto numberFormat = std::unique_ptr<UNumberFormat,
> UNumberFormatDeleter>(unum_open(style, nullptr, 0, m_locale.utf8().data(),
> nullptr, &status));
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:406
> > +        unum_setTextAttribute(numberFormat, UNUM_CURRENCY_CODE, StringView(m_currency).upconvertedCharacters(), 3, &status);
> 
> If we used std::unique_ptr, these lines would need to use get().
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:419
> > +        unum_close(numberFormat);
> 
> We would not need this if we used std::unique_ptr.
> 
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:423
> > +    m_numberFormat = numberFormat;
> 
> This line would need to be:
> 
>     m_numberFormat = WTFMove(numberFormat);

Done. Thanks. I will update other Intl classes to std::unique_ptr in a separate patch.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:436
> > +    if (bitwise_cast<uint64_t>(number) == bitwise_cast<uint64_t>(-0.0))
> > +        number = 0.0;
> 
> How is the bitwise_cast code better than:
> 
>     // Map negative zero to positive zero.
>     if (!number)
>         number = 0;
> 
> I don’t think it is.

Fixed.

> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:447
> > +        return state.vm().throwException(&state, createError(&state, ASCIILiteral("Failed to format a number.")));
> 
> Where is the test coverage for this? When can this fail?

The ICU spec doesn't say what can cause unum_formatUFormattable to fail, and I can't find a case that makes it fail. I put the check in there just in case.
Comment 14 Sukolsak Sakshuwong 2016-02-15 16:41:53 PST
Created attachment 271390 [details]
Patch
Comment 15 Michael Catanzaro 2016-02-19 16:30:41 PST
Since Darin gave this r+, if you're just responding to his comments it's OK to add his name to the Reviewed by line and commit, unless you're looking for more feedback.

If you're not a committer, you could reupload the patch and request only cq?

Or: you could wait for Darin to see this comment and click the button again. :)
Comment 16 Sukolsak Sakshuwong 2016-02-19 17:05:56 PST
Created attachment 271828 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2016-02-19 17:07:52 PST
Comment on attachment 271828 [details]
Patch for landing

Rejecting attachment 271828 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 271828, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/856604
Comment 18 Sukolsak Sakshuwong 2016-02-19 17:10:52 PST
Created attachment 271830 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2016-02-19 17:58:04 PST
Comment on attachment 271830 [details]
Patch for landing

Clearing flags on attachment: 271830

Committed r196850: <http://trac.webkit.org/changeset/196850>
Comment 20 WebKit Commit Bot 2016-02-19 17:58:09 PST
All reviewed patches have been landed.  Closing bug.