Bug 137352 - Back TextRun with a StringView
Summary: Back TextRun with a StringView
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: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-02 11:42 PDT by Myles C. Maxfield
Modified: 2014-10-02 13:55 PDT (History)
7 users (show)

See Also:

Attachments
Patch (9.72 KB, patch)
2014-10-02 11:44 PDT, Myles C. Maxfield 		koivisto: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-10-02 11:42:30 PDT 
Back TextRun with a StringView
Comment 1 Myles C. Maxfield 2014-10-02 11:44:28 PDT 
Created attachment 239124 [details]
Patch
Comment 2 Antti Koivisto 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?
Comment 3 Antti Koivisto 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.
Comment 4 Antti Koivisto 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.
Comment 5 Antti Koivisto 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.
Comment 6 Myles C. Maxfield 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.
Comment 7 Myles C. Maxfield 2014-10-02 13:55:01 PDT 
http://trac.webkit.org/changeset/174228