Closed Bug 236889 Opened 21 years ago Closed 20 years ago

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

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: bzbarsky, Assigned: dbaron)

References

Details

(Whiteboard: [patch])

Attachments

(3 files)

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.
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

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
> 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....
Blocks: 188955
*** Bug 180142 has been marked as a duplicate of this bug. ***
Blocks: 180142
Blocks: 243264
I'll steal this one.
Assignee: bzbarsky → dbaron
Target Milestone: --- → mozilla1.8beta
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Attached patch patchSplinter Review
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.
Attachment #152219 - Flags: superreview?(bzbarsky)
Attachment #152219 - Flags: review?(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!
Attachment #152219 - Flags: superreview?(bzbarsky)
Attachment #152219 - Flags: superreview+
Attachment #152219 - Flags: review?(bzbarsky)
Attachment #152219 - Flags: review+
(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.
And I'll file a separate bug for the conversion.  I wouldn't want to do that
without review, either...
This caused a significant Tp problem and was backed out ... about 2% on btek and
monkey, and about 5% on silverstone.
Blocks: 250578
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.
Loading just the nsCSSDataBlock.cpp and the first line of the nsRuleNode.cpp
changes had pretty much the same effect.
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...
(an extra listener for an animated image, that is)
I suspect the problem has to do with extra listeners for animated images. 
However, this breaks some things too.
Depends on: 252730
Filed bug 253627 per comment 9.

Fix checked in to trunk, 2004-07-29 13:07 -0700.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Backed out due to bug 253755.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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....
Why don't they block onload now?
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.)
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.
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.
Oh, I see.  How it affects onload depends on whether LOAD_BACKGROUND is set,
which is done in nsCSSValue::Image::Image.
Attached patch patchSplinter Review
This makes everything but background block onload, and background uses the env.
var at the caller.
Attachment #155070 - Flags: superreview?(bzbarsky)
Attachment #155070 - Flags: review?(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.
Attachment #155070 - Flags: superreview?(bzbarsky)
Attachment #155070 - Flags: superreview+
Attachment #155070 - Flags: review?(bzbarsky)
Attachment #155070 - Flags: review+
Fix checked in to trunk, 2004-08-03 20:23 -0700.
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Applying this patch (or parts of it) to the Aviary branch might fix bug 255372.
Please see bug 255372 comment 67.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: