[webkit-reviews] review denied: [Bug 91665] When a block element is made inline positioned and has static left and right, it does not follow inline formatting context : [Attachment 178018] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Pravin D
<pravind.2k4 at gmail.com>'s request for review:
Bug 91665: When a block element is made inline positioned and has static left
and right,  it does not follow inline formatting context
https://bugs.webkit.org/show_bug.cgi?id=91665

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178018&action=review


> Source/WebCore/ChangeLog:13
> +

Please put the result of the performance measurement.

> Source/WebCore/dom/Node.cpp:381
> +    bool wasFloatingOrOutOfFlowPositioned = s1 && (s1->isFloating() ||
s1->hasOutOfFlowPosition());
> +    bool isFloatingOrOutOfFlowPositioned = s2 && (s2->isFloating() ||
s2->hasOutOfFlowPosition());

Nit: It would be more readable if you split the condition into floating and
out-of-flow.

> LayoutTests/ChangeLog:31
> +	      Rebaselined results.

That's not super useful: you are touching some baselines and I can't say
without doing some research if it's a progression or not. Help me help you here
by indicating what's changing.

Here is my interpretation:
* The 2 repaint tests are progressing: we used to repaint the whole page but
now we only paint the old position and the new one.
* full-screen-fixed-pos-parent-expected.txt is orthogonal
* fast/dynamic/002-expected.txt seems that we remove the anonymous wrappers as
we detach after the change.

Last point that we discussed on IRC, I would just drop the cover and migrate
the tests to check-layout.js as it would mean less boiler plate code in your
tests, thus increasing the readibility by a lot!

> LayoutTests/fast/css/first-letter-removed-added-expected.txt:37
> -FAIL document.getElementById('test7').offsetWidth ==
document.getElementById('ref7').offsetWidth should be true. Was false.
> +PASS document.getElementById('test7').offsetWidth ==
document.getElementById('ref7').offsetWidth is true

Woooot!

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:18
> +	  
shouldBe('document.getElementById(\"'+testElems[i].id+'\").offsetTop',
'document.getElementById(\"'+testElems[i].id+'_Cover'+'\").offsetTop');

Usually Javascript follows the same style as C++ (ie spaces before and after
'+').

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:26
> +<h4> Testcase for <a
href="https://bugs.webkit.org/show_bug.cgi?id=91665">Bug #91665. </a> <br> This
testcase checks

There is tons of unneeded spaces in the test. They don't add much and make the
test cases (already massive) less readable.

Also I would put the bug title but it's more a preference than a requirement.

> LayoutTests/fast/dynamic/non-floating-to-floating.html:28
> +<h4> Testcase for <a
href="https://bugs.webkit.org/show_bug.cgi?id=91665">Bug #91665. </a> <br> This
testcase checks
> +if an element is properly placed wrt to its previous sibling when 
float:none changes to float:left for 
> +various combinations of display property toggeled b/w inline-block and block
for both testElement and its previous sibling. </h4> 

Let's not abbreviate:
* wrt => with respect to
* b/w => between

> LayoutTests/fast/dynamic/resources/helper-bug91665.js:4
> +    if(sample.offsetTop != sampleCover.offsetTop) {

This is a definite WTF.

> LayoutTests/fast/dynamic/resources/style-bug91665.css:17
> +.adjustTop20 {
> +  top:-20px;
> +}

FWIW, this name while perfectly fine is tied to 'testElem' being 20px tall. A
more appropriate name would be coverTestElement

> LayoutTests/fast/dynamic/resources/style-bug91665.css:22
> +  left:50px;

Again our regular style is to put a space after a colon.

> LayoutTests/fast/dynamic/resources/style-bug91665.css:24
> +.testElem {

Let's not abbreviate: testElement.