119390 2013-08-01 04:59:01 -0700 Remove dependency on CoreFoundation from WebURLResponse 2013-08-07 00:18:39 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 1 paroga paroga andersca bfulgham commit-queue oldest_to_newest 913705 0 paroga 2013-08-01 04:59:01 -0700 Remove dependency on CoreFoundation from WebURLResponse 913706 1 207913 paroga 2013-08-01 05:00:10 -0700 Created attachment 207913 Patch 914165 2 207913 bfulgham 2013-08-02 08:53:54 -0700 Comment on attachment 207913 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207913&action=review This is really more a renaming than actually removing a dependency (I guess the CFIndex type definition might have forced a CoreFoundation reference). Looks fine to me. Was there a CoreFoundation include that could be removed here as well? > Source/WebKit/win/WebURLResponse.cpp:375 > + String statusText = localizedShortDescriptionForStatusCode(statusCode); Seems like this could be "const String& statusText" = ...; might allow return-value-optimization. 914183 3 207913 paroga 2013-08-02 09:53:00 -0700 Comment on attachment 207913 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207913&action=review >> Source/WebKit/win/WebURLResponse.cpp:375 >> + String statusText = localizedShortDescriptionForStatusCode(statusCode); > > Seems like this could be "const String& statusText" = ...; might allow return-value-optimization. but then i need to change the whole function implementation, do you think it makes sense for this function? imho it's not called so often... 914193 4 207913 bfulgham 2013-08-02 10:27:35 -0700 Comment on attachment 207913 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207913&action=review >>> Source/WebKit/win/WebURLResponse.cpp:375 >>> + String statusText = localizedShortDescriptionForStatusCode(statusCode); >> >> Seems like this could be "const String& statusText" = ...; might allow return-value-optimization. > > but then i need to change the whole function implementation, do you think it makes sense for this function? imho it's not called so often... What? Why? Does the BString constructor want a non-const String argument? 914230 5 paroga 2013-08-02 11:53:11 -0700 (In reply to comment #4) > (From update of attachment 207913 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207913&action=review > > >>> Source/WebKit/win/WebURLResponse.cpp:375 > >>> + String statusText = localizedShortDescriptionForStatusCode(statusCode); > >> > >> Seems like this could be "const String& statusText" = ...; might allow return-value-optimization. > > > > but then i need to change the whole function implementation, do you think it makes sense for this function? imho it's not called so often... > > What? Why? Does the BString constructor want a non-const String argument? AFAIK WEB_UI_STRING returns String by value and not as a const reference (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LocalizedStrings.h?rev=152117#L263), so we must return String by value. Maybe it's already too late for me today and I can't find my C++ knowledge about return values and object lifetime. :-) BString is good with a const reference. 914248 6 207913 bfulgham 2013-08-02 13:17:58 -0700 Comment on attachment 207913 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207913&action=review >>>>> Source/WebKit/win/WebURLResponse.cpp:375 >>>>> + String statusText = localizedShortDescriptionForStatusCode(statusCode); >>>> >>>> Seems like this could be "const String& statusText" = ...; might allow return-value-optimization. >>> >>> but then i need to change the whole function implementation, do you think it makes sense for this function? imho it's not called so often... >> >> What? Why? Does the BString constructor want a non-const String argument? > > AFAIK WEB_UI_STRING returns String by value and not as a const reference (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LocalizedStrings.h?rev=152117#L263), so we must return String by value. > Maybe it's already too late for me today and I can't find my C++ knowledge about return values and object lifetime. :-) > BString is good with a const reference. Right. But under C++03 and newer, when you bind a temporary return value to a const reference, the compiler extends the lifetime of the return value to the lifetime of the const reference. See http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/ for a brief overview. 914698 7 208149 paroga 2013-08-05 14:41:32 -0700 Created attachment 208149 Patch 914702 8 paroga 2013-08-05 14:48:26 -0700 (In reply to comment #6) > (From update of attachment 207913 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207913&action=review > > >>>>> Source/WebKit/win/WebURLResponse.cpp:375 > >>>>> + String statusText = localizedShortDescriptionForStatusCode(statusCode); > >>>> > >>>> Seems like this could be "const String& statusText" = ...; might allow return-value-optimization. > >>> > >>> but then i need to change the whole function implementation, do you think it makes sense for this function? imho it's not called so often... > >> > >> What? Why? Does the BString constructor want a non-const String argument? > > > > AFAIK WEB_UI_STRING returns String by value and not as a const reference (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/LocalizedStrings.h?rev=152117#L263), so we must return String by value. > > Maybe it's already too late for me today and I can't find my C++ knowledge about return values and object lifetime. :-) > > BString is good with a const reference. > > Right. But under C++03 and newer, when you bind a temporary return value to a const reference, the compiler extends the lifetime of the return value to the lifetime of the const reference. See http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/ for a brief overview. Sorry, I though about changing the return value to const ref... My fault! 915047 9 208149 commit-queue 2013-08-07 00:18:38 -0700 Comment on attachment 208149 Patch Clearing flags on attachment: 208149 Committed r153779: <http://trac.webkit.org/changeset/153779> 915048 10 commit-queue 2013-08-07 00:18:39 -0700 All reviewed patches have been landed. Closing bug. 207913 2013-08-01 05:00:10 -0700 2013-08-05 14:41:23 -0700 Patch bug-119390.patch text/plain 1573 paroga ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvd2luL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQv d2luL0NoYW5nZUxvZwppbmRleCAwNjhlMGJjLi5lNzkxMmEyIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L3dpbi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC93aW4vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTQgQEAKKzIwMTMtMDgtMDEgIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dhQHdl YmtpdC5vcmc+CisKKyAgICAgICAgUmVtb3ZlIGRlcGVuZGVuY3kgb24gQ29yZUZvdW5kYXRpb24g ZnJvbSBXZWJVUkxSZXNwb25zZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTE5MzkwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgKiBXZWJVUkxSZXNwb25zZS5jcHA6CisgICAgICAgIChsb2NhbGl6ZWRTaG9y dERlc2NyaXB0aW9uRm9yU3RhdHVzQ29kZSk6CisgICAgICAgIChXZWJVUkxSZXNwb25zZTo6bG9j YWxpemVkU3RyaW5nRm9yU3RhdHVzQ29kZSk6CisKIDIwMTMtMDctMjUgIEFuZHJlYXMgS2xpbmcg IDxha2xpbmdAYXBwbGUuY29tPgogCiAgICAgICAgIENocm9tZUNsaWVudDo6Zm9jdXNlZE5vZGVD aGFuZ2VkKCkgc2hvdWxkIGJlIGZvY3VzZWRFbGVtZW50Q2hhbmdlZCgpLgpkaWZmIC0tZ2l0IGEv U291cmNlL1dlYktpdC93aW4vV2ViVVJMUmVzcG9uc2UuY3BwIGIvU291cmNlL1dlYktpdC93aW4v V2ViVVJMUmVzcG9uc2UuY3BwCmluZGV4IGY4YWZhOWEuLjE0ZDA0M2YgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJLaXQvd2luL1dlYlVSTFJlc3BvbnNlLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L3dp bi9XZWJVUkxSZXNwb25zZS5jcHAKQEAgLTQ3LDcgKzQ3LDcgQEAKIAogdXNpbmcgbmFtZXNwYWNl IFdlYkNvcmU7CiAKLXN0YXRpYyBTdHJpbmcgQ0ZIVFRQTWVzc2FnZUNvcHlMb2NhbGl6ZWRTaG9y dERlc2NyaXB0aW9uRm9yU3RhdHVzQ29kZShDRkluZGV4IHN0YXR1c0NvZGUpCitzdGF0aWMgU3Ry aW5nIGxvY2FsaXplZFNob3J0RGVzY3JpcHRpb25Gb3JTdGF0dXNDb2RlKGludCBzdGF0dXNDb2Rl KQogewogICAgIFN0cmluZyByZXN1bHQ7CiAgICAgaWYgKHN0YXR1c0NvZGUgPCAxMDAgfHwgc3Rh dHVzQ29kZSA+PSA2MDApCkBAIC0zNzIsNyArMzcyLDcgQEAgSFJFU1VMVCBTVERNRVRIT0RDQUxM VFlQRSBXZWJVUkxSZXNwb25zZTo6bG9jYWxpemVkU3RyaW5nRm9yU3RhdHVzQ29kZSgKICAgICBB U1NFUlQobV9yZXNwb25zZS5pc0hUVFAoKSk7CiAgICAgaWYgKHN0YXR1c1N0cmluZykKICAgICAg ICAgKnN0YXR1c1N0cmluZyA9IDA7Ci0gICAgU3RyaW5nIHN0YXR1c1RleHQgPSBDRkhUVFBNZXNz YWdlQ29weUxvY2FsaXplZFNob3J0RGVzY3JpcHRpb25Gb3JTdGF0dXNDb2RlKHN0YXR1c0NvZGUp OworICAgIFN0cmluZyBzdGF0dXNUZXh0ID0gbG9jYWxpemVkU2hvcnREZXNjcmlwdGlvbkZvclN0 YXR1c0NvZGUoc3RhdHVzQ29kZSk7CiAgICAgaWYgKCFzdGF0dXNUZXh0KQogICAgICAgICByZXR1 cm4gRV9GQUlMOwogICAgIGlmIChzdGF0dXNTdHJpbmcpCg== 208149 2013-08-05 14:41:32 -0700 2013-08-07 00:18:38 -0700 Patch bug-119390-20130805234128.patch text/plain 1721 paroga U3VidmVyc2lvbiBSZXZpc2lvbjogMTUzNzI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L3dp bi9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0L3dpbi9DaGFuZ2VMb2cKaW5kZXggYTFmODhmYjVi Y2M3OTVhMTFjNWZmYjgzMDNiYmJhNDI5NzRhNzYzMS4uMzkwNGQ4ODM0NTczNWY4NWQ3ODI0MDlj Y2EzMDljOGFlODZiNzVmZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC93aW4vQ2hhbmdlTG9n CisrKyBiL1NvdXJjZS9XZWJLaXQvd2luL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEz LTA4LTA1ICBQYXRyaWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQub3JnPgorCisgICAgICAg IFJlbW92ZSBkZXBlbmRlbmN5IG9uIENvcmVGb3VuZGF0aW9uIGZyb20gV2ViVVJMUmVzcG9uc2UK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExOTM5MAor CisgICAgICAgIFJldmlld2VkIGJ5IEJyZW50IEZ1bGdoYW0uCisKKyAgICAgICAgKiBXZWJVUkxS ZXNwb25zZS5jcHA6CisgICAgICAgIChsb2NhbGl6ZWRTaG9ydERlc2NyaXB0aW9uRm9yU3RhdHVz Q29kZSk6CisgICAgICAgIChXZWJVUkxSZXNwb25zZTo6bG9jYWxpemVkU3RyaW5nRm9yU3RhdHVz Q29kZSk6CisKIDIwMTMtMDgtMDEgIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUBhcHBsZS5jb20+ CiAKICAgICAgICAgW1dpbmRvd3NdIFdlYktpdDEgRnVsbHNjcmVlbiBWaWRlbyBQbGF5IGlzIEJy b2tlbgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC93aW4vV2ViVVJMUmVzcG9uc2UuY3BwIGIv U291cmNlL1dlYktpdC93aW4vV2ViVVJMUmVzcG9uc2UuY3BwCmluZGV4IGY4YWZhOWEzZTFjMGRi Yjg0YWI4OWZmZjAyMzA3M2FjZGNmZWE5N2IuLmYwYzQzMDIyMDBlOTVhYjFmNTE3NmMzYjc0MDZj MWQxOWNmN2FkOWQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvd2luL1dlYlVSTFJlc3BvbnNl LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L3dpbi9XZWJVUkxSZXNwb25zZS5jcHAKQEAgLTQ3LDcg KzQ3LDcgQEAKIAogdXNpbmcgbmFtZXNwYWNlIFdlYkNvcmU7CiAKLXN0YXRpYyBTdHJpbmcgQ0ZI VFRQTWVzc2FnZUNvcHlMb2NhbGl6ZWRTaG9ydERlc2NyaXB0aW9uRm9yU3RhdHVzQ29kZShDRklu ZGV4IHN0YXR1c0NvZGUpCitzdGF0aWMgU3RyaW5nIGxvY2FsaXplZFNob3J0RGVzY3JpcHRpb25G b3JTdGF0dXNDb2RlKGludCBzdGF0dXNDb2RlKQogewogICAgIFN0cmluZyByZXN1bHQ7CiAgICAg aWYgKHN0YXR1c0NvZGUgPCAxMDAgfHwgc3RhdHVzQ29kZSA+PSA2MDApCkBAIC0zNzIsNyArMzcy LDcgQEAgSFJFU1VMVCBTVERNRVRIT0RDQUxMVFlQRSBXZWJVUkxSZXNwb25zZTo6bG9jYWxpemVk U3RyaW5nRm9yU3RhdHVzQ29kZSgKICAgICBBU1NFUlQobV9yZXNwb25zZS5pc0hUVFAoKSk7CiAg ICAgaWYgKHN0YXR1c1N0cmluZykKICAgICAgICAgKnN0YXR1c1N0cmluZyA9IDA7Ci0gICAgU3Ry aW5nIHN0YXR1c1RleHQgPSBDRkhUVFBNZXNzYWdlQ29weUxvY2FsaXplZFNob3J0RGVzY3JpcHRp b25Gb3JTdGF0dXNDb2RlKHN0YXR1c0NvZGUpOworICAgIGNvbnN0IFN0cmluZyYgc3RhdHVzVGV4 dCA9IGxvY2FsaXplZFNob3J0RGVzY3JpcHRpb25Gb3JTdGF0dXNDb2RlKHN0YXR1c0NvZGUpOwog ICAgIGlmICghc3RhdHVzVGV4dCkKICAgICAgICAgcmV0dXJuIEVfRkFJTDsKICAgICBpZiAoc3Rh dHVzU3RyaW5nKQo=