[webkit-reviews] review denied: [Bug 28781] Add QWebFrame::renderElement to API : [Attachment 39619] Updated QWebElement::render implementation with API autotest

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at trolltech.com> has denied Viatcheslav Ostapenko
<ostapenko.viatcheslav at nokia.com>'s request for review:
Bug 28781: Add QWebFrame::renderElement to API
https://bugs.webkit.org/show_bug.cgi?id=28781

Attachment 39619: Updated QWebElement::render implementation with API autotest
https://bugs.webkit.org/attachment.cgi?id=39619&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at trolltech.com>

> +	   Add QWebElement::render API which allows rendering of single
> +	   element in custom position.

Is the "custom position" part necessary? The render function only takes a
QPainter, custom positioning is implied by QPainter transformation.

> +#include <qpainter.h>

Use the norm of QPainter or QtGui/QPainter like in other files.

> +void QWebElement::render(QPainter *painter)

This function needs the API documentation.

> +    // fall back to full rendering if no cached image

Comment could be better and more descriptive.

> +    pRect.move(-pRect.x(), -pRect.y());
> +    WebCore::RenderObject::PaintInfo cPaintInfo(&context, pRect,
PaintPhaseBlockBackground, false, 0, 0);
> +    WebKitRenderer->paint(cPaintInfo, 0, 0);
> +    cPaintInfo.phase = PaintPhaseForeground;
> +    WebKitRenderer->paint(cPaintInfo, 0, 0);

All the variables don't have prefix, we're not using Hungarian notation in Qt
:)

> +    QImage resource(":/image.png");
> +    QPicture picture1;
> +    QPainter painter1(&picture1);
> +    imgs[0].render(&painter1);
> +    painter1.end();

The idea with QPicture is interesting, but I seriously doubt the test needs to
be complicated.

In principle, just paint to a QImage (of a given size) and compare the pixels
to what you would expect
(you know this because of your test HTML fragment, you can find out e.g. the
position of image.png)

Also no need for ImageDiff code reuse for this, as we simply needs to compare
pixel-per-pixel (ImageDiff supports "fuzzy comparison").
If a pixel is not correct, for this use case we can safely assume that
something has gone wrong.

Painting to QImage, you don't need to worry about settings the graphics system
to raster. It will be raster when doing the painting to image.


r- until these issues are addressed.