593426 - Bump discard timer to something like 1-5 minutes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

limi+beltzner thought this would be preferable. We should do it pretty late in the cycle, since it'll hamper our ability to detect bugs in discarding.

I'm going to propose setting the pref to 1.5 minutes, which means that the actual discarding timeout will range between 1.5 minutes and 3 minutes.

Comment 1

[Joe Drew (not getting mail)]

This doesn't block, but I'd totally take this patch.

Comment 2

[Bobby Holley (:bholley)]

(In reply to comment #1)
> This doesn't block, but I'd totally take this patch.

I think it should. This is the behavior we want, and the only reason we're not doing it now is so that we get better test coverage of our discarding behavior. It's just a 1-line pref patch, and so it won't delay any release. Marking it as a blocker just makes sure that we remember to do it (a useful precaution given that I'm not full-time anymore).

Renominating.

Comment 3

[Joe Drew (not getting mail)]

Okay, okay.

Comment 4

[Bobby Holley (:bholley)]

I still think we should do this for ff4. Flipping that pref will have an enormous UX improvement. We specifically set suboptimal prefs to get better testing feedback, and it seems pretty silly to me that we'd ship that way.

I haven't really kept up with all of the changes with respect to how we treat blockers, so the best I know to do is to renominate. Let me know if there's something different I should do.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

bholley asked me to post a patch for this.

Comment 6

[Joe Drew (not getting mail)]

This just languished too long for 4.0. We will put it in once we branch.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 499740 [details] [diff] [review]
Patch v1

Looks fine, but as I mentioned earlier, this can't go in for 4.0.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

What's the potential risk of taking this? I think upping the timer will probably mean fewer image reloads, though admittedly I can't say that I'm feeling ill effects from the current timeout in the nightlies, so if there's any risk, I agree with Joe that there's no strong need to take it.

Comment 9

[Bobby Holley (:bholley)]

(In reply to comment #8)
> What's the potential risk of taking this? I think upping the timer will
> probably mean fewer image reloads, though admittedly I can't say that I'm
> feeling ill effects from the current timeout in the nightlies, so if there's
> any risk, I agree with Joe that there's no strong need to take it.

I don't believe there's any risk other than the standard "it's a patch" risk. We'd just be changing an arbitrary value, so no code paths would change.

In the common case it doesn't make much different. However it does make a difference when tabbing back to a page with a lot of large images. If they're just under the sync threshold then things will hang momentarily, and if they're over it things will flicker.

Comment 10

[Justin Lebar (not reading bugmail)]

Re-requesting a2.0 based on comments 8 and 9 above.

Comment 11

[Justin Lebar (not reading bugmail)]

This is pretty noticeable on image-heavy sites, such as [1].  Open it, wait 30s or so, and then switch back.  There's a period of a second or two as the images are re-decoded where they're all blank and FF is less responsive.

[1] http://www.boston.com/bigpicture/2011/01/protest_spreads_in_the_middle.html

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 499740 [details] [diff] [review]
Patch v1

If this doesn't land in the next 24 hours, consider your approval RESCINDED!!!

Comment 13

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/b4f2b87f03a0