Closed
Bug 377349
Opened 17 years ago
Closed 14 years ago
Page info : Media Tab. The broken image indicator is often wrong
Categories
(Firefox :: Page Info Window, defect)
Firefox
Page Info Window
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)
3.63 KB,
patch
|
db48x
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•17 years ago
|
Updated•17 years ago
|
Flags: blocking-firefox3?
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Updated•16 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Assignee | ||
Comment 1•14 years ago
|
||
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
Reporter | ||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
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
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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)
Reporter | ||
Comment 6•14 years ago
|
||
(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".
Assignee | ||
Comment 7•14 years ago
|
||
(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.
Assignee | ||
Comment 8•14 years ago
|
||
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 9•14 years ago
|
||
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-
Assignee | ||
Comment 10•14 years ago
|
||
I don't quite understand: how does this patch treat <img> elements as if they were not HTMLImageElements?
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
Comment on attachment 453307 [details] [diff] [review] Patch v. 1.2 good, r=db48x
Attachment #453307 -
Flags: review?(db48x) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 14•14 years ago
|
||
pushed http://hg.mozilla.org/mozilla-central/rev/99d33490f673
Keywords: checkin-needed
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Target Milestone: --- → Firefox 3.7a6
Comment 15•13 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•