241140 - Consolidate DrawImage and DrawScaledImage

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

They really do the same thing, and share a bunch of code.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

I'll try to get to this on saturday

cc'ing some people who may like to know about this change...

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yeah, I planned on getting tor to sr once I had a review.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

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

it would be nice if DrawImage were documented, especially the units of aSrcRect
and aDestRect (looks like twips)

-    float xScaleRatio = float(dr.width) / sr.width;
+    nscoord scaled_x = sr.x;
+    if (dr.width != sr.width) {
+      PRFloat64 scale_ratio = PRFloat64(dr.width) / PRFloat64(sr.width);
+      scaled_x = NSToIntRound(scaled_x * scale_ratio);
+    }

What's the point of this change..?

and, as scaled_x
NSToCoordRound, it seems NSToCoordRound would be the better function to use
than NSToIntRound

(same for the .y/.height change below)

+	   nsRect innerArea(0, 0,
			    mRect.width - (mPadding.left + mPadding.right),
			    mRect.height - (mPadding.top + mPadding.bottom));

hmm, I wonder why not the entire image should be drawn here...

+	 nsRect source(0,0,size,size);
+	 nsRect dest(inner.x, inner.y,size,size);

inconsistent spacing after the comma here...


+	     // Destination rect
+	     nsRect d(paintArea);
+
+	     aRenderingContext.DrawImage(imgCon, &r, &d);

hm, why are you copying paintArea here? DrawImage won't modify the rect (it
gets a pointer to a const rect)

While you're touching this function... would it be a good idea to make the
rects "const nsRect&" instead of "const nsRect*"? looks nicer in my opinion



sorry for not getting to this sooner

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Updated per comments

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 147440 [details] [diff] [review]
Updated per comments

could you also document the units of aSrcRect and aDestRect (looks like twips)?

Comment 7

[Boris Zbarsky [:bzbarsky]]

So.. I think the interface comment on nsIRenderingContext is pretty bogus. 
Something more like this would probably be better:

807   /**
808    *  Draw a piece of an image, scaling it to fit a specified rectangle.
809    *  @param aImage     The image to draw
810    *  @param aSrcRect   The portion (in twips) of the image to draw.
811    *                    [x,y] denotes the top left corner of the region.
812    *  @param aDestRect  The region (in twips) of that aScrRect should
813    *                    occupy. [x,y] denotes the top left corner.
814    *                    [height,width] denotes the desired size.
815    */

The point is that aDestRect is what we want to fill with aSrcRect, not with the
whole image (as the comment made it sound) and that aDestRect is _not_ relative
to the page (it's relative to the device context origin, which will depend on
the exact situation; perhaps we should say "the region in the device context (in
twips)" etc).

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Note that I checked this in 2004-05-03 18:32 thru 18:49 PDT (for some bustage
fixes) with the note about twips.  I'll update the comment.  Marking fixed.

Comment 9

[R.K.Aa.]

between 2004-05-03 14:33:00 and 2004-05-03 22:37:00 something was checked in 
causing bug 242560. First i thought it was the fix for bug 242111, but looking
at this checkin here, it could be relevant as well. Any ideas?

Comment 10

[Boris Zbarsky [:bzbarsky]]

This just changed painting, not events, which is what you're seeing... the
PIDOMWindow bug is probably the culprit for the focus/scrolling thing.

Comment 11

[Boris Zbarsky [:bzbarsky]]

This caused bug 242691