Created attachment 270488 [details] Patch

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 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?

Created attachment 270639 [details] Patch for landing

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.

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 on attachment 270639 [details] Patch for landing Clearing flags on attachment: 270639 Committed r196115: <http://trac.webkit.org/changeset/196115>

All reviewed patches have been landed. Closing bug.

Failed on win bots

Created attachment 270657 [details] Trying to fix win build

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 on attachment 270657 [details] Trying to fix win build Clearing flags on attachment: 270657 Committed r196128: <http://trac.webkit.org/changeset/196128>

All reviewed patches have been landed. Closing bug.