[webkit-reviews] review denied: [Bug 26397] Changing position:relative to position:static results in mis-positioned div : [Attachment 168756] Patch

Wed Oct 24 15:28:02 PDT 2012

Ojan Vafai <ojan at chromium.org> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 26397: Changing position:relative to position:static results in
mis-positioned div
https://bugs.webkit.org/show_bug.cgi?id=26397

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168756&action=review

R- mainly for the cpp comment. Conceptually, this looks good and I'm happy to
approve the next iteration.

As far as the test, we don't have strict guidelines on test style, but we try
to keep tests and minimal and clean as possible since we'll need to maintain
them. Also, we try to avoid setTimeout as much as possible as that often leads
to slow and/or flaky tests.

> Source/WebCore/rendering/RenderBlock.cpp:3736
> +	       if (o || (containingBlockPosition ==
ContainingBlockPositionChanged &&
!r->style()->hasStaticBlockPosition(isHorizontalWritingMode())))

I'd rather we change this to only look at containingBlockPosition. I think the
code would be more clear if we just passed in ContainingBlockPositionChanged
for both callers in styleWillChange.

> Source/WebCore/rendering/RenderBlock.h:67
> +enum ContainingBlockPosition { ContainingBlockPositionChanged,
ContainingBlockPositionUnchanged };

How about calling this ContainingBlockPositionDifference or something?
ContainingBlockPosition would make me think it would have position values (e.g.
static, relative, etc).

>
LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.h
tml:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
> +	"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

Typically for new tests we just use the HTML doctype:
<!DOCTYPE html>

Also, can you remove the cruft from this test:
-lang attribute
-type attributes
-no need for head/link/meta elements
-use 4 space tabs, not actual tab characters

>
LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.h
tml:13
> +  if (window.testRunner) {
> +	 testRunner.waitUntilDone();
> +  }

One-line if statements should not have brackets.

>
LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.h
tml:20
> +  window.addEventListener('load', function() {
> +    setTimeout(function() {
> +	var testEl = document.getElementById('container');
> +	testEl.style.position = 'static';
> +	   setTimeout(testRunner.notifyDone(), 0);
> +    }, 0);
> +  }, false);

I think you can get rid of some of these events/setTimeouts and make the test
run faster.

1. Move this block of code to a script element at the end of the body element.
2. Force a layout by grabbing testEl.offsetHeight
3. Change the position.

Then you shouldn't need to waitUntilDone at all and this can be a sync test.

>
LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.h
tml:33
> +	padding: 1em;

This isn't needed?

>
LayoutTests/fast/block/abspos-child-container-changes-from-relative-to-static.h
tml:51
> +	position: absolute;
> +	top: 0;
> +	right: 0;
> +	width: 100px;
> +	height: 100px;

Could move this into a #inner, #marker shared clause to make it more clear that
the only difference between them is the background-color.