Closed Bug 1118926 Opened 5 years ago Closed 4 years ago

Remove -moz-resolution media fragment

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
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.
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?
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.)
Perfect, thanks for the clarification.
Blocks: 1191437
What is the status of this bug?
Depends on: 1201796
(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.
Thanks! :-)
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.
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.
Attachment #8663965 - Flags: review?(tnikkel)
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.
Attachment #8663966 - Flags: review?(dolske)
Another 146 lines deleted in these two patches!
Attachment #8663965 - Flags: review?(tnikkel) → review+
Attachment #8663966 - Flags: review?(dolske) → review?(gijskruitbosch+bugs)
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?
Attachment #8663966 - Flags: review?(gijskruitbosch+bugs) → review+
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!)
Attachment #8663966 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d177c3dbbd15
https://hg.mozilla.org/mozilla-central/rev/f82ed42b855d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1121693
No longer blocks: 1121693
You need to log in before you can comment on or make changes to this bug.