Closed Bug 1278275 Opened 8 years ago Closed 8 years ago

Headers.get should incorporate getAll behaviour

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jdm, Assigned: saurabhs, Mentored)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [lang=c++] btpp-backlog)

Attachments

(1 file, 6 obsolete files)

It appears our implementation of the Headers interface is out of date with the specification. We have an extra getAll method which isn't present upstream, and our get method returns the first header that matches, rather than the combined value of all matching headers.

Specification:
https://fetch.spec.whatwg.org/#dom-headers-get

Code: dom/webidl/Headers.webidl, dom/fetch/Headers.h, dom/fetch/InternalHeaders.cpp

Tests:
./mach mochitest dom/tests/mochitest/fetch/
* this directory contains tests that will need to be updated
./mach web-platform testing/web-platform/fetch/api/headers/
* these tests should show unexpected results after changing the code; the results will need to be updated in testing/web-platform/meta/fetch/api/headers/
Summary: Headers.get should incorporate getAll behaviour to match spec → Headers.get should incorporate getAll behaviour
Mentor: josh
Whiteboard: [lang=c++] → [lang=c++] btpp-backlog
Hi, I would like to tackle this bug. Need some info about what the Headers interface is all about. Also, how can I get the source code?

Thanks.
You'll need to follow the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build for getting the code and compiling it. https://developer.mozilla.org/en-US/docs/Web/API/Headers should be able to explain what you want to know about the Headers interface.
Thanks Josh. I will start now.
Hi Josh, So are you planning to completely remove GetAll function because I tried doing that and it creates lots of other issues so I was thinking that we could keep the GetAll function and just change get definition, which says all the name values must be separated by commas.
Flags: needinfo?(josh)
Attached patch patch.diff (obsolete) — Splinter Review
This patch fixes the functionality of get Function, now we are appending all the fetched values to result.
Attachment #8773840 - Flags: review?(josh)
I am already working on this issue and I have got the code to compile after removing the GetAll method everywhere it is accessed. Tests still need to be modified.
Comment on attachment 8773840 [details] [diff] [review]
patch.diff

