Closed Bug 1077312 Opened 5 years ago Closed 5 years ago

send a more appropriate Accept: header when fetching webfonts

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(1 file, 1 obsolete file)

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.
And while we're here, I suggest we eliminate the Accept-Language and Accept-Encoding headers when fetching fonts.
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: nobody → jfkthame
Status: NEW → ASSIGNED
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
(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?
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 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)
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 */*.
(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....)
So would something like this be what you'd like to have?
Attachment #8501885 - Flags: review?(mcmanus)
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+
Attachment #8499435 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/18b961f0fb00
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.