[webkit-reviews] review granted: [Bug 96848] Text Autosizing: Ignore constrained heights in certain circumstances. : [Attachment 164262] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted John Mellor
<johnme at chromium.org>'s request for review:
Bug 96848: Text Autosizing: Ignore constrained heights in certain
circumstances.
https://bugs.webkit.org/show_bug.cgi?id=96848

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

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


r=me, please leave some time for Kenneth to object if he prefers to go with
evangelism.

> so I'll file a follow-up bug about looking into that but this seems to be the
more practical short term solution.

Please do, detecting the general case where we overflow will remove the need
for this. As it will involve deeper changes (the currently tracked overflows
are not sufficient for our needs), it's better to defer that until we can make
a better assessment.

> Source/WebCore/ChangeLog:8
> +	   Ignores constrained heights on html and body elements, as some sites


typo: Ignore?

> Source/WebCore/rendering/TextAutosizer.cpp:90
> +	       // Some sites (wikipedia) accidentally set their html and/or
body

Some sites (for example wikipedia) ...

>> Source/WebCore/rendering/TextAutosizer.cpp:93
>> +		return !container->isRoot() && !container->isBody();
> 
> Wouldn't it be better to contact wikipedia? I know some people there.

Evangelism would be really nice but reading Adam's comment, it really looks
like we will need this change anyway.

> LayoutTests/fast/text-autosizing/constrained-then-float-ancestors.html:11
> +<script type="text/javascript">

No 'type' attribute needed. This applies to the other tests.