Re: [PATCH] Bug 1961 pt1: Use URL object in HttpRequest and cleanup scheme handling

On 04/26/2014 01:59 AM, Amos Jeffries wrote:

> Also document HttpMsg::http_ver which is a copy of the HTTP-Version
> field in the "on-wire" message syntax. It is unrelated to the socket
> transport protocol and the URL scheme protocol.

I understand the above, but

> + /**
> + * The HTTP-Version label of this message.
> + */
> Http::ProtocolVersion http_ver;

the actual comment wording, especially its "label" part, sounds
confusing to me. Consider (quoting RFC 2616):

    /// HTTP-Version field in the first line of the message.

> AnyP::UriScheme const & getScheme() const {return scheme_;}
>
> + /// convert the URL scheme to that given
> + void makeScheme(const AnyP::ProtocolType &p) {scheme_=p;}
> +

I would call this setScheme() for consistency with other
setters/getters, to preserve symmetry with the existing getScheme(), and
because the method does not really "make" schemes.

Alternatively, consider just making the scheme data member public and
deleting setters/getters. They are totally redundant right now can can
be added later if they become needed.

> - %PROTO Requested protocol
> + %PROTO Requested URL scheme (protocol)

I think the old description covered more cases because many URLs do not
have a scheme and some future FTP gatewayed requests may not even have
true URLs. How about something like:

    %PROTO Requested origin server protocol (URL scheme)

None of the polishing touches above require another round of reviews as
far as I am concerned.

Thank you,

Alex.
Received on Sat Apr 26 2014 - 20:13:24 MDT