Bug 153787 - [Fetch API] Add support for iterating over Headers
Summary: [Fetch API] Add support for iterating over Headers
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: 151937 153859
  Show dependency treegraph
 
Reported: 2016-02-02 06:54 PST by youenn fablet
Modified: 2016-02-04 09:43 PST (History)
3 users (show)

See Also:


Attachments
Patch (28.37 KB, patch)
2016-02-02 08:09 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (29.61 KB, patch)
2016-02-04 01:08 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Trying to fix win build (30.01 KB, patch)
2016-02-04 08:07 PST, 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-02-02 06:54:18 PST
As per https://fetch.spec.whatwg.org/#headers-class, Headers are iterable, entries being sorted by name lexicographically.
Comment 1 youenn fablet 2016-02-02 08:09:54 PST
Created attachment 270488 [details]
Patch
Comment 2 WebKit Commit Bot 2016-02-02 08:12:22 PST
Attachment 270488 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSKeyValueIterator.h:56:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2016-02-02 08:49:58 PST
Comment on attachment 270488 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:190
> +    do {
> +        if (m_currentIndex >= m_keys.size())
> +            return false;
> +        key = m_keys[m_currentIndex++];
> +        value = m_headers->m_headers.get(key);
> +    } while (value.isNull());
> +    return true;

I would reverse the logic here and use a normal while:

    while (m_currentIndex < m_keys.size()) {
        key = m_keys[m_currentIndex++];
        value = m_headers->m_headers.get(key);
        if (!value.isNull())
            return true;
    }
    return false;

Might also want to not touch the out arguments at all if the return value is false:

        auto& nextKey = m_keys[m_currentIndex++];
        String nextValue = m_headers->m_headers.get(key);
        if (!nextValue.isNull()) {
            key = nextKey;
            value = nextValue;
            return true;
        }

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:201
> +    , m_currentIndex(0)

Should initialize this in the class definition rather than the constructor.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:203
> +    m_keys.reserveCapacity(headers.m_headers.size());

reserveInitialCapacity

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:205
> +        m_keys.append(header.key.convertToASCIILowercase());

uncheckedAppend

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:207
> +    std::sort(m_keys.begin(), m_keys.end(), WTF::codePointCompareLessThan);

Strange that you need the WTF prefix here. Normally you should not.

> Source/WebCore/Modules/fetch/FetchHeaders.h:67
> +        Iterator(FetchHeaders&);

Should mark this explicit.

> Source/WebCore/Modules/fetch/FetchHeaders.h:71
> +        // FIXME: Binding generator should be able to generate iterator key and value types.
> +        using Key = String;
> +        using Value = String;

This seems peculiar. Unclear what it’s for.

> Source/WebCore/Modules/fetch/FetchHeaders.h:77
> +        RefPtr<FetchHeaders> m_headers;

How valuable is it to null this pointer out in the finish function? I’d prefer that this use a Ref rather than a RefPtr.

> Source/WebCore/bindings/js/JSFetchHeadersCustom.cpp:1
> +#include "config.h"

File needs a copyright header.

> Source/WebCore/bindings/js/JSFetchHeadersCustom.cpp:8
> +// FIXME: Move that code to JSFetchHeaders.

"this code", not "that code"

> Source/WebCore/bindings/js/JSFetchHeadersCustom.cpp:13
> +const JSC::ClassInfo FetchHeadersIterator::s_info = { "Headers Iterator", &Base::s_info, 0, CREATE_METHOD_TABLE(FetchHeadersIterator) };

Is the string "Headers Iterator" really correct here? That does not seem like a class name. Where does this string appear?

> Source/WebCore/bindings/js/JSKeyValueIterator.h:31
> +#include <runtime/JSObject.h>

Pretty sure this include is not needed, since JSDestructibleObject.h includes it.

> Source/WebCore/bindings/js/JSKeyValueIterator.h:67
> +public:
> +
> +    typedef JSC::JSDestructibleObject Base;

Please omit this blank line.

> Source/WebCore/bindings/js/JSKeyValueIterator.h:90
> +    void finish() { m_iterator.finish(); }

Why is this needed? If we changed this to a no-op, would it break one of the tests?
Comment 4 youenn fablet 2016-02-04 01:08:17 PST
Created attachment 270639 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2016-02-04 01:09:44 PST
Attachment 270639 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSKeyValueIterator.h:55:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 youenn fablet 2016-02-04 01:51:32 PST
Thanks for the review.

