In preparation for a new XMLHttpRequest implementation based on using the fetch engine (as per the XHR spec, see bug 1449613), there are several issues with our Fetch implementation's header support which need to be addressed: - header names are considered case-insensitive (when calling Set/etc) - the Combine method from the spec is missing - the Get method does not return a VoidString when the passed-in header name is invalid This bug will address these issues in separate patches.

A try run is fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=541f9219eea52736d372c942d0f47e21f0ab4047 Anne, I thought I'd fold you into the reviews here just in case. I don't think there's anything I can do to test these specific details using WPTs (they're internal), but they *do* make a difference when trying to make an XHR implementation which follows the spec-text and uses fetch features, as I'm doing in bug 1449613.

Comment on attachment 8963988 [details] Bug 1450168 part 1 - Fix fetch InternalHeaders implementation to internally match spec in how it treats case-sensitivity. https://reviewboard.mozilla.org/r/232810/#review238436 I want to see tests for all of these. Plus I suspect you can get rid of lowerName variables. ::: dom/fetch/InternalHeaders.cpp:91 (Diff revision 1) > > SetListDirty(); > > // remove in reverse order to minimize copying > for (int32_t i = mList.Length() - 1; i >= 0; --i) { > - if (lowerName == mList[i].mName) { > + if (mList[i].mName.EqualsIgnoreCase(lowerName.get())) { Can you pass aName here instead of lowerName? ::: dom/fetch/InternalHeaders.cpp:111 (Diff revision 1) > > const char* delimiter = ", "; > bool firstValueFound = false; > > for (uint32_t i = 0; i < mList.Length(); ++i) { > - if (lowerName == mList[i].mName) { > + if (mList[i].mName.EqualsIgnoreCase(lowerName.get())) { same here ::: dom/fetch/InternalHeaders.cpp:137 (Diff revision 1) > if (IsInvalidName(lowerName, aRv)) { > return; > } > > for (uint32_t i = 0; i < mList.Length(); ++i) { > - if (lowerName == mList[i].mName) { > + if (mList[i].mName.EqualsIgnoreCase(lowerName.get())) { and here. ::: dom/fetch/InternalHeaders.cpp:158 (Diff revision 1) > if (IsInvalidName(lowerName, aRv)) { > return false; > } > > for (uint32_t i = 0; i < mList.Length(); ++i) { > - if (lowerName == mList[i].mName) { > + if (mList[i].mName.EqualsIgnoreCase(lowerName.get())) { here as well.

Comment on attachment 8963989 [details] Bug 1450168 part 2 - Add missing internal spec method Combine to InternalHeaders implementation. https://reviewboard.mozilla.org/r/232812/#review238440 This method is not used. Why adding it? We already have a similar code in ::Get and in ::MaybeSortList

Comment on attachment 8963990 [details] Bug 1450168 part 3 - Set the return value of Get/GetFirst to a VoidString when the passed-in name is invalid. https://reviewboard.mozilla.org/r/232814/#review238442 ::: dom/fetch/InternalHeaders.cpp:130 (Diff revision 1) > { > nsAutoCString lowerName; > ToLowerCase(aName, lowerName); > > if (IsInvalidName(lowerName, aRv)) { > + aValue.SetIsVoid(true); IsInvalidName() throws an exception. JS code will not be able to use aValue.

Comment on attachment 8963990 [details] Bug 1450168 part 3 - Set the return value of Get/GetFirst to a VoidString when the passed-in name is invalid. https://reviewboard.mozilla.org/r/232814/#review238444

Comment on attachment 8963988 [details] Bug 1450168 part 1 - Fix fetch InternalHeaders implementation to internally match spec in how it treats case-sensitivity. https://reviewboard.mozilla.org/r/232810/#review238436 I don't think I can do that with the available JS APIs. While I *can* engineer the InternalHeaders state for such a test using Headers.set/append, the Headers.keys/entries methods always normalize the header names to lowercase, obscuring the internal state. However, I can't tell if that lowercasing is the "correct" behavior; the spec doesn't explicitly mention lowercasing for the iterators, though it's the way Chrome and Firefox currently work. If it's incorrect and we should return case-sensitive keys/header names, then we could alter the behavior and I can write these tests. Another option would be to just add this patch in or after my other patch which adds the XMLHttpRequestWeb class, since without this change that XHR class will fail some of its own headers WPTs.

Comment on attachment 8963989 [details] Bug 1450168 part 2 - Add missing internal spec method Combine to InternalHeaders implementation. https://reviewboard.mozilla.org/r/232812/#review238440 It will be, by my XHR reimplementation. The XHR spec calls this internal method which is given in the Fetch spec, so I opted to implement it to keep things as close to the spec-text as possible. Perhaps it would be better to fold this patch into my one which adds the XMLHttpRequestWeb class instead, so it's more obvious why it is being added?

Comment on attachment 8963990 [details] Bug 1450168 part 3 - Set the return value of Get/GetFirst to a VoidString when the passed-in name is invalid. https://reviewboard.mozilla.org/r/232814/#review238442 > IsInvalidName() throws an exception. JS code will not be able to use aValue. Good point. I will drop this patch and just have my new XHR check the error result instead.

Comment on attachment 8963988 [details] Bug 1450168 part 1 - Fix fetch InternalHeaders implementation to internally match spec in how it treats case-sensitivity. https://reviewboard.mozilla.org/r/232810/#review238436 > Can you pass aName here instead of lowerName? I don't believe so, as they're differing types (aName is nsACstring, mName is nsCString). Is there another method that compares ACString and CString directly, but case-insensitively? Otherwise, is this an acceptable alternative? nsAutoCString name(aName); mList[i].mName.EqualsIgnoreCase(name.get());

Comment on attachment 8963988 [details] Bug 1450168 part 1 - Fix fetch InternalHeaders implementation to internally match spec in how it treats case-sensitivity. https://reviewboard.mozilla.org/r/232810/#review238436 Actually, the headers-basic fetch WPTs are testing that the iterators return lowercased header names, which makes it seem intentional per-spec.

Comment on attachment 8963988 [details] Bug 1450168 part 1 - Fix fetch InternalHeaders implementation to internally match spec in how it treats case-sensitivity. https://reviewboard.mozilla.org/r/232810/#review238760 ::: dom/fetch/InternalHeaders.cpp:60 (Diff revision 1) > nsAutoCString lowerName; > ToLowerCase(aName, lowerName); > nsAutoCString trimValue; > NS_TrimHTTPWhitespace(aValue, trimValue); > > - if (IsInvalidMutableHeader(lowerName, trimValue, aRv)) { > + if (IsInvalidMutableHeader(aName, trimValue, aRv)) { You don't change how you call this method elsewhere and I don't directly see an update to this method definition that ensures it deals with uppercase header names.

Comment on attachment 8963989 [details] Bug 1450168 part 2 - Add missing internal spec method Combine to InternalHeaders implementation. https://reviewboard.mozilla.org/r/232812/#review238762 I should probably not review the C++. ::: dom/fetch/InternalHeaders.cpp:86 (Diff revision 1) > + nsAutoCString lowerName; > + ToLowerCase(aName, lowerName); > + nsAutoCString trimValue; > + NS_TrimHTTPWhitespace(aValue, trimValue); > + > + if (IsInvalidMutableHeader(aName, trimValue, aRv)) { Here is that method call again :-)