Last Comment Bug 170789 - HTTP Accept: header should be tailored to context of the request
: HTTP Accept: header should be tailored to context of the request
Status: RESOLVED FIXED
[SNAP]
: embed
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: P5 enhancement with 1 vote (vote)
: mozilla1.2beta
Assigned To: Darin Fisher
: Tom Everingham
Mentors:
: 82526 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-09-25 12:02 PDT by Darin Fisher
Modified: 2012-12-25 08:45 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch (8.77 KB, patch)
2002-09-25 15:55 PDT, Darin Fisher
no flags Details | Diff | Review
v2 patch (12.55 KB, patch)
2002-09-25 17:07 PDT, Darin Fisher
no flags Details | Diff | Review
v2.1 patch -- minor tweak (12.57 KB, patch)
2002-09-25 17:11 PDT, Darin Fisher
dougt: review+
bzbarsky: superreview+
Details | Diff | Review

Description Darin Fisher 2002-09-25 12:02:14 PDT
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 [:bz] (Out June 25-July 6) 2002-09-25 13:05:17 PDT
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 2002-09-25 13:28:56 PDT
yup, that's all it takes... i'm working on a patch.
Comment 3 Darin Fisher 2002-09-25 15:55:43 PDT
Created attachment 100643 [details] [diff] [review]
v1 patch

please review
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2002-09-25 16:12:00 PDT
this is a duplicate
Comment 5 Christian :Biesinger (don't email me, ping me on IRC) 2002-09-25 16:17:16 PDT
...of bug 82526

both have a patch.
Comment 6 Darin Fisher 2002-09-25 16:18:25 PDT
*** Bug 125682 has been marked as a duplicate of this bug. ***
Comment 7 Darin Fisher 2002-09-25 16:20:21 PDT
*** Bug 82526 has been marked as a duplicate of this bug. ***
Comment 8 Darin Fisher 2002-09-25 16:22:07 PDT
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 [:bz] (Out June 25-July 6) 2002-09-25 16:26:44 PDT
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 2002-09-25 17:07:35 PDT
Created attachment 100653 [details] [diff] [review]
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 2002-09-25 17:11:23 PDT
Created attachment 100654 [details] [diff] [review]
v2.1 patch -- minor tweak

the v2 patch lacked ",*/*;q=0.1" for XSLT loads.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2002-09-25 17:17:30 PDT
Comment on attachment 100654 [details] [diff] [review]
v2.1 patch -- minor tweak

sr=bzbarsky
Comment 13 Erik Talvola 2002-09-25 18:22:59 PDT
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 [:bz] (Out June 25-July 6) 2002-09-25 18:25:28 PDT
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 2002-09-25 19:29:40 PDT
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] 2002-09-25 23:24:56 PDT
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 2002-09-26 00:53:38 PDT
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] 2002-09-26 01:31:46 PDT
> 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 2002-09-26 11:36:55 PDT
right, i believe that's correct. the CSS loader only loads CSS :)
Comment 20 Darin Fisher 2002-09-26 20:52:18 PDT
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) 2002-09-26 21:21:57 PDT
Comment on attachment 100654 [details] [diff] [review]
v2.1 patch -- minor tweak

r=dougt
Comment 22 Darin Fisher 2002-09-26 21:29:01 PDT
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 2002-09-26 21:33:04 PDT
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) 2002-09-27 05:20:36 PDT
> 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 2002-12-23 16:04:00 PST
-> tever, needs verification

Note You need to log in before you can comment on or make changes to this bug.