Bug 50311 – [Qt] No way to paint unclipped frame contents with current Qt API.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Viatcheslav Ostapenko]

QWebFrame::render methods always clip painted contents to viewport.

Patch for bug 30530 was intended to replace "QWebFrame::clipContentsToViewport with better API", but it seems original intention have been lost in coding.

[Viatcheslav Ostapenko]

Created attachment 75252 [details]
Return clipRenderToViewport property to allow unclipped painting of frame contents.

[Kenneth Rohde Christiansen]

You need to explain why you need this in the first place. what is the use-case.

[Kenneth Rohde Christiansen]

Comment on attachment 75252 [details]
Return clipRenderToViewport property to allow unclipped painting of frame contents.

I dislike the API, but r- due to not detailing the use-case in the ChangeLog. With the use-case explained we might be able to find a better API.

Like for instance, could it be possible by default to not clip the contents, so that the user of the API would have to do that? or is that not possible?

[Viatcheslav Ostapenko]

(In reply to comment #2)
> You need to explain why you need this in the first place. what is the use-case.

One example is custom backing store implementation, where some content have to be rendered on backing store outside of current viewport.
Or othere cases of rendering on QPixmap or QImage, like this: https://bugs.webkit.org/show_bug.cgi?id=38044

[Viatcheslav Ostapenko]

(In reply to comment #3)
> (From update of attachment 75252 [details])
> I dislike the API, but r- due to not detailing the use-case in the ChangeLog. With the use-case explained we might be able to find a better API.
> Like for instance, could it be possible by default to not clip the contents, so that the user of the API would have to do that? or is that not possible?

Yes, that's was my original suggestion here: https://bugs.webkit.org/show_bug.cgi?id=38044#c10

I think that's was your original intention, as I understand looking at the render functions code. It would be very logical.

But now we have several releases of qtwebkit with this API already and, as Benjamin wrote here https://bugs.webkit.org/show_bug.cgi?id=38044#c13 , we cannot change behavior of public APIs.

Another option is to use QWebElement::render for root element, but problem with it that it doesn't have clip rect/region parameter and will traverse the whole render tree (too much overhead in some cases). Adding cliprect to QWebElement::render would work also.

[Viatcheslav Ostapenko]

(In reply to comment #3)
> (From update of attachment 75252 [details])
> I dislike the API, but r- due to not detailing the use-case in the ChangeLog. With the use-case explained we might be able to find a better API.
> 
> Like for instance, could it be possible by default to not clip the contents, so that the user of the API would have to do that? or is that not possible?

So, what are we going to do with this?
1. Leave it with switcher function to enable unclipped painting.
2. Do not clip to viewport if user have specified clip region. This way it will change API behavior, but the worst thing can happen is painting something extra in the region user have specified.

[Benjamin Poulain]

I also wonder what is the use case. It will be rather strange to render the full page while still having the scrollbar and so on.

One could want a snapshot, but then QWebElement::render() make more sense to me than using the _frame_ to render the whole content. In my mind, the frame is the viewport you have on the content, that is always why frames have scrollbars...

> One example is custom backing store implementation, where some content have to be rendered on backing store outside of current viewport.

This is the last thing we would like to promote. I said that before, if something is wrong with tiling, you can improve it :)


> But now we have several releases of qtwebkit with this API already and, as 
> Benjamin wrote here https://bugs.webkit.org/show_bug.cgi?id=38044#c13 , we 
> cannot change behavior of public APIs.

This is what I said exactly:
"If you want to change the API of QWebFrame::render(), this is behavior breakage of public APIs, and you need to start a thread on the mailing list about that."

To break the public API, start a _discussion_ on the freaking mailing list so anyone has a chance to comment. It is not that it cannot be done, but you have to be open about it. :)

[Viatcheslav Ostapenko]

Created attachment 79824 [details]
Clipped version of QWebElement::render

Added clipped version of QWebElement::render as agreed in email discussion.

[WebKit Review Bot]

Attachment 79824 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebelement.cpp', u'S..." exit_code: 1

Source/WebKit/qt/Api/qwebelement.h:159:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Style issues addressed by https://bugs.webkit.org/show_bug.cgi?id=53017

[Benjamin Poulain]

Comment on attachment 79824 [details]
Clipped version of QWebElement::render

View in context: https://bugs.webkit.org/attachment.cgi?id=79824&action=review

Quick comments.

> Source/WebKit/qt/Api/qwebelement.cpp:1494
> +void QWebElement::render(QPainter* painter, const QRect& clipRect)

I am not sure the name clipRect is that good for the argument. contentRect?

The doc would be clearer IMHO:
"Render the rect \a contentRect of the element into \a painter.

> Source/WebKit/qt/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=50311
> +
> +        * Api/qwebelement.cpp:

Empty changelog is empty :)

> Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1020
> +    QRect barRect(0, 0, barWidth, tableRect.width());

Why is everything called bar?

