deps: backport fix to v8 bug in toUpper('ç') #9828

srl295 · 2016-11-28T19:29:09Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

deps - v8 floating patch

Description of change

add test for ç/Ç in various locales
work around v8 bug per @jungshik
https://bugs.chromium.org/p/v8/issues/detail?id=5681

Fixes: #9785

MylesBorins · 2016-11-28T19:36:23Z

@srl295 AFAIK we will not land floating patches on V8 that have not landed upstream.

once this has landed upstream it can be backported using this guide

/cc @nodejs/v8 @ofrobots

jungshik · 2016-11-28T19:40:18Z

test/parallel/test-intl.js

+// Always run these. They should always pass, even if the locale
+// param is ignored.
+assert.strictEqual('Ç'.toLocaleLowerCase('el'), 'ç');
+assert.strictEqual('Ç'.toLocaleLowerCase('tr'), 'ç');


You may want to try 'lt' as well.
Otherwise, LGTM

srl295 · 2016-11-28T20:06:38Z

@thealphanerd thanks. will wait for upstream fix. Version wise, this may need a backport . @ofrobots thanks, fixed.

ofrobots · 2016-11-28T20:06:57Z

@srl295 why is this labelled with semver-minor. Seems like a bug fix to me.

jungshik · 2016-11-28T20:42:27Z

https://codereview.chromium.org/2533033003/ is a v8 CL to fix this issue.

srl295 · 2016-11-29T01:56:19Z

… and landed in v8 as https://chromium.googlesource.com/v8/v8/+/2f5da9a551785899ae1c899fd3c31b834a731e7d == v8/v8@2f5da9a

ofrobots · 2016-12-14T16:03:01Z

This should be okay to review for landing onto master and v7.x – V8 5.4. The the fix has landed upstream and upstream is no longer maintaining V8 5.4. The fix should be formatted as a cherry-pick as per this guide however: https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md

I have requested a upstream merge for 5.5 and 5.6.

ofrobots

The fix should be formatted as a cherry-pick as per this guide: https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md

MylesBorins · 2016-12-29T18:11:29Z

@srl295 are you able to update this following the guide mentioned above? It will need to include the original commit information, and also bump the V8 patch number.

I'm more than happy to take it over if you like.

srl295 · 2016-12-30T00:42:51Z

@thealphanerd Let me see if I can do it…

srl295 · 2016-12-30T01:08:11Z

@ofrobots and @thealphanerd (and anyone) PTAL, updated as per guide mentioned above.

gibfahn · 2016-12-30T11:10:30Z

