116210 2013-05-16 00:29:56 -0700 Use ICU_INCLUDE_DIRS in BlackBerry CMake files 2013-05-27 11:09:42 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Other Other RESOLVED FIXED P2 Normal --- 1 paroga paroga benjamin cmarcelo commit-queue efidler gyuyoung.kim mxie rakuco rwlbuis oldest_to_newest 889725 0 paroga 2013-05-16 00:29:56 -0700 Use ICU_INCLUDE_DIRS in BlackBerry CMake files 889727 1 201932 paroga 2013-05-16 00:38:19 -0700 Created attachment 201932 Patch 889827 2 201932 rakuco 2013-05-16 08:00:01 -0700 Comment on attachment 201932 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. 889899 3 mxie 2013-05-16 10:47:44 -0700 (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>. 890038 4 paroga 2013-05-16 16:39:01 -0700 (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. 890045 5 mxie 2013-05-16 16:47:43 -0700 (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. 894080 6 201932 rwlbuis 2013-05-27 10:21:58 -0700 Comment on attachment 201932 Patch Looks good. Thanks for the patch! 894093 7 201932 commit-queue 2013-05-27 11:09:38 -0700 Comment on attachment 201932 Patch Clearing flags on attachment: 201932 Committed r150767: <http://trac.webkit.org/changeset/150767> 894094 8 commit-queue 2013-05-27 11:09:42 -0700 All reviewed patches have been landed. Closing bug. 201932 2013-05-16 00:38:19 -0700 2013-05-27 11:09:38 -0700 Patch bug-116210.patch text/plain 5521 paroga ZGlmZiAtLWdpdCBhL0NoYW5nZUxvZyBiL0NoYW5nZUxvZwppbmRleCA4NjA4ODgzLi4yMDc5ODFl IDEwMDY0NAotLS0gYS9DaGFuZ2VMb2cKKysrIGIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAK KzIwMTMtMDUtMTYgIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dhQHdlYmtpdC5vcmc+CisKKyAg ICAgICAgVXNlIElDVV9JTkNMVURFX0RJUlMgaW4gQmxhY2tCZXJyeSBDTWFrZSBmaWxlcworICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE2MjEwCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgU2V0IGFuZCB1c2Ug dGhlIElDVV9JTkNMVURFX0RJUlMgdmFyaWFibGUgdG8gYXZvaWQKKyAgICAgICAgZHVwbGljYXRl ZCBhZGRpbmcgb2YgdGhlIElDVSBpbmNsdWRlIGRpcmVjdG9yeS4KKworICAgICAgICAqIFNvdXJj ZS9jbWFrZS9PcHRpb25zQmxhY2tCZXJyeS5jbWFrZToKKwogMjAxMy0wNS0xNSAgUGF0cmljayBH YW5zdGVyZXIgIDxwYXJvZ2FAd2Via2l0Lm9yZz4KIAogICAgICAgICBDb25zb2xpZGF0ZSBsaXN0 cyBpbiBXVEYgQ01ha2UgZmlsZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGFhNzZjZWIu LjBlYTk5MWEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysr IGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEz LTA1LTE2ICBQYXRyaWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQub3JnPgorCisgICAgICAg IFVzZSBJQ1VfSU5DTFVERV9ESVJTIGluIEJsYWNrQmVycnkgQ01ha2UgZmlsZXMKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExNjIxMAorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFNldCBhbmQgdXNlIHRoZSBJ Q1VfSU5DTFVERV9ESVJTIHZhcmlhYmxlIHRvIGF2b2lkCisgICAgICAgIGR1cGxpY2F0ZWQgYWRk aW5nIG9mIHRoZSBJQ1UgaW5jbHVkZSBkaXJlY3RvcnkuCisKKyAgICAgICAgKiBQbGF0Zm9ybUJs YWNrQmVycnkuY21ha2U6CisKIDIwMTMtMDUtMTUgIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dh QHdlYmtpdC5vcmc+CiAKICAgICAgICAgQ29uc29saWRhdGUgbGlzdHMgaW4gSmF2YVNjcmlwdENv cmUgQ01ha2UgZmlsZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9QbGF0Zm9y bUJsYWNrQmVycnkuY21ha2UgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvUGxhdGZvcm1CbGFja0Jl cnJ5LmNtYWtlCmluZGV4IDBkYWU0MGMuLjE1ZTQ1ZjIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9QbGF0Zm9ybUJsYWNrQmVycnkuY21ha2UKKysrIGIvU291cmNlL0phdmFTY3Jp cHRDb3JlL1BsYXRmb3JtQmxhY2tCZXJyeS5jbWFrZQpAQCAtMSw3ICsxLDMgQEAKLWxpc3QoSU5T RVJUIEphdmFTY3JpcHRDb3JlX0lOQ0xVREVfRElSRUNUT1JJRVMgMAotICAgICIke0JMQUNLQkVS UllfVEhJUkRfUEFSVFlfRElSfS9pY3UiCi0pCi0KIGxpc3QoUkVNT1ZFX0lURU0gSmF2YVNjcmlw dENvcmVfU09VUkNFUwogICAgIHJ1bnRpbWUvR0NBY3Rpdml0eUNhbGxiYWNrLmNwcAogKQpkaWZm IC0tZ2l0IGEvU291cmNlL1dURi9DaGFuZ2VMb2cgYi9Tb3VyY2UvV1RGL0NoYW5nZUxvZwppbmRl eCBhYzdlMjMwLi5lYWMyZWM0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL0NoYW5nZUxvZworKysg Yi9Tb3VyY2UvV1RGL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTA1LTE2ICBQYXRy aWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQub3JnPgorCisgICAgICAgIFVzZSBJQ1VfSU5D TFVERV9ESVJTIGluIEJsYWNrQmVycnkgQ01ha2UgZmlsZXMKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExNjIxMAorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFNldCBhbmQgdXNlIHRoZSBJQ1VfSU5DTFVERV9E SVJTIHZhcmlhYmxlIHRvIGF2b2lkCisgICAgICAgIGR1cGxpY2F0ZWQgYWRkaW5nIG9mIHRoZSBJ Q1UgaW5jbHVkZSBkaXJlY3RvcnkuCisKKyAgICAgICAgKiB3dGYvUGxhdGZvcm1CbGFja0JlcnJ5 LmNtYWtlOgorCiAyMDEzLTA1LTE1ICBQYXRyaWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQu b3JnPgogCiAgICAgICAgIENvbnNvbGlkYXRlIGxpc3RzIGluIFdURiBDTWFrZSBmaWxlcwpkaWZm IC0tZ2l0IGEvU291cmNlL1dURi93dGYvUGxhdGZvcm1CbGFja0JlcnJ5LmNtYWtlIGIvU291cmNl L1dURi93dGYvUGxhdGZvcm1CbGFja0JlcnJ5LmNtYWtlCmluZGV4IGZiOWNjNTEuLmI1ZGUzMDUg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL1BsYXRmb3JtQmxhY2tCZXJyeS5jbWFrZQorKysg Yi9Tb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybUJsYWNrQmVycnkuY21ha2UKQEAgLTEsNyArMSwzIEBA CiBsaXN0KEFQUEVORCBXVEZfU09VUkNFUwogICAgIGJsYWNrYmVycnkvTWFpblRocmVhZEJsYWNr QmVycnkuY3BwCiApCi0KLWxpc3QoSU5TRVJUIFdURl9JTkNMVURFX0RJUkVDVE9SSUVTIDAKLSAg ICAiJHtCTEFDS0JFUlJZX1RISVJEX1BBUlRZX0RJUn0vaWN1IgotKQpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IDFi YTM5MDAuLjQwNDJmZmEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZworKysg Yi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxMy0wNS0xNiAg UGF0cmljayBHYW5zdGVyZXIgIDxwYXJvZ2FAd2Via2l0Lm9yZz4KKworICAgICAgICBVc2UgSUNV X0lOQ0xVREVfRElSUyBpbiBCbGFja0JlcnJ5IENNYWtlIGZpbGVzCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTYyMTAKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBTZXQgYW5kIHVzZSB0aGUgSUNVX0lOQ0xV REVfRElSUyB2YXJpYWJsZSB0byBhdm9pZAorICAgICAgICBkdXBsaWNhdGVkIGFkZGluZyBvZiB0 aGUgSUNVIGluY2x1ZGUgZGlyZWN0b3J5LgorCisgICAgICAgICogUGxhdGZvcm1CbGFja0JlcnJ5 LmNtYWtlOgorCiAyMDEzLTA1LTE1ICBQYXRyaWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQu b3JnPgogCiAgICAgICAgIFBvcnQgZnVuY2lvbnMgZm9yIHBhdGhoYW5kbGluZyB0byBXaW5kb3dz IENFIGluIEZpbGVTeXN0ZW1XaW4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL1BsYXRmb3Jt QmxhY2tCZXJyeS5jbWFrZSBiL1NvdXJjZS9XZWJDb3JlL1BsYXRmb3JtQmxhY2tCZXJyeS5jbWFr ZQppbmRleCA0ODZkYzMxLi5kZTI5NjQwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9QbGF0 Zm9ybUJsYWNrQmVycnkuY21ha2UKKysrIGIvU291cmNlL1dlYkNvcmUvUGxhdGZvcm1CbGFja0Jl cnJ5LmNtYWtlCkBAIC0xLDYgKzEsNSBAQAogbGlzdChJTlNFUlQgV2ViQ29yZV9JTkNMVURFX0RJ UkVDVE9SSUVTIDAKICAgICAiJHtCTEFDS0JFUlJZX1RISVJEX1BBUlRZX0RJUn0iICMgRm9yIDx1 bmljb2RlLmg+LCB3aGljaCBpcyBpbmNsdWRlZCBmcm9tIDxzeXMva2V5Y29kZXMuaD4uCi0gICAg IiR7QkxBQ0tCRVJSWV9USElSRF9QQVJUWV9ESVJ9L2ljdSIKICkKIAogbGlzdChSRU1PVkVfSVRF TSBXZWJDb3JlX1NPVVJDRVMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIv U291cmNlL1dlYktpdC9DaGFuZ2VMb2cKaW5kZXggNTQ3ZmIxMy4uMWIzYTYyMSAxMDA2NDQKLS0t IGEvU291cmNlL1dlYktpdC9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9DaGFuZ2VMb2cK QEAgLTEsMyArMSwxNSBAQAorMjAxMy0wNS0xNiAgUGF0cmljayBHYW5zdGVyZXIgIDxwYXJvZ2FA d2Via2l0Lm9yZz4KKworICAgICAgICBVc2UgSUNVX0lOQ0xVREVfRElSUyBpbiBCbGFja0JlcnJ5 IENNYWtlIGZpbGVzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMTYyMTAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBTZXQgYW5kIHVzZSB0aGUgSUNVX0lOQ0xVREVfRElSUyB2YXJpYWJsZSB0byBhdm9pZAor ICAgICAgICBkdXBsaWNhdGVkIGFkZGluZyBvZiB0aGUgSUNVIGluY2x1ZGUgZGlyZWN0b3J5Lgor CisgICAgICAgICogUGxhdGZvcm1CbGFja0JlcnJ5LmNtYWtlOgorCiAyMDEzLTA1LTE0ICBDYXJs b3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBbQmxhY2tC ZXJyeV0gSW1wbGVtZW50IHBsYXRmb3JtIHN0cmF0ZWdpZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJLaXQvUGxhdGZvcm1CbGFja0JlcnJ5LmNtYWtlIGIvU291cmNlL1dlYktpdC9QbGF0Zm9ybUJs YWNrQmVycnkuY21ha2UKaW5kZXggYjI5NTIwNS4uY2RiYWM5MCAxMDA2NDQKLS0tIGEvU291cmNl L1dlYktpdC9QbGF0Zm9ybUJsYWNrQmVycnkuY21ha2UKKysrIGIvU291cmNlL1dlYktpdC9QbGF0 Zm9ybUJsYWNrQmVycnkuY21ha2UKQEAgLTEsNiArMSw1IEBACiBsaXN0KElOU0VSVCBXZWJLaXRf SU5DTFVERV9ESVJFQ1RPUklFUyAwCiAgICAgIiR7QkxBQ0tCRVJSWV9USElSRF9QQVJUWV9ESVJ9 IiAjIEZvciA8dW5pY29kZS5oPiwgd2hpY2ggaXMgaW5jbHVkZWQgZnJvbSA8c3lzL2tleWNvZGVz Lmg+LgotICAgICIke0JMQUNLQkVSUllfVEhJUkRfUEFSVFlfRElSfS9pY3UiCiApCiAKIGxpc3Qo QVBQRU5EIFdlYktpdF9JTkNMVURFX0RJUkVDVE9SSUVTCmRpZmYgLS1naXQgYS9Tb3VyY2UvY21h a2UvT3B0aW9uc0JsYWNrQmVycnkuY21ha2UgYi9Tb3VyY2UvY21ha2UvT3B0aW9uc0JsYWNrQmVy cnkuY21ha2UKaW5kZXggYmExNTE1Yy4uOGMxN2NhZCAxMDA2NDQKLS0tIGEvU291cmNlL2NtYWtl L09wdGlvbnNCbGFja0JlcnJ5LmNtYWtlCisrKyBiL1NvdXJjZS9jbWFrZS9PcHRpb25zQmxhY2tC ZXJyeS5jbWFrZQpAQCAtMjMzLDMgKzIzMyw1IEBAIGFkZF9kZWZpbml0aW9ucygtREVOQUJMRV9C TEFDS0JFUlJZX0NSRURFTlRJQUxfUEVSU0lTVD0xKQogc2V0KEJMQUNLQkVSUllfVEhJUkRfUEFS VFlfRElSICIke0NNQUtFX0JJTkFSWV9ESVJ9L1RoaXJkUGFydHlCbGFja0JlcnJ5IikKIGZpbGUo Q09QWSAke1RISVJEX1BBUlRZX0lDVV9ESVJ9IERFU1RJTkFUSU9OICIke0JMQUNLQkVSUllfVEhJ UkRfUEFSVFlfRElSfS9pY3UiKQogZmlsZShDT1BZICR7VEhJUkRfUEFSVFlfVU5JQ09ERV9GSUxF fSBERVNUSU5BVElPTiAke0JMQUNLQkVSUllfVEhJUkRfUEFSVFlfRElSfSkKKworc2V0KElDVV9J TkNMVURFX0RJUlMgIiR7QkxBQ0tCRVJSWV9USElSRF9QQVJUWV9ESVJ9L2ljdSIpCg==