201076 - NS_FRAME_HAS_LOADED_IMAGE bit set inconsistently on table cell frames

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[John Keiser (jkeiser)]

The layout regression tests give several false positives because frames with
background images do not always have the has_loaded_image bit set by the time
frame bits are compared.  This may not be a problem in our code; but if not, the
regression tests need to not compare this bit.

NS_FRAME_HAS_LOADED_IMAGE:
layout/html/tests/table/viewer_tests/test4.html TableCell(th)
layout/html/tests/table/bugs/bug10633.html TableCell(td)
layout/html/tests/table/bugs/bug12008.html TableCell(td)
layout/html/tests/table/bugs/bug1271.html TableCell(td)
layout/html/tests/table/bugs/bug1296.html TableCell(td)
layout/html/tests/table/bugs/bug1430.html TableCell(td)
layout/html/tests/table/bugs/bug45055-2.html TableCell(td)

Comment 1

[Bernd]

The bit is set only at one place
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsPresContext.cpp#1450

The LoadImage routine is called from inside the  PaintBackgroundWithSC

http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSRendering.cpp#2965

My guess would be that we start to write the frame dump before the painting has
finished. Does increasing the mDelay at
http://lxr.mozilla.org/seamonkey/source/webshell/tests/viewer/nsWebCrawler.cpp#461
change the picture?

Comment 2

[John Keiser (jkeiser)]

CC'ing bz ... the basic problem is that onload may fire before background images
are loaded.  There are 3 alternatives here that I see:

(1) do not compare the NS_FRAME_HAS_LOADED_IMAGE bit
(2) wait a little while for the image to load (bernd's suggestion in the last
comment)
(3) do not fire onload until background images have loaded.

bz may have insight, especially on this last one.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The last one is kinda hard... for one thing, it involves moving background image
loading out of frames and into content/style.  ;)  That's sorta the plan, but
even then I'm not sure we'll want to delay onload for background images.....
Except perhaps we could in debug builds or have a pref or env var for doing it
in debug builds (so the regression tests could use it).

Though I'm somewhat leery of testing different things between regression tests
and releases....

Comment 4

[John Keiser (jkeiser)]

nah, if we don't want to do it for one we shouldn't do it for anything else.  I
think I prefer the solution of not comparing these bits at all, even though it
may miss a regression or two.  Increasing time between tests does not seem
desirable to me.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

At some point we should start loading images from the stylesheets that reference
them.  That would fix this.  It's also a rather large change.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: remove NS_FRAME_HAS_LOADED_IMAGES bit, since it's write-only

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138914 [details] [diff] [review]
remove NS_FRAME_HAS_LOADED_IMAGES bit, since it's write-only

r+sr=bzbarsky, but it may be a good idea to renumber the last three bits to not
leave a hole.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in, 2004-01-12 17:18 -0800.  I didn't change the rest of the bits --
there are lots of them.