[webkit-reviews] review denied: [Bug 46203] getClientRects()[0] != getBoundingClientRect() : [Attachment 126194] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied  review:
Bug 46203: getClientRects()[0] != getBoundingClientRect()
https://bugs.webkit.org/show_bug.cgi?id=46203

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126194&action=review


You can set cq? from the beginning. If one bot fails, it will set the cq
automatically to cq-.

> LayoutTests/svg/custom/boundingBox.html:5
> +    body { margin: 0 0 0 0; }

margin: 0; would be enough.

> LayoutTests/svg/custom/boundingBox.html:24
> +	   document.getElementById("result").innerHTML =
boundingBoxMatchesExpected ? 
> +	       "PASS" : "FAIL: boundingBox is " + boundingBox.width + " x " +
boundingBox.height + " @ (" + boundingBox.left + ", " + boundingBox.top + "),
but should be  "
> +	       + expectedBoundingBox.width + " x " + expectedBoundingBox.height
+ " @ (" + expectedBoundingBox.left + ", " + expectedBoundingBox.top + ").";

Who is responsible for that?!? Can you order it a bit more and use more lines?
An if else might make it more readable.

> LayoutTests/svg/custom/getClientRects.html:20
> +	       function $(id)
> +	       {

We usually use the same style rules for tests like for normal code, so
function name() {
    ....

> Source/WebCore/ChangeLog:50
> +	       Now, both Element::getClientRects and
Element::getBoundingClientRect use the helper
> +	       method Element::clientQuads, eliminating duplicate code that
adjusted the element's
> +	       absolute quads for viewport zoom and scroll position.

This code differed before your patch!

> Source/WebCore/ChangeLog:60
> +	       Now, this method calls Element::clientQuads. It unites the quads
and returns a single
> +	       bounding ClientRect, as before.

It does something different than before.

> Source/WebCore/dom/Element.cpp:506
> +    RenderObject* ro = renderer();

please use RenderObject* renderer = this->renderer():

At first: WebKit demands full names; second: renderer is common name.

> Source/WebCore/dom/Element.cpp:507
> +    if (isSVGElement() && (!ro || (ro && !ro->isSVGRoot()))) {

that doesn't make sense. Remove the !ro, use:
if (isSVGElement() && renderer && !renderer->isSVGRoot()) instead.

> Source/WebCore/dom/Element.cpp:508
> +	   // This case handles inner SVG elements which do not have a CSS
layout box.

This comment is wrong, at least partly. Just

// This case handles SVG elements which do not have a CSS layout box.

Remove the 'inner'.

> Source/WebCore/dom/Element.cpp:520
> +    if (RenderBoxModelObject* box = renderBoxModelObject()) {
> +	   // This case handles elements with a CSS layout box.

Can you move that to an early return:

RenderBoxModelObject* box = renderBoxModelObject():
if (!box)
    return;

When you are on it, can you name it renderBoxModelObject please?

> Source/WebCore/dom/Element.cpp:552
> +    clientQuads(quads);

This is different than it was before. Before you just asked the current
renderer for the absoluteQuads(quads) and than moved just the first entry. Now
you move every quad in the list.

The problem is...

> Source/WebCore/dom/Element.cpp:559
>      for (size_t i = 1; i < quads.size(); ++i)
>	   result.unite(quads[i].boundingBox());

...that you create the unit from quads that might now be different than on the
previous code.

> Source/WebCore/dom/Element.cpp:-564
> -	   adjustFloatRectForPageScale(result, page->pageScaleFactor());

also here you adjust the result, not every quad.

At least moving every quad instead of just the first is a code difference. I
can not say if it is correct with your patch or before your patch. You have to
test it. http://msdn.microsoft.com/en-us/library/ms536433(VS.85).aspx might
help.

But we can not land it before we are not sure if the code change is correct.
For me it is indeed strange that we just move the first quad in the list on
getBoundingClientRect(). Your behavior seems to make more sense. Nevertheless
we need to verify that.