Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(177)

Issue 2740673002: Prepare Chromium and Blink for ICU 59 (Closed)

Created:
3 years, 9 months ago by jungshik at Google
Modified:
3 years, 9 months ago
CC:
chromium-reviews, aheninger
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Prepare Chromium and Blink for ICU 59 ICU 59 uses char16_t as UChar instead of {wchar_t, uint16_t}. As a result, char16_t is not compatible with char16 any more. When constructing string16 from UnicodeString/UChar buffer, we need to reinterpret_cast with a barrier (to avoid an anti-aliasing optimzation by some compilers). Add UnicodeStringToString16() to base/i18n that utilizes ICU 59-to-be's helper for the casting regardless of anti-aliasing optimization. And, refactor UnicodeString->string16->UTF8 string to UnicodeString->UTF8 in a few places. For ICU C API "clients", UChar will be configured to be {wchar_t, uint16_t} so that there's little to be changed. This was tested with an ICU branch with char16_t as UChar. http://source.icu-project.org/repos/icu/branches/markus/ucharptr2/ BUG=693640 TEST=trybots are all green. Review-Url: https://codereview.chromium.org/2740673002 Cr-Commit-Position: refs/heads/master@{#459034} Committed: https://chromium.googlesource.com/chromium/src/+/6d1b9d119afca4a0384aba439b398188c0f6827b

Patch Set 1 #

Patch Set 2 : UCHAR_TYPE defined for C API #

Patch Set 3 : roll icu with two more upstream changes merged #

Patch Set 4 : drop an accidentally added conversion operator #

Patch Set 5 : UnicodeStringToString16 #

Patch Set 6 : more UnicodeString->string16 fixes #

Patch Set 7 : fix one more file in Blink; blink_tests can be built without any error on Linux #

Total comments: 1

Patch Set 8 : Roll ICU to bb7765c to use toUCharPtr2 instead of reinterpret_cast #

Patch Set 9 : use UnicodeString calling Collator::compare() to avoid casting and use toUCharPtr in Blink #

Patch Set 10 : Roll v8 to fd5b3e7 with v8 fix for ICU 59.x #

Patch Set 11 : Roll sfntly to 84f710 to fix an IWYU violation #

Patch Set 12 : Roll ICU back to 450be7 to land prep parts only without updating ICU #

Patch Set 13 : drop ICU roll; keep only preparation for ICU update; add sfntly roll #

Patch Set 14 : TimeFormat... in base:: #

Total comments: 1

Patch Set 15 : fix a UnicodeString=>UTF-8 conversion on iOS #

Patch Set 16 : revert sftnly roll; it's in another CL #

Total comments: 1

Patch Set 17 : iOS: drop TODO comment #

Patch Set 18 : toUTF8String appends; dest needs to be cleared #

Patch Set 19 : revert accidental revert of sftnly roll during rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+91 lines, -71 lines) Patch
M ash/common/system/date/date_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -27 lines 0 comments Download
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M base/i18n/message_formatter.cc View 1 2 3 4 3 chunks +3 lines, -2 lines 0 comments Download
M base/i18n/number_formatting.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +3 lines, -2 lines 0 comments Download
M base/i18n/string_compare.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -2 lines 0 comments Download
M base/i18n/time_formatting.cc View 1 2 3 4 6 chunks +6 lines, -8 lines 0 comments Download
M base/i18n/time_formatting_unittest.cc View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M base/i18n/timezone.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
A base/i18n/unicodestring.h View 1 2 3 4 5 6 7 1 chunk +32 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/system/timezone_util.cc View 1 2 3 4 5 6 7 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M chromeos/settings/timezone_settings.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M components/autofill/core/browser/autofill_profile_comparator.cc View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M components/autofill/core/browser/credit_card.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -2 lines 0 comments Download
M components/bookmarks/browser/titled_url_index.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/android/date_time_chooser_android.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M content/renderer/android/email_detector.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -2 lines 0 comments Download
M ios/chrome/browser/notification_promo_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -7 lines 0 comments Download
M net/ftp/ftp_util.cc View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/html/forms/EmailInputType.cpp View 1 2 3 4 5 6 7 8 3 chunks +11 lines, -2 lines 0 comments Download

Messages

