Bug 155967 - Remove forEach use from Fetch Headers builtin constructor
Summary: Remove forEach use from Fetch Headers builtin constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-29 03:04 PDT by youenn fablet
Modified: 2016-03-31 01:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (26.59 KB, patch)
2016-03-29 03:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (28.29 KB, patch)
2016-03-31 00:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-03-29 03:04:12 PDT
As a follow-up to bug 155923, we should remove forEach use in Headers built-in constructor.
Comment 1 youenn fablet 2016-03-29 03:07:29 PDT
Created attachment 275085 [details]
Patch
Comment 2 Joseph Pecoraro 2016-03-29 11:28:43 PDT
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);
Comment 3 youenn fablet 2016-03-30 12:48:58 PDT
(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.
Comment 4 youenn fablet 2016-03-31 00:29:29 PDT
Created attachment 275270 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2016-03-31 01:31:23 PDT
Comment on attachment 275270 [details]
Patch for landing

Clearing flags on attachment: 275270

Committed r198889: <http://trac.webkit.org/changeset/198889>
Comment 6 WebKit Commit Bot 2016-03-31 01:31:27 PDT
All reviewed patches have been landed.  Closing bug.