Bug 100580 - buildHTTPHeaders() should use a StringBuilder instead of a Vector<UChar>
Summary: buildHTTPHeaders() should use a StringBuilder instead of a Vector<UChar>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-26 16:47 PDT by Michael Saboff
Modified: 2012-10-27 14:00 PDT (History)
1 user (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2012-10-26 17:44 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Fixed bug title and URL in ChangeLog - Rest patch unchanged (2.72 KB, patch)
2012-10-27 00:02 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2012-10-26 16:47:14 PDT
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.
Comment 1 Michael Saboff 2012-10-26 17:44:57 PDT
Created attachment 171063 [details]
Patch
Comment 2 Eric Carlson 2012-10-26 19:30:54 PDT
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) ?
Comment 3 Michael Saboff 2012-10-27 00:02:33 PDT
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.
Comment 4 WebKit Review Bot 2012-10-27 11:17:46 PDT
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>
Comment 5 WebKit Review Bot 2012-10-27 11:17:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2012-10-27 14:00:52 PDT
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