592641 - Image document doesn't show dimensions of cached images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

ImageListener::OnStopRequest never seems to get called for cached images.

Comment 2

[Bobby Holley (:bholley)]

Off-hand, I would guess that this is a regression from bug 572520. Investigating.

Comment 3

[Bobby Holley (:bholley)]

(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?

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: patch v1

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.

Comment 7

[[e-mail]]

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?]

Comment 8

[Bobby Holley (:bholley)]

Attachment: patch v2

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.

Comment 9

[Bobby Holley (:bholley)]

Attachment: tests - v1

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.

Comment 10

[Bobby Holley (:bholley)]

Pushed this to try:
http://hg.mozilla.org/try/rev/4e9d51770de5

Comment 11

[Bobby Holley (:bholley)]

Attachment: tests - v2

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 12

[[e-mail]]

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.

Comment 13

[[e-mail]]

(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 14

[[e-mail]]

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.

Comment 15

[Bobby Holley (:bholley)]

Attachment: test - v3

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 16

[[e-mail]]

Comment on attachment 473689 [details] [diff] [review]
tests - v2

I actually meant to deny review on this...

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 473785 [details] [diff] [review]
test - v3

In that case, removing the self-review and flagging Neil.

Comment 18

[[e-mail]]

Comment on attachment 473785 [details] [diff] [review]
test - v3

This version is fine although there's probably a way to avoid the duplication.

Comment 19

[Bobby Holley (:bholley)]

did a final push to try as http://hg.mozilla.org/try/rev/3dda0feb0a06

Comment 20

[Bobby Holley (:bholley)]

Pushed to mozilla-central:

Patch: http://hg.mozilla.org/mozilla-central/rev/3496dda447df
Tests: http://hg.mozilla.org/mozilla-central/rev/4252fe859077

Resolving fixed.

Comment 21

[[e-mail]]

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 :-(