Bug 100580 – buildHTTPHeaders() should use a StringBuilder instead of a Vector<UChar>

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Michael Saboff]

In Plugins/PluginView.cpp, the function static String buildHTTPHeaders() uses a Vector<UChar> to construct HTTP headers.  In should be changed to use a StringBuilder.

[Michael Saboff]

Created attachment 171063 [details]
Patch

[Eric Carlson]

Comment on attachment 171063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171063&action=review

> Source/WebKit2/ChangeLog:4
> +        WKStringCopyCFString() should directly use 8 bit Strings data instead of up converting
> +        https://bugs.webkit.org/show_bug.cgi?id=100579

This is the wrong bug title and number.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:167
>      String separator(": ");

Is there a benefit to creating this String rather than just appending the characters directly - stringBuilder.append(": ", 2) ?

[Michael Saboff]

Created attachment 171081 [details]
Fixed bug title and URL in ChangeLog - Rest patch unchanged

(In reply to comment #2)
> (From update of attachment 171063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171063&action=review
> 
> > Source/WebKit2/ChangeLog:4
> > +        WKStringCopyCFString() should directly use 8 bit Strings data instead of up converting
> > +        https://bugs.webkit.org/show_bug.cgi?id=100579
> 
> This is the wrong bug title and number.

Indeed!  Fixed this.

> > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:167
> >      String separator(": ");
> 
> Is there a benefit to creating this String rather than just appending the characters directly - stringBuilder.append(": ", 2) ?

I'm inclined to keep separator as a named string variable.  The append(": ", 2) construct doesn't say what it is for and could be prone to errors if either the string or length was changed without changing the other.  This is a weak argument and I could be convinced the other way.

[WebKit Review Bot]

Comment on attachment 171081 [details]
Fixed bug title and URL in ChangeLog - Rest patch unchanged

Clearing flags on attachment: 171081

Committed r132734: <http://trac.webkit.org/changeset/132734>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 171063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171063&action=review

>>> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:167
>>>      String separator(": ");
>> 
>> Is there a benefit to creating this String rather than just appending the characters directly - stringBuilder.append(": ", 2) ?
> 
> I'm inclined to keep separator as a named string variable.  The append(": ", 2) construct doesn't say what it is for and could be prone to errors if either the string or length was changed without changing the other.  This is a weak argument and I could be convinced the other way.

stringBuilder.appendLiteral(": ") is what we should use