170789 - HTTP Accept: header should be tailored to context of the request

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P5)

Description

[Darin Fisher]

HTTP Accept: header should be tailored to context of the request.  This impacts
imagelib, CSS loader, and JS loader, (potentially) like so:

For inline image loads:
  Accept: video/x-mng,image/png,image/jpeg,image/gif;q=0.2,*/*;q=0.1

For CSS loads:
  Accept: text/css,*/*;q=0.1

For JS loads:
  Accept: */*

This would really help reduce the amount of data we send w/ each request.  (I
think Accept: */* makes sense for JS loads since our current Accept: header
value doesn't mention any preference for javascript content.)

I know there is a bug already about doing this for imagelib, but I think we
should really do this for CSS and JS as well.  There may also be some benefit to
doing this for plugins too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

As far as that goes, we coudl even not list */* for CSS in standards mode (since
we won't take it anyway).

I agree that this would be a good idea (and would reduce the size of the Accept
header).  Is this just a matter of having the relevant modules call
SetRequestHeader() on the channel before calling Open/AsyncOpen?

Comment 2

[Darin Fisher]

yup, that's all it takes... i'm working on a patch.

Comment 3

[Darin Fisher]

Attachment: v1 patch

please review

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

this is a duplicate

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

...of bug 82526

both have a patch.

Comment 6

[Darin Fisher]

*** Bug 125682 has been marked as a duplicate of this bug. ***

Comment 7

[Darin Fisher]

*** Bug 82526 has been marked as a duplicate of this bug. ***

Comment 8

[Darin Fisher]

note: this patch doesn't do any pref checking.  i think that's ok for now.  we
can always go back over this code and add prefs if that makes sense.  i figure
since our code is already pretty hard coded to respond to certain MIME types,
hardcoded Accept headers aren't really that bad.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Doing SetRequestHeader twice like that is odd-looking and moreover the
HttpChannel idl does not make it at all clear that you have to do it like
that...  We should fix this API (Darin suggested just having a third aReplace
param to SetRequestHeader).  Not necessarily as part of this patch, but...

nsStreamLoader::Init can be called from script (chrome script) and so it should
really do NS_ENSURE_ARG_POINTER on at least "channel" and probably "observer"
due to our "Chrome JS should not cause crashes" mantra.

The rest looks quite nice.

Comment 10

[Darin Fisher]

Attachment: v2 patch

fixed up the comments in nsIHttpChannel.idl, added some NS_ENSURE_ARG_POINTERs,
and did similar Accept foo for XSLT.

Comment 11

[Darin Fisher]

Attachment: v2.1 patch -- minor tweak

the v2 patch lacked ",*/*;q=0.1" for XSLT loads.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 100654 [details] [diff] [review]
v2.1 patch -- minor tweak

sr=bzbarsky

Comment 13

[Erik Talvola]

Should XBM be added to the image list, since Mozilla can handle them just 
fine?  image/x-xbitmap is the mime type.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I would say "no".  For the same reason that bmp and ico should not be.  If
people want to send that crap over the web, fine.  But we should not ask them to.

Comment 15

[Darin Fisher]

i agree, the Accept header says what we prefer to handle... it doesn't have to
enumerate all the data types we do handle.

Comment 16

[Gervase Markham [:gerv]]

As Guardian of the Accept Header (;-)), I'm definitely in favour in principle.
The problem here is that the SVG people, for example, can now no longer add
their MIME type to the image Accept header.

Does this patch completely abandon the pref for Accept:, or does it just use it
only for "top-level" page requests?

When I wrote a patch for this over in bug 82526, darin suggested I use
preflisteners so these values could still be prefs. What's changed? :-)

Gerv

Comment 17

[Darin Fisher]

gerv:

the pref is now used only for toplevel page loads.  hmm... i did not realize
that the SVG folks were overriding the pref settings.  that worries me a bit,
since the present of SVG increases the size of HTTP requests.  of course, that's
the whole point of the Accept header :-/

ok, so i should probably incorporate your patch for the image case.  i don't
think there's any reason right now to add prefs for the others like CSS, JS, and
XSLT.  agreed?

Comment 18

[Gervase Markham [:gerv]]

> the pref is now used only for toplevel page loads.  hmm... i did not realize
> that the SVG folks were overriding the pref settings.  

They may not be right now, but if they aren't then they'd like to. This is the
obvious way of telling which Mozilla builds support SVG.

> that worries me a bit,
> since the present of SVG increases the size of HTTP requests.  of course, 
> that's the whole point of the Accept header :-/

The increase will be more than offset by the decrease caused by this patch.
We've lived with a large Accept: header for everything for ages, so it can't be
that big a deal. Brendan told me (possibly partly in jest) to keep an eye on its
size, and I have been doing.

> ok, so i should probably incorporate your patch for the image case.  i don't

Don't flatter me - my patch was pretty hacky :-)

> think there's any reason right now to add prefs for the others like CSS, JS, 
> and XSLT.  agreed?

Agreed. Unlike "images", CSS, JS and XSLT are individual file types by
themselves. The CSS Accept: header is CSS only, right - it's not used for all
"stylesheets"?

Gerv

Comment 19

[Darin Fisher]

right, i believe that's correct. the CSS loader only loads CSS :)

Comment 20

[Darin Fisher]

i've been thinking some more about how to get this right for imagelib, and while
a preference would work, it almost seems like it would be better to utilize the
fact that imagelib decoders must be registered.  we could add an attribute to
imgIDecoder stating its MIME-type and whether or not it should be advertized in
the Accept header.  we'd just need a way to iterate the set of registered decoders.

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 100654 [details] [diff] [review]
v2.1 patch -- minor tweak

r=dougt

Comment 22

[Darin Fisher]

dougt and i spoke a bit about either using the category manager straight or
possibly defining some service that could manage the Accept header.  solving
this definitely seems like another bug IMO, so i'm going to go ahead now with
the patch as is.

Comment 23

[Darin Fisher]

fixed-on-trunk

but 125682 seems to be exactly the bug for dealing with the SVG problem (in
fact, that's why it was filed in the first place).

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

> we could add an attribute to
>imgIDecoder stating its MIME-type and whether or not it should be advertized in
>the Accept header. 

bug 104906 seems related.

Comment 25

[benc]

-> tever, needs verification