I think this is a valid thing to fix, but I'm a bit concerned about the
logic of the patch. Since it is Friday afternoon I might have made a
mistake, so let me explain.
This table shows how the first part of the patch will set the values of
the content_encoding field for a particular variant:
Variant
no encoding has an encoding X
Request with
no A-E: 1 1
Request with
empty A-E: 1 0
Request with
A-E: X 1 2 }
}
Request with }
A-E: without }
X 1 0 }
The brackets (}) show that the last two lines can all occur on a single
request, if there are variants with both matching an non-matching
variants.
Now consider the second part of the patch below, when a request comes in
with a non-empty A-E: line. Say A-E: gzip. Consider having variants
x.gz and x, which are found _in that order_.
Now the first variant, x.gz gets picked as the best so far (since it is
the first, and is acceptable). So best->encoding is non-empty (it would
also get picked in preference to any other variants without an encoding,
because of encoding_quality = 2, as desired, using the second part of
the "if" in the patch below).
Now mod_negotiation looks at variant x. It is acceptable. So it gets to
the code in the patch below. This variant's encoding_quality is 1, so the
first part of the "if" below gets activated. This says that the best so
far _has_ an encoding (which is does) _and_ the new variant does not have
an encoding, we should pick the new variant (x) as the best so far. So we
flip back to prefering the non-encoded variant.
I think I'd prefer to see the logic of the second part of the patch below
changed to:
prefer the variant with the highest value of encoding_quality
if both the best and variant's encoding qualities are the
same, prefer the variant without an encoding
The first bit means we prefer the variants with quality 2 over those with
quality 1 (which is what this PR is about). The second part comes into
play if the request as no A-E: and we have variants with and without
encodings (the first line of the table), and prefers unencoded variants
(the current default).
Paul
On 25 Nov 1998, Dirk-Willem van Gulik wrote:
> > Line 1490 of set_encoding_quality() changed from:
> > variant->encoding_quality = 1;
> > to:
> > variant->encoding_quality = 2; /* Client explicity wants encoded
> > variants. PJA */
> >
> > Lines 1667 - 1677 of is_variant_better() were changed from:
> > /* encoding -- can only be 1 or 0, and if 0 we eliminated this
> > * variant at the start of this function. However we
> > * prefer variants with no encoding over those with encoding */
> > if (best->content_encoding == NULL && variant->content_encoding) {
> > return 0;
> > }
> > if (best->content_encoding && variant->content_encoding == NULL) {
> > *p_bestq = q;
> > return 1;
> > }
> > to:
> > if (variant->encoding_quality != 2) /* Client says nothing about
> > encodings. PJA */
> > {
> > /* encoding -- can only be 1 or 0, and if 0 we eliminated this
> > * variant at the start of this function. However we
> > * prefer variants with no encoding over those with encoding */
> > if (best->content_encoding == NULL && variant->content_encoding) {
> > return 0;
> > }
> > if (best->content_encoding && variant->content_encoding == NULL) {
> > *p_bestq = q;
> > return 1;
> > }
> > }
> > else /* Client explicity wants encoded variants. PJA */
> > {
> > /* encoding -- can only be 1 or 0, and if 0 we eliminated this
> > * variant at the start of this function. However we
> > * prefer variants with no encoding over those with encoding */
> > if (best->content_encoding && !variant->content_encoding) {
> > return 0;
> > }
> > if (!best->content_encoding && variant->content_encoding) {
> > *p_bestq = q;
> > return 1;
> > }
> > }
> >
> > The idea is to mark an encoding variant clearly accepted by the client
> > as such in set_encoding_quality(). Later in is_variant_better() the
> > preferred encoded variant is marked as superior to the unencoded
> > variant.
>
>
>
Paul Sutton, C2Net Europe http://www.eu.c2.net/~paul/
Editor, Apache Week .. the latest Apache news http://www.apacheweek.com/
