WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
155967
Remove forEach use from Fetch Headers builtin constructor
https://bugs.webkit.org/show_bug.cgi?id=155967
Summary
Remove forEach use from Fetch Headers builtin constructor
youenn fablet
Reported
2016-03-29 03:04:12 PDT
As a follow-up to
bug 155923
, we should remove forEach use in Headers built-in constructor.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2016-03-29 03:07:29 PDT
Created
attachment 275085
[details]
Patch
Joseph Pecoraro
Comment 2
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);
youenn fablet
Comment 3
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.
youenn fablet
Comment 4
2016-03-31 00:29:29 PDT
Created
attachment 275270
[details]
Patch for landing
WebKit Commit Bot
Comment 5
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
>
WebKit Commit Bot
Comment 6
2016-03-31 01:31:27 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug