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::').
Created attachment 34994 [details] patch to add 'icu::'
I assume this is not up for review since it's not marked r=?. It will need a ChangeLog before it can be reviewed.
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.
(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 :-) ).
Created attachment 35045 [details] patch with ChangeLog I forgot to include ChangeLog in the previous patch as pointed out by Eric. This is ready for review.
Comment on attachment 35045 [details] 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.
(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.
Comment on attachment 35045 [details] 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
Comment on attachment 35045 [details] patch with ChangeLog Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436
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.
I've just landed manually ( http://trac.webkit.org/changeset/47506 ).
Created attachment 35136 [details] 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.
Comment on attachment 35136 [details] patch pt2 (I missed a file) LGTM.
Comment on attachment 35136 [details] patch pt2 (I missed a file) I'll land it manually because ChangeLog will be outdated again.