398066 - Image requests should include image/* in Accept header

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u81239]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7

If I have the following resource:

  /photo/3

And that resource has two representations:

  text/html
  image/jpg

What I would like to do is, have the text/html version be this:

  <a href="2">Previous</a> | <a href="4">Next</a><br>
  <img src="">

So when I browse to a photo with the browser, it will show the HTML
representation with navigation links, which then loads the JPEG representation
inside it. Maybe also a title.

If the Accept header for images had contained image/*, then this would
have worked. However, currently, the HTML representation is chosen over the
image, because both are part of */*. So I couldn’t get this to work. Sending
the following accept header would be an improvement over the current one:

  Accept: image/png,image/*;q=0.8,*/*;q=0.5

I will attach a patch to this effect.

By the way, maybe */*;q=0.5 should be removed? Or are there image types that don’t start with image/?


~Grauw

Reproducible: Always

Steps to Reproduce:
1. Go to the URL http://www.grauw.nl/etc/tech/accept/sneeuw (a testcase)

Actual Results:  
No image appears. If you view the HTTP request that are done, you’ll see the image’s request will get a response with the type text/html.

Expected Results:  
An image of a snowy road should appear.

Comment 1

[u81239]

Attachment: Include image/* in Accept header for image requests

The patch that adds image/*;q=0.8 to the Accept header of image requests.

Comment 2

[Jo Hermans]

(In reply to comment #0)

> By the way, maybe */*;q=0.5 should be removed? Or are there image types that
> don’t start with image/?

'cos MNG used to be sent with video/x-mng (since it was animated)

Comment 3

[Martijn Wargers (dead)]

Comment on attachment 282893 [details] [diff] [review]
Include image/* in Accept header for image requests

I'm not sure how active Darin is currently, I'm adding biesi as an additional reviewer.

Comment 4

[Damon Sicore (:damons)]

Do we need any additional reviews on this before we approve?

Comment 5

[Reed Loden [:reed]]

(In reply to comment #4)
> Do we need any additional reviews on this before we approve?

No, biesi's r= (and implied sr=) should be enough, as he has reviewed lots of libpr0n patches before, but if you all would like, I could request sr= from stuart or tor.

Comment 6

[Damon Sicore (:damons)]

Comment on attachment 282893 [details] [diff] [review]
Include image/* in Accept header for image requests

a1.9+=damons

Comment 7

[Reed Loden [:reed]]

Checking in modules/libpr0n/src/imgLoader.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgLoader.cpp,v  <--  imgLoader.cpp
new revision: 1.95; previous revision: 1.94
done

Comment 8

[u81239]

Very cool, thanks!

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Add test, add CGI support to reftest

Review-pinging dbaron on this so he knows about the reftest change...

Writing the binary data directly from the file is doable but difficult without relying on the exact working directory from which reftests are run, which I'd really rather not do, so I just inlined the data.  It's basically an NxN green square; I don't remember exact dimensions.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 302520 [details] [diff] [review]
Add test, add CGI support to reftest

r=dbaron on the reftest.js change.  Seems simple enough, although I'm sure the complexity lurks elsewhere.

Comment 11

[Jeff Walden [:Waldo]]

Reftest and harness change checked in and passing.