Bug 110490 – [TextAutosizing] Refactoring to eliminate boolean parameter

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anton Vayvod]

[TextAutosizing] Refactoring to eliminate boolean parameter

[Anton Vayvod]

Created attachment 189556 [details]
Patch

[John Mellor]

Comment on attachment 189556 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189556&action=review

> Source/WebCore/rendering/TextAutosizer.cpp:178
> +    float multiplier = clusterShouldBeAutosized(clusterInfo, textWidth) ? clusterMultiplier(clusterInfo, windowInfo, textWidth) : 1.0f;

Nit: Might be clearer to first compute a "bool shouldBeAutosized" on a separate line.

[John Mellor]

This a nice refactoring - definitely makes things clearer (though I had to resort to "git diff --word-diff" to understand the patch!).

Julien (as I think it was your suggestion to remove the boolean parameter), what do you think?

[Anton Vayvod]

Created attachment 189587 [details]
Patch

[Julien Chaffraix]

Comment on attachment 189587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189587&action=review

> Source/WebCore/rendering/TextAutosizer.cpp:157
> +    multiplier = std::max(1.0f, multiplier);
>  
> +    return multiplier;

Nit: These 2 lines could be squashed in one:

return std::max(1.0f, multiplier);

> Source/WebCore/rendering/TextAutosizer.h:32
> +#include <WebCore/platform/text/WritingMode.h>

We don't use absolute path when #including in WebCore. This should be enough:

#include "WritingMode.h"

[Anton Vayvod]

Created attachment 189760 [details]
Patch

[Julien Chaffraix]

Comment on attachment 189760 [details]
Patch

You don't need to ask for review again, only commit-queue so that you are not stuck waiting on a reviewer.

[WebKit Review Bot]

Comment on attachment 189760 [details]
Patch

Clearing flags on attachment: 189760

Committed r143749: <http://trac.webkit.org/changeset/143749>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.