118055 2013-06-26 05:22:48 -0700 [GTK] [WK2] Check value of WEBKIT_INJECTED_BUNDLE_PATH 2013-06-27 01:49:52 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 berto webkit-unassigned cgarcia commit-queue gustavo mrobinson oldest_to_newest 903648 0 berto 2013-06-26 05:22:48 -0700 If the WEBKIT_INJECTED_BUNDLE_PATH if exists, then the injected bundle path is taken from there and the system directory is completely ignored. This is a problem if the custom directory does not exist. The MiniBrowser for example has WEBKIT_INJECTED_BUNDLE_PATH hardcoded to $(top_builddir)/.libs. This means that it cannot be distributed since it won't work outside the build tree (unless we manually override that variable). Although maybe being distributable is not the purpose of the MiniBrowser, there's also no reason not to make it work if webkit is properly installed in the system. One solution is to fall back to the system directory if the other one does not exist, which is also what we do with the web inspector in WebInspectorProxyGtk.cpp. 903650 1 205477 berto 2013-06-26 05:31:49 -0700 Created attachment 205477 Patch 903651 2 commit-queue 2013-06-26 05:33:03 -0700 Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API 903683 3 205477 cgarcia 2013-06-26 06:34:19 -0700 Comment on attachment 205477 Patch I think providing an invalid directory in WEBKIT_INJECTED_BUNDLE_PATH is an error of the one who set the variable. This env var is only useful for internal tests and tools, users of the WebKit2GTK+ API should use web extensions instead. So, I'm not sure it's a good idea to add a blocking I/O call here. On the other hand, the check is only done when the env var contains something (this shouldn't happen in most of the cases), so I guess it's harmless. I wonder whether we should return NULL and/or print an error in stderr in that case instead of falling back to system directory. It can be very confusing if I make a typographic error and the installed injected bundle is used silently. What do you think? 904098 4 berto 2013-06-27 01:09:06 -0700 (In reply to comment #3) > I think providing an invalid directory in > WEBKIT_INJECTED_BUNDLE_PATH is an error of the one who set the > variable. I was wondering how to make MiniBrowser work outside the build tree. I know it's just an internal tool, but I guess it still can be useful (of course if a distributor wants to ship it they can easily remove the hardcoded WEBKIT_INJECTED_BUNDLE_PATH value). It's worth noting that the behavior in the case of WEBKIT_INSPECTOR_PATH is different (we silently fall back to the system directory if the other one doesn't exist). So I'd say we should make them the same for the sake of consistency at least. I don't have a strong opinion in this case since it's really not so important, but printing and message to stderr sounds good to me. 904103 5 205477 commit-queue 2013-06-27 01:49:50 -0700 Comment on attachment 205477 Patch Clearing flags on attachment: 205477 Committed r152084: <http://trac.webkit.org/changeset/152084> 904104 6 commit-queue 2013-06-27 01:49:52 -0700 All reviewed patches have been landed. Closing bug. 205477 2013-06-26 05:31:49 -0700 2013-06-27 01:49:50 -0700 Patch 118055.diff text/plain 1599 berto ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA4YTQ2NjAzLi5hNTQ4NWVmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTMtMDYtMjYgIEFsYmVydG8gR2FyY2lhICA8YWdhcmNpYUBpZ2FsaWEuY29tPgorCisg ICAgICAgIFtHVEtdIFtXSzJdIENoZWNrIHZhbHVlIG9mIFdFQktJVF9JTkpFQ1RFRF9CVU5ETEVf UEFUSAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE4 MDU1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSWYg dGhlIGRpcmVjdG9yeSBwb2ludGVkIGF0IGJ5IFdFQktJVF9JTkpFQ1RFRF9CVU5ETEVfUEFUSCBk b2VzCisgICAgICAgIG5vdCBleGlzdCwgZmFsbCBiYWNrIHRvIHRoZSBzeXN0ZW0gZGlyZWN0b3J5 LgorCisgICAgICAgIFRoaXMgbWFrZXMgTWluaUJyb3dzZXIgd29yayBvdXRzaWRlIHRoZSBidWls ZCB0cmVlLgorCisgICAgICAgICogVUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0V2ViQ29udGV4dC5j cHA6CisgICAgICAgIChpbmplY3RlZEJ1bmRsZURpcmVjdG9yeSk6CisKIDIwMTMtMDYtMjQgIFNp bW9uIENvb3BlciAgPHNjb29wZXJAYXBwbGUuY29tPgogCiAgICAgICAgIFNpbHZlcmxpZ2h0IHRy aWdnZXJzIHNhbmRib3ggdmlvbGF0aW9uczogZGVueSBuZXR3b3JrLWJpbmQgMC4wLjAuMDowCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRXZWJDb250 ZXh0LmNwcCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYkNvbnRl eHQuY3BwCmluZGV4IDhiYTgzYzQuLjMwNjM5NDQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYkNvbnRleHQuY3BwCisrKyBiL1NvdXJjZS9XZWJL aXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFdlYkNvbnRleHQuY3BwCkBAIC0xNzQsNyArMTc0 LDggQEAgc3RhdGljIHZvaWQgd2Via2l0X3dlYl9jb250ZXh0X2NsYXNzX2luaXQoV2ViS2l0V2Vi Q29udGV4dENsYXNzKiB3ZWJDb250ZXh0Q2xhc3MKIAogc3RhdGljIENTdHJpbmcgaW5qZWN0ZWRC dW5kbGVEaXJlY3RvcnkoKQogewotICAgIGlmIChjb25zdCBjaGFyKiBidW5kbGVEaXJlY3Rvcnkg PSBnX2dldGVudigiV0VCS0lUX0lOSkVDVEVEX0JVTkRMRV9QQVRIIikpCisgICAgY29uc3QgY2hh ciogYnVuZGxlRGlyZWN0b3J5ID0gZ19nZXRlbnYoIldFQktJVF9JTkpFQ1RFRF9CVU5ETEVfUEFU SCIpOworICAgIGlmIChidW5kbGVEaXJlY3RvcnkgJiYgZ19maWxlX3Rlc3QoYnVuZGxlRGlyZWN0 b3J5LCBHX0ZJTEVfVEVTVF9JU19ESVIpKQogICAgICAgICByZXR1cm4gYnVuZGxlRGlyZWN0b3J5 OwogCiAgICAgc3RhdGljIGNvbnN0IGNoYXIqIGluamVjdGVkQnVuZGxlUGF0aCA9IExJQkRJUiBH X0RJUl9TRVBBUkFUT1JfUyAid2Via2l0Mmd0ay0iIFdFQktJVEdUS19BUElfVkVSU0lPTl9TVFJJ TkcK