28410 icuns 2009-08-17 15:24:19 -0700 Add 'icu::' qualifier to ICU C++ names 2009-08-19 14:26:32 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Normal --- 1 jshin jshin dglazkov eric oldest_to_newest 140846 0 jshin 2009-08-17 15:24:19 -0700 Chromium is going to upgrade its ICU to ICU 4.2.1. In the process, it's found that there's a name collision between Chromium and ICU because both has StringPiece in the global namespace. To prevent this, Chromium decided to build its ICU with U_USING_ICU_NAMESPACE set to 0 (the default is 1 and ICU dumps all the names to the global namespace). With that change, all the references to ICU's C++ names must be qualified with 'icu::'. Fortunately, there are very few of them (most references to ICU apis are C APIs rather than C++ APIs). Chromium on Linux and Mac build just fine without any change. Chromium on Windows needs a minor change in a single file. However, there's a possibility that new references to ICU C++ names will be introduced to Webkit in the future. I'll send an email to webkit-dev about the issue (hoping that nobody will be against having to use 'icu::'). 140847 1 34994 jshin 2009-08-17 15:32:05 -0700 Created attachment 34994 patch to add 'icu::' 140848 2 eric 2009-08-17 15:34:32 -0700 I assume this is not up for review since it's not marked r=?. It will need a ChangeLog before it can be reviewed. 140849 3 eric 2009-08-17 15:36:14 -0700 I think there could be problems with this type of policy change, if this is not the default ICU compile options. Mac WebKit is likely going to use the Mac OS X system installed ICU for the forseeable future. 141098 4 jshin 2009-08-18 09:14:06 -0700 (In reply to comment #3) > I think there could be problems with this type of policy change, if this is not > the default ICU compile options. Mac WebKit is likely going to use the Mac OS > X system installed ICU for the forseeable future. However, it can't hurt because what setting U_USING_ICU_NAMESPACE does is putting "using namespace icu;" for compilers that support C++ namespace (in the old days of ICU, I heard not many compilers support namespace). So, referring to ICU C++ names as 'icu::Foo' or 'Foo' does not make any difference when 'using namespace icu:' is in effect. See my Chromium CL ( http://codereview.chromium.org/171012/show ) that adds an explicit 'icu::' qualifier with U_USING_ICU_NAMESPACE still set to 1 (the CL was submitted yesterday and everything went fine :-) Somehow, our review tool didn't notice that it had been submitted :-) ). 141101 5 35045 jshin 2009-08-18 09:19:09 -0700 Created attachment 35045 patch with ChangeLog I forgot to include ChangeLog in the previous patch as pointed out by Eric. This is ready for review. 141103 6 35045 eric 2009-08-18 09:22:59 -0700 Comment on attachment 35045 patch with ChangeLog Ok, sounds fine. I wonder if we shouldn't change the rest of our ICU uses to use icu:: if namespace icu { } is always defined and used by ICU these days. 141118 7 jshin 2009-08-18 10:18:16 -0700 (In reply to comment #6) > (From update of attachment 35045 [details]) > Ok, sounds fine. I wonder if we shouldn't change the rest of our ICU uses to > use icu:: if namespace icu { } is always defined and used by ICU these days. I think that's a good idea as long as all the ports that use ICU also use compilers that support namespace, which is very likely in 2009 :-). 'namespace icu { }' is defined and used by ICU for compilers that support namespace. (actually, icu:: is an 'alias' to icu_M_N:: for icu M.N, but callers should stick to icu::). BTW, there are very few ICU C++ APIs in use in Webkit (actually, I haven't found one yet in non-Chromium part of WebKit). They all use C APIs which are not affected by this. 141220 8 35045 eric 2009-08-18 14:12:10 -0700 Comment on attachment 35045 patch with ChangeLog Rejecting patch 35045 from commit-queue. This patch will require manual commit. Failed to run "['git', 'svn', 'rebase']" exit_code: 1 cwd: None 141231 9 35045 eric 2009-08-18 14:24:12 -0700 Comment on attachment 35045 patch with ChangeLog Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436 141271 10 35045 eric 2009-08-18 15:08:32 -0700 Comment on attachment 35045 patch with ChangeLog Rejecting patch 35045 from commit-queue. This patch will require manual commit. Failed to run "['git', 'svn', 'rebase']" exit_code: 1 cwd: None 141273 11 eric 2009-08-18 15:10:39 -0700 Parsing ChangeLog: WebCore/ChangeLog Updating working directory error: patch failed: WebCore/ChangeLog:1 error: WebCore/ChangeLog: patch does not apply rebase refs/remotes/trunk: command returned error: 1 bugzilla-tool is not yet smart enough to know hot to run resolve-ChangeLogs for you. 141497 12 jshin 2009-08-19 09:39:51 -0700 I've just landed manually ( http://trac.webkit.org/changeset/47506 ). 141574 13 35136 jshin 2009-08-19 13:30:58 -0700 Created attachment 35136 patch pt2 (I missed a file) Sorry that I missed this file earlier. With bug 28441 fixed, I built Webkit for Chromium on Windows and I'm sure there's no other reference to ICU C++ names. 141583 14 35136 eric 2009-08-19 13:51:32 -0700 Comment on attachment 35136 patch pt2 (I missed a file) LGTM. 141602 15 35136 jshin 2009-08-19 14:26:32 -0700 Comment on attachment 35136 patch pt2 (I missed a file) I'll land it manually because ChangeLog will be outdated again. 34994 2009-08-17 15:32:05 -0700 2009-08-18 09:19:09 -0700 patch to add 'icu::' 28410.patch.1 text/plain 1703 jshin SW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlQ2hyb21p dW1XaW4uY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21p dW0vRm9udENhY2hlQ2hyb21pdW1XaW4uY3BwCShyZXZpc2lvbiA0NzM4NikKKysrIFdlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlQ2hyb21pdW1XaW4uY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC0yNjEsNyArMjYxLDcgQEAgc3RhdGljIEhGT05UIGNyZWF0ZUZvbnRJbmRp cmVjdEFuZEdldFdpbgogLy8gY2hhcmFjdGVycy4gQmVjYXVzZSBpdCdzIGZhbWlseSBuYW1lcyBy YXRoZXIgdGhhbiBmb250IGZhY2VzIHdlIHVzZQogLy8gYXMga2V5cywgdGhlcmUgbWlnaHQgYmUg ZWRnZSBjYXNlcyB3aGVyZSBvbmUgZmFjZSBvZiBhIGZvbnQgZmFtaWx5CiAvLyBoYXMgYSBkaWZm ZXJlbnQgcmVwZXJ0b2lyZSBmcm9tIGFub3RoZXIgZmFjZSBvZiB0aGUgc2FtZSBmYW1pbHkuIAot dHlwZWRlZiBIYXNoTWFwPGNvbnN0IHdjaGFyX3QqLCBVbmljb2RlU2V0Kj4gRm9udENtYXBDYWNo ZTsKK3R5cGVkZWYgSGFzaE1hcDxjb25zdCB3Y2hhcl90KiwgaWN1OjpVbmljb2RlU2V0Kj4gRm9u dENtYXBDYWNoZTsKIAogc3RhdGljIGJvb2wgZm9udENvbnRhaW5zQ2hhcmFjdGVyKGNvbnN0IEZv bnRQbGF0Zm9ybURhdGEqIGZvbnREYXRhLAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgIGNvbnN0IHdjaGFyX3QqIGZhbWlseSwgVUNoYXIzMiBjaGFyYWN0ZXIpCkBAIC0yNzcsNyAr Mjc3LDcgQEAgc3RhdGljIGJvb2wgZm9udENvbnRhaW5zQ2hhcmFjdGVyKGNvbnN0IAogICAgIGlm ICghZm9udENtYXBDYWNoZSkKICAgICAgICAgZm9udENtYXBDYWNoZSA9IG5ldyBGb250Q21hcENh Y2hlOwogCi0gICAgSGFzaE1hcDxjb25zdCB3Y2hhcl90KiwgVW5pY29kZVNldCo+OjppdGVyYXRv ciBpdCA9IGZvbnRDbWFwQ2FjaGUtPmZpbmQoZmFtaWx5KTsKKyAgICBIYXNoTWFwPGNvbnN0IHdj aGFyX3QqLCBpY3U6OlVuaWNvZGVTZXQqPjo6aXRlcmF0b3IgaXQgPSBmb250Q21hcENhY2hlLT5m aW5kKGZhbWlseSk7CiAgICAgaWYgKGl0ICE9IGZvbnRDbWFwQ2FjaGUtPmVuZCgpKSAKICAgICAg ICAgcmV0dXJuIGl0LT5zZWNvbmQtPmNvbnRhaW5zKGNoYXJhY3Rlcik7CiAgICAgCkBAIC0zMDgs NyArMzA4LDcgQEAgc3RhdGljIGJvb2wgZm9udENvbnRhaW5zQ2hhcmFjdGVyKGNvbnN0IAogICAg IC8vIDEpIHBvcnQgYmFjayBJQ1UgNC4wJ3MgZmFzdGVyIGxvb2stdXAgY29kZSBmb3IgVW5pY29k ZVNldAogICAgIC8vIDIpIHBvcnQgTW96aWxsYSdzIENvbXByZXNzZWRDaGFyTWFwIG9yIGdmeFNw YXJzZUJpdHNldAogICAgIHVuc2lnbmVkIGkgPSAwOwotICAgIFVuaWNvZGVTZXQqIGNtYXAgPSBu ZXcgVW5pY29kZVNldDsKKyAgICBpY3U6OlVuaWNvZGVTZXQqIGNtYXAgPSBuZXcgaWN1OjpVbmlj b2RlU2V0OwogICAgIHdoaWxlIChpIDwgZ2x5cGhzZXQtPmNSYW5nZXMpIHsKICAgICAgICAgV0NI QVIgc3RhcnQgPSBnbHlwaHNldC0+cmFuZ2VzW2ldLndjTG93OyAKICAgICAgICAgY21hcC0+YWRk KHN0YXJ0LCBzdGFydCArIGdseXBoc2V0LT5yYW5nZXNbaV0uY0dseXBocyAtIDEpOwo= 35045 2009-08-18 09:19:09 -0700 2009-08-18 15:08:32 -0700 patch with ChangeLog 28410.patch.2 text/plain 2342 jshin SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NzM5MykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMDktMDgtMTcgIEp1bmdzaGlrIFNoaW4gIDxqc2hpbkBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg QWRkICdpY3U6OicgcXVhbGlmaWVyIHdoZW4gcmVmZXJpbmcgdG8gSUNVIEMrKyBuYW1lcy4KKwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Mjg0MTAKKwor ICAgICAgICBObyBjaGFuZ2UgaW4gdGhlIHRlc3QgcmVzdWx0LgorCisgICAgICAgICogcGxhdGZv cm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlQ2hyb21pdW1XaW4uY3BwOgorICAgICAgICAo V2ViQ29yZTo6Zm9udENvbnRhaW5zQ2hhcmFjdGVyKToKKwogMjAwOS0wOC0xNyAgUGV0ZXIgS2Fz dGluZyAgPHBrYXN0aW5nQGdvb2dsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgU3RldmUg RmFsa2VuYnVyZy4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9u dENhY2hlQ2hyb21pdW1XaW4uY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvY2hyb21pdW0vRm9udENhY2hlQ2hyb21pdW1XaW4uY3BwCShyZXZpc2lvbiA0NzM4NikK KysrIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udENhY2hlQ2hyb21pdW1X aW4uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yNjEsNyArMjYxLDcgQEAgc3RhdGljIEhGT05UIGNy ZWF0ZUZvbnRJbmRpcmVjdEFuZEdldFdpbgogLy8gY2hhcmFjdGVycy4gQmVjYXVzZSBpdCdzIGZh bWlseSBuYW1lcyByYXRoZXIgdGhhbiBmb250IGZhY2VzIHdlIHVzZQogLy8gYXMga2V5cywgdGhl cmUgbWlnaHQgYmUgZWRnZSBjYXNlcyB3aGVyZSBvbmUgZmFjZSBvZiBhIGZvbnQgZmFtaWx5CiAv LyBoYXMgYSBkaWZmZXJlbnQgcmVwZXJ0b2lyZSBmcm9tIGFub3RoZXIgZmFjZSBvZiB0aGUgc2Ft ZSBmYW1pbHkuIAotdHlwZWRlZiBIYXNoTWFwPGNvbnN0IHdjaGFyX3QqLCBVbmljb2RlU2V0Kj4g Rm9udENtYXBDYWNoZTsKK3R5cGVkZWYgSGFzaE1hcDxjb25zdCB3Y2hhcl90KiwgaWN1OjpVbmlj b2RlU2V0Kj4gRm9udENtYXBDYWNoZTsKIAogc3RhdGljIGJvb2wgZm9udENvbnRhaW5zQ2hhcmFj dGVyKGNvbnN0IEZvbnRQbGF0Zm9ybURhdGEqIGZvbnREYXRhLAogICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIGNvbnN0IHdjaGFyX3QqIGZhbWlseSwgVUNoYXIzMiBjaGFyYWN0ZXIp CkBAIC0yNzcsNyArMjc3LDcgQEAgc3RhdGljIGJvb2wgZm9udENvbnRhaW5zQ2hhcmFjdGVyKGNv bnN0IAogICAgIGlmICghZm9udENtYXBDYWNoZSkKICAgICAgICAgZm9udENtYXBDYWNoZSA9IG5l dyBGb250Q21hcENhY2hlOwogCi0gICAgSGFzaE1hcDxjb25zdCB3Y2hhcl90KiwgVW5pY29kZVNl dCo+OjppdGVyYXRvciBpdCA9IGZvbnRDbWFwQ2FjaGUtPmZpbmQoZmFtaWx5KTsKKyAgICBIYXNo TWFwPGNvbnN0IHdjaGFyX3QqLCBpY3U6OlVuaWNvZGVTZXQqPjo6aXRlcmF0b3IgaXQgPSBmb250 Q21hcENhY2hlLT5maW5kKGZhbWlseSk7CiAgICAgaWYgKGl0ICE9IGZvbnRDbWFwQ2FjaGUtPmVu ZCgpKSAKICAgICAgICAgcmV0dXJuIGl0LT5zZWNvbmQtPmNvbnRhaW5zKGNoYXJhY3Rlcik7CiAg ICAgCkBAIC0zMDgsNyArMzA4LDcgQEAgc3RhdGljIGJvb2wgZm9udENvbnRhaW5zQ2hhcmFjdGVy KGNvbnN0IAogICAgIC8vIDEpIHBvcnQgYmFjayBJQ1UgNC4wJ3MgZmFzdGVyIGxvb2stdXAgY29k ZSBmb3IgVW5pY29kZVNldAogICAgIC8vIDIpIHBvcnQgTW96aWxsYSdzIENvbXByZXNzZWRDaGFy TWFwIG9yIGdmeFNwYXJzZUJpdHNldAogICAgIHVuc2lnbmVkIGkgPSAwOwotICAgIFVuaWNvZGVT ZXQqIGNtYXAgPSBuZXcgVW5pY29kZVNldDsKKyAgICBpY3U6OlVuaWNvZGVTZXQqIGNtYXAgPSBu ZXcgaWN1OjpVbmljb2RlU2V0OwogICAgIHdoaWxlIChpIDwgZ2x5cGhzZXQtPmNSYW5nZXMpIHsK ICAgICAgICAgV0NIQVIgc3RhcnQgPSBnbHlwaHNldC0+cmFuZ2VzW2ldLndjTG93OyAKICAgICAg ICAgY21hcC0+YWRkKHN0YXJ0LCBzdGFydCArIGdseXBoc2V0LT5yYW5nZXNbaV0uY0dseXBocyAt IDEpOwo= 35136 2009-08-19 13:30:58 -0700 2009-08-19 14:26:32 -0700 patch pt2 (I missed a file) 28410.patch.pt2 text/plain 1749 jshin SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NzUxNikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMDktMDgtMTkgIEp1bmdzaGlrIFNoaW4gIDxqc2hpbkBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg QWRkICdpY3U6OicgcXVhbGlmaWVyIHdoZW4gcmVmZXJpbmcgdG8gSUNVIEMrKyBuYW1lcyB0byB0 aGUgb3RoZXIKKyAgICAgICAgZmlsZSB3aXRoIHRoaXMgaXNzdWUgbWlzc2VkIGluIHRoZSBwcmV2 aW91cyBjaGVjay1pbi4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9Mjg0MTAKKworICAgICAgICBObyBjaGFuZ2UgaW4gdGhlIHRlc3QgcmVzdWx0Lgor CisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udFV0aWxzQ2hyb21pdW1X aW4uY3BwOgorICAgICAgICAoV2ViQ29yZTo6KToKKwogMjAwOS0wOC0xOSAgRXJpYyBDYXJsc29u ICA8ZXJpYy5jYXJsc29uQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBFcmljIFNl aWRlbC4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udFV0aWxz Q2hyb21pdW1XaW4uY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vRm9udFV0aWxzQ2hyb21pdW1XaW4uY3BwCShyZXZpc2lvbiA0NzM4NikKKysrIFdl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vRm9udFV0aWxzQ2hyb21pdW1XaW4uY3Bw CSh3b3JraW5nIGNvcHkpCkBAIC0xMDAsMTEgKzEwMCwxMSBAQCB2b2lkIGluaXRpYWxpemVTY3Jp cHRGb250TWFwKFNjcmlwdFRvRm9uCiAgICAgLy8gSW5pdGlhbGl6ZSB0aGUgbG9jYWxlLWRlcGVu ZGVudCBtYXBwaW5nLgogICAgIC8vIFNpbmNlIENocm9tZSBzeW5jaHJvbml6ZXMgdGhlIElDVSBk ZWZhdWx0IGxvY2FsZSB3aXRoIGl0cyBVSSBsb2NhbGUsCiAgICAgLy8gdGhpcyBJQ1UgbG9jYWxl IHRlbGxzIHRoZSBjdXJyZW50IFVJIGxvY2FsZSBvZiBDaHJvbWUuCi0gICAgTG9jYWxlIGxvY2Fs ZSA9IExvY2FsZTo6Z2V0RGVmYXVsdCgpOworICAgIGljdTo6TG9jYWxlIGxvY2FsZSA9IGljdTo6 TG9jYWxlOjpnZXREZWZhdWx0KCk7CiAgICAgY29uc3QgVUNoYXIqIGxvY2FsZUZhbWlseSA9IDA7 Ci0gICAgaWYgKGxvY2FsZSA9PSBMb2NhbGU6OmdldEphcGFuZXNlKCkpCisgICAgaWYgKGxvY2Fs ZSA9PSBpY3U6OkxvY2FsZTo6Z2V0SmFwYW5lc2UoKSkKICAgICAgICAgbG9jYWxlRmFtaWx5ID0g c2NyaXB0Rm9udE1hcFtVU0NSSVBUX0hJUkFHQU5BXTsKLSAgICBlbHNlIGlmIChsb2NhbGUgPT0g TG9jYWxlOjpnZXRLb3JlYW4oKSkKKyAgICBlbHNlIGlmIChsb2NhbGUgPT0gaWN1OjpMb2NhbGU6 OmdldEtvcmVhbigpKQogICAgICAgICBsb2NhbGVGYW1pbHkgPSBzY3JpcHRGb250TWFwW1VTQ1JJ UFRfSEFOR1VMXTsKICAgICBlbHNlIHsKICAgICAgICAgLy8gVXNlIFNpbXBsaWZpZWQgQ2hpbmVz ZSBmb250IGZvciBhbGwgb3RoZXIgbG9jYWxlcyBpbmNsdWRpbmcK