> QRect barRect(0, 0, barWidth, tableRect.width());
the height of barRect is the width of tableRect? Not sure to follow there.

[Viatcheslav Ostapenko]

Created attachment 80343 [details]
Test fixes and Changelog updates by Benjamin  comments.

[WebKit Review Bot]

Attachment 80343 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebelement.cpp', u'S..." exit_code: 1

Source/WebKit/qt/Api/qwebelement.h:159:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

(In reply to comment #11)
> (From update of attachment 79824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79824&action=review
> 
> Quick comments.
> 
> > Source/WebKit/qt/Api/qwebelement.cpp:1494
> > +void QWebElement::render(QPainter* painter, const QRect& clipRect)
> 
> I am not sure the name clipRect is that good for the argument. contentRect?
> 
> The doc would be clearer IMHO:
> "Render the rect \a contentRect of the element into \a painter.

It follows the style of the QWebFrame::render.
Renamed it to "clip" to be totally consistent with QWebFrame API ;)

> > Source/WebKit/qt/ChangeLog:8
> > +        https://bugs.webkit.org/show_bug.cgi?id=50311
> > +
> > +        * Api/qwebelement.cpp:
> 
> Empty changelog is empty :)

? 
Added some description why this change is made. 
Do you have something more to add there?

> > Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1020
> > +    QRect barRect(0, 0, barWidth, tableRect.width());
> 
> Why is everything called bar?

Because they are vertical bars.

> > QRect barRect(0, 0, barWidth, tableRect.width());
> the height of barRect is the width of tableRect? Not sure to follow there.

Fixed. Worked because they were equal, but it was wrong.

Thanks,
  Sl

[Ademar Reis]

New APIs can't go into a minor release (as is the case with 2.1.x). This change would have to be part of QtWebKit-2.1, which is already a release-candidate.

If the change lands on trunk, it'll probably be part of the next major release (2.2).

[Alexis Menard (darktears)]

Comment on attachment 80343 [details]
Test fixes and Changelog updates by Benjamin  comments.

I don't have any problem with the patch, I think the feature can be useful, QGraphicsScene has the same kind of feature too. My only 2 cents and nitpick is the variable names on the test case. QImage bar looks really debug coding to me :). Why can't you use some normal variable names? You said they are bars but I'm not sure to get your comment. what about chunkHeight, chunkWidth...

[Alexis Menard (darktears)]

Have you consider an API like that instead :

http://doc.qt.nokia.com/latest/qwidget.html#render

or 

http://doc.qt.nokia.com/latest/qgraphicsscene.html#render

They basically do the same things : rendering a part of the subtree (and save painting ops).

[Viatcheslav Ostapenko]

(In reply to comment #16)
> (From update of attachment 80343 [details])
> I don't have any problem with the patch, I think the feature can be useful, QGraphicsScene has the same kind of feature too. My only 2 cents and nitpick is the variable names on the test case. QImage bar looks really debug coding to me :). Why can't you use some normal variable names? You said they are bars but I'm not sure to get your comment. what about chunkHeight, chunkWidth...

Ok. I'll change it.

[Viatcheslav Ostapenko]

(In reply to comment #17)
> Have you consider an API like that instead :
> 
> http://doc.qt.nokia.com/latest/qwidget.html#render
> 
> or 
> 
> http://doc.qt.nokia.com/latest/qgraphicsscene.html#render
> 
> They basically do the same things : rendering a part of the subtree (and save painting ops).

Yes.
Normally, setting clipping on QPainter would work, but going through webkit render tree is CPU consuming also. Adding clip rect stops traversing early for render nodes that do not intersect clip rect.

[Viatcheslav Ostapenko]

Created attachment 83721 [details]
Replace bars with chunks in test.

[WebKit Review Bot]

Attachment 83721 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/Api/qwebelement.cpp', u'S..." exit_code: 1

Source/WebKit/qt/Api/qwebelement.h:159:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 83721 [details]
Replace bars with chunks in test.

r=me

[Benjamin Poulain]

Removing the dependancy. We agreed many time not to add any API to 2.1.x. This has been discussed many time on the mailing list.

[WebKit Commit Bot]

Comment on attachment 83721 [details]
Replace bars with chunks in test.

Clearing flags on attachment: 83721

Committed r79884: <http://trac.webkit.org/changeset/79884>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Misha]

Created attachment 84439 [details]
Workaround patch for qtwebkit 2.1.x

I'm attaching workaround patch for qtwebkit 2.1.x

[Alexis Menard (darktears)]

Comment on attachment 84439 [details]
Workaround patch for qtwebkit 2.1.x

It's ugly...:(...Won't say more...

[Ademar Reis]

Blocking 2.1.x, as discussed in the program meeting and on IRC (will use the workaround recently attached).

[Ademar Reis]

workaround (patch attached to this bug) has been added to 2.1.x as 7f9fbf492c35ed6c8302e1056dc21fbe7072168f