Seth Fowler [:seth] [:s2h] Assignee
	Description • 9 years ago

The way the -moz-resolution media fragment currently works prevents us from implementing other kinds of media fragments correctly, because it forces us to treat requests for the same URI with different fragment identifiers as different requests.

It's a hack that we should remove. Once downscale-during-decode is implemented for ICO files, it will do everything that -moz-resolution did automatically and transparently.

	Justin Dolske [:Dolske]
	Comment 1 • 9 years ago

Hmm, how does downscale-during-decode fix this?

AIUI we implemented this to hint which image to extract from the ICO (since it's a multi-image container). Depending on the user's display DPI and/or context of where the icon is shown, we would want to select different icons.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 2 • 9 years ago

When downscale-during-decode is implemented for ICO files, it will automatically select the appropriate resolution layer. I have not seen a use-case for selecting it manually.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 3 • 9 years ago

One additional note, because I just realized it might not be clear: part of downscale-during-decode is that the same image can have multiple decoded versions simultaneously. We had the #-moz-resolution media fragment in part because that used to be impossible. So if you draw the same ICO file in two different places in the UI at two different sizes, the correct resolution layer will be decoded and displayed for each.

	Justin Dolske [:Dolske]
	Comment 4 • 9 years ago

So if I have <img src=favicon.ico width=16 height=16> and favicon.ico has 16x16 and 32x32 icon... On a Retina display will this automagically use the 32x32 bitmap at full native resolution, and the 16x16 if you're on a normal display?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 9 years ago

Yup, exactly! It decides based upon what size the image is actually _drawn_ at, in screen pixels. (Well, technically in layer pixels, if we want to get precise.)

	Justin Dolske [:Dolske]
	Comment 6 • 9 years ago

Perfect, thanks for the clarification.

	(no longer active)
	Comment 7 • 9 years ago

What is the status of this bug?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #7)
> What is the status of this bug?

Patches to implement downscale-during-decode for the ICO decoder should arrive soon in bug 1201796. Once that's done, we'll remove #-moz-resolution in this bug.

Also, CC'ing some folks based upon our discussion today on IRC.

	(no longer active)
	Comment 9 • 9 years ago

Thanks! :-)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 9 years ago

We'll handle removing all the remnants of #-moz-resolution throughout the code in this bug, but it turns out that it made more sense to actually disable its behavior in bug 1201796. I'll needinfo Justin there. If anyone else wants to jump into bug 1201796 and give feedback based upon the try build there, I'd appreciate it.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 11 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=041d9b48b902

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 12 • 9 years ago

In bug 1201796, #-moz-resolution stopped working. This patch removes the rest of
the C++ code related to it, which now doesn't actually do anything.

Comment 11 has a try job for this part.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 13 • 9 years ago

This part removes everything related to #-moz-resolution in our frontend
JavaScript.

There were a couple of widely-called methods that added #-moz-resolution to
URIs, so this patch basically just deletes those methods and updates the calling
code to not use them.

I'll upload a try job that includes this part shortly.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 14 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6580f1f75e58

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 15 • 9 years ago

Another 146 lines deleted in these two patches!

	:Gijs (he/him)
	Comment 16 • 9 years ago

Comment on attachment 8663966 [details] [diff] [review]
(Part 2) - Remove remnants of -moz-resolution in JavaScript code

Review of attachment 8663966 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me assuming you actually checked we don't use outsized icons in random places (esp. the history/bookmarks manager + sidebars & menus) now. :-)

One note below on the test.

::: browser/base/content/test/general/browser_action_searchengine_alias.js
@@ -41,1 @@
>    ok(result.getAttribute("image").startsWith(engine.iconURI.spec),

So shouldn't this now just be an equals check?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 17 • 9 years ago

Thanks for the reviews! I've updated the test in part 2 mentioned in comment 16.

As far as I can tell, these patches don't break anything in the UI. (Remember,
-moz-resolution already doesn't do anything today!)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d177c3dbbd15
https://hg.mozilla.org/integration/mozilla-inbound/rev/f82ed42b855d

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/d177c3dbbd15
https://hg.mozilla.org/mozilla-central/rev/f82ed42b855d