Bug 155967 – Remove forEach use from Fetch Headers builtin constructor

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

[youenn fablet]

As a follow-up to bug 155923, we should remove forEach use in Headers built-in constructor.

[youenn fablet]

Created attachment 275085 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 275085 [details]
Patch

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

r=me

> Source/WebCore/Modules/fetch/FetchHeaders.js:38
> +    if (headersInit instanceof this.constructor) {

Is this change allowing something that previously wouldn't have worked to work now? Does this affect subclasses? Maybe we should test this:

    let SpecialHeaders = class SpecialHeaders extends Headers {
        constructor() { console.log("Subclass!"); super(...arguments); }
    };

    let specialHeaders = new SpecialHeaders({a:1});
    let otherHeaders = new Headers(specialHeaders);
    shouldBeEqualToString(otherHeaders.get("a"), "1");

> LayoutTests/fetch/shadowing-forEach.html:14
> +new Headers({a:1});

Can you add a new test here covering what you just fixed. Namely, constructing Headers with a Headers param.

    let headers = new Headers({a:1});
    let headers2 = new Headers(headers);

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 275085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275085&action=review
> 
> r=me
> 
> > Source/WebCore/Modules/fetch/FetchHeaders.js:38
> > +    if (headersInit instanceof this.constructor) {
> 
> Is this change allowing something that previously wouldn't have worked to
> work now? Does this affect subclasses? Maybe we should test this:
>     let SpecialHeaders = class SpecialHeaders extends Headers {
>         constructor() { console.log("Subclass!"); super(...arguments); }
>     };
> 
>     let specialHeaders = new SpecialHeaders({a:1});
>     let otherHeaders = new Headers(specialHeaders);
>     shouldBeEqualToString(otherHeaders.get("a"), "1");

I was thinking so but the test below is working without this patch.
I am not exactly sure why.

Anyway, I think this change is an improvement.
It is a good idea to add this test, I will do it in the landing patch.

> 
> > LayoutTests/fetch/shadowing-forEach.html:14
> > +new Headers({a:1});
> 
> Can you add a new test here covering what you just fixed. Namely,
> constructing Headers with a Headers param.
> 
>     let headers = new Headers({a:1});
>     let headers2 = new Headers(headers);

Sure, I'll do that at landing time.

[youenn fablet]

Created attachment 275270 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 275270 [details]
Patch for landing

Clearing flags on attachment: 275270

Committed r198889: <http://trac.webkit.org/changeset/198889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.