Consolidate DrawImage and DrawScaledImage



Core Graveyard
14 years ago
9 years ago


(Reporter: Christopher Aillon (sabbatical, not receiving bugmail), Assigned: Christopher Aillon (sabbatical, not receiving bugmail))


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

They really do the same thing, and share a bunch of code.
Attachment #146630 - Flags: review?(cbiesinger)
I'll try to get to this on saturday

cc'ing some people who may like to know about this change...
Yeah, I planned on getting tor to sr once I had a review.
Comment on attachment 146630 [details] [diff] [review]

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.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
Attachment #146630 - Flags: review?(cbiesinger) → review+
Created attachment 147440 [details] [diff] [review]
Updated per comments
Attachment #146630 - Attachment is obsolete: true
Attachment #147440 - Flags: superreview?(tor)


14 years ago
Attachment #147440 - Flags: superreview?(tor) → superreview+
Comment on attachment 147440 [details] [diff] [review]
Updated per comments

could you also document the units of aSrcRect and aDestRect (looks like twips)?
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).
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.
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 9

14 years ago
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?
This just changed painting, not events, which is what you're seeing... the
PIDOMWindow bug is probably the culprit for the focus/scrolling thing.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.