I'm clearing review on this for the time being, since Saurabh was working on this already.
Flags: needinfo?(josh)
Attachment #8773840 - Flags: review?(josh)
(In reply to Josh Matthews [:jdm] from comment #7)
> Comment on attachment 8773840 [details] [diff] [review]
> patch.diff
> 
> I'm clearing review on this for the time being, since Saurabh was working on
> this already.

yeah sure I submitted a patch as I saw no activity on this bug from long time. As Saurabh he is working on it still I could delete my patch.
Hi Josh, on running
./mach mochitest dom/tests/mochitest/fetch/test_fetch_app_protocol.html

I am getting TypeError: navigator.mozApps is undefined

I tried reading up on mozApps but couldn't find anything related to the problem. How can I solve this issue? Also, how is navigator.mozApps defined for the test environment?
You can ignore that failures - it is automatically skipped in non-Firefox OS build environments (since navigator.mozApps is a Firefox OS API). Update the test as best you can, and we'll run it against our CI system to ensure that it passes.
Oh, and in the sharing how I knew this, I looked at mochitest.ini in the same directory and read the `skip-if` conditions for that particular test file.
Attached patch HeadersgetAll.patch (obsolete) — Splinter Review
Attachment #8780790 - Flags: review?(josh)
Comment on attachment 8780790 [details] [diff] [review]
HeadersgetAll.patch

Review of attachment 8780790 [details] [diff] [review]:
-----------------------------------------------------------------

Great work! I'd like to see a new patch that includes the API suggestions I've made; I think it will reduce the number of changes that are required. Please also add a commit messages like "Bug 1278275 - Update Headers::GetAll and Headers::Get to match the specification. r=jdm".

::: dom/cache/AutoUtils.cpp
@@ +263,5 @@
>  
>          ErrorResult headerRv;
>          nsAutoCString value;
> +        nsTArray<nsCString> values;
> +        requestHeaders->GetAll(header, values, headerRv);

Let's add a GetFirst C++ API so we don't duplicate this pattern in many places. That should let us get rid of GetAll, too.

::: dom/fetch/FetchDriver.cpp
@@ +339,5 @@
>      mRequest->GetBody(getter_AddRefs(bodyStream));
>      if (bodyStream) {
>        nsAutoCString method;
>        mRequest->GetMethod(method);
> +      nsAutoCString aValue;

The `a` prefix is used for arguments; this should be called contentTypeValue.

::: dom/fetch/InternalHeaders.cpp
@@ +86,5 @@
>    if (IsInvalidName(lowerName, aRv)) {
>      return;
>    }
>  
> +  const char* delimiter = ", ";

The specification says there should be no space after ,: https://fetch.spec.whatwg.org/#concept-header-value-combined

@@ +93,3 @@
>    for (uint32_t i = 0; i < mList.Length(); ++i) {
>      if (lowerName == mList[i].mName) {
> +      if( firstValueFound ) {

nit: use `if (firstValueFound) {`

@@ +100,5 @@
>      }
>    }
>  
>    // No value found, so return null to content
> +  if(aValue.IsEmpty()) {

nit: space after if

@@ +363,4 @@
>    MOZ_ASSERT(!result.Failed());
>  
>    AutoTArray<nsCString, 5> exposeNamesArray;
> +  nsAutoCString aValue;

This should be called exposedNamesValue.

::: testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini
@@ +5,3 @@
>  
>    [Check append methods when called with already used name]
> +    expected: PASS 

This file can be removed now; the default state is pass for all tests if no ini file is present.
Attachment #8780790 - Flags: review?(josh) → feedback+
Assignee: nobody → singhal.saurabh.mnnit
Thanks for the review Josh. I will fix the outstanding issues soon.
(In reply to Josh Matthews [:jdm] (away until 9/3) from comment #13)
> Let's add a GetFirst C++ API so we don't duplicate this pattern in many
> places. That should let us get rid of GetAll, too.

This piece of code uses GetAll:
dom/cache/AutoUtils.cpp:
    AutoTArray<nsCString, 16> varyHeaders;
    ErrorResult rv;
    cachedResponseHeaders->GetAll(NS_LITERAL_CSTRING("vary"), varyHeaders, rv);
    MOZ_ALWAYS_TRUE(!rv.Failed());

    // Assume the vary headers match until we find a conflict
    bool varyHeadersMatch = true;

    for (uint32_t j = 0; j < varyHeaders.Length(); ++j) {

I don't think we can get rid of GetAll.
Attached patch HeadersgetAll.patch (obsolete) — Splinter Review
Addressing review comments from jdm@
Attachment #8780790 - Attachment is obsolete: true
Attachment #8786799 - Flags: review?(josh)
Hey Josh, is there anything else that needs to be fixed in the latest patch?
Comment on attachment 8786799 [details] [diff] [review]
HeadersgetAll.patch

Review of attachment 8786799 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Sorry about the delay; I was on vacation last week and Monday was a Canadian holiday.
Attachment #8786799 - Flags: review?(josh) → review+
Saurabh, do you have access to the tryserver to run the automated testsuite against these changes? If not, please follow the steps at https://www.mozilla.org/hacking/committer/ and I'll vouch for you.
Flags: needinfo?(singhal.saurabh.mnnit)
I don't know about tryserver. I will follow the link you have given.
Flags: needinfo?(singhal.saurabh.mnnit)
I have filed BUG1301685 for commit access to the tryserver.
Pushed the changes to the try server: ./mach try -b d -p all -u saurabh dom/tests/ testing/web-platform/tests/dom/
Unfortunately, `./mach try` doesn't understand directories like that, I believe. We ended up only testing subsets of a bunch of different test suites. Let's use the following syntax instead (obtained from selecting all the mochitest-N, e10s-mochitest-N, web-platform-test-N, and e10s-web-platform-test-N entries):
./mach try -b do -p linux,linux64,macosx64,win32,win64 -u mochitest-1,mochitest-2,mochitest-3,mochitest-4,mochitest-5,mochitest-e10s-1,mochitest-e10s-2,mochitest-e10s-3,mochitest-e10s-4,mochitest-e10s-5,web-platform-tests-1,web-platform-tests-2,web-platform-tests-3,web-platform-tests-4,web-platform-tests-5,web-platform-tests-6,web-platform-tests-7,web-platform-tests-8,web-platform-tests-e10s-1,web-platform-tests-e10s-2,web-platform-tests-e10s-3,web-platform-tests-e10s-4,web-platform-tests-e10s-5,web-platform-tests-e10s-6,web-platform-tests-e10s-7,web-platform-tests-e10s-8 -t none
Flags: needinfo?(singhal.saurabh.mnnit)
For the record, I used http://trychooser.pub.build.mozilla.org/ to create that syntax.
Thanks Josh. I scheduled the tests again. But, it looks like this time my changes were not pushed. The command says this:

remote: added 1 changesets with 0 changes to 0 files (+1 heads)

How can I push my changes?
Flags: needinfo?(singhal.saurabh.mnnit) → needinfo?(josh)
Was there a link to treeherder as part of the output? I am not surprised by the "1 changesets with 0 changes", since no code changed since your previous push - only one extra commit with the trychooser syntax as the commit message.
Flags: needinfo?(josh)
Oh ok. Yes that is the one. I will look at the results.
Hi Josh. I was debugging this failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=27719108&filter-searchStr=e7d624b19ab4d63b029b64af1514107b91eee896

I am unable to understand what the test is doing. Can you give me some pointers?
Flags: needinfo?(josh)
Oh, wow. I read your patch over and over, and read the test over and over, and I had to step through the code in a debugger to figure out what was happening. test_fetch_event.html tests various interactions with a service worker which intercepts network requests and provides responses. That means that the XHR to "empty-header" is intercepted in fetch_event_worker.js, which then calls Headers.get looking for the header named "emptyheader" that was originally set on the XHR. Previously, this would return the empty string because Headers.get used to return the first value that matched the requested header name. Now, we return null because Headers.get checks whether the final combined header value is empty. We need to modify the check in Headers.get to only return null if !firstValueFound. Does that make sense?
Flags: needinfo?(josh)
Thanks Josh for debugging this. Your explanation makes sense.
 How did you attach a debugger to the test? This is not working for me: sudo gdb ./mach mochitest dom/workers/test/serviceworkers/test_fetch_event.html
Flags: needinfo?(josh)
Next I was looking at this failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=27717335&filter-searchStr=Linux%20debug%20W3C%20Web%20Platform%20Tests%20e10s%20W3C%20Web%20Platform%20Tests%20e10s%20W-e10s(4)

When I run the test on my system, it skips the test instead of running it. Is it because the application cache feature is deprecated?
You can use `./mach mochitest dom/workers/test/serviceworkers/test_fetch_event.html --debugger=gdb` to attach to the parent process, or `MOZ_DEBUG_CHILD_PROCESS=1 ./mach mochitest dom/workers/test/serviceworkers/test_fetch_event.html` and then attach gdb manually to the process id that is printed on stdout if you want to attach to the child process. On the other hand, I used `--disble-e10s --debugger=gdb` so that there was only one process that needed debugging.
Flags: needinfo?(josh)
According to https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html.ini that test is skipped on OS X. You could remove that line in order to run it locally, but given that bug 1295445 exists and the test doesn't use the Headers API, I believe you can ignore that test failure.
I am facing a weird issue. After I submitted the patch to bugzilla, I had to move to a different laptop where I did hg clone and all the stuff again. Also, I took this patch and applied it in the latest mozilla-central repository but testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini file was not there already so the part of the patch where the file was deleted did not apply properly. This caused a test failure here : https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=27721630&filter-searchStr=Windows%207%20VM%20opt%20W3C%20Web%20Platform%20Tests%20W3C%20Web%20Platform%20Tests%20W(2)
Notice the test unexpected pass. How do I resolve this issue?
(In reply to Saurabh Singhal [:saurabh] from comment #39)
> This job reports memleak :
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=2
> 7717055&filter-searchStr=OS%20X%2010.
> 10%20debug%20Mochitest%20Mochitest%20M(2)
> 
> But I am unable to find out which test is run here.

If we aren't seeing similar memory leaks on other platforms for the same testsuite, we can ignore this (otherwise we would need to investigate more). The leak warnings mean "we ran a bunch of tests in a browser window, and when we closed the browser there was memory that was leaked", so it's difficult to associate with a particular test. However, bug 1293324 looks like it matches that leak, and it's a known intermittent leak, so we don't need to worry about it here.
Flags: needinfo?(josh)
(In reply to Saurabh Singhal [:saurabh] from comment #40)
> I am facing a weird issue. After I submitted the patch to bugzilla, I had to
> move to a different laptop where I did hg clone and all the stuff again.
> Also, I took this patch and applied it in the latest mozilla-central
> repository but
> testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini file
> was not there already so the part of the patch where the file was deleted
> did not apply properly. This caused a test failure here :
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=2
> 7721630&filter-
> searchStr=Windows%207%20VM%20opt%20W3C%20Web%20Platform%20Tests%20W3C%20Web%2
> 0Platform%20Tests%20W(2)
> Notice the test unexpected pass. How do I resolve this issue?

I agree that this is odd, because headers-combine.html.ini is definitely still present in mozilla-central: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini . Make sure that you have an updated mozilla-central and reapply the patch?
(In reply to Josh Matthews [:jdm] from comment #42)
> (In reply to Saurabh Singhal [:saurabh] from comment #40)
> > I am facing a weird issue. After I submitted the patch to bugzilla, I had to
> > move to a different laptop where I did hg clone and all the stuff again.
> > Also, I took this patch and applied it in the latest mozilla-central
> > repository but
> > testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini file
> > was not there already so the part of the patch where the file was deleted
> > did not apply properly. This caused a test failure here :
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=2
> > 7721630&filter-
> > searchStr=Windows%207%20VM%20opt%20W3C%20Web%20Platform%20Tests%20W3C%20Web%2
> > 0Platform%20Tests%20W(2)
> > Notice the test unexpected pass. How do I resolve this issue?
> 
> I agree that this is odd, because headers-combine.html.ini is definitely
> still present in mozilla-central:
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/
> fetch/api/headers/headers-combine.html.ini . Make sure that you have an
> updated mozilla-central and reapply the patch?

I cloned the repo in a different directory and made sure that the ini file was there. Then, I applied the patch and got this:

MacBook-Pro:mozilla-central saurabhs$ hg import ~/HeadersgetAll.patch
applying /Users/saurabhs/HeadersgetAll.patch
patching file testing/web-platform/tests/fetch/api/headers/headers-basic.html
Hunk #1 FAILED at 60
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/fetch/api/headers/headers-basic.html.rej
abort: patch failed to apply

On doing hg diff after this, I did not see the headers-combine.html.ini deleted part there although, the file is no longer there in my local repo. It looks like the file got deleted from the disk but did not show up as deleted in the patch. Do you know what can I do here?
Flags: needinfo?(josh)
What does `hg status` show?
Flags: needinfo?(josh)
Another thing you can try is `hg rm -A testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini` which should properly record the removal of headers-combine.html.ini.
(In reply to Josh Matthews [:jdm] from comment #44)
> What does `hg status` show?

MacBook-Pro:headers saurabhs$ hg status
M dom/cache/AutoUtils.cpp
M dom/cache/DBSchema.cpp
M dom/fetch/FetchDriver.cpp
M dom/fetch/Headers.h
M dom/fetch/InternalHeaders.cpp
M dom/fetch/InternalHeaders.h
M dom/flyweb/FlyWebPublishedServer.cpp
M dom/flyweb/HttpServer.cpp
M dom/tests/mochitest/fetch/test_headers_common.js
M dom/tests/mochitest/fetch/test_headers_mainthread.html
M dom/webidl/Headers.webidl
M testing/web-platform/meta/fetch/api/headers/headers-normalize.html.ini
M testing/web-platform/tests/fetch/api/headers/headers-combine.html
M testing/web-platform/tests/fetch/api/headers/headers-normalize.html
! testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini
? testing/web-platform/tests/fetch/api/headers/headers-basic.html.rej
The `hg status --help` output says "! = missing (deleted by non-hg command, but still tracked)", so I recommend running the `hg rm` command I suggested.
Thanks Josh. Your suggestion worked. I have pushed the code to try again.
Looks good to me!
Keywords: checkin-needed
The webidl changes from this patch need review from a DOM peer before this can land.
Keywords: checkin-needed
Attachment #8773840 - Attachment is obsolete: true
Comment on attachment 8786799 [details] [diff] [review]
HeadersgetAll.patch

This needs a DOM peer's review of the Header.webidl changes.
Attachment #8786799 - Flags: review?(bkelly)
Comment on attachment 8786799 [details] [diff] [review]
HeadersgetAll.patch

Review of attachment 8786799 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should clarify what Cache API Vary header matching should use before landing this.  I filed this spec issue:

https://github.com/w3c/ServiceWorker/issues/991

Overall, though, it looks good.  Sorry for the spec issue.

::: dom/cache/AutoUtils.cpp
@@ +262,5 @@
>                     "TypeUtils::ToPCacheResponseWithoutBody()");
>  
>          ErrorResult headerRv;
>          nsAutoCString value;
> +        requestHeaders->GetFirst(header, value, headerRv);

See my comments for DBSchema.cpp.  This should be `Get()` per the current spec:

https://w3c.github.io/ServiceWorker/#query-cache-algorithm

@@ +269,5 @@
>            MOZ_ASSERT(value.IsEmpty());
>          }
>  
>          nsAutoCString cachedValue;
> +        cachedRequestHeaders->GetFirst(header, cachedValue, headerRv);

Keep `Get()` here.

::: dom/cache/DBSchema.cpp
@@ +1299,5 @@
>                   "TypeUtils::ToPCacheResponseWithoutBody()");
>  
>        ErrorResult errorResult;
>        nsAutoCString queryValue;
> +      queryHeaders->GetFirst(header, queryValue, errorResult);

This seems wrong to me or there is a spec bug.  The service workers spec says to use Headers `get()` which includes combining.  I think we should keep the `Get()` call here instead of using `GetFirst()`.

See:

https://w3c.github.io/ServiceWorker/#query-cache-algorithm

@@ +1306,5 @@
>          MOZ_ASSERT(queryValue.IsEmpty());
>        }
>  
>        nsAutoCString cachedValue;
> +      cachedHeaders->GetFirst(header, cachedValue, errorResult);

Keep `Get()` here.

::: dom/fetch/Headers.h
@@ +85,5 @@
>    {
>      mInternalHeaders->Get(aName, aValue, aRv);
>    }
>  
> +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const

I think this should be `nsACString& aValue`.

::: dom/fetch/InternalHeaders.h
@@ +74,5 @@
>    void Append(const nsACString& aName, const nsACString& aValue,
>                ErrorResult& aRv);
>    void Delete(const nsACString& aName, ErrorResult& aRv);
>    void Get(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const;
> +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const;

I think this should be `nsACString& aValue).

::: dom/tests/mochitest/fetch/test_headers_mainthread.html
@@ +142,5 @@
>    }, TypeError, "Should not be able to delete immutable headers");
>  
>    checkHas(headers, "foo", "Should be able to check immutable headers");
>    ok(headers.get("foo"), "Should be able to get immutable headers");
> +  ok(headers.get("foo").length, "Should be able to get all immutable headers");

This test assertion seems wrong to me now.  In the old case it was checking the .length of an array.  This is checking the .length of the value returned which is effectively checked by the truthy assertion before it.

Since this is a single value in the 'foo' header, perhaps we should just remove this ok() assertion?
Attachment #8786799 - Flags: review?(bkelly) → review-
Initial feedback in the github issue is that we should probably be using the combined value in the Vary header logic.  So my comments about switching from `GetFirst()` back to `Get()` stand.  Sorry for the runaround and thanks for working on this!
(In reply to Ben Kelly [:bkelly] from comment #53) 
> ::: dom/fetch/Headers.h
> @@ +85,5 @@
> >    {
> >      mInternalHeaders->Get(aName, aValue, aRv);
> >    }
> >  
> > +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const
> 
> I think this should be `nsACString& aValue`.

Why?
(In reply to Saurabh Singhal [:saurabh] from comment #55)
> > > +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const
> > 
> > I think this should be `nsACString& aValue`.
> 
> Why?

Its preferable to use the abstract `nsA*String` string types in method signatures so the caller can decide which concrete string type to actually use.  I think the only time a method should take explicitly `nsString` or `nsCString` is if you require a flat string (not a dependent substring, etc).

I see that the previous `Get()` method also has an `nsCString`.  That could also be switched to `nsACString` I think.

The `GetAll()` method returns an `nsTArray<nsCString>` because it requires a concrete type.

As a side note, why are we leaving `GetAll()` in the code?  Can't that be removed now?

Thanks.
(In reply to Ben Kelly [:bkelly] from comment #56)
> As a side note, why are we leaving `GetAll()` in the code?  Can't that be
> removed now?

According to https://bugzilla.mozilla.org/show_bug.cgi?id=1278275#c15 GetAll() is still required.
Attached patch HeadersgetAll.patch (obsolete) — Splinter Review
Addressing comments from Ben. Thanks for the review.
Attachment #8786799 - Attachment is obsolete: true
Attachment #8800748 - Flags: review?(bkelly)
Attached patch HeadersgetAll.patch (obsolete) — Splinter Review
Uploading the correct diff.
Attachment #8800748 - Attachment is obsolete: true
Attachment #8800748 - Flags: review?(bkelly)
Attachment #8800754 - Flags: review?(bkelly)
(In reply to Saurabh Singhal [:saurabh] from comment #57)
> (In reply to Ben Kelly [:bkelly] from comment #56)
> > As a side note, why are we leaving `GetAll()` in the code?  Can't that be
> > removed now?
> 
> According to https://bugzilla.mozilla.org/show_bug.cgi?id=1278275#c15
> GetAll() is still required.

Removed GetAll from dom/fetch/Headers.h since the GetAll() being called in AutoUtils.cpp is the one provided by dom/fetch/InternalHeaders.h
(In reply to Saurabh Singhal [:saurabh] from comment #60)
> Removed GetAll from dom/fetch/Headers.h since the GetAll() being called in
> AutoUtils.cpp is the one provided by dom/fetch/InternalHeaders.h

You should remove the other one as well.  Just use Get() in AutoUtils and parse the combined value.
Comment on attachment 8800754 [details] [diff] [review]
HeadersgetAll.patch

Review of attachment 8800754 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, can you implement comment 61?  Now that Get() combines values we don't need that last GetAll() in AutoUtils.  You can just call Get() and let the parsing code that is already there extract the values.
Attachment #8800754 - Flags: review?(bkelly)
I just found that FetchBody<Derived>::SetMimeType() function in dom/fetch/Fetch.cpp also uses GetAll(). Should I remove this too and add the parsing code here also?
I believe you can replace that with Get().  Then for this section:

  // HTTP ABNF states Content-Type may have only one value.
  // This is from the "parse a header value" of the fetch spec.
  if (contentTypeValues.Length() == 1) {
    mMimeType = contentTypeValues[0];
    ToLowerCase(mMimeType);
  }

You can instead search for "," to see if there is more than one value. Sound reasonable?
Attached patch HeadersgetAll.patch (obsolete) — Splinter Review
I could not figure out a good place to put the parsing code. So, I have duplicated it for now in AutoUtils.cpp and TypeUtils.cpp
Attachment #8800754 - Attachment is obsolete: true
Attachment #8801564 - Flags: review?(bkelly)
Comment on attachment 8801564 [details] [diff] [review]
HeadersgetAll.patch

Review of attachment 8801564 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I think there was a miscommunication.  The parsing code is already in the code.  See:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/AutoUtils.cpp#255
https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#56

What it was doing before was getting all headers, but then parsing each one.  That was because we couldn't tell if a single value returned from Headers.getAll() was combined or not.

So you don't need to implement your own parsing here.  Just remove the outer loop from the strtok() loops that are already there.
Attachment #8801564 - Flags: review?(bkelly) → review-
Fix the parsing code in AutoUtils.cpp and TypeUtils.cpp
Attachment #8801564 - Attachment is obsolete: true
Attachment #8801810 - Flags: review?(bkelly)
Comment on attachment 8801810 [details] [diff] [review]
HeadersgetAll.patch

Review of attachment 8801810 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

In the future we may want to rejigger the logic so Headers::Append creates the combined string immediately instead of recalculating it on every Get().  That can be done in a follow-up, though.
Attachment #8801810 - Flags: review?(bkelly) → review+
Thanks Ben! I have pushed the changes to try. The results are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7a8502c1d1271d1114cd49bc6264d3b7e4f39d
Looks green.  Thanks again!
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/add68bb69193
Update Headers::GetAll and Headers::Get to match the specification. r=jdm, r=bkelly
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/add68bb69193
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: