| Summary: | Back TextRun with a StringView | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | darin, dino, jonlee, koivisto, simon.fraser, thorton, zalan | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Myles C. Maxfield
2014-10-02 11:42:30 PDT
Created attachment 239124 [details]
Patch
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 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 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 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 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. |