Simplify TextRun to only accept StringViews
Created attachment 252521 [details] Patch
I'm not sure that this is actually better.
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.
Created attachment 252537 [details] Patch
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.
Created attachment 252539 [details] Patch
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.
I'm not really sure that this is much better either
Created attachment 252543 [details] Patch
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.
Created attachment 252544 [details] Patch
Created attachment 252545 [details] Patch
Committed r183904: <http://trac.webkit.org/changeset/183904>
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.
Please see https://bugs.webkit.org/show_bug.cgi?id=144752
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)
Addressed comments in https://bugs.webkit.org/show_bug.cgi?id=144752