[webkit-reviews] review denied: [Bug 106133] document.body.scrollTop & document.documentElement.scrollTop differ cross-browser : [Attachment 209345] Patch

Thu Aug 22 14:27:19 PDT 2013

Darin Adler <darin at apple.com> has denied gur.trio at gmail.com's request for
review:
Bug 106133: document.body.scrollTop & document.documentElement.scrollTop differ
cross-browser
https://bugs.webkit.org/show_bug.cgi?id=106133

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209345&action=review

> Source/WebCore/dom/Element.cpp:768
> +    bool inQuirksMode = document()->inQuirksMode();
> +    if (document()->documentElement() == this && inQuirksMode)   
> +	   return 0;

Why put inQuirksMode into a local variable since it’s only used once?

> Source/WebCore/dom/Element.cpp:787
> +    bool inQuirksMode = document()->inQuirksMode();
> +    if (document()->documentElement() == this && inQuirksMode)   
> +	   return 0;

Why put inQuirksMode into a local variable since it’s only used once?

> Source/WebCore/html/HTMLBodyElement.cpp:271
>      // Update the document's layout.
> -    Document* document = this->document();
> -    document->updateLayoutIgnorePendingStylesheets();
> +    Document* document = this->document();  
>      FrameView* view = document->view();
> -    return view ? adjustForZoom(view->scrollX(), document) : 0;
> +    if (document->inQuirksMode()) {
> +	   document->updateLayoutIgnorePendingStylesheets();
> +	   return view ? adjustForZoom(view->scrollX(), document) : 0;
> +    }
> +    return 0;

Much cleaner to use early return:

    if (!document->inQuirksMode())
	return 0;

Then you don’t have to touch the rest of the function at all, except probably
for removing the bogus comment “Update the document's layout”, which just says
the same thing that the function name updateLayoutIgnorePendingStylesheets
already does.

> Source/WebCore/html/HTMLBodyElement.cpp:296
>      // Update the document's layout.
> -    Document* document = this->document();
> -    document->updateLayoutIgnorePendingStylesheets();
> +    Document* document = this->document();  
>      FrameView* view = document->view();
> -    return view ? adjustForZoom(view->scrollY(), document) : 0;
> +    if (document->inQuirksMode()) {
> +	   document->updateLayoutIgnorePendingStylesheets();
> +	   return view ? adjustForZoom(view->scrollY(), document) : 0;
> +    }
> +    return 0;

Same suggestion: Use early return.

> LayoutTests/fast/css/zoom-body-scroll.html:3
> -    <div style="width: 1000px; height: 1000px; position: absolute; top: 0;
left: 0;"></div>
> +<body onload="pageScroll()">
> +    <div style="width: 9999px; height: 9999px; position: absolute; top: 0;
left: 0;"></div>

Why the change from 1000 to 9999?

> LayoutTests/fast/css/zoom-body-scroll.html:18
> +	       setTimeout(bodyScroll(),3000);

Why the 3 second delay? We don’t want test that take 3 seconds to run!

> LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:14
> +				setTimeout(function() {

This file still has tab characters in it.

> LayoutTests/fast/dom/Element/scrollLeft-Quirks.html:18
> +		       shouldBe("500","window.pageXOffset");
> +		       shouldBe("500","document.body.scrollLeft");
> +		       shouldBe("0","document.documentElement.scrollLeft");

These are backwards. The test needs to be in the left expression, and the
expected value on the right.

> LayoutTests/fast/dom/Element/scrollLeft.html:19
> +		       shouldBe("500","window.pageXOffset");
> +		       shouldBe("0","document.body.scrollLeft");
> +		       shouldBe("500","document.documentElement.scrollLeft");

These are backwards. The test needs to be in the left expression, and the
expected value on the right.

> LayoutTests/fast/dom/Element/scrollTop-Quirks.html:18
> +		       shouldBe("500","window.pageYOffset");		  
> +				       
shouldBe("500","document.body.scrollTop");
> +		       shouldBe("0","document.documentElement.scrollTop");

These are backwards. The test needs to be in the left expression, and the
expected value on the right. And there are some tabs in here.

> LayoutTests/fast/dom/Element/scrollTop.html:19
> +		       shouldBe("500","window.pageYOffset");
> +		       shouldBe("0","document.body.scrollTop");
> +		       shouldBe("500","document.documentElement.scrollTop");

These are backwards. The test needs to be in the left expression, and the
expected value on the right.

>
LayoutTests/http/tests/navigation/resources/frame-with-anchor-same-origin.html:
18
> -<body>
> +<body onload="runTest()">

How did this test run before? I don’t understand why this ever worked.

> LayoutTests/http/tests/navigation/resources/frame-with-anchor.html:7
> -
> +	  

No reason to add this whitespace.