[webkit-reviews] review denied: [Bug 80714] [Qt] [WK2] Shouldn't use item for clipping rect calculation in paint node. : [Attachment 131067] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Viatcheslav Ostapenko
<ostapenko.viatcheslav at nokia.com>'s request for review:
Bug 80714: [Qt] [WK2] Shouldn't use item for clipping rect calculation in paint
node.
https://bugs.webkit.org/show_bug.cgi?id=80714

Attachment 131067: Patch
https://bugs.webkit.org/attachment.cgi?id=131067&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=131067&action=review


> Source/WebKit2/ChangeLog:8
> +	   Replace item based clipping rect calculation with clipping nodes
based.

Explain that this is needed for threaded rendering, where you don't have access
to the items.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:121
> +    static void adjustBoundingRect(QRectF& rect, const QPointF& point)

rename uniteRectWithPoint

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:131
> +	   if (rect.top() > point.y())
> +	       rect.setTop(point.y());
> +	   else if (rect.bottom() < point.y())
> +	       rect.setBottom(point.y());
> +
> +	   if (rect.left() > point.x())
> +	       rect.setLeft(point.x());
> +	   else if (rect.right() < point.x())
> +	       rect.setRight(point.x());

rect |= QRectF(point, QSizeF(1, 1));

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:137
> +	   QRectF retRect(0, 0, -1, -1);

retRect -> resultRect

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:139
> +	   while (clip) {

Would be nicer as a for loop
for (const QSGClipNode* clip = clipList(); clip; clip = clip->clipList)

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:140
> +	       QMatrix4x4 m;

m -> matrix or clipMatrix

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:142
> +		   m *= *clip->matrix();

Why multiply instead of assign?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:152
> +		   if (geometry->vertexCount() > 1) {

Shouldn't it be 2? I don't see the point in a single-line clip.
Also, this requires a comment.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:153
> +		       currentClip.setTopLeft(m.map(QPoint(geometryPoints[0].x,
geometryPoints[0].y)));

If you use my unite suggestion, you don't need this and can simply start from
0.