[webkit-reviews] review denied: [Bug 6957] Rewrite image rendering with C++ classes and move it to WebCore. : [Attachment 6201] Patch for review.

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Dave Hyatt <hyatt at apple.com>'s request
for review:
Bug 6957: Rewrite image rendering with C++ classes and move it to WebCore.
http://bugzilla.opendarwin.org/show_bug.cgi?id=6957

Attachment 6201: Patch for review.
http://bugzilla.opendarwin.org/attachment.cgi?id=6201&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
In WebHTMLView.m, we still need to handle elements with an image URL but no
loaded image. There are two places that used to check for image of nil and need
some modern equivalent.

+initialize should be removed from WebImageData.m

getCGImageRef should return a CGImageRef, so we don't need to do the type
casting.

CGPDFDocumentRelease can handle nil so we don't need a nil check in
PDFDocumentImage::~PDFDocumentImage.

+    float sina   = sin (m_rotation);
+    float cosa   = cos (m_rotation);

The two above should be sinf and cosf, to avoid converting to double and then
back.

+    rotatedRect.size.width  = ceil(fabs(rx.x - ry.x));
+    rotatedRect.size.height = ceil(fabs(ry.y - rx.y));

These should be ceilf for the same reason.

+    CGContextTranslateCTM(context, floor(-MIN(0,MIN(rx.x, ry.x))),
floor(-MIN(0,MIN(rx.y, ry.y))));

and floorf.

+	 CGRect r;
+    float hScale, vScale;

These can be declared as they are initialized instead of at the top, since it's
C++ now (in PDFDocumentImage::setCurrentPage and PDFDocumentImage::draw).

Why is the ImageData constructor inlined, but not the destructor? Seems it
should be both or neither.

I think m_frameTimer could just be a Timer<ImageData> instead of a
Timer<ImageData>*, unless it's important to save a few buts of data. You would
do m_frameTimer.isActive() instead of checking it for nil and
m_frameTimer.stop() instead of deleting it.

It's strange to do this:

+			 float value = 0.f;
+			 CFNumberGetValue(num, kCFNumberFloat32Type, &value);
+			 m_repetitionCount = (int)value;

I think if we want to get an int, we can do kCFNumberIntType, no need for the
conversion. Also, if we want to get a float we should be using
kCFNumberFloatType, not kCFNumberFloat32Type. I see a couple more times below
where we get floats from CFNumber, just to turn them to ints.

Second CFNumberGetValue needs to use kCFNumberFloatType, not
kCFNumberFloat32Type.

+    IntSize s = size();
+    return s.width() == 0 || s.height() == 0;

You can write the above as return size().isEmpty(). And maybe the operation
should be called isEmpty instead of isNull?

+    if (!m_frames || m_frames[index] == 0)

Why !m_frames, but m_frames[index] == 0? Lets do ! for both.

You didn't fix the tiling code to use setPatternPhaseInUserSpace. Also, I think
the naming of that method is awkward, because the parameter is not "user space"
maybe toPointInUserSpace: woud be better or toUserSpacePoint:.

+	 Image* image = new Image();

I hate those extra parens, but maybe you like them.

IntRect(IntPoint(0,0), IntSize(0,0)) is a long way to write IntRect(). And
IntSize(0,0) can be written IntSize() if you like.

+    if (context == 0)

Usually we use if !context.

+    virtual bool shouldStopAnimation(const Image* image);
+    virtual void animationAdvanced(const Image* image);

I'm "sweet on" leaving out the parameter name in declarations like these.

Those are all my comments for now.