Bug 144712 – Clean up TextRun constructors

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Simplify TextRun to only accept StringViews

[Myles C. Maxfield]

Created attachment 252521 [details]
Patch

[Myles C. Maxfield]

I'm not sure that this is actually better.

[WebKit Commit Bot]

Attachment 252521 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 252537 [details]
Patch

[WebKit Commit Bot]

Attachment 252537 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 252539 [details]
Patch

[WebKit Commit Bot]

Attachment 252539 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

I'm not really sure that this is much better either

[Myles C. Maxfield]

Created attachment 252543 [details]
Patch

[WebKit Commit Bot]

Attachment 252543 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 252544 [details]
Patch

[Myles C. Maxfield]

Created attachment 252545 [details]
Patch

[Myles C. Maxfield]

Committed r183904: <http://trac.webkit.org/changeset/183904>

[Darin Adler]

Comment on attachment 252545 [details]
Patch

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

> Source/WebCore/platform/graphics/TextRun.h:57
> +    explicit TextRun(StringView s, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = AllowTrailingExpansion | ForbidLeadingExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true, RoundingHacks roundingHacks = RunRounding | WordRounding)
> +        : m_text(s)
> +        , m_charactersLength(s.length())

I suggest we consider a variable name other than "s" for this.

> Source/WebCore/platform/graphics/TextRun.h:76
> +    explicit TextRun(const String& s)
> +        : TextRun(StringView(s))
>      {
>      }

We should remove this overload if the performance characteristic of converting the String each time to a StringView is acceptable. If we remove the other overloads below, then this will just automatically do the right thing with explicitly writing code.

> Source/WebCore/platform/graphics/TextRun.h:81
> +    TextRun(const LChar* c, unsigned len)
> +        : TextRun(StringView(c, len))
>      {
>      }

If these overloads are not a performance problem, then I suggest we return to this class and remove them entirely, making the StringView at the call sites. I don’t think we need a convenience that builds a StringView for the caller. Moving the code that makes the StringView back to the call site will also help us make things more elegant in future refactoring.

Also, "c" and "len"? Those don’t sound like WebKit project argument names.

> Source/WebCore/platform/graphics/TextRun.h:86
> +    TextRun(const UChar* c, unsigned len)
> +        : TextRun(StringView(c, len))
>      {
>      }

Ditto.

[Myles C. Maxfield]

Please see https://bugs.webkit.org/show_bug.cgi?id=144752

[Myles C. Maxfield]

Comment on attachment 252545 [details]
Patch

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

>> Source/WebCore/platform/graphics/TextRun.h:81
>>      }
> 
> If these overloads are not a performance problem, then I suggest we return to this class and remove them entirely, making the StringView at the call sites. I don’t think we need a convenience that builds a StringView for the caller. Moving the code that makes the StringView back to the call site will also help us make things more elegant in future refactoring.
> 
> Also, "c" and "len"? Those don’t sound like WebKit project argument names.

It's funny you should mention this - A previous version of this patch (https://bugs.webkit.org/attachment.cgi?id=252539&action=review) did exactly that, but I moved away from it because I thought that littering up the call sites with StringView() calls was just added noise (https://bugs.webkit.org/show_bug.cgi?id=144712#c8)

[Myles C. Maxfield]

Addressed comments in https://bugs.webkit.org/show_bug.cgi?id=144752