[Webkit-unassigned] [Bug 108205] Text Autosizing: prevent oscillation of font sizes during autosizing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=108205





--- Comment #6 from John Mellor <johnme at chromium.org>  2013-02-15 11:10:42 PST ---
(From update of attachment 188593)
View in context: https://bugs.webkit.org/attachment.cgi?id=188593&action=review

Added some comments.

> Source/WebCore/ChangeLog:8
> +        On some websites font sizes appear to oscillate or grow very large due to

You can delete "appear to", since they really do oscillate.
And it's not really a case of them growing very large, it's more that on some sites it takes many layouts before the font sizes settle to a steady size.

> Source/WebCore/page/Settings.cpp:369
> +    // TODO: I wonder if this needs to traverse frames like in WebViewImpl::resize, or whether there is only one document per Settings instance?

s/TODO/FIXME/ according to WebKit style.

> Source/WebCore/rendering/TextAutosizer.cpp:99
> +            setMultiplier(renderer, 1);

Please add a check for renderer->style()->textAutosizingMultiplier() != 1, as setting the multiplier unnecessarily is quite expensive.

> Source/WebCore/rendering/TextAutosizer.cpp:199
> +            if (localMultiplier != descendant->style()->textAutosizingMultiplier()

Nit: might be simpler/cheaper to write: localMultiplier != 1 && descendant->style()->textAutosizingMultiplier() == 1

> LayoutTests/ChangeLog:8
> +        Changed old tests to layout without the scrollbar initially, because the

A little unclear. Perhaps change this sentence to "Added overflow-y:hidden to some existing tests, since previously those tests would start off with incorrect multipliers (because mainFrame->view()->layoutSize() is initially 785 instead of 800 as ScrollView wrongly guesses a scrollbar will be needed), and then the multipliers would get corrected on a subsequent layout. Now that we don't allow the multiplier to change after being set, it needs to be right first time."

> LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:42
> +if(element.offsetHeight){

Nit: space before ( and { please (ditto below).

> LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:43
> +    // force layout (to computation of offsetHeight triggers reflow)

Nit: s/to //

> LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:45
> +element.className='largersize';

Nit: spaces around = please (ditto below).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.