Add WTF::String::emptyString16Bit

Add WTF::String::emptyString16Bit.

This makes sure WTF::String::make16BitFrom8BitSource returns a 16-bit string,
which some callers count on even if the source string happens to be empty.

BUG=412032
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182606

[Mathias Hällman, 2014-09-22 09:57:18]

mathiash@opera.com changed reviewers:
+ abarth@chromium.org

[Mathias Hällman, 2014-09-22 10:01:10]

On 2014/09/22 09:57:18, Mathias Hällman wrote:
> mailto:mathiash@opera.com changed reviewers:
> + mailto:abarth@chromium.org

This issue was introduced by https://codereview.chromium.org/531913002
Could you see if this looks like the right solution here?

[Mikhail, 2014-09-22 10:26:17]

mikhail.pozdnyakov@intel.com changed reviewers:
+ mikhail.pozdnyakov@intel.com

[Mikhail, 2014-09-22 10:26:18]

https://codereview.chromium.org/587173005/diff/1/Source/wtf/text/WTFString.cpp
File Source/wtf/text/WTFString.cpp (right):

https://codereview.chromium.org/587173005/diff/1/Source/wtf/text/WTFString.cp...
Source/wtf/text/WTFString.cpp:1227: return is8Bit ? empty8BitString :
empty16BitString;
I'm afraid this might affect the performance, what about

const String& emptyString() {}

and

enum Force16Bit { Force16Bit };
const String& emptyString(Force16Bit) {}

[Mathias Hällman, 2014-09-23 06:25:08]

On 2014/09/22 10:26:18, Mikhail wrote:
> https://codereview.chromium.org/587173005/diff/1/Source/wtf/text/WTFString.cpp
> File Source/wtf/text/WTFString.cpp (right):
> 
>
https://codereview.chromium.org/587173005/diff/1/Source/wtf/text/WTFString.cp...
> Source/wtf/text/WTFString.cpp:1227: return is8Bit ? empty8BitString :
> empty16BitString;
> I'm afraid this might affect the performance, what about
> 
> const String& emptyString() {}
> 
> and
> 
> enum Force16Bit { Force16Bit };
> const String& emptyString(Force16Bit) {}

I did the same thing to StringImpl::empty, was that unnecessary?

[Mikhail, 2014-09-23 09:48:10]

https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/StringIm...
File Source/wtf/text/StringImpl.h (right):

https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/StringIm...
Source/wtf/text/StringImpl.h:134: , m_is8Bit(tag == ConstructEmpty8BitString)
think this is not enough, see below there is:
STRING_STATS_ADD_8BIT_STRING(m_length);

but it could be 

STRING_STATS_ADD_16BIT_STRING(m_length);

Also introducing an extra comparison ("tag == ConstructEmpty8BitString") is not
positive performance wise, WTF::String code is extremely hot.

IMO it's better to have a separate
enum ConstructEmpty16BitStringTag { ConstructEmpty16BitString };

and a separate constructor.

https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/WTFString.h
File Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/WTFStrin...
Source/wtf/text/WTFString.h:656: enum Force16Bit { Force16BitEmpty };
Sorry, I see that I lead you to a bit wrong direction in my previous comment:
we need to introduce such enum hacks (like enum ConstructEmptyStringTag) only
when we deal with constructors, otherwise a wisely-renamed method/function
should suffice (like "emptyString16" or maybe you propose something better).

[Mathias Hällman, 2014-09-23 11:43:36]

On 2014/09/23 09:48:10, Mikhail wrote:
>
https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/StringIm...
> File Source/wtf/text/StringImpl.h (right):
> 
>
https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/StringIm...
> Source/wtf/text/StringImpl.h:134: , m_is8Bit(tag == ConstructEmpty8BitString)
> think this is not enough, see below there is:
> STRING_STATS_ADD_8BIT_STRING(m_length);
> 
> but it could be 
> 
> STRING_STATS_ADD_16BIT_STRING(m_length);
> 
> Also introducing an extra comparison ("tag == ConstructEmpty8BitString") is
not
> positive performance wise, WTF::String code is extremely hot.
> 
> IMO it's better to have a separate
> enum ConstructEmpty16BitStringTag { ConstructEmpty16BitString };
> 
> and a separate constructor.
> 
>
https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/WTFString.h
> File Source/wtf/text/WTFString.h (right):
> 
>
https://codereview.chromium.org/587173005/diff/20001/Source/wtf/text/WTFStrin...
> Source/wtf/text/WTFString.h:656: enum Force16Bit { Force16BitEmpty };
> Sorry, I see that I lead you to a bit wrong direction in my previous comment:
> we need to introduce such enum hacks (like enum ConstructEmptyStringTag) only
> when we deal with constructors, otherwise a wisely-renamed method/function
> should suffice (like "emptyString16" or maybe you propose something better).

Ah completely missed the bit inside the constructor. Well there we go, all good?
Naming ok?

[aandrey, 2014-09-23 11:50:53]

aandrey@chromium.org changed reviewers:
+ aandrey@chromium.org

[aandrey, 2014-09-23 11:50:54]

https://codereview.chromium.org/587173005/diff/40001/Source/wtf/text/StringSt...
File Source/wtf/text/StringStatics.cpp (right):

https://codereview.chromium.org/587173005/diff/40001/Source/wtf/text/StringSt...
Source/wtf/text/StringStatics.cpp:40: WTF_ANNOTATE_BENIGN_RACE(&emptyString,
"Benign race on StringImpl::emptyString reference counter");
StringImpl::empty

https://codereview.chromium.org/587173005/diff/40001/Source/wtf/text/StringSt...
Source/wtf/text/StringStatics.cpp:47: WTF_ANNOTATE_BENIGN_RACE(&emptyString,
"Benign race on StringImpl::emptyString reference counter");
StringImpl::empty16Bit

[Mathias Hällman, 2014-09-23 11:52:16]

mathiash@opera.com changed reviewers:
- aandrey@chromium.org

[Mathias Hällman, 2014-09-23 12:20:51]

On 2014/09/23 11:50:54, aandrey wrote:
>
https://codereview.chromium.org/587173005/diff/40001/Source/wtf/text/StringSt...
> File Source/wtf/text/StringStatics.cpp (right):
> 
>
https://codereview.chromium.org/587173005/diff/40001/Source/wtf/text/StringSt...
> Source/wtf/text/StringStatics.cpp:40: WTF_ANNOTATE_BENIGN_RACE(&emptyString,
> "Benign race on StringImpl::emptyString reference counter");
> StringImpl::empty
> 
>
https://codereview.chromium.org/587173005/diff/40001/Source/wtf/text/StringSt...
> Source/wtf/text/StringStatics.cpp:47: WTF_ANNOTATE_BENIGN_RACE(&emptyString,
> "Benign race on StringImpl::emptyString reference counter");
> StringImpl::empty16Bit

How about this instead?

[Mathias Hällman, 2014-09-24 12:00:11]

Giving this one a little bump. Is everybody happy with Patch Set 4?

[Mikhail, 2014-09-24 13:47:48]

On 2014/09/24 12:00:11, Mathias Hällman wrote:
> Giving this one a little bump. Is everybody happy with Patch Set 4?

lgtm

[Mathias Hällman, 2014-09-24 14:00:55]

The CQ bit was checked by mathiash@opera.com

[commit-bot: I haz the power, 2014-09-24 14:01:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587173005/60001

[commit-bot: I haz the power, 2014-09-24 15:09:09]

Committed patchset #4 (id:60001) as 182606