Closed Bug 592641 Opened 9 years ago Closed 9 years ago

Image document doesn't show dimensions of cached images

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: neil, Assigned: bholley)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

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.
blocking2.0: --- → ?
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
Attached patch patch v1 (obsolete) — Splinter Review
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
Attached patch patch v2Splinter Review
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.
Attachment #472817 - Attachment is obsolete: true
Attachment #473681 - Flags: review+
Attached patch tests - v1 (obsolete) — Splinter Review
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.
Attachment #473683 - Flags: review?(neil)
Attached patch tests - v2 (obsolete) — Splinter Review
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.
Attachment #473683 - Attachment is obsolete: true
Attachment #473689 - Flags: review?(neil)
Attachment #473683 - Flags: review?(neil)
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+
Attached patch test - v3Splinter 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.
Attachment #473689 - Attachment is obsolete: true
Attachment #473785 - Flags: 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: 9 years ago
Resolution: --- → FIXED
Blocks: 596237
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.