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

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: #9828Fixes: #9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

* add test for ç/Ç in various locales PR-URL: #9828Fixes: #9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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#9828Fixes: nodejs#9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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: #9828Fixes: #9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

* add test for ç/Ç in various locales PR-URL: #9828Fixes: #9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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#9828Fixes: nodejs#9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

* add test for ç/Ç in various locales PR-URL: nodejs#9828Fixes: nodejs#9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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#9828Fixes: nodejs#9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

* add test for ç/Ç in various locales PR-URL: nodejs#9828Fixes: nodejs#9785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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 b1a4f0dCompare 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

test/parallel/test-intl.js Outdated
Copy link
jungshikNov 28, 2016
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to try 'lt' as well.
Otherwise, LGTM

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 42df557Compare December 30, 2016 00:47

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

srl295 force-pushed the fix-ç branch from a783344 to f3d373dCompare 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

Uh oh!

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

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

Uh oh!

Conversation

srl295 commented Nov 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

MylesBorins commented Nov 28, 2016

Uh oh!

jungshikNov 28, 2016

Choose a reason for hiding this comment

Uh oh!

srl295 commented Nov 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofrobots commented Nov 28, 2016

Uh oh!

jungshik commented Nov 28, 2016

Uh oh!

srl295 commented Nov 29, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofrobots commented Dec 14, 2016

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Dec 29, 2016

Uh oh!

srl295 commented Dec 30, 2016

Uh oh!

srl295 commented Dec 30, 2016

Uh oh!

gibfahn commented Dec 30, 2016

Uh oh!

srl295 commented Dec 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Dec 30, 2016

Uh oh!

srl295 commented Dec 30, 2016

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

MylesBorins commented Jan 6, 2017

Uh oh!

MylesBorins commented Jan 6, 2017

Uh oh!

ofrobots commented Jan 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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