236889 - Load list-style-image from style system, not frames

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

This is a followup to bug 57607.  We should load the images for the
list-style-image property (and possibly content: url()) from the style system.

Comment 1

[Garrett Smith]

Will this resolve the clipping issues with list images?
Will list item markers' intrinsic height be determined by the image? 

I'm noticing that the frame (<div id='contents'>) is expanding on reload,
causing some other rendering problems to occur:
http://bugzilla.mozilla.org/show_bug.cgi?id=206602

Comment 2

[Garrett Smith]

Will this resolve the clipping issues with list images?
Will list item markers' intrinsic height be determined by the image? 

I'm noticing that the frame (<div id='contents'>) is expanding on reload,
causing some other rendering problems to occur here:
http://www.dhtmlkitchen.com/g/affiliates/index.jsp

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Will this resolve the clipping issues with list images?

No.  It may make some testcases work by accident, but the general case will
still be a problem.

> Will list item markers' intrinsic height be determined by the image? 

It should be already.  Is it not?

It sounds like you're talking about bug 187419, which is something I'll
hopefully look into as I work on this....

Comment 4

[Boris Zbarsky [:bzbarsky]]

*** Bug 180142 has been marked as a duplicate of this bug. ***

Comment 5

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

I'll steal this one.

Comment 6

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

Attachment: patch

The changes to nsImageBoxFrame could probably use some explanation: 
mSizeFrozen and mHasImage were always (after Init) opposites of each other, and
mHasImage was basically equivalent to mImageRequest being non-null.  So I
removed both.  The mHasImage check in PaintImage was already duplicated by the
mImageRequest null-check.  The mHasImage && mSizeFrozen case in GetImageSize
was unreachable.  The mHasImage also roughly duplicated the
(mIntrinsicSize.width > 0 && mIntrinsicSize.height > 0) test, although I
improved that by setting mIntrinsicSize.width and height to 0 when releasing
the old image.

GetImageSize was a mess.  It had two different complicated ways of computing
the answer 0 which I simplified by simply typing "0".

I didn't touch the tree code very much because, although the image cache is
probably now unnecessary for images specified with list-style-image, it's still
probably necessary for other ways of specifying images.

Along with the patch to bug 249168 this also fixes the testcase at
http://dbaron.org/css/test/2004/referer/list-style-image

I still need to figure out if this fixes bug 180142 -- I don't see any images
in the testcases.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 152219 [details] [diff] [review]
patch

>Index: layout/html/base/src/nsBulletFrame.cpp

>+            // XXX Do we still want to do this?
>             mImageRequest->Cancel(NS_ERROR_FAILURE);

Yes, we still want to do this in case something else keeps mImageRequest alive
past the point where mBulletListener dies.  The request holds a weak ref to the
listener.

>Index: layout/xul/base/src/nsImageBoxFrame.h

>+   * Update and mUseSrcAttr from appropriate content attributes or from

Remove that "and"?

>Index: layout/xul/base/src/nsImageBoxFrame.cpp

> nsImageBoxFrame::AttributeChanged(nsIPresContext* aPresContext,
>+    // XXX need to ensure AttributeChanged isn't called when attribute changed
>+    // to same value

I'm pretty sure we do ensure that in all the SetAttr functions.  I distinctly
recall fixing that at some point.  So this comment can probably go away.

>+    nsCOMPtr<imgILoader> il(do_GetService("@mozilla.org/image/loader;1", &rv));
...etc

I think I'd rather this used nsContentUtils::LoadImage (have to hack the
makefile to make it includable, but that's not so hard).  For that matter, I
would sort of like it if we did a nsContentUtils::CanLoadImage check here, with
mContent as the context.

>Index: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

>@@ -1661,48 +1664,53 @@ nsTreeBodyFrame::GetImage(PRInt32 aRowIn
>+      nsCOMPtr<imgILoader> il =
>+        do_GetService("@mozilla.org/image/loader;1", &rv);

Again, I'd prefer it if this used nsContentUtils (for example, this code screws
up loadgroups in the call to LoadImage -- it just passes null).

If you really don't have the time to convert those LoadImage callers right now,
file a followup bug on that, ok?

r+sr=bzbarsky with that.  Thanks for picking this up!

Comment 8

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

(In reply to comment #7)
> > nsImageBoxFrame::AttributeChanged(nsIPresContext* aPresContext,
> >+    // XXX need to ensure AttributeChanged isn't called when attribute changed
> >+    // to same value
> 
> I'm pretty sure we do ensure that in all the SetAttr functions.  I distinctly
> recall fixing that at some point.  So this comment can probably go away.

I'm actually not sure this is done for the XUL element code.

Comment 9

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

And I'll file a separate bug for the conversion.  I wouldn't want to do that
without review, either...

Comment 10

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

This caused a significant Tp problem and was backed out ... about 2% on btek and
monkey, and about 5% on silverstone.

Comment 11

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

Landing just the style system part of the patch along with stub changes to the
frame implementations that just got the URI from the imgIRequest caused a ~30ms
change on btek, so the problem is somewhere in the style system or image loading
part of the patch.

Comment 12

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

Loading just the nsCSSDataBlock.cpp and the first line of the nsRuleNode.cpp
changes had pretty much the same effect.

Comment 13

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

Well, it's not the fault of
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpr0n/src/imgRequest.cpp&rev=1.77&mark=241#241
although I'm thinking it could be the fault of having an extra listener...

Comment 14

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

(an extra listener for an animated image, that is)

Comment 15

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

Attachment: this helps the performance problems (but breaks some things)

I suspect the problem has to do with extra listeners for animated images. 
However, this breaks some things too.

Comment 16

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

Filed bug 253627 per comment 9.

Fix checked in to trunk, 2004-07-29 13:07 -0700.

Comment 17

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

Backed out due to bug 253755.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Sounds like people are assuming that list-style-image images block onload
(calling sizeToContent onload and assuming that the list-style-image on the
image box will know its size by then).

We _could_ make list-style-image block onload, or we could have image box frames
post a dummy layout request in Init() and take it down once they know the size.

Ideally, we'd have a way to count "things that are not request but should block
onload" and image box frames knowing their size would be one of those....

Comment 19

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

Why don't they block onload now?

Comment 20

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

nsCSSValue::Image::Image passes the document to nsContentUtils::LoadImage, which
gets the document's load group and passes that to imgLoader, which I'd think
would make the image load block onload.  Why doesn't that work?  (And in both
the old and new code it's the document retrieved from the pres context.)

Comment 21

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

Oh, I see.  Images are only actually added to the load group if there's not a
request already in the cache.  I think case #3 in imgLoader::LoadImage needs to
check if the image in the cache has actually finished loading.

Comment 22

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

Er, no, why should that make any difference?  Whether it's loaded from the
stylesheet or the frame shouldn't make any difference as to whether it's in the
cache.

Comment 23

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

Oh, I see.  How it affects onload depends on whether LOAD_BACKGROUND is set,
which is done in nsCSSValue::Image::Image.

Comment 24

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

Attachment: patch

This makes everything but background block onload, and background uses the env.
var at the caller.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155070 [details] [diff] [review]
patch

r+sr=bzbarsky if you move the env var check back into the constructor there and
change the boolean arg to aIsBGImage as we discussed on irc.

Comment 26

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

Fix checked in to trunk, 2004-08-03 20:23 -0700.

Comment 27

[Uri Bernstein]

Applying this patch (or parts of it) to the Aviary branch might fix bug 255372.
Please see bug 255372 comment 67.