104906 - imgRequest::SniffMimeType and nsUnknownDecoder::SniffForImageMimeType should be one function

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

nsUnknownDecoder::SniffForImageMimeType and imgRequest::SniffMimeType contain
the same code - there should be only one function to avoid code duplication.

Comment 1

[Stuart Parmenter]

accepting

Comment 2

[Stuart Parmenter]

Attachment: Part 1

This patch adds some info to each decoder which is then pulled in and stored in
imgLoader..  need to add accessors from there next.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, I think we need to do this in addition to pavlov's patch:

We need a new interface, say imgIDecoderManager, and a class to implement it,
say with a contractid of @mozilla.org/image/decodermanager.

It has two methods, namely:
bool supportImageWithMimeType(in string mimeType); /* this would would be used
by the places I patched in bug 41333 */
bool supportImageWithContents(in string contents /* or whatever is appropriate
for char* */, integer length);

Do we need more functions there? I don't think an enumerator is necessary...

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

oh yeah, to clarify: supportImageWithContents would do what's currently done by
imgRequest::SniffMimeType and nsUnknownDecoder::SniffForImageMimeType.

Comment 5

[Stuart Parmenter]

just add the stuff to imgILoader since it is already a service

Comment 6

[Boris Zbarsky [:bzbarsky]]

supportImageWithContents has one drawback -- it doesn't return the type.  The
uknown decoder doesn't care whether we can handle the image, it needs to know
what the type is so it can set that type on the channel so that things further
downstream will pick up the right type...

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Oops, I forgot. Well, I'll just add |out nsAString contentType|, then.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: Patch for the interface I suggested

My last comment wasn't quite correct. I changed the return type to string, so
that the mime type is now returned.

pavlov, please review.

Comment 9

[Stuart Parmenter]

Comment on attachment 74358 [details] [diff] [review]
Patch for the interface I suggested

r=pavlov

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

according to smfr, no mac build changes are needed; so it would be great if you
could super-review, tor.

Comment 11

[tor]

Comment on attachment 74358 [details] [diff] [review]
Patch for the interface I suggested

Remove image/x-bitmap check from imgLoader::SupportImageWithContents() (since
we don't support it).
That minor change and sr=tor.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 74358 [details] [diff] [review]
Patch for the interface I suggested

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 13

[Peter Lubczynski]

Has this landed yet?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

arg, this patch no longer builds due to darin's checkin for bug 128508.
I'll change the type of imgRequest::mContentType to nsXPIDLCString (+obvious
changes in imgRequest::SniffMimeType) to fix this.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: un-bitrotted patch

Comment 16

[tor]

Comment on attachment 75748 [details] [diff] [review]
un-bitrotted patch

Trivial changes from previous patch - copying over r/sr/a.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 75748 [details] [diff] [review]
un-bitrotted patch

checked in

but leaving bug open for doing the content check in each decoder, like what
pavlov's patch aims at.

Comment 18

[Jean-Francois Ducarroz]

This fix has caused regression 134106!!

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

marking fixed, that functionality has been added in bug 391667