(In reply to comment #3)
> Comment on attachment 270488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270488&action=review
> 
> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:190
> > +    do {
> > +        if (m_currentIndex >= m_keys.size())
> > +            return false;
> > +        key = m_keys[m_currentIndex++];
> > +        value = m_headers->m_headers.get(key);
> > +    } while (value.isNull());
> > +    return true;
> 
> I would reverse the logic here and use a normal while:
> 
>     while (m_currentIndex < m_keys.size()) {
>         key = m_keys[m_currentIndex++];
>         value = m_headers->m_headers.get(key);
>         if (!value.isNull())
>             return true;
>     }
>     return false;
> Might also want to not touch the out arguments at all if the return value is
> false:
> 
>         auto& nextKey = m_keys[m_currentIndex++];
>         String nextValue = m_headers->m_headers.get(key);
>         if (!nextValue.isNull()) {
>             key = nextKey;
>             value = nextValue;
>             return true;
>         }


Done.
Since it is a next() function mapping to JS next(), it returns a isDone which is false when there is a value.
I updated in this method and the caller accordingly.

> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:201
> > +    , m_currentIndex(0)
> 
> Should initialize this in the class definition rather than the constructor.

OK

> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:203
> > +    m_keys.reserveCapacity(headers.m_headers.size());
> 
> reserveInitialCapacity

OK

> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:205
> > +        m_keys.append(header.key.convertToASCIILowercase());
> 
> uncheckedAppend

OK

> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:207
> > +    std::sort(m_keys.begin(), m_keys.end(), WTF::codePointCompareLessThan);
> 
> Strange that you need the WTF prefix here. Normally you should not.

This does not compile without.

> > Source/WebCore/Modules/fetch/FetchHeaders.h:67
> > +        Iterator(FetchHeaders&);
> 
> Should mark this explicit.

OK

> > Source/WebCore/Modules/fetch/FetchHeaders.h:71
> > +        // FIXME: Binding generator should be able to generate iterator key and value types.
> > +        using Key = String;
> > +        using Value = String;
> 
> This seems peculiar. Unclear what it’s for.

It is used in JSKeyValueIterator::next() to know which types the DOM iterator is expecting.
The binding generator can add these definitions in JSFetchHeaders based on FetchHeaders.idl.
type_traits might also probably help there.

> > Source/WebCore/Modules/fetch/FetchHeaders.h:77
> > +        RefPtr<FetchHeaders> m_headers;
> 
> How valuable is it to null this pointer out in the finish function? I’d
> prefer that this use a Ref rather than a RefPtr.

I removed it.
m_keys is still cleaned when iterator goes to the end.

> 
> > Source/WebCore/bindings/js/JSFetchHeadersCustom.cpp:1
> > +#include "config.h"
> 
> File needs a copyright header.

OK

> > Source/WebCore/bindings/js/JSFetchHeadersCustom.cpp:8
> > +// FIXME: Move that code to JSFetchHeaders.
> 
> "this code", not "that code"

I forgot this one... but I will fix that in a later patch.

> > Source/WebCore/bindings/js/JSFetchHeadersCustom.cpp:13
> > +const JSC::ClassInfo FetchHeadersIterator::s_info = { "Headers Iterator", &Base::s_info, 0, CREATE_METHOD_TABLE(FetchHeadersIterator) };
> 
> Is the string "Headers Iterator" really correct here? That does not seem
> like a class name. Where does this string appear?

It follows naming of other JavaScriptCore iterators.
I think (to be confirmed) it may be used when printing an object in JS for instance.

It does not seem very consistent with WebCore naming like "HeadersConstructor" though.

> > Source/WebCore/bindings/js/JSKeyValueIterator.h:31
> > +#include <runtime/JSObject.h>
> 
> Pretty sure this include is not needed, since JSDestructibleObject.h
> includes it.

OK

> > Source/WebCore/bindings/js/JSKeyValueIterator.h:67
> > +public:
> > +
> > +    typedef JSC::JSDestructibleObject Base;
> 
> Please omit this blank line.

OK

> > Source/WebCore/bindings/js/JSKeyValueIterator.h:90
> > +    void finish() { m_iterator.finish(); }
> 
> Why is this needed? If we changed this to a no-op, would it break one of the
> tests?

Purpose was to do some clean-up but it is not necessary.
I removed it.
Comment 7 WebKit Commit Bot 2016-02-04 02:39:41 PST
Comment on attachment 270639 [details]
Patch for landing

Clearing flags on attachment: 270639

Committed r196115: <http://trac.webkit.org/changeset/196115>
Comment 8 WebKit Commit Bot 2016-02-04 02:39:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 youenn fablet 2016-02-04 05:31:34 PST
Failed on win bots
Comment 10 youenn fablet 2016-02-04 08:07:14 PST
Created attachment 270657 [details]
Trying to fix win build
Comment 11 WebKit Commit Bot 2016-02-04 08:09:09 PST
Attachment 270657 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSKeyValueIterator.h:57:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Commit Bot 2016-02-04 09:43:09 PST
Comment on attachment 270657 [details]
Trying to fix win build

Clearing flags on attachment: 270657

Committed r196128: <http://trac.webkit.org/changeset/196128>
Comment 13 WebKit Commit Bot 2016-02-04 09:43:12 PST
All reviewed patches have been landed.  Closing bug.