[webkit-reviews] review denied: [Bug 43022] 100% height elements to not respond to vertical browser rescaling : [Attachment 130828] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Terry Anderson
<tdanderson at chromium.org>'s request for review:
Bug 43022: 100% height elements to not respond to vertical browser rescaling
https://bugs.webkit.org/show_bug.cgi?id=43022

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

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


r- because of the tests should really be revised.

> Source/WebCore/rendering/RenderBox.cpp:2345
> +		      
toRenderBlock(cb)->addPercentHeightDescendant(const_cast<RenderBox*>(this));

I think this should be moved after the cb = cb->containingBlock() to match the
other invocations of addPercentHeightDescendant. Also because you may miss the
|cb| when you break out of the |while|. Btw this should be tested if your 2
tests don't cover it.

> LayoutTests/ChangeLog:12
> +	   * fast/replaced/vertical-resize-100percent-contents.html: Added.
> +	   * fast/replaced/vertical-resize-100percent-image.html: Added.

I really don't understand why we need them to be pixel tests here. It really
looks like a getComputedStyle() test would be enough. If not, a ref test would
be better.

> LayoutTests/fast/replaced/vertical-resize-100percent-image.html:16
> +setTimeout(run, 1000);

Please don't check-in a test that takes 1 second to run! We have 25,000+ tests
so imagine if all took 1 second to run!

You could wait for the load event either on the main frame or on a subframe and
resize your frame and that would work AFAICT.