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

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

Index: third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp
diff --git a/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp b/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp
index ca24581c50d6d8947f534060708b79cfea3f77c1..2130a8fada120dab028126015202454fe3f6f194 100644
--- a/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp
+++ b/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp
@@ -87,13 +87,19 @@ void FetchHeaderList::remove(const String& name) {
 
 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 {