Closed Bug 713889 Opened 9 years ago Closed 3 years ago
In Page Info -> Media, the "(animated, N frames)" text is missing from the "Type" line, for animated <img>
spun off from Bug 550686 Reproducible: Always Steps to Reproduce: 1. Open the testcase 2. Tools -> Page Info -> Media Actual Results: the frame count for regular images is missing. Expected Results: The frame count should be shown. Regression window: Works; http://hg.mozilla.org/mozilla-central/rev/6281bc7f1bbf Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100813 Minefield/4.0b4pre ID:20100813205257 Fails: http://hg.mozilla.org/mozilla-central/rev/17f4064c1d23 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100813 Minefield/4.0b4pre ID:20100813230431 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6281bc7f1bbf&tochange=17f4064c1d23 Suspecred: Bug 584841
Confirmed on 64-bit linux. My 1.9.2 debug build has: > Type: GIF Image (animated, 21 frames) whereas my nightly build has: > Type: GIF Image It looks like pageInfo gets the number of frames here, by querying image.numFrames (where "image" is an imgIContainer): http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.js?mark=890-892#886 BUT -- that property was indeed removed from the IDL (moved to the C++ class) in bug 584841: http://hg.mozilla.org/mozilla-central/rev/f99cedbbc5f6 > Bug 584841 patch 11: Move remaining imagelib-private methods from imgIContainer interface to Image abstract class. r=bholley sr=vlad a=blocking Note that when the "numFrames" attribute existed, it was under the giant > /************ Internal libpr0n use only below here. *****************/ header in imgIContainer.idl, so it was technically naughty for pageInfo to be using it. I don't think we want to re-expose the number of frames via imgIContainer.idl (joe, correct me if I'm wrong on that). It's a raster-image-specific value, and we shifted away from passing individual frame numbers around (instead passing enum values FRAME_FIRST vs FRAME_CURRENT). Plus, even if it were available, I don't think it's particularly useful piece of information for Page Info to expose. (nice as a "factoid" but not particularly important) However -- the "(animated)" label *is* a useful piece of information to display (and is available on imgIContainer.idl via the boolean "animated" attr). I think we should fix pageInfo to make sure *that* shows up correctly, regardless of whether we expose the frame count.
OS: Windows 7 → All
Hardware: x86 → All
So, to sum up, my vision for this bug would be the following three things: (a) Replace the numFrames check here with an "image.animated" check, to be sure we correctly report when an image is animated: > 911 if (numFrames > 1) > 912 imageType = gBundle.getFormattedString("mediaAnimatedImageType", > 913 [imageType, numFrames]); http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.js?mark=911-913#906 (b) Remove all other mentions of "numFrames" from pageInfo.js (since imagelib no longer exposes that information) (c) Update the "mediaAnimatedImageType" string, to no longer include a frame count: > 64 mediaAnimatedImageType=%S Image (animated, %S frames) http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/pageInfo.properties?mark=64-64#64 Part "c" would be a string change & hence has l10n implications.
Summary: Page Info -> Media, the frame count for regular images is missing. → In Page Info -> Media, the "(animated, N frames)" text is missing from the "Type" line, for animated <img>
For this you'll want to do the standard way of changing a string, that is, changing the key. The "right" way to do so is to choose a good variable name, which would be much easier if the string was mediaAnimatedImageWithFrameCountType ;-). You'll find something, worst case, mediaAnimatedImage2Type
OK, so part "(c)" in comment 2 should instead read: (c) deprecate the "mediaAnimatedImageType" string, and instead use a new string named e.g. "mediaAnimatedImageTypeNoFrameCount" that doesn't include a frame count.
(In reply to Daniel Holbert [:dholbert] from comment #4) > OK, so part "(c)" in comment 2 should instead read: > (c) deprecate the "mediaAnimatedImageType" string, and instead use a new > string > named e.g. "mediaAnimatedImageTypeNoFrameCount" that doesn't include a > frame count. mediaImageTypeAnimated ;)
Remember that reading image.animated can throw if we haven't downloaded or decoded the whole image yet.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.