Closed
Bug 1077312
Opened 10 years ago
Closed 10 years ago
send a more appropriate Accept: header when fetching webfonts
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jfkthame, Assigned: jfkthame)
Details
Attachments
(1 file, 1 obsolete file)
1.46 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
Currently, we fetch webfonts with an HTTP request that has Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 which is just silly. Chrome uses Accept:*/*, and Safari omits the Accept header altogether (which is equivalent to */*, AIUI). Either of these would be more reasonable (and shorter) than what we currently send. We could consider specifically preferring WOFF (and WOFF2 once bug 1064737 lands), though in practice I doubt anyone pays attention.
Assignee | ||
Comment 1•10 years ago
|
||
And while we're here, I suggest we eliminate the Accept-Language and Accept-Encoding headers when fetching fonts.
Assignee | ||
Comment 2•10 years ago
|
||
This cleans things up a bit. We could consider adding specific (preferred) types to Accept, but at this point it's not clear to me that there'd be any real benefit, and it would bloat our requests; think it's worth pursuing?
Attachment #8499435 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment 3•10 years ago
|
||
Chrome uses Accept: */* on everything, IIRC. As Guardian of the Accept Header, using */* for fonts is fine with me. No-one chooses the font to send based on this header, there are better mechanisms. Gerv
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #3) > Chrome uses Accept: */* on everything, IIRC. It seems to add other types in some cases, FWIW: Accept:text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 Accept:image/webp,*/*;q=0.8 Accept:text/css,*/*;q=0.1 > As Guardian of the Accept Header, using */* for fonts is fine with me. > No-one chooses the font to send based on this header, there are better > mechanisms. Indeed. Would it be preferable to remove the header altogether, and save a few more bytes on the network?
Comment 5•10 years ago
|
||
Interesting that they decided to use Accept: for advertising webp support. I think you'd need to do more compat testing before removing it entirely than you would need to do for switching to */*. And it may not be worth your time. Gerv
Comment on attachment 8499435 [details] [diff] [review] Clean up Accept headers on @font-face HTTP requests. Review of attachment 8499435 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsFontFaceLoader.cpp @@ +378,4 @@ > httpChannel->SetReferrer(aFontFaceSrc->mReferrer); > + // We don't want the default accept headers for HTTP requests here. > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept"), > + NS_LITERAL_CSTRING("*/*"), Is there really nothing better we can send here. McManus seemed to be in support of sending something here, so he might have ideas. @@ +382,5 @@ > + false); > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept-Language"), > + NS_LITERAL_CSTRING(""), > + false); > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept-Encoding"), I don't actually know if it makes sense to send empty Accept-Encoding and Accept-Language headers. Someone from the networking team should review.
Attachment #8499435 -
Flags: review?(jonas) → review?(mcmanus)
Comment 7•10 years ago
|
||
Comment on attachment 8499435 [details] [diff] [review] Clean up Accept headers on @font-face HTTP requests. Review of attachment 8499435 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsFontFaceLoader.cpp @@ +378,4 @@ > httpChannel->SetReferrer(aFontFaceSrc->mReferrer); > + // We don't want the default accept headers for HTTP requests here. > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept"), > + NS_LITERAL_CSTRING("*/*"), I think the chrome */* thing is silly.. I'd rather we didn't do it. what I'd really rather see is woff2, woff, */* (assuming that's what you really prefer). note how the chrome webp header (in an image context, not a font context I assume) is used by their mobile proxy to negotiate transcoding for the client. They avoid ua sniffing by doing that - its to be encouraged. @@ +380,5 @@ > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept"), > + NS_LITERAL_CSTRING("*/*"), > + false); > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept-Language"), > + NS_LITERAL_CSTRING(""), it doesn't seem implausible that font selection is part of the normal accept-language localization.. I would think we should be consistent with Accept-Language across all contexts. @@ +382,5 @@ > + false); > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept-Language"), > + NS_LITERAL_CSTRING(""), > + false); > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept-Encoding"), so you definitely want accept-encoding: [normal gzip, deflate stuff] .. compression of ttf for example would be very good.. it looks from a google search that the cloduflare cdn automagically applies that transformation if its negotiated.
Attachment #8499435 -
Flags: review?(mcmanus)
Comment 8•10 years ago
|
||
Accept: */* is required as a minimum as otherwise servers are known to break (if you simply omit the header). Given that @font-face has client-side negotiation it seems that we should follow Blink here. Leaving Accept-Encoding and Accept-Language to their defaults seems okay.
(In reply to Patrick McManus [:mcmanus] from comment #7) > what I'd really rather see is woff2, woff, */* (assuming that's what you > really prefer). Are there registered MIME types for these? Last I heard, there was an attempt to register some types for fonts at the IETF, but it failed. Also, it really seems like the "default" Accept: header is silly; we should set that header for loads of toplevel Web pages, and default everything else to */*.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #9) > (In reply to Patrick McManus [:mcmanus] from comment #7) > > what I'd really rather see is woff2, woff, */* (assuming that's what you > > really prefer). > > Are there registered MIME types for these? Last I heard, there was an > attempt to register some types for fonts at the IETF, but it failed. For WOFF 1.0: http://www.iana.org/assignments/media-types/application/font-woff (For OpenType/TrueType, though perhaps we don't care about explicitly advertising that: http://www.iana.org/assignments/media-types/application/font-sfnt) For WOFF2, application/font-woff2 is proposed in the current draft spec, but is not yet actually registered. There's some talk of trying to register a new top-level type for fonts, so then we'd have font/sfnt, font/woff, font/woff2, etc., but it's unclear whether this will happen. (Of course, what people are actually using is another story altogether....)
Assignee | ||
Comment 11•10 years ago
|
||
So would something like this be what you'd like to have?
Attachment #8501885 -
Flags: review?(mcmanus)
Comment 12•10 years ago
|
||
Comment on attachment 8501885 [details] [diff] [review] Clean up Accept headers on @font-face HTTP requests. Review of attachment 8501885 [details] [diff] [review]: ----------------------------------------------------------------- I think that's fine.. but its a traditional to sort them the other way.. (even our existing one has */* at the end) Accept: application/font-woff2, application/font-woff, */*;q=0.8 its fine if you want to apply q values to the woffs, though in practice its essentially a compatibility list.
Attachment #8501885 -
Flags: review?(mcmanus) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8499435 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b961f0fb00
Target Milestone: --- → mozilla35
https://hg.mozilla.org/mozilla-central/rev/18b961f0fb00
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•