208990 - nsImageLoader::RedrawDirtyFrame() invalidates too much

Status: NEW

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Simon Montagu :smontagu]

nsImageLoader::RedrawDirtyFrame() has logic to only invalidate the entire frame
if the frame has a tiled background image, but it is #if 0'd with this comment:

 // XXX this should be ok, but there is some crappy ass bug causing it not to work

Nobody can tell me what the crappy ass bug was, and I have been running without
the #if 0 as dogfood for a few weeks without seeing any problems. I suggest we
take it out.

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

I also had to change the GetStyleContext() call to make it compile in the new
world (changes from bug 114713 and bug 197205 obviously didn't make it into the
#if 0)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125358 [details] [diff] [review]
Patch

This code is wrong.... The image position inside the frame depends on the
background-position.

Note that the body fixup rule is long gone, which is why the code may even
sorta work as long as you don't have background-position set.

Comment 3

[Boris Zbarsky [:bzbarsky]]

roc, is it actually reasonably to try and optimize it?  My gut feeling is no....

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You mean, is it worth it?

I can imagine pages where it's worth it, especially if you've got an animated
image where only part of the image is changing.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  Then we need to somehow factor out the nsCSSRendering code that figures out
image painting anchors and tiling and use it here....

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would call this very low priority, since we don't actually have a testcase
that we're sure would benefit.

Comment 7

[Simon Montagu :smontagu]

Sorry, I filed this while working on bug 124150 but didn't mark dependency or
cross-refer to any test cases. Attachment 111651 [details] is a minimized test case based
on  http://access4less.net/, and comment 4 is right on the button: there's a
massive background image with a relatively tiny animated area.

Comment 8

[Simon Montagu :smontagu]

Thats attachment 111651 [details],
http://bugzilla.mozilla.org/attachment.cgi?id=111651&action=view ;-)

Comment 9

[Simon Montagu :smontagu]

Mass reassign my image bugs to [e-mail]