Bug 119390 – Remove dependency on CoreFoundation from WebURLResponse

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Patrick R. Gansterer]

Remove dependency on CoreFoundation from WebURLResponse

[Patrick R. Gansterer]

Created attachment 207913 [details]
Patch

[Brent Fulgham]

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.

[Patrick R. Gansterer]

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...

[Brent Fulgham]

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?

[Patrick R. Gansterer]

(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.

[Brent Fulgham]

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.

[Patrick R. Gansterer]

Created attachment 208149 [details]
Patch

[Patrick R. Gansterer]

(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!

[WebKit Commit Bot]

Comment on attachment 208149 [details]
Patch

Clearing flags on attachment: 208149

Committed r153779: <http://trac.webkit.org/changeset/153779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.