RESOLVED FIXED 137352
Back TextRun with a StringView 
https://bugs.webkit.org/show_bug.cgi?id=137352
Summary Back TextRun with a StringView
Myles C. Maxfield
Reported 2014-10-02 11:42:30 PDT
Back TextRun with a StringView
Attachments
Patch (9.72 KB, patch)
2014-10-02 11:44 PDT, Myles C. Maxfield 		koivisto: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1 2014-10-02 11:44:28 PDT
Created attachment 239124 [details] Patch
Antti Koivisto
Comment 2 2014-10-02 12:25:47 PDT
Comment on attachment 239124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239124&action=review > Source/WebCore/platform/graphics/TextRun.h:219 > + StringView m_data; m_data is not very descriptive. Just m_string perhaps?
Antti Koivisto
Comment 3 2014-10-02 12:29:15 PDT
Comment on attachment 239124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239124&action=review >> Source/WebCore/platform/graphics/TextRun.h:219 >> + StringView m_data; > > m_data is not very descriptive. Just m_string perhaps? Or m_text.
Antti Koivisto
Comment 4 2014-10-02 12:31:20 PDT
Comment on attachment 239124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239124&action=review > Source/WebCore/platform/graphics/TextRun.h:160 > + String string() const { return m_data.toString(); } This should be called text() to match setText. > Source/WebCore/platform/graphics/TextRun.h:193 > + StringView stringView() const { return m_data; } I don't see any clients to this accessor. Is it needed? If not, please remove it. >>> Source/WebCore/platform/graphics/TextRun.h:219 >>> + StringView m_data; >> >> m_data is not very descriptive. Just m_string perhaps? > > Or m_text. Or m_text.
Antti Koivisto
Comment 5 2014-10-02 12:36:38 PDT
Comment on attachment 239124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239124&action=review >> Source/WebCore/platform/graphics/TextRun.h:160 >> + String string() const { return m_data.toString(); } > > This should be called text() to match setText. Hmm, I guess the name is ok for something that constructs a String. It would be better to have an accessor called text() that returns a StringView and leave it to the callers to construct a String if they really need it.
Myles C. Maxfield
Comment 6 2014-10-02 13:24:29 PDT
Comment on attachment 239124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239124&action=review >>> Source/WebCore/platform/graphics/TextRun.h:160 >>> + String string() const { return m_data.toString(); } >> >> This should be called text() to match setText. > > Hmm, I guess the name is ok for something that constructs a String. It would be better to have an accessor called text() that returns a StringView and leave it to the callers to construct a String if they really need it. I've kept this and renamed stringView() to text(). Done. >> Source/WebCore/platform/graphics/TextRun.h:193 >> + StringView stringView() const { return m_data; } > > I don't see any clients to this accessor. Is it needed? If not, please remove it. Done. >>>> Source/WebCore/platform/graphics/TextRun.h:219 >>>> + StringView m_data; >>> >>> m_data is not very descriptive. Just m_string perhaps? >> >> Or m_text. > > Or m_text. Done.
Myles C. Maxfield
Comment 7 2014-10-02 13:55:01 PDT
http://trac.webkit.org/changeset/174228
Note You need to log in before you can comment on or make changes to this bug.