Bug 200475 – Fix inefficiency in HTTPHeaderMap::set(CFStringRef, const String&)

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Fix inefficiency in HTTPHeaderMap::set(CFStringRef, const String&).

[Chris Dumez]

Created attachment 375639 [details]
Patch

[Darin Adler]

Comment on attachment 375639 [details]
Patch

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

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:75
> +        if (findHTTPHeaderName(StringView(reinterpret_cast<const LChar*>(nameCharacters), length), headerName))
> +            set(headerName, value);
> +        else
> +            setUncommonHeader(String(nameCharacters, length), value);

Strikes me as strange that StringView requires the cast to LChar* and String does not.

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 375639 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375639&action=review
> 
> > Source/WebCore/platform/network/HTTPHeaderMap.cpp:75
> > +        if (findHTTPHeaderName(StringView(reinterpret_cast<const LChar*>(nameCharacters), length), headerName))
> > +            set(headerName, value);
> > +        else
> > +            setUncommonHeader(String(nameCharacters, length), value);
> 
> Strikes me as strange that StringView requires the cast to LChar* and String
> does not.

    StringView(const LChar*, unsigned length);
    StringView(const UChar*, unsigned length);
    StringView(const char*);

We're missing:
    StringView(const char*, unsigned length);

Should I add it?

[Darin Adler]

(In reply to Chris Dumez from comment #3)
>     StringView(const LChar*, unsigned length);
>     StringView(const UChar*, unsigned length);
>     StringView(const char*);
> 
> We're missing:
>     StringView(const char*, unsigned length);
> 
> Should I add it?

Yes.

If we already have the one without a length, then yes I think you should add it. The counter argument is that if we have a "const char*" with UTF-8 in it, we don’t want someone to accidentally interpret it as Latin-1. But I’m not sure that leaving this one particular overload out of StringView helps much with that problem.

[Chris Dumez]

Created attachment 375643 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375643 [details]
Patch

Clearing flags on attachment: 375643

Committed r248316: <https://trac.webkit.org/changeset/248316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54004727>