Closed Bug 592641 Opened 10 years ago Closed 10 years ago
Image document doesn't show dimensions of cached images
Steps to reproduce problem: 1. View an image (such as the image linked in the given URL) 2. Click Reload to ensure that the document loads from a cached image Expected results: Title includes image type and dimensions Actual result: Title only includes iamge type Additional information: Shift+Reload redisplays the dimensions. This also has other side-effects, such as breaking image resizing.
ImageListener::OnStopRequest never seems to get called for cached images.
Off-hand, I would guess that this is a regression from bug 572520. Investigating.
(In reply to comment #1) > ImageListener::OnStopRequest never seems to get called for cached images. Hm, on face it seems like there's nothing wrong going on with the notifications. In particular, there is no nsImageListener::OnStopRequest (it inherits from the stub observer, and doesn't override it). Is there something else weird that you saw happening with the notifications?
Err, scratch that - just talked to Neil on IRC and he pointed me to: http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsImageDocument.cpp#241 investigating.
Assignee: nobody → bobbyholley+bmo
blocking2.0: ? → final+
Duplicate of this bug: 585371
Added a patch. Basically, in bug 572520, we made imgIDecoderObserver events asynchronous. This means that imgIDecoderObserver::OnStartContainer can be called _after_ nsIRequest::OnStopRequest. Unfortunately, this code breaks in that case. So the solution is just to listen for the imgIRequest variant of OnStopRequest instead, which is guaranteed to come in the correct order. Flagging Neil for review. He said he'd take a look at it to see if he was comfortable reviewing it, and if not, forward it on to smaug.
Attachment #472817 - Flags: review?(neil)
Comment on attachment 472817 [details] [diff] [review] patch v1 Although the in-place update made the patch easier to review, OnStopDecode should probably be moved to after OnStartContainer in the .cpp file too. (After all, you're changing most of it, so you're not losing that much more blame.) [For some reason this patch triggers corruption warnings, probably due to it missing its trailing newline, perhaps due to an edit?]
Attachment #472817 - Flags: review?(neil) → review+
OS: Windows XP → All
Hardware: x86 → All
Added an updated patch addressing Neil's concerns. Carrying over review. Looks like the corruption issue was the result of a tweak I'd made to git-bz. Should be fixed.
Added some tests. They don't test things as well as they should (see followup bug 594902), but I don't have more time to spend on them at the moment. At least they're something.
Pushed this to try: http://hg.mozilla.org/try/rev/4e9d51770de5
Oh bother - the git-bz change I made also turned off binary diffs. Should be fixed now. Repushed to try as http://hg.mozilla.org/try/rev/2daa948b9308.
Comment on attachment 473689 [details] [diff] [review] tests - v2 I haven't actually tried this out yet. >+ ok(title.match("^bug592641_img\.jpg \\\(JPEG Image, 1500x1500 pixels\\\) - Scaled \\\(\\\d+%\\\)$"), I don't think you can rely on the image being smaller than the window. Perhaps you should just check for 1500 pixels? Also, you should use /regexp/.test(title) instead of trying to use match. >+ // After the first load, try reloading the tab to make sure things work in the >+ // cached case. We're still listening, so loadDone() should get called again. >+ // XXXbholley - For some reason, the mochitest case doesn't seem to be loading the >+ // image from the cache. Unfortunately, I don't really have time to look into it >+ // right now. Some coverage is better than none... Try loading the image into a second tab. >+ // Clean up >+ gBrowser.tabContainer.advanceSelectedTab(1, true); >+ gBrowser.removeCurrentTab(); Use gBrowser.removeTab(imgTab) instead.
(In reply to comment #12) >(From update of attachment 473689 [details] [diff] [review]) > >+ ok(title.match("^bug592641_img\.jpg \\\(JPEG Image, 1500x1500 pixels\\\) - Scaled \\\(\\\d+%\\\)$"), > Also, you should use /regexp/.test(title) instead of trying to use match. Which I just noticed will avoid all your quoting bugs.
Comment on attachment 473689 [details] [diff] [review] tests - v2 In fact it also depends on you having automatic image resizing enabled by default. So it would fail if the pref default was off for whatever reason.
Attachment #473689 - Flags: review?(neil) → review+
Added new tests addressing Neil's comments. With his suggestion of opening a third tab, they now fail properly without the relevant patch. Huzzah! Carrying over review.
Comment on attachment 473689 [details] [diff] [review] tests - v2 I actually meant to deny review on this...
Attachment #473689 - Flags: review+ → review-
Comment on attachment 473785 [details] [diff] [review] test - v3 In that case, removing the self-review and flagging Neil.
Attachment #473785 - Flags: review+ → review?(neil)
Comment on attachment 473785 [details] [diff] [review] test - v3 This version is fine although there's probably a way to avoid the duplication.
Attachment #473785 - Flags: review?(neil) → review+
did a final push to try as http://hg.mozilla.org/try/rev/3dda0feb0a06
Pushed to mozilla-central: Patch: http://hg.mozilla.org/mozilla-central/rev/3496dda447df Tests: http://hg.mozilla.org/mozilla-central/rev/4252fe859077 Resolving fixed.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
D'oh. We wanted to change the title of image documents in bug 136556 but unfortunately that "broke" this test so it got backed out :-(
You need to log in before you can comment on or make changes to this bug.