Allow response header value to contain a colon

[rylwin]

HTTP response header field values are allowed to contain separators (which
includes the colon) as long as they appear within quotes
(http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2). This commit
enables Browser#response_headers to handle a field value that contains a colon
followed by a space (i.e., ": ") without breaking.

Previously, a colon followed by a space would cause #response_headers to break
as #split(": ") would lead to an array with three parts which Hash[] cannot handle.

[mhoran]

Thanks for looking into this, @rylwin! I think the best fix here may be to encode the response as JSON as we do with other methods, and decode on the Ruby side. This will simplify the code that needs to be maintained and brings this method in line with the others. Check out Driver#console_messages for an example. Note that this would require a C++ change, in the corresponding Headers.cpp file. Check out ConsoleMessages.cpp for an example of that change.

[rylwin]

Thanks for the guidance, @mhoran! I've attempted a fix based on your suggestions and I'd like to discuss what I've found so far.

I've updated Headers.cpp to use JsonSerializer, so when we JSON.parse the headers in ruby-land we no longer need to split("\n") since JSON correctly deserializes into an array for us:

JSON.parse(command("Headers")) #=> [
  "Content-Type: text/plain",
  "Content-Length: 8",
  "Content-Disposition: inline; filename=\"Name: with colon.txt\""
]

In ruby we still need to convert these items to a hash. For the last item in the example above, splitting on ":" causes the same error as before as the array splits into three parts (since there are two colons).

I think working with headers differs from console messages because each item in the console messages is actually a QVariantMap. JSON.parse yields an array of hashes (maps) with the keys/values already properly split (with the heavy lifting done in WebPage.cpp:171).

If the above is correct, then I don't think json serializing the Headers solves this problem. I think we need to add a bit of additional logic to split each header into key/value pairs that correctly handles the situation where the value contains an additional colon. I think we just need to decide whether this belongs somewhere in the cpp or ruby code. Thoughts?

[mhoran]

It looks like a few additional changes are required to make this possible. In WebPage::setFrameProperties, QStringList headers will need to be replaced with QVariantMap headers. This will make it possible to extract a hash of headers. Within the foreach loop, instead of concatenating the header name with the value, we'll need to insert into the headers QVariantMap the appropriate values.

With these changes in place, the signature of WebPage::pageHeaders can be changed from QStringList to QVariantMap and the call to toStringList can be removed. This also requires a change in the WebPage.h file, to reflect the new return type. With that change in place, I believe the serializer should correctly recognize the value as a hash, which should be sufficient for Ruby to deserialize a hash.

[rylwin]

I see exactly what you're talking about. After looking at setFrameProperties what you propose makes perfect sense. I'll work on this when I get a chance (probably this weekend?). Thanks for the additional guidance!

[rylwin]

@mhoran I took another stab at it following your latest advice. Converting the headers property into a map made it really simple!

[jferris]

There are existing specs for response_headers in spec/driver_spec.rb. I think it would be worth just adding one more example where there's a colon, rather than duplicating the code for booting up a server.

[jferris]

@rylwin thanks for this! The C++ changes look awesome.

I had one comment about the specs. If you're able to make that change, I think this will be ready to merge.

[rylwin]

@jferris moved the spec to driver_spec. Definitely much simpler this way.

@jferris @mhoran thanks for helping me out with this PR! Happy to make any additional changes/tweaks. Or if all looks good I can go ahead and rebase this branch if you'd like.

[jferris]

Thanks! I merged this into master as c0c9d70.