[Fetch API] Appending headers of the same name should have their values

third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/fetch/FetchHeaderList.h"
 
 #include "core/fetch/FetchUtils.h"
 #include "platform/network/HTTPParsers.h"
 #include "wtf/PtrUtil.h"
 #include <algorithm>
@@ skipping 69 matching lines @@
   for (size_t i = 0; i < m_headerList.size();) {
     if (m_headerList[i]->first == lowercasedName)
       m_headerList.remove(i);
     else
       ++i;
   }
 }
 
 bool FetchHeaderList::get(const String& name, String& result) const {
   const String lowercasedName = name.lower();
-  for (size_t i = 0; i < m_headerList.size(); ++i) {
-    if (m_headerList[i]->first == lowercasedName) {
-      result = m_headerList[i]->second;
-      return true;
+  bool flag = false;
+  for (auto&& header : m_headerList) {

[jsbell, 2016/12/16 17:58:26]

Do we have a preference for auto&& vs. const auto& ?

(I just learned about auto&& - thanks! - so I haven't seen it used in the code
base before, but maybe I'm just looking at old code.)

[dhna, 2016/12/16 18:33:35]

I like your suggestion.

+    if (header->first == lowercasedName) {
+      if (result.isEmpty()) {

[jsbell, 2016/12/16 17:58:26]

My reading of the spec is that values may not be empty (i.e. this is correct,
and e.g. ",," would never be generated by this)

Can you DCHECK(!header->second.isEmpty()); (or whatever the right incantation
is) outside the if() blocks?

[dhna, 2016/12/16 18:33:35]

jsbell@
If we meet a first key value matched.
In that case result.size() will be zero. and there will be no any string.
So in that case, we should append only a string without appending ','.

[jsbell, 2016/12/16 18:55:59]

To clarify: I agree the code is correct. I do not think it should change.

As a reviewer, I was not sure that header values could not be empty strings, and
I had to check the spec. After reading the spec I agree that the code is
correct. My suggestion is to add a DCHECK to reinforce that the header values
cannot be empty, which would catch bugs elsewhere and also documents to a future
reviewer/reader that values can not be empty.

[jsbell, 2016/12/16 18:57:32]

And to clarify one more time: I mean that your proposed code (this CL, what is
being reviewed) is correct. 

+        result = header->second;
+        flag = true;
+      } else {
+        result.append(",");
+        result.append(header->second);
+      }
     }
   }
-  return false;
+  return flag;

[jsbell, 2016/12/16 17:58:26]

Do we need flag, or can it just be !result.isEmpty() ?

[dhna, 2016/12/16 18:33:35]

I like this suggestion also.

 }
 
 void FetchHeaderList::getAll(const String& name, Vector<String>& result) const {
   const String lowercasedName = name.lower();
   result.clear();
   for (size_t i = 0; i < m_headerList.size(); ++i) {
     if (m_headerList[i]->first == lowercasedName)
       result.append(m_headerList[i]->second);
   }
 }
@@ skipping 47 matching lines @@
   return isValidHTTPToken(name);
 }
 
 bool FetchHeaderList::isValidHeaderValue(const String& value) {
   // "A value is a byte sequence that matches the field-value token production
   // and contains no 0x0A or 0x0D bytes."
   return isValidHTTPHeaderValue(value);
 }
 
 }  // namespace blink