Bug 110490 - [TextAutosizing] Refactoring to eliminate boolean parameter
Summary: [TextAutosizing] Refactoring to eliminate boolean parameter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anton Vayvod
URL:
Keywords:
Depends on:
Blocks: 107300
  Show dependency treegraph
 
Reported: 2013-02-21 10:43 PST by Anton Vayvod
Modified: 2013-02-22 10:22 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.87 KB, patch)
2013-02-21 10:51 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2013-02-21 13:13 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2013-02-22 05:47 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anton Vayvod 2013-02-21 10:43:11 PST
[TextAutosizing] Refactoring to eliminate boolean parameter
Comment 1 Anton Vayvod 2013-02-21 10:51:20 PST
Created attachment 189556 [details]
Patch
Comment 2 John Mellor 2013-02-21 11:09:53 PST
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.
Comment 3 John Mellor 2013-02-21 11:12:10 PST
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?
Comment 4 Anton Vayvod 2013-02-21 13:13:19 PST
Created attachment 189587 [details]
Patch
Comment 5 Julien Chaffraix 2013-02-21 19:54:15 PST
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"
Comment 6 Anton Vayvod 2013-02-22 05:47:13 PST
Created attachment 189760 [details]
Patch
Comment 7 Julien Chaffraix 2013-02-22 09:10:13 PST
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.
Comment 8 WebKit Review Bot 2013-02-22 10:22:05 PST
Comment on attachment 189760 [details]
Patch

Clearing flags on attachment: 189760

Committed r143749: <http://trac.webkit.org/changeset/143749>
Comment 9 WebKit Review Bot 2013-02-22 10:22:09 PST
All reviewed patches have been landed.  Closing bug.