Total messages: 98 (66 generated)
jungshik at Google
Markus and Andy, Below is one of errors from PS #2 (that does not include ...
3 years, 9 months ago (2017-03-08 07:54:50 UTC) #14
jungshik at Google
https://cs.chromium.org/search/?q=string16.*getBuffer+package:%5Echromium$&p=1&type=cs is an expected/known source of a trouble which can be worked around by replacing ...
3 years, 9 months ago (2017-03-08 08:37:08 UTC) #19
jungshik at Google
On 2017/03/08 08:37:08, jungshik at Google wrote: > https://cs.chromium.org/search/?q=string16.*getBuffer+package:%5Echromium$&p=1&type=cs > is an expected/known source of ...
3 years, 9 months ago (2017-03-08 11:22:03 UTC) #20
jungshik at Google
will still fail because v8 has a few issues and it's in a separate repo. ...
3 years, 9 months ago (2017-03-08 18:33:59 UTC) #27
mscherer
On 2017/03/08 07:54:50, jungshik at Google wrote: > UnicodeString::setTo(Bool, char16_t, ...) is called in ures.h ...
3 years, 9 months ago (2017-03-08 19:01:48 UTC) #30
jungshik at Google
On 2017/03/08 19:01:48, mscherer wrote: > On 2017/03/08 07:54:50, jungshik at Google wrote: > > ...
3 years, 9 months ago (2017-03-08 20:56:41 UTC) #31
jungshik at Google
blink_tests (multiple targets including content_shell, unit tests, etc) are built without any error on Linux ...
3 years, 9 months ago (2017-03-08 21:02:39 UTC) #32
jungshik at Google
v8 standalone builds worked fine as long as GN (a new build system) is used. ...
3 years, 9 months ago (2017-03-08 22:03:27 UTC) #33
jungshik at Google
Can you take a look? yfriedman@ - content/{browser,renderer}/android sky@ - components/bookmarks estade@ - component/autofill derat@ ...
3 years, 9 months ago (2017-03-14 17:20:52 UTC) #57
jungshik at Google
3 years, 9 months ago (2017-03-14 17:25:50 UTC) #59
Dan Beam
lgtm
3 years, 9 months ago (2017-03-14 18:15:59 UTC) #60
Lei Zhang
jshin: Can we land the sfntly roll separately? https://codereview.chromium.org/2745933002/
3 years, 9 months ago (2017-03-14 18:25:43 UTC) #61
jungshik at Google
On 2017/03/14 18:25:43, Lei Zhang (super slow) wrote: > jshin: Can we land the sfntly ...
3 years, 9 months ago (2017-03-14 19:51:05 UTC) #62
Daniel Erat
lgtm for //chromeos and //c/b/chromeos https://codereview.chromium.org/2740673002/diff/260001/chromeos/settings/timezone_settings.cc File chromeos/settings/timezone_settings.cc (right): https://codereview.chromium.org/2740673002/diff/260001/chromeos/settings/timezone_settings.cc#newcode487 chromeos/settings/timezone_settings.cc:487: return base::i18n::UnicodeStringToString16(timezone.getID(id)); wow, this ...
3 years, 9 months ago (2017-03-14 19:58:07 UTC) #63
mscherer
FYI On 2017/03/14 19:58:07, Daniel Erat wrote: > https://codereview.chromium.org/2740673002/diff/260001/chromeos/settings/timezone_settings.cc > File chromeos/settings/timezone_settings.cc (right): > > ...
3 years, 9 months ago (2017-03-14 20:14:59 UTC) #64
Daniel Erat
On 2017/03/14 20:14:59, mscherer wrote: > FYI > > On 2017/03/14 19:58:07, Daniel Erat wrote: ...
3 years, 9 months ago (2017-03-14 20:51:50 UTC) #65
sky
LGTM
3 years, 9 months ago (2017-03-14 21:23:30 UTC) #66
Mark Mentovai
I don’t know if this needs to be inline, but I trust your judgment. LGTM ...
3 years, 9 months ago (2017-03-14 21:39:25 UTC) #70
jungshik at Google
sdefresne@: can you look at changes in ios/chrome/browser ? phajdan.jr@ : can you look at ...
3 years, 9 months ago (2017-03-15 06:59:59 UTC) #73
jungshik at Google
sdefresne@: can you look at changes in ios/chrome/browser ? phajdan.jr@ : can you look at ...
3 years, 9 months ago (2017-03-15 07:02:40 UTC) #75
sdefresne
ios/chrome/browser lgtm (with comments) https://codereview.chromium.org/2740673002/diff/300001/ios/chrome/browser/notification_promo_unittest.cc File ios/chrome/browser/notification_promo_unittest.cc (right): https://codereview.chromium.org/2740673002/diff/300001/ios/chrome/browser/notification_promo_unittest.cc#newcode34 ios/chrome/browser/notification_promo_unittest.cc:34: // TODO(jungshik): Use an ICU ...
3 years, 9 months ago (2017-03-15 08:57:59 UTC) #76
Evan Stade
autofill lgtm
3 years, 9 months ago (2017-03-15 14:49:30 UTC) #77
jungshik at Google
Ted, can you take a look at content/.*/android? Thanks
3 years, 9 months ago (2017-03-15 22:09:44 UTC) #79
jungshik at Google
On 2017/03/15 08:57:59, sdefresne wrote: > ios/chrome/browser lgtm (with comments) > > https://codereview.chromium.org/2740673002/diff/300001/ios/chrome/browser/notification_promo_unittest.cc > File ...
3 years, 9 months ago (2017-03-15 22:14:42 UTC) #80
stevenjb
ash/ lgtm
3 years, 9 months ago (2017-03-15 22:35:02 UTC) #81
Ted C
On 2017/03/15 22:35:02, stevenjb wrote: > ash/ lgtm android - lgtm
3 years, 9 months ago (2017-03-15 23:04:07 UTC) #82
Paweł Hajdan Jr.
net/ftp LGTM
3 years, 9 months ago (2017-03-16 08:47:22 UTC) #83
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2740673002/320001
3 years, 9 months ago (2017-03-21 20:10:59 UTC) #86
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/254698)
3 years, 9 months ago (2017-03-21 21:26:32 UTC) #88
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2740673002/340001
3 years, 9 months ago (2017-03-23 07:51:38 UTC) #91
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2740673002/360001
3 years, 9 months ago (2017-03-23 08:26:40 UTC) #95
commit-bot: I haz the power
3 years, 9 months ago (2017-03-23 09:58:13 UTC) #98
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/6d1b9d119afca4a0384aba439b39...

Powered by Google App Engine
This is Rietveld 408576698