[webkit-dev] Implement threaded model of Coordinated Graphics
Hi, webkit folks. Our team is currently implementing threaded model of Coordinated Graphics in GTK+ port. https://bugs.webkit.org/show_bug.cgi?id=100341 The purpose of sharing is to report our progress and make reviewers easier to understand the overview picture. We have updated the design document in the link below: https://docs.google.com/document/pub?id=1UoI1zk-6nTUFtz8i4evURM8aQIjkDRC8boO1zPdMMBg Our prototype for this implementation is shared in the GitHub. https://github.com/ryumiel/webkit-experimental The prototype is still in development, and only implemented up to step 1 in our design document. Any comments/concerns are appreciated. Best regards, Jae Hyun Park ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
2012/11/15 Ryosuke Niwa rn...@webkit.org: On Thu, Nov 15, 2012 at 4:28 PM, Mike Lawther mikelawt...@google.com wrote: On 16 November 2012 09:59, Ryosuke Niwa rn...@webkit.org wrote: While I don’t want to further agitate the issue or go off on a tangent, and agree that we must address the security aspect before getting rid of RenderArena, only WebKit reviewers can r- patches written by other contributors. You’re not even supposed to set r- on your own patches. See http://www.webkit.org/coding/commit-review-policy.html I see that page says 'Note that you should not put r+ nor r- on patches in such unofficial reviews' with respect to a non-reviewer doing a shadow review. I can't see the extrapolation from that to 'you can't r- your own patches'. I thought r-'ing your own patch was a relatively common practice when uploading a WIP patch, as a signal that 'I have no intention of landing this patch', and as a courtesy so a reviewer will not waste any time looking at it (unless specifically asked). r+ and r- flags are supposed to be set only by reviewers. If you wanted to withdraw your patch from the review queue, then you should be clearing r? flag, instead of setting r-. If you’re uploading a WIP patch, then it should not bear either r?, r-, or r+ flags. You can accomplish this by either not setting the flag when you upload a patch on Bugzilla, clearing flag on the Bugzilla, or using --no-review option on webkit-patch. A patch with r- should be a patch rejected by a reviewer, not a WIP patch the contributor decided not to proceed with. Otherwise, reviewers can’t differentiate the patches rejected by other reviewers and patches authors didn’t like (unless you recognize author’s IRC nickname). - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev Seconded. I also think only the one who submitted the patch can clear the r? flag. Others should NOT do that, please, even you are a reviewer. You can r- the patch if you believe it is bad. -yong ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
From: ext Ryosuke Niwa rn...@webkit.orgmailto:rn...@webkit.org r+ and r- flags are supposed to be set only by reviewers. If you wanted to withdraw your patch from the review queue, then you should be clearing r? flag, instead of setting r-. If you’re uploading a WIP patch, then it should not bear either r?, r-, or r+ flags. You can accomplish this by either not setting the flag when you upload a patch on Bugzilla, clearing flag on the Bugzilla, or using --no-review option on webkit-patch. Regarding WIP patches, what I've seen a few times is us reviewers adding an r- flag to a WIP patch with no r?, when we think it's horribly wrong… I think the flip side of the guideline for non-reviewers to avoid r- is to have reviewers use r- only when the patch is up for review. This will encourage people to use no flags instead of putting r- for WIP patches. No'am ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
Seconded. I also think only the one who submitted the patch can clear the r? flag. Others should NOT do that, please, even you are a reviewer. You can r- the patch if you believe it is bad. I disagree with that. You seem to think that patches falls into either good or bad. However the reality is more complex and there are levels of goodness and badness. I use r- for patches that I really think are not in the right direction or shouldn't be landed: it is a statement in this direction. Clearing the flag is for patches that are close enough but still not up to our standards and that I want to kick off the review queue. This is my reasoning on that and other people likely have different views. However I don't think it's unreasonable to clear the flag instead of r-'ing in some contexts. Julien ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Implement threaded model of Coordinated Graphics
Qt WK2 port already runs layer tree painting on separate thread. Layer tree renderer is created on main thread, but rendering is performed only on paint thread. Qt5 creates paint nodes for painting and QQuickWebPage paint node keeps threaded reference on LayerTreeRenderer. Messages to LayerTreeRenderer are delivered through dispatchUpdate/bind/renderQueue . All updates from renderQueue to renderer are applied in QQuickWebPage::updatePaintNode call. QQuickItem::updatePaintNode is very special call. During this call main thread is locked and it is safe to access main thread objects from paint thread. http://doc-snapshot.qt-project.org/5.0/qquickitem.html#updatePaintNode So, the only thing that needs to be implemented for GTK is replacement for Qt paint node and sync point similar to QQuickItem::updatePaintNode . Slava On Fri, Nov 16, 2012 at 12:44 AM, Jae Hyun Park jae.p...@company100.netwrote: Hi, webkit folks. Our team is currently implementing threaded model of Coordinated Graphics in GTK+ port. https://bugs.webkit.org/show_bug.cgi?id=100341 The purpose of sharing is to report our progress and make reviewers easier to understand the overview picture. We have updated the design document in the link below: https://docs.google.com/document/pub?id=1UoI1zk-6nTUFtz8i4evURM8aQIjkDRC8boO1zPdMMBg Our prototype for this implementation is shared in the GitHub. https://github.com/ryumiel/webkit-experimental The prototype is still in development, and only implemented up to step 1 in our design document. Any comments/concerns are appreciated. Best regards, Jae Hyun Park ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Implement threaded model of Coordinated Graphics
Slava, I think the new proposal is about threading in the web process, not in the UI process like we do in Qt. I've posted some comments on the meta bug, thank you for the heads up on the mailing list! No'am From: ext Vyacheslav Ostapenko osta...@gmail.commailto:osta...@gmail.com Date: Friday, November 16, 2012 10:12 AM To: Jae Hyun Park jae.p...@company100.netmailto:jae.p...@company100.net Cc: webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Implement threaded model of Coordinated Graphics Qt WK2 port already runs layer tree painting on separate thread. Layer tree renderer is created on main thread, but rendering is performed only on paint thread. Qt5 creates paint nodes for painting and QQuickWebPage paint node keeps threaded reference on LayerTreeRenderer. Messages to LayerTreeRenderer are delivered through dispatchUpdate/bind/renderQueue . All updates from renderQueue to renderer are applied in QQuickWebPage::updatePaintNode call. QQuickItem::updatePaintNode is very special call. During this call main thread is locked and it is safe to access main thread objects from paint thread. http://doc-snapshot.qt-project.org/5.0/qquickitem.html#updatePaintNode So, the only thing that needs to be implemented for GTK is replacement for Qt paint node and sync point similar to QQuickItem::updatePaintNode . Slava On Fri, Nov 16, 2012 at 12:44 AM, Jae Hyun Park jae.p...@company100.netmailto:jae.p...@company100.net wrote: Hi, webkit folks. Our team is currently implementing threaded model of Coordinated Graphics in GTK+ port. https://bugs.webkit.org/show_bug.cgi?id=100341 The purpose of sharing is to report our progress and make reviewers easier to understand the overview picture. We have updated the design document in the link below: https://docs.google.com/document/pub?id=1UoI1zk-6nTUFtz8i4evURM8aQIjkDRC8boO1zPdMMBg Our prototype for this implementation is shared in the GitHub. https://github.com/ryumiel/webkit-experimental The prototype is still in development, and only implemented up to step 1 in our design document. Any comments/concerns are appreciated. Best regards, Jae Hyun Park ___ webkit-dev mailing list webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Fri, Nov 16, 2012 at 7:42 AM, Julien Chaffraix julien.chaffr...@gmail.com wrote: Seconded. I also think only the one who submitted the patch can clear the r? flag. Others should NOT do that, please, even you are a reviewer. You can r- the patch if you believe it is bad. I disagree with that. You seem to think that patches falls into either good or bad. However the reality is more complex and there are levels of goodness and badness. I use r- for patches that I really think are not in the right direction or shouldn't be landed: it is a statement in this direction. Clearing the flag is for patches that are close enough but still not up to our standards and that I want to kick off the review queue. This is my reasoning on that and other people likely have different views. However I don't think it's unreasonable to clear the flag instead of r-'ing in some contexts. I would be very confused if anyone but me cleared the r? on one of my bugs, unless it was obvious that the patch was old and buried and I had just forgotten about it. If I think a patch has a few flaws but is on the right track (and I just have questions), I will often leave the r? as-is and expect the contributor to figure it out (meaning that someone else might then be okay to r+ it. I will usually only r- a patch if I think the patch should not be landed as-is and I don't want someone else to r+ it. -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
On Fri, Nov 16, 2012 at 7:16 AM, noam.rosent...@nokia.com wrote: From: ext Ryosuke Niwa rn...@webkit.org r+ and r- flags are supposed to be set only by reviewers. If you wanted to withdraw your patch from the review queue, then you should be clearing r? flag, instead of setting r-. If you’re uploading a WIP patch, then it should not bear either r?, r-, or r+ flags. You can accomplish this by either not setting the flag when you upload a patch on Bugzilla, clearing flag on the Bugzilla, or using --no-review option on webkit-patch. Regarding WIP patches, what I've seen a few times is us reviewers adding an r- flag to a WIP patch with no r?, when we think it's horribly wrong… I think the flip side of the guideline for non-reviewers to avoid r- is to have reviewers use r- only when the patch is up for review. This will encourage people to use no flags instead of putting r- for WIP patches. Yeah, I agree that's misleading as well. On Fri, Nov 16, 2012 at 7:42 AM, Julien Chaffraix julien.chaffr...@gmail.com wrote: Seconded. I also think only the one who submitted the patch can clear the r? flag. Others should NOT do that, please, even you are a reviewer. You can r- the patch if you believe it is bad. I disagree with that. You seem to think that patches falls into either good or bad. However the reality is more complex and there are levels of goodness and badness. I use r- for patches that I really think are not in the right direction or shouldn't be landed: it is a statement in this direction. Clearing the flag is for patches that are close enough but still not up to our standards and that I want to kick off the review queue. I don't think reviewers should be clearing r? flags unless the patch has become obsolete (and/or the author is no longer actively working on it) and we want to get it off of the review queue. I would leave a comment and leave r? or I would r+ it with comments when I feel a patch is at the borderline condition. If you believe the patch should not be landed as is, then you should just r- the patch. If anything, the author can set r? flag back after arguing against your objection. - R.Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] r- your own patches [was: Re: RenderArena: Teaching an old dog new tricks]
2012/11/16 Julien Chaffraix julien.chaffr...@gmail.com: Seconded. I also think only the one who submitted the patch can clear the r? flag. Others should NOT do that, please, even you are a reviewer. You can r- the patch if you believe it is bad. I disagree with that. You seem to think that patches falls into either good or bad. However the reality is more complex and there are levels of goodness and badness. I use r- for patches that I really think are not in the right direction or shouldn't be landed: it is a statement True. I was inaccurate in that statement. Clearing the flag is for patches that are close enough but still not up to our standards and that I want to kick off the review queue. In this case you should still r- it. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
