Use ICU_INCLUDE_DIRS in BlackBerry CMake files
Created attachment 201932 [details] Patch
Comment on attachment 201932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201932&action=review > Source/cmake/OptionsBlackBerry.cmake:237 > +set(ICU_INCLUDE_DIRS "${BLACKBERRY_THIRD_PARTY_DIR}/icu") I'd add a comment here explaining that this was supposed to be set by FindICU.cmake, but the BlackBerry port does not seem to use it.
(In reply to comment #2) > (From update of attachment 201932 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201932&action=review > > > Source/cmake/OptionsBlackBerry.cmake:237 > > +set(ICU_INCLUDE_DIRS "${BLACKBERRY_THIRD_PARTY_DIR}/icu") > > I'd add a comment here explaining that this was supposed to be set by FindICU.cmake, but the BlackBerry port does not seem to use it. +Eli It looks like we are using a different version of ICU which the current FindICU.cmake doesn't support. (For example, I couldn't find uversion.h and U_ICU_VERSION_MAJOR_NUM is defined in uvernum.h) I don't know much about ICU, but there is a comment added above kind of covers this: 225 # Some of our files, such as platform/graphics/chromium/ComplexTextControllerLinux.cpp, require a 226 # newer ICU version than the version associated with the headers in {WebCore, JavaScriptCore}/icu. 227 # Because of <https://bugs.webkit.org/show_bug.cgi?id=70913> we can't directly reference these newer 228 # ICU headers within the QNX system header directory. As a workaround, we copy these newer ICU headers 229 # from the QNX system header directory to a third-party directory under the CMake binary tree. 230 # 231 # FIXME: Make this mechanism more general purpose. Maybe accept a list or directories/files to copy 232 # instead of individual variables. Generalizing this solution may allow us to fix <https://bugs.webkit.org/show_bug.cgi?id=70913>.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 201932 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=201932&action=review > > > > > Source/cmake/OptionsBlackBerry.cmake:237 > > > +set(ICU_INCLUDE_DIRS "${BLACKBERRY_THIRD_PARTY_DIR}/icu") > > > > I'd add a comment here explaining that this was supposed to be set by FindICU.cmake, but the BlackBerry port does not seem to use it. > > +Eli > > It looks like we are using a different version of ICU which the current FindICU.cmake doesn't support. (For example, I couldn't find uversion.h and U_ICU_VERSION_MAJOR_NUM is defined in uvernum.h) > > I don't know much about ICU, but there is a comment added above kind of covers this: > > 225 # Some of our files, such as platform/graphics/chromium/ComplexTextControllerLinux.cpp, require a > 226 # newer ICU version than the version associated with the headers in {WebCore, JavaScriptCore}/icu. > 227 # Because of <https://bugs.webkit.org/show_bug.cgi?id=70913> we can't directly reference these newer > 228 # ICU headers within the QNX system header directory. As a workaround, we copy these newer ICU headers > 229 # from the QNX system header directory to a third-party directory under the CMake binary tree. > 230 # > 231 # FIXME: Make this mechanism more general purpose. Maybe accept a list or directories/files to copy > 232 # instead of individual variables. Generalizing this solution may allow us to fix <https://bugs.webkit.org/show_bug.cgi?id=70913>. It's ok to not use FindICU.cmake if it does not work with BB ATM, but as long as you do not add the "wrong" ICU header to the include directory list, I see no problem in reusing the ICU_INCLUDE_DIRS variable. This change does just removes the duplication of the ICU include directory across the CMake files.
(In reply to comment #4) > > It's ok to not use FindICU.cmake if it does not work with BB ATM, but as long as you do not add the "wrong" ICU header to the include directory list, I see no problem in reusing the ICU_INCLUDE_DIRS variable. This change does just removes the duplication of the ICU include directory across the CMake files. Yes, this change looks fine to me.
Comment on attachment 201932 [details] Patch Looks good. Thanks for the patch!
Comment on attachment 201932 [details] Patch Clearing flags on attachment: 201932 Committed r150767: <http://trac.webkit.org/changeset/150767>
All reviewed patches have been landed. Closing bug.