| Summary: | [Qt] No way to paint unclipped frame contents with current Qt API. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Viatcheslav Ostapenko <ostap73> |
| Component: | WebKit Qt | Assignee: | Viatcheslav Ostapenko <ostap73> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | ademar, benjamin, commit-queue, kenneth, laszlo.gombos, menard, nancy.piedra, tonikitoo, webkit.review.bot |
| Priority: | P3 | Keywords: | Qt, QtTriaged |
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | |||
|
Description
Viatcheslav Ostapenko
2010-11-30 22:27:54 PST
Created attachment 75252 [details]
Return clipRenderToViewport property to allow unclipped painting of frame contents.
You need to explain why you need this in the first place. what is the use-case. 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?
(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 (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. (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. 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. :) Created attachment 79824 [details]
Clipped version of QWebElement::render
Added clipped version of QWebElement::render as agreed in email discussion.
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.
Style issues addressed by https://bugs.webkit.org/show_bug.cgi?id=53017 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. Created attachment 80343 [details]
Test fixes and Changelog updates by Benjamin comments.
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.
(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 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). 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...
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). (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. (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. Created attachment 83721 [details]
Replace bars with chunks in test.
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.
Comment on attachment 83721 [details]
Replace bars with chunks in test.
r=me
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. Comment on attachment 83721 [details] Replace bars with chunks in test. Clearing flags on attachment: 83721 Committed r79884: <http://trac.webkit.org/changeset/79884> All reviewed patches have been landed. Closing bug. Created attachment 84439 [details]
Workaround patch for qtwebkit 2.1.x
I'm attaching workaround patch for qtwebkit 2.1.x
Comment on attachment 84439 [details]
Workaround patch for qtwebkit 2.1.x
It's ugly...:(...Won't say more...
Blocking 2.1.x, as discussed in the program meeting and on IRC (will use the workaround recently attached). workaround (patch attached to this bug) has been added to 2.1.x as 7f9fbf492c35ed6c8302e1056dc21fbe7072168f |