@srl295 I think you need to bump the patch number in v8-version.h (as in #10525).

srl295 · 2016-12-30T21:51:08Z

@thealphanerd OK thanks. DO you want to land it in master also? If so please go ahead.

MylesBorins · 2016-12-30T22:37:28Z

once @ofrobots approves anyone can it 😁

srl295 · 2016-12-30T23:04:14Z

@thealphanerd ok… also fixed the title, as it's no longer a workaround.

ofrobots

Thanks!

jasnell · 2017-01-06T18:20:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5733/

MylesBorins · 2017-01-06T20:39:38Z

CI passed despite reporting that arm failed.

Landed in 6d3c5b7...e9b7291

Original commit message: Fix the uppercasing of U+00E7(ç) and U+00F7(÷) Due to a typo in runtime-i18n.js, 'ç'(U+00E7) was not uppercased while '÷'(U+00F7) was incorrectly uppercased to '×'(U+00D7). Add a comprehensive test for Latin-1 supplemental block (U+00A0 ~ U+00FF). (they're special-cased for speed-up and needs to have a test for the range.). TEST=intl/general/case-mapping BUG=v8:5681 Review-Url: https://codereview.chromium.org/2533033003 Cr-Commit-Position: refs/heads/master@{#41331} PR-URL: #9828 Fixes: #9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

* add test for ç/Ç in various locales PR-URL: #9828 Fixes: #9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins · 2017-01-06T21:13:17Z

This does not appear to affect v4 or v6 and as such I have added the dont-land labels. Feel free to modify if I am incorrect

ofrobots · 2017-01-11T20:16:49Z

Merged upstream into V8 5.5 and 5.6.

* add test for ç/Ç in various locales PR-URL: nodejs#9828 Fixes: nodejs#9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Original commit message: Fix the uppercasing of U+00E7(ç) and U+00F7(÷) Due to a typo in runtime-i18n.js, 'ç'(U+00E7) was not uppercased while '÷'(U+00F7) was incorrectly uppercased to '×'(U+00D7). Add a comprehensive test for Latin-1 supplemental block (U+00A0 ~ U+00FF). (they're special-cased for speed-up and needs to have a test for the range.). TEST=intl/general/case-mapping BUG=v8:5681 Review-Url: https://codereview.chromium.org/2533033003 Cr-Commit-Position: refs/heads/master@{#41331} PR-URL: #9828 Fixes: #9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

* add test for ç/Ç in various locales PR-URL: #9828 Fixes: #9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Original commit message: Fix the uppercasing of U+00E7(ç) and U+00F7(÷) Due to a typo in runtime-i18n.js, 'ç'(U+00E7) was not uppercased while '÷'(U+00F7) was incorrectly uppercased to '×'(U+00D7). Add a comprehensive test for Latin-1 supplemental block (U+00A0 ~ U+00FF). (they're special-cased for speed-up and needs to have a test for the range.). TEST=intl/general/case-mapping BUG=v8:5681 Review-Url: https://codereview.chromium.org/2533033003 Cr-Commit-Position: refs/heads/master@{nodejs#41331} PR-URL: nodejs#9828 Fixes: nodejs#9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

* add test for ç/Ç in various locales PR-URL: nodejs#9828 Fixes: nodejs#9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Original commit message: Fix the uppercasing of U+00E7(ç) and U+00F7(÷) Due to a typo in runtime-i18n.js, 'ç'(U+00E7) was not uppercased while '÷'(U+00F7) was incorrectly uppercased to '×'(U+00D7). Add a comprehensive test for Latin-1 supplemental block (U+00A0 ~ U+00FF). (they're special-cased for speed-up and needs to have a test for the range.). TEST=intl/general/case-mapping BUG=v8:5681 Review-Url: https://codereview.chromium.org/2533033003 Cr-Commit-Position: refs/heads/master@{nodejs#41331} PR-URL: nodejs#9828 Fixes: nodejs#9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

* add test for ç/Ç in various locales PR-URL: nodejs#9828 Fixes: nodejs#9785 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Nov 28, 2016

srl295 force-pushed the fix-ç branch 2 times, most recently from 54298b5 to b1a4f0d Compare November 28, 2016 19:31

srl295 self-assigned this Nov 28, 2016

srl295 added i18n-api Issues and PRs related to the i18n implementation. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 28, 2016

jungshik approved these changes Nov 28, 2016

View reviewed changes

srl295 added confirmed-bug Issues with confirmed bugs. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 28, 2016

jasnell approved these changes Dec 23, 2016

View reviewed changes

ofrobots suggested changes Dec 23, 2016

View reviewed changes

srl295 force-pushed the fix-ç branch from b1a4f0d to 42df557 Compare December 30, 2016 00:47

srl295 force-pushed the fix-ç branch from 42df557 to a783344 Compare December 30, 2016 01:06

srl295 force-pushed the fix-ç branch from a783344 to f3d373d Compare December 30, 2016 01:07

MylesBorins self-assigned this Dec 30, 2016

srl295 changed the title ~~deps: work around v8 bug in toUpper('ç')~~ deps: backport fix to v8 bug in toUpper('ç') Dec 30, 2016

ofrobots approved these changes Jan 3, 2017

View reviewed changes

jasnell approved these changes Jan 3, 2017

View reviewed changes

MylesBorins closed this Jan 6, 2017

MylesBorins added dont-land-on-v4.x labels Jan 6, 2017

targos mentioned this pull request Jan 12, 2017

String.prototype.toLocaleUpperCase bug in tr_TR.UTF-8 Locale #10765

Closed

italoacasas mentioned this pull request Jan 19, 2017

v7.x-staging update proposal #10886

Closed

srl295 deleted the fix-ç branch January 19, 2017 01:38

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

targos removed the confirmed-bug Issues with confirmed bugs. label Jan 28, 2017

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: backport fix to v8 bug in toUpper('ç') #9828

deps: backport fix to v8 bug in toUpper('ç') #9828

srl295 commented Nov 28, 2016 •

edited

Loading

MylesBorins commented Nov 28, 2016

jungshik Nov 28, 2016

srl295 commented Nov 28, 2016 •

edited

Loading

ofrobots commented Nov 28, 2016

jungshik commented Nov 28, 2016

srl295 commented Nov 29, 2016 •

edited

Loading

ofrobots commented Dec 14, 2016

ofrobots left a comment

MylesBorins commented Dec 29, 2016

srl295 commented Dec 30, 2016

srl295 commented Dec 30, 2016

gibfahn commented Dec 30, 2016

srl295 commented Dec 30, 2016 •

edited

Loading

MylesBorins commented Dec 30, 2016

srl295 commented Dec 30, 2016

ofrobots left a comment

jasnell commented Jan 6, 2017

MylesBorins commented Jan 6, 2017

MylesBorins commented Jan 6, 2017

ofrobots commented Jan 11, 2017 •

edited

Loading

deps: backport fix to v8 bug in toUpper('ç') #9828

deps: backport fix to v8 bug in toUpper('ç') #9828

Conversation

srl295 commented Nov 28, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

MylesBorins commented Nov 28, 2016

jungshik Nov 28, 2016

Choose a reason for hiding this comment

srl295 commented Nov 28, 2016 • edited Loading

ofrobots commented Nov 28, 2016

jungshik commented Nov 28, 2016

srl295 commented Nov 29, 2016 • edited Loading

ofrobots commented Dec 14, 2016

ofrobots left a comment

Choose a reason for hiding this comment

MylesBorins commented Dec 29, 2016

srl295 commented Dec 30, 2016

srl295 commented Dec 30, 2016

gibfahn commented Dec 30, 2016

srl295 commented Dec 30, 2016 • edited Loading

MylesBorins commented Dec 30, 2016

srl295 commented Dec 30, 2016

ofrobots left a comment

Choose a reason for hiding this comment

jasnell commented Jan 6, 2017

MylesBorins commented Jan 6, 2017

MylesBorins commented Jan 6, 2017

ofrobots commented Jan 11, 2017 • edited Loading

srl295 commented Nov 28, 2016 •

edited

Loading

srl295 commented Nov 28, 2016 •

edited

Loading

srl295 commented Nov 29, 2016 •

edited

Loading

srl295 commented Dec 30, 2016 •

edited

Loading

ofrobots commented Jan 11, 2017 •

edited

Loading