Johannes Koch Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

1. Use Aurora 33.0a2 (2014-07-26)
2. switch on dom.image.picture.enabled
3. Load http://waldbaer.leute.server.de/picture/
4. See test 'picture: type selection' (this is the third test on the page)
5. read currentSrc


Actual results:

currentSrc is bee_480.webp
No image shown


Expected results:

currentSrc should be bee_480.jpg
Image shown

As Firefox does not support WEBP (AFAIK), the selection algo should select the JPEG image.

	Simon Pieters [:zcorpan]
	Comment 1 • 10 years ago

http://w3c-test.org/submissions/1234/html/semantics/embedded-content/the-img-element/update-the-source-set.html has some tests for <source type>.

	John Schoenick [:johns] Assignee
	Comment 2 • 10 years ago

Per the "update the image data" steps, we can just drop this alongside media

	John Schoenick [:johns] Assignee
	Comment 3 • 10 years ago

This should trigger a mutation identical to media changing -- and since we re-evaluate all <sources> when either happen, we don't actually need to be passing what they changed to around here

	John Schoenick [:johns] Assignee
	Comment 4 • 10 years ago

Just drops the todo from the bug 1037643 tests

Bug 1023519 <picture> tests will cover the non-mutation tests of this

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 10 years ago

Comment on attachment 8502051 [details] [diff] [review]
Part 1 - Drop <picture> sources with unsupported types.

>+!imgLoader::SupportImageWithMimeType(NS_LossyConvertUTF16toASCII(type).get())) {

This is wrong.  Say "type" were "image/giŦ".  That last character is U+0166.  After the lossy conversion, you'd get "image/gif", because it would just drop the high byte, ending up with U+0066, which is "f".

What you want here is a conversion to UTF-8, not ASCII.  Please add a testcase.

Also add some tests that check that "image/GIF" works but "İmage/gif" does not (that first char in the last string is U+0130 LATIN CAPITAL LETTER I WITH DOT ABOVE).

r=me with the lossiness fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

Comment on attachment 8502053 [details] [diff] [review]
Part 2 - Trigger <picture> source selection when a previous source type is updated

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 10 years ago

Comment on attachment 8502055 [details] [diff] [review]
Update picture mutations test

r=me

	John Schoenick [:johns] Assignee
	Comment 8 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #5)
> >+!imgLoader::SupportImageWithMimeType(NS_LossyConvertUTF16toASCII(type).get())) {
>
> This is wrong.  Say "type" were "image/giŦ".  That last character is U+0166.
> After the lossy conversion, you'd get "image/gif", because it would just
> drop the high byte, ending up with U+0066, which is "f".
> What you want here is a conversion to UTF-8, not ASCII.  Please add a
> testcase.
>
> Also add some tests that check that "image/GIF" works but "İmage/gif" does
> not (that first char in the last string is U+0130 LATIN CAPITAL LETTER I
> WITH DOT ABOVE).

You're right, I was looking at if the mime type service expected UTF8 data and saw this pattern being used in other places -- and was under the impression NS_LossyConver* lossy convert was "İmage/gif -> ?mage/gif" or similar. Fixed here, and I'll add unicode variables to the other type tests in bug 1023519

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 10 years ago

> and was under the impression NS_LossyConver* lossy convert was "İmage/gif -> ?mage/gif" 

For that case, the 'İ' would become U+30, which is '0'.  So even with the lossy conversion you would have been OK; the danger with 'İ' is the opposite: doing full-on Unicode case-insensitive comparisons instead of ASCII-only ones.

	Alexander Farkas
	Comment 10 • 10 years ago

While this might be a little bit offtopic, I'm curious, wether and which mime type you do provide to distinguish between apng and normal png support.

	John Schoenick [:johns] Assignee
	Comment 11 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf4e259a4609
https://hg.mozilla.org/integration/mozilla-inbound/rev/6187cd66e661
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e970084e7c0

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8af04bb0dc51

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/bf4e259a4609
https://hg.mozilla.org/mozilla-central/rev/6187cd66e661
https://hg.mozilla.org/mozilla-central/rev/1e970084e7c0

	Johannes Koch Reporter
	Comment 13 • 10 years ago

Works correctly in my test case with Firefox Developer Exidition. Thanks for fixing.