79344 2012-02-23 01:10:49 -0800 [CMake] Add FindDirectX 2012-02-23 11:16:06 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 72816 1 paroga paroga aroben dbates rakuco oldest_to_newest 562943 0 paroga 2012-02-23 01:10:49 -0800 [CMake] Add FindDirectX 562944 1 128427 paroga 2012-02-23 01:11:46 -0800 Created attachment 128427 Patch 563054 2 128427 rakuco 2012-02-23 05:53:36 -0800 Comment on attachment 128427 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128427&action=review > Source/cmake/FindDirectX.cmake:12 > + "$ENV{DXSDK_DIR}/Include" > + "C:/Program Files (x86)/Microsoft DirectX SDK*/Include" > + "C:/Program Files/Microsoft DirectX SDK*/Include" I'd be more explicit and add the PATHS keyword before this hardcoded path list. > Source/cmake/FindDirectX.cmake:15 > +SET(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIRS}/..") Isn't it cleaner to use GET_FILENAME_COMPONENT(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIRS}/.." ABSOLUTE)? > Source/cmake/FindDirectX.cmake:29 > +FIND_PACKAGE_HANDLE_STANDARD_ARGS(DirectX DEFAULT_MSG DirectX_LIBRARIES DirectX_INCLUDE_DIRS) Shouldn't you also mark_as_advanced the user-visible variables? 563195 3 128492 paroga 2012-02-23 10:13:55 -0800 Created attachment 128492 Patch 563210 4 128492 rakuco 2012-02-23 10:26:49 -0800 Comment on attachment 128492 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128492&action=review Looks OK to me. > Source/cmake/FindDirectX.cmake:21 > +IF (CMAKE_CL_64) > + SET(DirectX_LIBRARY_PATHS "${DirectX_ROOT_DIR}/Lib/x64") > +ELSE () > + SET(DirectX_LIBRARY_PATHS "${DirectX_ROOT_DIR}/Lib/x86" "${DirectX_ROOT_DIR}/Lib") > +ENDIF () Minor: I don't really remember if we were suppose to use IF() or IF () in our agreed CMake coding style. 563222 5 128492 paroga 2012-02-23 10:30:29 -0800 Comment on attachment 128492 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128492&action=review >> Source/cmake/FindDirectX.cmake:21 >> +ENDIF () > > Minor: I don't really remember if we were suppose to use IF() or IF () in our agreed CMake coding style. we have a space after an if in the c++ style too, so this matches WebKit style more 563235 6 128492 aroben 2012-02-23 10:38:34 -0800 Comment on attachment 128492 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128492&action=review > Source/cmake/FindDirectX.cmake:12 > + "C:/Program Files (x86)/Microsoft DirectX SDK*/Include" > + "C:/Program Files/Microsoft DirectX SDK*/Include" Should we use $ENV{PROGRAMFILES} and $ENV{PROGRAMFILES(X86)} here? Hard-coding "C:" doesn't seem so great. 563279 7 paroga 2012-02-23 11:16:06 -0800 Committed r108647: <http://trac.webkit.org/changeset/108647> 128427 2012-02-23 01:11:46 -0800 2012-02-23 10:13:47 -0800 Patch bug-79344-20120223101149.patch text/plain 2104 paroga U3VidmVyc2lvbiBSZXZpc2lvbjogMTA4NjIxCmRpZmYgLS1naXQgYS9Tb3VyY2UvY21ha2UvRmlu ZERpcmVjdFguY21ha2UgYi9Tb3VyY2UvY21ha2UvRmluZERpcmVjdFguY21ha2UKbmV3IGZpbGUg bW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MC4uN2M4NDExZDVkNTVlNDYzOWE5NDBjY2IwZWMxNWExZGI3OWRmNzkzNQotLS0gL2Rldi9udWxs CisrKyBiL1NvdXJjZS9jbWFrZS9GaW5kRGlyZWN0WC5jbWFrZQpAQCAtMCwwICsxLDI5IEBACisj IC0gRmluZCBEaXJlY3RYIFNESyBpbnN0YWxsYXRpb24NCisjIEZpbmQgdGhlIERpcmVjdFggaW5j bHVkZXMgYW5kIGxpYnJhcnkNCisjIFRoaXMgbW9kdWxlIGRlZmluZXMNCisjICBEaXJlY3RYX0lO Q0xVREVfRElSUywgd2hlcmUgdG8gZmluZCBkM2Q5LmgsIGV0Yy4NCisjICBEaXJlY3RYX0xJQlJB UklFUywgbGlicmFyaWVzIHRvIGxpbmsgYWdhaW5zdCB0byB1c2UgRGlyZWN0WC4NCisjICBEaXJl Y3RYX0ZPVU5ELCBJZiBmYWxzZSwgZG8gbm90IHRyeSB0byB1c2UgRGlyZWN0WC4NCisjICBEaXJl Y3RYX1JPT1RfRElSLCBkaXJlY3Rvcnkgd2hlcmUgRGlyZWN0WCB3YXMgaW5zdGFsbGVkLg0KKw0K K0ZJTkRfUEFUSChEaXJlY3RYX0lOQ0xVREVfRElSUyBkM2Q5LmgNCisgICAgIiRFTlZ7RFhTREtf RElSfS9JbmNsdWRlIg0KKyAgICAiQzovUHJvZ3JhbSBGaWxlcyAoeDg2KS9NaWNyb3NvZnQgRGly ZWN0WCBTREsqL0luY2x1ZGUiDQorICAgICJDOi9Qcm9ncmFtIEZpbGVzL01pY3Jvc29mdCBEaXJl Y3RYIFNESyovSW5jbHVkZSINCispDQorDQorU0VUKERpcmVjdFhfUk9PVF9ESVIgIiR7RGlyZWN0 WF9JTkNMVURFX0RJUlN9Ly4uIikNCisNCitJRiAoQ01BS0VfQ0xfNjQpDQorICAgIFNFVChEaXJl Y3RYX0xJQlJBUllfUEFUSFMgIiR7RGlyZWN0WF9ST09UX0RJUn0vTGliL3g2NCIpDQorRUxTRSAo KQ0KKyAgICBTRVQoRGlyZWN0WF9MSUJSQVJZX1BBVEhTICIke0RpcmVjdFhfUk9PVF9ESVJ9L0xp Yi94ODYiICIke0RpcmVjdFhfUk9PVF9ESVJ9L0xpYiIpDQorRU5ESUYgKCkNCisNCitGSU5EX0xJ QlJBUlkoRGlyZWN0WF9EM0Q5X0xJQlJBUlkgZDNkOSAke0RpcmVjdFhfTElCUkFSWV9QQVRIU30g Tk9fREVGQVVMVF9QQVRIKQ0KK0ZJTkRfTElCUkFSWShEaXJlY3RYX0QzRFg5X0xJQlJBUlkgZDNk eDkgJHtEaXJlY3RYX0xJQlJBUllfUEFUSFN9IE5PX0RFRkFVTFRfUEFUSCkNCitTRVQoRGlyZWN0 WF9MSUJSQVJJRVMgJHtEaXJlY3RYX0QzRDlfTElCUkFSWX0gJHtEaXJlY3RYX0QzRFg5X0xJQlJB Ull9KQ0KKw0KKyMgaGFuZGxlIHRoZSBRVUlFVExZIGFuZCBSRVFVSVJFRCBhcmd1bWVudHMgYW5k IHNldCBEaXJlY3RYX0ZPVU5EIHRvIFRSVUUgaWYgYWxsIGxpc3RlZCB2YXJpYWJsZXMgYXJlIFRS VUUNCitJTkNMVURFKEZpbmRQYWNrYWdlSGFuZGxlU3RhbmRhcmRBcmdzKQ0KK0ZJTkRfUEFDS0FH RV9IQU5ETEVfU1RBTkRBUkRfQVJHUyhEaXJlY3RYIERFRkFVTFRfTVNHIERpcmVjdFhfTElCUkFS SUVTIERpcmVjdFhfSU5DTFVERV9ESVJTKQ0KZGlmZiAtLWdpdCBhL0NoYW5nZUxvZyBiL0NoYW5n ZUxvZwppbmRleCA2ZmY2M2Q1N2QzZTdlNGRmYzQ1ZWZjZDBjYTJiMDk2MzFmMDdkN2QxLi5lMDFl NjM1NmVhNjExZTJmNjM0OWU4NTdlZjcxNGFiOWZiMWQwOTgyIDEwMDY0NAotLS0gYS9DaGFuZ2VM b2cKKysrIGIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIgQEAKKzIwMTItMDItMjMgIFBhdHJpY2sg R2Fuc3RlcmVyICA8cGFyb2dhQHdlYmtpdC5vcmc+CisKKyAgICAgICAgW0NNYWtlXSBBZGQgRmlu ZERpcmVjdFgKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTc5MzQ0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg KiBTb3VyY2UvY21ha2UvRmluZERpcmVjdFguY21ha2U6IEFkZGVkLgorCiAyMDEyLTAyLTIyICBS YXBoYWVsIEt1Ym8gZGEgQ29zdGEgIDxrdWJvQHByb2Z1c2lvbi5tb2JpPgogCiAgICAgICAgIFtD TWFrZV0gUmVtb3ZlIEZpbmRHREsuY21ha2UgYW5kIEZpbmRHREstUGl4QnVmLmNtYWtlCg== 128492 2012-02-23 10:13:55 -0800 2012-02-23 10:38:33 -0800 Patch bug-79344-20120223191400.patch text/plain 2239 paroga U3VidmVyc2lvbiBSZXZpc2lvbjogMTA4NjM2CmRpZmYgLS1naXQgYS9Tb3VyY2UvY21ha2UvRmlu ZERpcmVjdFguY21ha2UgYi9Tb3VyY2UvY21ha2UvRmluZERpcmVjdFguY21ha2UKbmV3IGZpbGUg bW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MC4uNGVlZjhlYjcwNDA2ODUyNWI1MTU2YzYzZWZiMWQ2YjEyZDU0YjcxYwotLS0gL2Rldi9udWxs CisrKyBiL1NvdXJjZS9jbWFrZS9GaW5kRGlyZWN0WC5jbWFrZQpAQCAtMCwwICsxLDMwIEBACisj IC0gRmluZCBEaXJlY3RYIFNESyBpbnN0YWxsYXRpb24NCisjIEZpbmQgdGhlIERpcmVjdFggaW5j bHVkZXMgYW5kIGxpYnJhcnkNCisjIFRoaXMgbW9kdWxlIGRlZmluZXMNCisjICBEaXJlY3RYX0lO Q0xVREVfRElSUywgd2hlcmUgdG8gZmluZCBkM2Q5LmgsIGV0Yy4NCisjICBEaXJlY3RYX0xJQlJB UklFUywgbGlicmFyaWVzIHRvIGxpbmsgYWdhaW5zdCB0byB1c2UgRGlyZWN0WC4NCisjICBEaXJl Y3RYX0ZPVU5ELCBJZiBmYWxzZSwgZG8gbm90IHRyeSB0byB1c2UgRGlyZWN0WC4NCisjICBEaXJl Y3RYX1JPT1RfRElSLCBkaXJlY3Rvcnkgd2hlcmUgRGlyZWN0WCB3YXMgaW5zdGFsbGVkLg0KKw0K K0ZJTkRfUEFUSChEaXJlY3RYX0lOQ0xVREVfRElSUyBkM2Q5LmggUEFUSFMNCisgICAgIiRFTlZ7 RFhTREtfRElSfS9JbmNsdWRlIg0KKyAgICAiQzovUHJvZ3JhbSBGaWxlcyAoeDg2KS9NaWNyb3Nv ZnQgRGlyZWN0WCBTREsqL0luY2x1ZGUiDQorICAgICJDOi9Qcm9ncmFtIEZpbGVzL01pY3Jvc29m dCBEaXJlY3RYIFNESyovSW5jbHVkZSINCispDQorDQorR0VUX0ZJTEVOQU1FX0NPTVBPTkVOVChE aXJlY3RYX1JPT1RfRElSICIke0RpcmVjdFhfSU5DTFVERV9ESVJTfS8uLiIgQUJTT0xVVEUpDQor DQorSUYgKENNQUtFX0NMXzY0KQ0KKyAgICBTRVQoRGlyZWN0WF9MSUJSQVJZX1BBVEhTICIke0Rp cmVjdFhfUk9PVF9ESVJ9L0xpYi94NjQiKQ0KK0VMU0UgKCkNCisgICAgU0VUKERpcmVjdFhfTElC UkFSWV9QQVRIUyAiJHtEaXJlY3RYX1JPT1RfRElSfS9MaWIveDg2IiAiJHtEaXJlY3RYX1JPT1Rf RElSfS9MaWIiKQ0KK0VORElGICgpDQorDQorRklORF9MSUJSQVJZKERpcmVjdFhfRDNEOV9MSUJS QVJZIGQzZDkgJHtEaXJlY3RYX0xJQlJBUllfUEFUSFN9IE5PX0RFRkFVTFRfUEFUSCkNCitGSU5E X0xJQlJBUlkoRGlyZWN0WF9EM0RYOV9MSUJSQVJZIGQzZHg5ICR7RGlyZWN0WF9MSUJSQVJZX1BB VEhTfSBOT19ERUZBVUxUX1BBVEgpDQorU0VUKERpcmVjdFhfTElCUkFSSUVTICR7RGlyZWN0WF9E M0Q5X0xJQlJBUll9ICR7RGlyZWN0WF9EM0RYOV9MSUJSQVJZfSkNCisNCisjIGhhbmRsZSB0aGUg UVVJRVRMWSBhbmQgUkVRVUlSRUQgYXJndW1lbnRzIGFuZCBzZXQgRGlyZWN0WF9GT1VORCB0byBU UlVFIGlmIGFsbCBsaXN0ZWQgdmFyaWFibGVzIGFyZSBUUlVFDQorSU5DTFVERShGaW5kUGFja2Fn ZUhhbmRsZVN0YW5kYXJkQXJncykNCitGSU5EX1BBQ0tBR0VfSEFORExFX1NUQU5EQVJEX0FSR1Mo RGlyZWN0WCBERUZBVUxUX01TRyBEaXJlY3RYX1JPT1RfRElSIERpcmVjdFhfTElCUkFSSUVTIERp cmVjdFhfSU5DTFVERV9ESVJTKQ0KK01BUktfQVNfQURWQU5DRUQoRGlyZWN0WF9JTkNMVURFX0RJ UlMgRGlyZWN0WF9EM0Q5X0xJQlJBUlkgRGlyZWN0WF9EM0RYOV9MSUJSQVJZKQ0KZGlmZiAtLWdp dCBhL0NoYW5nZUxvZyBiL0NoYW5nZUxvZwppbmRleCA2ZmY2M2Q1N2QzZTdlNGRmYzQ1ZWZjZDBj YTJiMDk2MzFmMDdkN2QxLi5lMDFlNjM1NmVhNjExZTJmNjM0OWU4NTdlZjcxNGFiOWZiMWQwOTgy IDEwMDY0NAotLS0gYS9DaGFuZ2VMb2cKKysrIGIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIgQEAK KzIwMTItMDItMjMgIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dhQHdlYmtpdC5vcmc+CisKKyAg ICAgICAgW0NNYWtlXSBBZGQgRmluZERpcmVjdFgKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTc5MzQ0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgKiBTb3VyY2UvY21ha2UvRmluZERpcmVjdFguY21ha2U6IEFk ZGVkLgorCiAyMDEyLTAyLTIyICBSYXBoYWVsIEt1Ym8gZGEgQ29zdGEgIDxrdWJvQHByb2Z1c2lv bi5tb2JpPgogCiAgICAgICAgIFtDTWFrZV0gUmVtb3ZlIEZpbmRHREsuY21ha2UgYW5kIEZpbmRH REstUGl4QnVmLmNtYWtlCg==