WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug