|
Description
Robin Whittleton
2018-05-07 06:53:16 PDT
Created attachment 340092 [details]
Patch
Attachment 340092 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 340094 [details]
Patch
Comment on attachment 340094 [details] Patch Attachment 340094 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7639651 New failing tests: js/intl-numberformat.html Created attachment 340101 [details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 340094 [details] Patch Attachment 340094 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7639661 New failing tests: js/intl-numberformat.html Created attachment 340102 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 340094 [details] Patch Attachment 340094 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7639669 New failing tests: jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout jsc-layout-tests.yaml/js/script-tests/intl-numberformat.js.layout-ftl-no-cjit apiTests Comment on attachment 340094 [details] Patch Attachment 340094 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7639795 New failing tests: js/intl-numberformat.html Created attachment 340108 [details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 340094 [details] Patch Attachment 340094 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7639721 New failing tests: js/intl-numberformat.html Created attachment 340110 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 340145 [details]
Separate tests for formatToParts, disabled on macOS
Comment on attachment 340145 [details]
Separate tests for formatToParts, disabled on macOS
Since this feature relies on ICU v59, expect it to fail in macOS for now.
Comment on attachment 340145 [details] Separate tests for formatToParts, disabled on macOS Attachment 340145 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7645109 New failing tests: js/intl-numberformat-format-to-parts.html Created attachment 340155 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 340145 [details] Separate tests for formatToParts, disabled on macOS Attachment 340145 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7645521 New failing tests: jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/intl-numberformat-format-to-parts.js.layout-ftl-no-cjit apiTests Comment on attachment 340145 [details] Separate tests for formatToParts, disabled on macOS Attachment 340145 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7645496 New failing tests: js/intl-numberformat-format-to-parts.html Created attachment 340158 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 340166 [details]
Patch
Comment on attachment 340166 [details] Patch Attachment 340166 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7648248 New failing tests: js/intl-numberformat-format-to-parts.html Created attachment 340176 [details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 340166 [details] Patch Attachment 340166 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7649039 New failing tests: js/intl-numberformat-format-to-parts.html Created attachment 340183 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 340191 [details]
Set test expectations properly
Comment on attachment 340191 [details] Set test expectations properly View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628 > + for (const auto &candidate : fields) { > + if (candidate.beginIndex <= currentIndex && currentIndex < candidate.endIndex && (!field.size() || candidate.size() < field.size())) > + field = candidate; > + if (currentIndex < candidate.beginIndex && candidate.beginIndex < nextStartIndex) > + nextStartIndex = candidate.beginIndex; > + } I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct? > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635 > + parts->push(&exec, part); Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]). Use putDirectIndex instead. > Source/JavaScriptCore/runtime/IntlNumberFormat.h:34 > +#define JSC_ICU_HAS_FORMAT_DOUBLE_FOR_FIELDS (U_ICU_VERSION_MAJOR_NUM >= 59) You can make this `#define HAVE_ICU_FORMAT_DOUBLE_FOR_FIELDS` and use it as `#if HAVE(ICU_FORMAT_DOUBLE_FOR_FIELDS)`. > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:86 > + JSFunction* formatToPartsFunction = JSFunction::create(vm, globalObject, 1, vm.propertyNames->formatToParts.string(), IntlNumberFormatPrototypeFuncFormatToParts); > + putDirectWithoutTransition(vm, vm.propertyNames->formatToParts, formatToPartsFunction, static_cast<unsigned>(PropertyAttribute::DontEnum)); Use JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION instead. > Source/cmake/WebKitFeatures.cmake:128 > + WEBKIT_OPTION_DEFINE(ENABLE_INTL_NUMBER_FORMAT_TO_PARTS "Toggle Intl.NumberFormat.prototype.formatToParts support" PRIVATE OFF) Add this option to OptionsJSCOnly.cmake and enable it for JSCOnly. (See ENABLE_INTL_PLURAL_RULES in OptionsJSCOnly.cmake). Comment on attachment 340191 [details] Set test expectations properly View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:631 > + auto value = jsString(&exec, resultString.substring(currentIndex, nextIndex - currentIndex)); Use `jsSubstring(VM*, const String&, unsigned offset, unsigned length)`. (In reply to Yusuke Suzuki from comment #26) > Comment on attachment 340191 [details] > Set test expectations properly > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340191&action=review > > > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628 > > + for (const auto &candidate : fields) { > > + if (candidate.beginIndex <= currentIndex && currentIndex < candidate.endIndex && (!field.size() || candidate.size() < field.size())) > > + field = candidate; > > + if (currentIndex < candidate.beginIndex && candidate.beginIndex < nextStartIndex) > > + nextStartIndex = candidate.beginIndex; > > + } > > I think if we sort this fields before iterating, we can iteratively accesses > fields. In that case, time is O(NlogN), correct? The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator. I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields. > > > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635 > > + parts->push(&exec, part); > > Do not use `push()` since it is used for ArrayPush and it is observable to > users (it uses [[Put]]). > Use putDirectIndex instead. Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ? > > > Source/JavaScriptCore/runtime/IntlNumberFormat.h:34 > > +#define JSC_ICU_HAS_FORMAT_DOUBLE_FOR_FIELDS (U_ICU_VERSION_MAJOR_NUM >= 59) > > You can make this `#define HAVE_ICU_FORMAT_DOUBLE_FOR_FIELDS` and use it as > `#if HAVE(ICU_FORMAT_DOUBLE_FOR_FIELDS)`. That looks much nicer. Will do. > > > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:86 > > + JSFunction* formatToPartsFunction = JSFunction::create(vm, globalObject, 1, vm.propertyNames->formatToParts.string(), IntlNumberFormatPrototypeFuncFormatToParts); > > + putDirectWithoutTransition(vm, vm.propertyNames->formatToParts, formatToPartsFunction, static_cast<unsigned>(PropertyAttribute::DontEnum)); > > Use JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION instead. Will do. > > > Source/cmake/WebKitFeatures.cmake:128 > > + WEBKIT_OPTION_DEFINE(ENABLE_INTL_NUMBER_FORMAT_TO_PARTS "Toggle Intl.NumberFormat.prototype.formatToParts support" PRIVATE OFF) > > Add this option to OptionsJSCOnly.cmake and enable it for JSCOnly. (See > ENABLE_INTL_PLURAL_RULES in OptionsJSCOnly.cmake). Will do. Created attachment 340194 [details]
Patch
Comment on attachment 340191 [details] Set test expectations properly View in context: https://bugs.webkit.org/attachment.cgi?id=340191&action=review >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628 >>> + } >> >> I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct? > > The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator. > > I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields. Even if they are nested, you can sort an expected order by using beginIndex, and endIndex. [ A ] [ B ] [ C ] In this case, we can sort [A, B, C] if we use sort fields with beginIndex and then compare endIndex if beginIndex is the same. Could you add FIXME, bug URL, and note about O(N^2) complexity? >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635 >>> + parts->push(&exec, part); >> >> Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]). >> Use putDirectIndex instead. > > Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ? Holding an index, and call `parts->putDirectIndex(&exec, index++, part);`. (In reply to Yusuke Suzuki from comment #30) > Comment on attachment 340191 [details] > Set test expectations properly > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340191&action=review > > >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:628 > >>> + } > >> > >> I think if we sort this fields before iterating, we can iteratively accesses fields. In that case, time is O(NlogN), correct? > > > > The fields are nested, so order doesn't trivialize this loop anyway. There is one integer field spanning all of the group separators and digits up to the decimal separator. > > > > I could do a sort first which is O(NlogN) itself, but given the bounds of N (Number.MAX_NUMBER has ~100), I'm not sure it's worth the additional code complexity. The time here is dominated by unum_formatDoubleForFields. > > Even if they are nested, you can sort an expected order by using beginIndex, > and endIndex. > > [ A ] > [ B ] > [ C ] > > In this case, we can sort [A, B, C] if we use sort fields with beginIndex > and then compare endIndex if beginIndex is the same. > Could you add FIXME, bug URL, and note about O(N^2) complexity? I spent some time trying to rework it to an O(N log N) algorithm, first sorting the fields, and then iterating through them, but I was having a rough time with it, so I'll add the FIXME and revisit another time: https://bugs.webkit.org/show_bug.cgi?id=185557 > > >>> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:635 > >>> + parts->push(&exec, part); > >> > >> Do not use `push()` since it is used for ArrayPush and it is observable to users (it uses [[Put]]). > >> Use putDirectIndex instead. > > > > Will do. `parts->putDirectIndex(&exec, parts->length(), part);` ? > > Holding an index, and call `parts->putDirectIndex(&exec, index++, part);`. k, I'll fix that. Created attachment 340216 [details]
Patch
Created attachment 340328 [details]
rebased
Comment on attachment 340328 [details]
rebased
Fixed conflict and removed accidental change to license header.
Comment on attachment 340328 [details] rebased Attachment 340328 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7679256 New failing tests: css3/filters/backdrop/add-remove-add-backdrop-filter.html Created attachment 340337 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 340418 [details]
Patch
Are there any other changes I should make, or is does this look ready to commit? Comment on attachment 340418 [details]
Patch
r=me
Comment on attachment 340418 [details] Patch Rejecting attachment 340418 [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-02', 'apply-attachment', '--no-update', '--non-interactive', 340418, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: LayoutTests/platform/ios-simulator/TestExpectations patching file LayoutTests/platform/mac/TestExpectations Hunk #1 FAILED at 1762. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/TestExpectations.rej patching file LayoutTests/platform/win/TestExpectations patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Yusuke Suzuki']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/7700846 (In reply to WebKit Commit Bot from comment #40) > Comment on attachment 340418 [details] > Patch > > Rejecting attachment 340418 [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-02', > 'apply-attachment', '--no-update', '--non-interactive', 340418, > '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit > > Last 500 characters of output: > LayoutTests/platform/ios-simulator/TestExpectations > patching file LayoutTests/platform/mac/TestExpectations > Hunk #1 FAILED at 1762. > 1 out of 1 hunk FAILED -- saving rejects to file > LayoutTests/platform/mac/TestExpectations.rej > patching file LayoutTests/platform/win/TestExpectations > patching file ChangeLog > Hunk #1 succeeded at 1 with fuzz 3. > > Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', > '--force', '--reviewer', u'Yusuke Suzuki']" exit_code: 1 cwd: > /Volumes/Data/EWS/WebKit > > Full output: http://webkit-queues.webkit.org/results/7700846 Could you rebase this? Created attachment 340516 [details]
rebased
Comment on attachment 340516 [details] rebased Clearing flags on attachment: 340516 Committed r231867: <https://trac.webkit.org/changeset/231867> All reviewed patches have been landed. Closing bug. Thanks Andy for all the hard work, that was much quicker than I was expecting :) (In reply to Robin Whittleton from comment #46) > Thanks Andy for all the hard work, that was much quicker than I was > expecting :) Happy to help. It's still not enabled on macOS, so it could still take a long while before it ends up in a Safari release. This bug has been marked as RESOLVED FIXED for months, now, and the fix is already available in Safari Technology Preview. I was really hoping to see it included in Safari with macOS 10.14.4 and iOS 12.2, but it wasn't. Is there any ETA for this? What is holding this back? |