Bug 153787 – [Fetch API] Add support for iterating over Headers

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

As per https://fetch.spec.whatwg.org/#headers-class, Headers are iterable, entries being sorted by name lexicographically.

[youenn fablet]

Created attachment 270488 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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?

[youenn fablet]

Created attachment 270639 [details]
Patch for landing

[WebKit Commit Bot]

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.

[youenn fablet]

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.

[WebKit Commit Bot]

Comment on attachment 270639 [details]
Patch for landing

Clearing flags on attachment: 270639

Committed r196115: <http://trac.webkit.org/changeset/196115>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Failed on win bots

[youenn fablet]

Created attachment 270657 [details]
Trying to fix win build

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 270657 [details]
Trying to fix win build

Clearing flags on attachment: 270657

Committed r196128: <http://trac.webkit.org/changeset/196128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.