| Summary: | String constructed from Literals should be non-empty | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
| Component: | Web Template Framework | Assignee: | Benjamin Poulain <benjamin> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | andersca, darin, eric.carlson, feature-media-reviews, ojan.autocc, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Benjamin Poulain
2013-01-28 12:41:40 PST
Created attachment 185038 [details]
Patch
Comment on attachment 185038 [details] Patch Clearing flags on attachment: 185038 Committed r141030: <http://trac.webkit.org/changeset/141030> All reviewed patches have been landed. Closing bug. Comment on attachment 185038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185038&action=review This patch disagrees with itself. > Source/WTF/wtf/text/StringImpl.cpp:154 > + ASSERT_WITH_MESSAGE(length, "Use StringImpl::empty() to create an empty string"); This says you should use StringImpl::empty().T > Source/WebCore/html/HTMLMediaElement.cpp:691 > + canPlay = emptyString(); This uses emptyString(). My rationale: > > Source/WTF/wtf/text/StringImpl.cpp:154 > > + ASSERT_WITH_MESSAGE(length, "Use StringImpl::empty() to create an empty string"); > > This says you should use StringImpl::empty().T The assertion is in StringImpl. You can get here from multiple path. From StringImpl, StringImpl::empty() is the empty String. > > Source/WebCore/html/HTMLMediaElement.cpp:691 > > + canPlay = emptyString(); > > This uses emptyString(). The variable "canPlay" is a WTFString, so here I used the empty string constructor for WTFString. If you think that can cause confusion, I can add similar assertions in WTFString, AtomicString and maybe JSString. No, probably not really confusing in practice. |