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

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> 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 41045: Updated QWebElement::render implementation with API autotest
https://bugs.webkit.org/attachment.cgi?id=41045&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

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

Coding style, please place the * to the left :)

> +{
> +    WebCore::Element* e = m_element;
> +    Document* doc = e->document();
> +    if (!doc)
> +	   return;

This is going to crash if the element is a null element. How about this?

Document* doc = e ? e->document() : 0;

> +    QImage resource(":/image.png");
> +    QRect imageRect(0, 0, resource.width(), resource.height());
> +
> +    QImage testImage(resource.width(), resource.height(),
QImage::Format_ARGB32);
> +    QPainter painter0(&testImage);
> +    painter0.fillRect(imageRect, Qt::white);
> +    painter0.drawImage(0, 0, resource);
> +    painter0.end();
> +
> +    QImage image1(resource.width(), resource.height(),
QImage::Format_ARGB32);
> +    QPainter painter1(&image1);
> +    painter1.fillRect(imageRect, Qt::white);
> +    imgs[0].render(&painter1);
> +    painter1.end();
> +
> +    QCOMPARE(image1 == testImage, true);

It's easier to write

QVERIFY(image1 == testImage)

:-)

> +    // render image 2nd time to make sure that cached rendering works fine
> +    QImage image2(resource.width(), resource.height(),
QImage::Format_ARGB32);
> +    QPainter painter2(&image2);
> +    painter2.fillRect(imageRect, Qt::white);
> +    imgs[0].render(&painter2);
> +    painter2.end();
> +
> +    QCOMPARE(image2 == testImage, true);
> +
> +    // compare table rendered through QWebElement::render to whole page
table rendering
> +    QRect tableRect(0, 0, 300, 300);
> +    QList<QWebElement> tables = page.mainFrame()->findAllElements("table");
> +    QCOMPARE(tables.count(), 1);
> +
> +    QImage image3(300, 300, QImage::Format_ARGB32);
> +    QPainter painter3(&image3);
> +    painter3.fillRect(tableRect, Qt::white);
> +    tables[0].render(&painter3);
> +    painter3.end();
> +
> +    QImage image4(300, 300, QImage::Format_ARGB32);
> +    QPainter painter4(&image4);
> +    page.mainFrame()->setClipRenderToViewport(false);
> +    page.mainFrame()->render(&painter4, tableRect);
> +    painter4.end();

It's probably a matter of preference, but instead of allocating QImage123 and
QPainter123 you
could also use scopes. Just an idea :)

> +    QCOMPARE(image3 == image4, true);

QVERIFY() is simpler here, too.

r- because of the missing null pointer check, the rest looks fine to me :)