Closed Bug 377349 Opened 15 years ago Closed 12 years ago

Page info : Media Tab. The broken image indicator is often wrong

Categories

(Firefox :: Page Info Window, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b1

People

(Reporter: florian, Assigned: mozilla.bugs)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

All the images that can't be found in the cache are grayed out in the tree view of the media tab. But images from the chrome scheme will never be in the cache. Also, if the cache is disabled, it doesn't work any more.

We need a better check to determine if an image is broken or not. For images (img tags) it would be better to check whether the represented dom element has imageloadingcontent but that wouldn't work for background images.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Also, if we land Bug 448630, Ogg Videos and Audio files are rarely in the cache since they are queried in intervals, so they are also indicated as broken.
Depends on: 448630
I don't plan to work on this in the near future, so reassigning to nobody. Feel free to take it.
Assignee: florian → nobody
Status: ASSIGNED → NEW
I don't have a lot of time to work on this, but I would take it on if I had a better idea of what exactly we want implement.

Personally, I suggest having the broken image container used only if the element has activated the "brokenimagecontainer" which removes all dependency on the cache, but it would involve adding HTMLElement type checking to gImageView.getCellProperties.

Another method would be to include more regex tests to that function that would automatically include chrome urls, videos, audio files, and other items that interact differently with the cache.  If we do this, I would like to know which of these we want to ignore in the same way that we already ignore https urls.

I will tentatively assign this to myself for the moment, and I will act on this when we choose which strategy is best.
Assignee: nobody → mozilla.bugs
Status: NEW → ASSIGNED
Graying out and italicizing a line shouldn't be done if the image simply isn't found in the cache.  Grey italics imply that an item is disabled and unusable.  But if an item isn't in the cache, it can still be viewed in the media tab and still be downloaded (albeit as "index" and with no filetype).  If the image isn't cached, the experience is a bit wonky - but it's still functional, so we shouldn't grey out.

On the other hand, if the image loads a broken container and is completely unusable (no preview, no download, etc), we should grey out and italicize.  This implies correctly that the item's unusable.
Attached patch Patch v. 1.0 (obsolete) — Splinter Review
This should only show the broken indicator on elements that load the broken image container.  It creates a new function, checkProtocol(), to share code between the makePreview() and getCellProperties() functions.
Attachment #448961 - Flags: review?(dao)
(In reply to comment #4)
> On the other hand, if the image loads a broken container and is completely
> unusable (no preview, no download, etc), we should grey out and italicize. 
> This implies correctly that the item's unusable.

If I remember well, the original intent with this feature was to give a visual indication that some files weren't usable because they couldn't be loaded (eg. Error 404 or something...). Checking if the file was in the cache was the only way I found at the time to know if the file was loadable without attempting to load each file while building the tree view. In the general case (a page loaded through HTTP, and default cache settings) it worked.

If you decide to grey out the line based on the display of the broken container, the meaning is no longer "this file is unusable", but "Displaying information about this is disabled".
(In reply to comment #6)
> If you decide to grey out the line based on the display of the broken
> container, the meaning is no longer "this file is unusable", but "Displaying
> information about this is disabled".

This would be the preferable behavior, since there are too many cases where this doesn't work the way that it should (by no fault of yours), since even if a file is in the cache, it doesn't mean it will work, and if a file is not in the cache, it doesn't mean it wont load, as with https:// files and ssl caching.  Making the feature mean that the view is disabled is more consistent and understandable for users.

This way for current and future media, we can easily indicate what the window can and can't show regardless of what the server and cache are doing.
Attached patch Patch v. 1.1 (obsolete) — Splinter Review
This removes the gopher protocol which was removed by Bug 388195.
Attachment #448961 - Attachment is obsolete: true
Attachment #452520 - Flags: review?(dao)
Attachment #448961 - Flags: review?(dao)
Comment on attachment 452520 [details] [diff] [review]
Patch v. 1.1

This isn't right. An <img> element will always be an HTMLImageElement whether the image was 404 or not, and also whether the image is in the cache or not.
Attachment #452520 - Flags: review?(dao) → review-
I don't quite understand: how does this patch treat <img> elements as if they were not HTMLImageElements?
Ah, our irc conversation cleared up my misconception. Still r-, however, as it'll be simpler and easier to under stand if you invert the test so that it only tests for things that need the broken tag, rather than testing for things that don't need it.
Attached patch Patch v. 1.2Splinter Review
My plan with the "item instanceof HTMLEmbedElement ||" line is that it will be dropped by Bug 515549, but since this will probably land before it, I will update that patch when this is landed.
Attachment #452520 - Attachment is obsolete: true
Attachment #453307 - Flags: review?(db48x)
Comment on attachment 453307 [details] [diff] [review]
Patch v. 1.2

good, r=db48x
Attachment #453307 - Flags: review?(db48x) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a6
Depends on: 632119
The protocol check added here is just broken.  Not only is it bogus in the face of extension protocols, or even just new ones we add, but the data: part is just wrong.  See bug 632119 and bug 599756.
Depends on: 599756
You need to log in before you can comment on or make changes to this bug.