Closed Bug 170789 Opened 21 years ago Closed 21 years ago

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

Categories

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

Product:
Core
Component:
Networking: HTTP
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.2beta

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

(Keywords: embed, Whiteboard: [SNAP])

Attachments

(1 file, 2 obsolete files)

v2.1 patch -- minor tweak
12.57 KB, patch
dougt
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
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.
Severity: normal → enhancement
Status: NEW → ASSIGNED
Keywords: embed
Priority: -- → P5
Whiteboard: [SNAP]
Target Milestone: --- → mozilla1.2beta
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?
yup, that's all it takes... i'm working on a patch.
Attached patch v1 patch (obsolete) — — Splinter Review 
please review
*** Bug 125682 has been marked as a duplicate of this bug. ***
*** Bug 82526 has been marked as a duplicate of this bug. ***
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.
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.
Attached patch v2 patch (obsolete) — — Splinter Review 
fixed up the comments in nsIHttpChannel.idl, added some NS_ENSURE_ARG_POINTERs,
and did similar Accept foo for XSLT.
Attachment #100643 - Attachment is obsolete: true
the v2 patch lacked ",*/*;q=0.1" for XSLT loads.
Attachment #100653 - Attachment is obsolete: true
Comment on attachment 100654 [details] [diff] [review]
v2.1 patch -- minor tweak

sr=bzbarsky
Attachment #100654 - Flags: superreview+
Should XBM be added to the image list, since Mozilla can handle them just 
fine?  image/x-xbitmap is the mime type.
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.
i agree, the Accept header says what we prefer to handle... it doesn't have to
enumerate all the data types we do handle.
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
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?
> 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
right, i believe that's correct. the CSS loader only loads CSS :)
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 on attachment 100654 [details] [diff] [review]
v2.1 patch -- minor tweak

r=dougt
Attachment #100654 - Flags: review+
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.
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).
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
> 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.
-> tever, needs verification
QA Contact: httpqa → tever
You need to log in before you can comment on or make changes to this bug.