713889 - In Page Info -> Media, the "(animated, N frames)" text is missing from the "Type" line, for animated <img>

Status: RESOLVED INACTIVE

(Firefox :: Page Info Window, defect)

Description

[Alice0775 White]

Attachment: testcase

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

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Axel Hecht [:Pike]]

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

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Dão Gottwald [:dao]]

(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 ;)

Comment 6

[Joe Drew (not getting mail)]

Remember that reading image.animated can throw if we haven't downloaded or decoded the whole image yet.