Bug 116210 – Use ICU_INCLUDE_DIRS in BlackBerry CMake files

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Patrick R. Gansterer]

Use ICU_INCLUDE_DIRS in BlackBerry CMake files

[Patrick R. Gansterer]

Created attachment 201932 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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.

[Ming Xie]

(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>.

[Patrick R. Gansterer]

(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.

[Ming Xie]

(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.

[Rob Buis]

Comment on attachment 201932 [details]
Patch

Looks good. Thanks for the patch!

[WebKit Commit Bot]

Comment on attachment 201932 [details]
Patch

Clearing flags on attachment: 201932

Committed r150767: <http://trac.webkit.org/changeset/150767>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.