Bug 28781

By comments from irc discussion: Ariya Hidayat: Move method to QWebElement as QWebElement::render((QPainter*) The position argument is not needed, one can always translate the QPainter. Laszlo Gombos: QWebFrame:render will not work in this case because it will draw other elements on top of element we need it they overlap (background image, layers). <richmoore1> ariya: i like QWebElement::render(), i'd asked for that already

Created attachment 38748 [details] QWebElement::render API implementation

Comment on attachment 38748 [details] QWebElement::render API implementation > +void QWebElement::render(QPainter *painter) Need docs > + Frame* fr = doc->frame(); Style, use meaningful variable names. > + if ( !fr || !fr->view() || !fr->contentRenderer()) > + return; > + > + IntRect p_rect = e->getRect(); Same as above + camelCase, not _ Goes for all of the variables in the method. > + void render(QPainter *painter); Need autotest

Created attachment 39619 [details] Updated QWebElement::render implementation with API autotest

(In reply to comment #4) > Created an attachment (id=39619) [details] > Updated QWebElement::render implementation with API autotest Cool feature you are implementing and nice with the test. I didn't look at the patch itself, but the coding style it at least not right, so please fix that first :-) >+ Frame* cFrame = doc->frame(); Why cFrame? what does the c stand for? >+ IntRect pRect = e->getRect(); name here with pRect! This is not WebKit coding style, you need to write out the name, or just use 'frame' and 'rect' >+ WebCore::RenderObject* WebKitRenderer = e->renderer(); This one is even worse. What is a WebKit Renderer? it is actually a RenderObject. Plus, the variable should always start with a lowercase letter. RenderTheme just uses o for RenderObjects, so RenderObject* o = e->renderer() would be OK I guess. Or spell it out renderObject. >+ Image* img = toRenderImage(WebKitRenderer)->cachedImage()->image(); I would spell out img as image >+ WebCore::RenderObject::PaintInfo cPaintInfo(&context, pRect, PaintPhaseBlockBackground, false, 0, 0); In Qt, we try not to pass booleans without comments when it is not obvious what they mean. Examples: setEnabled(true) is right doSomething(rect, true) is wrong doSomething(rect, /* clear cache */ true); That was the code is easier to read and understand afterward.

> > This is not WebKit coding style, you need to write out the name, or just use > 'frame' and 'rect' with this I mean cFrame -> coreFrame (assuming the c means that) and pRect -> painterRect (assuming the p means that) Personally I think frame and rect are better options.

So I looked a bit more at the patch. Ariya added some code to the DumpRenderTree to compare images, maybe that is better for the auto test, than just comparing sizes? The code looks fine, but im wondering about one thing. The cache optimization is only for images right?, so that makes this comment a bit confusing + // fall back to full rendering if no cached image Maybe change the other comment to // Optimization when the render object is an image and has been cached. Then you could leave out the other comment, or write something like // No optimization possible, fall back to normal rendering.

(In reply to comment #5) > (In reply to comment #4) > > Created an attachment (id=39619) [details] [details] > > Updated QWebElement::render implementation with API autotest > > Cool feature you are implementing and nice with the test. > > I didn't look at the patch itself, but the coding style it at least not right, > so please fix that first :-) > > > >+ Frame* cFrame = doc->frame(); > > Why cFrame? what does the c stand for? > > >+ IntRect pRect = e->getRect(); > > name here with pRect! > > This is not WebKit coding style, you need to write out the name, or just use > 'frame' and 'rect' > > >+ WebCore::RenderObject* WebKitRenderer = e->renderer(); > > This one is even worse. What is a WebKit Renderer? it is actually a > RenderObject. Plus, the variable should always start with a lowercase letter. > RenderTheme just uses o for RenderObjects, so > > RenderObject* o = e->renderer() would be OK I guess. Or spell it out > renderObject. > > >+ Image* img = toRenderImage(WebKitRenderer)->cachedImage()->image(); > > I would spell out img as image > > >+ WebCore::RenderObject::PaintInfo cPaintInfo(&context, pRect, PaintPhaseBlockBackground, false, 0, 0); > > In Qt, we try not to pass booleans without comments when it is not obvious what > they mean. > > Examples: > > setEnabled(true) is right > doSomething(rect, true) is wrong > doSomething(rect, /* clear cache */ true); > > That was the code is easier to read and understand afterward. Ok, I'll fix that.

(In reply to comment #7) > So I looked a bit more at the patch. Ariya added some code to the > DumpRenderTree to compare images, maybe that is better for the auto test, than > just comparing sizes? Do you mean ImageDiff in DumpRenderTree/qt? I've tried to do image compare, but it didn't really work because of differences in text rendering on different platforms. I think ImageDiff compare with tolerance value would work. Is it possible to move this functionality somewhere as a library, or should I just copy/paste the code? > The code looks fine, but im wondering about one thing. The cache optimization > is only for images right?, so that makes this comment a bit confusing > > + // fall back to full rendering if no cached image > > Maybe change the other comment to > > // Optimization when the render object is an image and has been cached. > > Then you could leave out the other comment, or write something like > > // No optimization possible, fall back to normal rendering. As I understand, RenderImage is base class for the render objects, that are heavy on rendering/unpacking, like images or video thumbnails, so it's not really limited to images only. I'd suggest 1st comment like: // Optimization when the render object has a cached image. Thanks, Sl

> Do you mean ImageDiff in DumpRenderTree/qt? > I've tried to do image compare, but it didn't really work because of > differences in text rendering on different platforms. > I think ImageDiff compare with tolerance value would work. Is it possible to > move this functionality somewhere as a library, or should I just copy/paste the > code? Yes, I was refering to that. Maybe you also want to default the rendering to use the raster engine when comparing images, as we do with the DRT. (QApplication::setGraphicsSystem("raster") <- has to be before the QApplication is created.) About making it a library, I'm OK with that, but I think you should discuss it with Ariya as he is the author. > As I understand, RenderImage is base class for the render objects, that are > heavy on rendering/unpacking, like images or video thumbnails, so it's not > really limited to images only. > I'd suggest 1st comment like: > // Optimization when the render object has a cached image. Good! :-)

Comment on attachment 39619 [details] Updated QWebElement::render implementation with API autotest > + 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.

Viatcheslav, we would like to defer this new feature until we have released QtWebKit as part of Qt 4.6. We are trying to stabilize the codebase and try to avoid adding new APIs. Do you mind if we look into this again in December?

> 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. My problem, that in test I render table with some text (to test rendering of composite elements). Comparing of images works fine, but text could be rendered differently on different systems, as I understand. Or do you mean I should do image compare for text only and leave "table" test as it is?

> My problem, that in test I render table with some text (to test rendering of > composite elements). > Comparing of images works fine, but text could be rendered differently on > different systems, as I understand. > Or do you mean I should do image compare for text only and leave "table" test > as it is? Think simple. You know where the text is supposed to be located. So draw the text yourself on the reference image.

Created attachment 40143 [details] Updated QWebElement::render implementation with API autotest

(In reply to comment #15) > Created an attachment (id=40143) [details] > Updated QWebElement::render implementation with API autotest Could the implementation be simplified perhaps, similar to Frame::nodeImage() in WebCore/page/win/FrameCGWin.cpp?

(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=40143) [details] [details] > > Updated QWebElement::render implementation with API autotest > > Could the implementation be simplified perhaps, similar to Frame::nodeImage() > in WebCore/page/win/FrameCGWin.cpp? To clarify: This is just a thought/suggestion.

Comment on attachment 40143 [details] Updated QWebElement::render implementation with API autotest > +/*! > + Render the element into \a painter . > +*/ Needs \since tag. > +<file>image.png</file> Can we have other name that resembles the content of the test image? image.png sounds too generic, we might want to add images for future tests. > + QCOMPARE(image1 == testImage, true); I doubt this is going to work properly, we need pixel-by-pixel comparison (see autotest for QImage in Qt source tree). Since this is used in few places, it's a good idea to refactor to a function, e.g. static bool isImageEqual(const QImage &img1, const QImage &img2); Also: see Simon's comment on Frame::nodeImage().

(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=40143) [details] [details] > > Updated QWebElement::render implementation with API autotest > > Could the implementation be simplified perhaps, similar to Frame::nodeImage() > in WebCore/page/win/FrameCGWin.cpp? Thanks a lot! Now painting part looks like: context.save(); context.translate(-rect.x(), -rect.y()); view->setNodeToDraw(e); view->paintContents(&context, rect); view->setNodeToDraw(NULL); context.restore();

(In reply to comment #18) > (From update of attachment 40143 [details]) > > > +/*! > > + Render the element into \a painter . > > +*/ > > Needs \since tag. I assume it should be \since 4.6 But in this case the whole QWebElement class has \since 4.6 . Should I put the same tag also for render method? > > +<file>image.png</file> > > Can we have other name that resembles the content of the test image? > image.png sounds too generic, we might want to add images for future tests. I was sure it will cause questions. This image is just a copy from the qwebframe test. 1st I wanted to make a reference to the qwebframe image, but noticed that all test cases do not share any resources. I would suggest to move all test resource files (images, htmls and etc.) into common resource directory, but could it be done after this patch? > > + QCOMPARE(image1 == testImage, true); > > I doubt this is going to work properly, we need pixel-by-pixel comparison (see > autotest for QImage in Qt source tree). > Since this is used in few places, it's a good idea to refactor to a function, > e.g. > static bool isImageEqual(const QImage &img1, const QImage &img2); What do you mean "work properly"? I've looked at the QImage "==" operator and I think it does exactly what I need. IMHO, in QImage autotest the QImage class functionality is under test and that's why test cannot rely on it. In QWebElement test QImage functionality is assumed to be tested, as I think ;) > Also: see Simon's comment on Frame::nodeImage(). Done! Thanks a lot, Sl

Created attachment 40970 [details] Updated QWebElement::render implementation with API autotest

Comment on attachment 40970 [details] Updated QWebElement::render implementation with API autotest > +void QWebElement::render(QPainter *painter) coding style violation. > + Frame* frame = doc->frame(); > + if ( !frame || !frame->view() || !frame->contentRenderer()) coding style violation. > + FrameView* view = frame->view(); you need a null check here. > + view->setNodeToDraw(NULL); do not use NULL in C++ code. > + QWebPage page; > + QSignalSpy loadSpy(&page, SIGNAL(loadFinished(bool))); > + page.mainFrame()->setHtml(html); > + > + QTest::qWait(200); > + QCOMPARE(loadSpy.count(), 1); Please use the waitForSignal to make this test more robust. > + > + QList<QWebElement> imgs = page.mainFrame()->findAllElements("img"); > + QCOMPARE(imgs.count(), 1); > + > + QImage resource(":/image.png"); > + QRect imageRect(0, 0, resource.width(), resource.height()); do you load resource only for width/height?

> > + FrameView* view = frame->view(); > > you need a null check here. There is null check 2 lines upper. > > + QList<QWebElement> imgs = page.mainFrame()->findAllElements("img"); > > + QCOMPARE(imgs.count(), 1); > > + > > + QImage resource(":/image.png"); > > + QRect imageRect(0, 0, resource.width(), resource.height()); > > do you load resource only for width/height? No. Image get compared to the rendered image. Check testImage - this is resource image painted on white background - the same way as it rendered.

Created attachment 41045 [details] Updated QWebElement::render implementation with API autotest Updates from Holger Freyther comments.

Comment on attachment 41045 [details] Updated QWebElement::render implementation with API autotest > +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 :)

(In reply to comment #25) > (From update of attachment 41045 [details]) > > +void QWebElement::render(QPainter *painter) > Coding style, please place the * to the left :) Fixed. For a long time worked on another project, which has completely different coding style ;) BTW, webkit style check script doesn't detect this. > > +{ > > + 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; Fixed. > > + 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) > :-) Fixed. > > + // 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 :) Yes, that's would be better. I was looking at another test cases and doing the same. Now I prefer it to stay as it is, because if I start changing it a lot there is a good chance to miss-place some spaces ;) Thanks, Sl

Created attachment 41192 [details] Updated QWebElement::render implementation with API autotest

> What do you mean "work properly"? > I've looked at the QImage "==" operator and I think it does exactly what I > need. Ah, you are wrong, I was mistaken. Since you want to have perfect comparison (no fuzzy), then QImage operator= works just fine.

Comment on attachment 41192 [details] Updated QWebElement::render implementation with API autotest > +/*! > + Render the element into \a painter . > +*/ Why is this still missing \since tag?

(In reply to comment #29) > (From update of attachment 41192 [details]) > > +/*! > > + Render the element into \a painter . > > +*/ > > Why is this still missing \since tag? I'm sorry, but you've missed question in my previous reply. The whole qwebelement.cpp file already has \since 4.6 tag. Simon Hausmann added this bug to the 4.6 dependency list, so, as I understand, this decided to go out in the 4.6 release. In this case, does this method need separate \since tag or (as I understand) the whole file \since 4.6 applies to this method also? Thanks, Sl

> I'm sorry, but you've missed question in my previous reply. > The whole qwebelement.cpp file already has \since 4.6 tag. > Simon Hausmann added this bug to the 4.6 dependency list, so, as I understand, > this decided to go out in the 4.6 release. Ah, OK. Sorry for my confusion, I thought it has not been decided whether this is 4.7 or not. If this is 4.6, then even better :)

Comment on attachment 41192 [details] Updated QWebElement::render implementation with API autotest Rejecting patch 41192 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=41192 from bug 28781 failed to download and apply.

Comment on attachment 41192 [details] Updated QWebElement::render implementation with API autotest Patch does not apply cleanly; patching file WebKit/qt/tests/qwebelement/tst_qwebelement.cpp Hunk #1 FAILED at 90. Will land "manually". Laszlo

Landed as http://trac.webkit.org/changeset/49785.