Remove dependency on CoreFoundation from WebURLResponse
Created attachment 207913 [details] Patch
Comment on attachment 207913 [details] 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.
Comment on attachment 207913 [details] 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...
Comment on attachment 207913 [details] 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?
(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.
Comment on attachment 207913 [details] 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.
Created attachment 208149 [details] Patch
(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!
Comment on attachment 208149 [details] Patch Clearing flags on attachment: 208149 Committed r153779: <http://trac.webkit.org/changeset/153779>
All reviewed patches have been landed. Closing bug.