Bug 192288 – Merge parseAccessControlExposeHeadersAllowList into parseAccessControlAllowList

• Component: New Bugs
• Status: RESOLVED FIXED

[Rob Buis]

Merge parseAccessControlExposeHeadersAllowList into parseAccessControlAllowList as they do the same thing.

[Rob Buis]

Created attachment 356339 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 356339 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.h:-167
> -    return set;

parseAccessControlAllowList + addToAccessControlAllowList are less readable than parseAccessControlExposeHeadersAllowList as they try to avoid creating unnecessary substrings. I think you can rewrite this in a more concise way using StringViews:

String::split(UChar separator, const SplitFunctor& functor)
StringView::stripLeadingAndTrailingMatchedCharacters
StringView::isEmpty()

[WebKit Commit Bot]

Comment on attachment 356339 [details]
Patch

Clearing flags on attachment: 356339

Committed r238953: <https://trac.webkit.org/changeset/238953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46557902>

[Darin Adler]

Comment on attachment 356339 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.h:152
> -std::optional<HashSet<String, HashType>> parseAccessControlAllowList(const String& string)
> +void parseAccessControlAllowList(const String& string, HashSet<String, HashType>& set)

Why is this using a reference argument instead of a return value?

[Rob Buis]

(In reply to Darin Adler from comment #6)
> Comment on attachment 356339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356339&action=review
> 
> > Source/WebCore/platform/network/HTTPParsers.h:152
> > -std::optional<HashSet<String, HashType>> parseAccessControlAllowList(const String& string)
> > +void parseAccessControlAllowList(const String& string, HashSet<String, HashType>& set)
> 
> Why is this using a reference argument instead of a return value?

It seemed more convenient and natural to use out parameters here. Are you thinking return value is more efficient here because then we can skip the clear()s? I can do a follow up patch if so...

[Darin Adler]

I think return values are more natural.

We used out parameters for large things in the old days because we feared the cost of copying structures, but in modern C++ with the return value optimization and move semantics it seems we can use return values.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 356980 [details]
Patch

[Rob Buis]

(In reply to Darin Adler from comment #8)
> I think return values are more natural.
> 
> We used out parameters for large things in the old days because we feared
> the cost of copying structures, but in modern C++ with the return value
> optimization and move semantics it seems we can use return values.

True, I put up a patch for this.

[WebKit Commit Bot]

Comment on attachment 356980 [details]
Patch

Clearing flags on attachment: 356980

Committed r239372: <https://trac.webkit.org/changeset/239372>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.