Make calls to AtomicString(const String&) explicit in dom/

Make calls to AtomicString(const String&) explicit in dom/

Make calls to AtomicString(const String&) explicit in dom/ and avoid
useless conversions from String to AtomicString when possible.

This is one step towards making the AtomicString(const String&) constructor
explicit. This CL takes care only of the dom/ part so that it is more
easily reviewable.

R=eseidel
BUG=323739
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164398

[Inactive, 2013-12-13 14:12:11]

On 2013/12/11 14:13:17, Chris Dumez wrote:

ping review?

[abarth-chromium, 2013-12-15 05:31:09]

I suspect eseidel is your best reviewer here.  I haven't been following this
work in detail.

[eseidel, 2013-12-15 16:51:26]

It's always OK to take an AtomicString and pass it as a String. The only cost is
when we take a String and go the other way.

This seems like a lot of hoops to jump through and makes me start to wonder if
we're still winning with this change.

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (left):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp#old...
Source/core/dom/Attr.cpp:144: void Attr::setNodeValue(const String& v)
Does this eventually change to AtomicString too?

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/ClassNodeList.h
File Source/core/dom/ClassNodeList.h (right):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/ClassNodeLis...
Source/core/dom/ClassNodeList.h:42: static PassRefPtr<ClassNodeList>
create(PassRefPtr<Node> rootNode, CollectionType type, const AtomicString&
classNames)
Odd that this would be AtomicString?  I guess it's stored in the DOM as the
class= attribute?

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/DOMTokenList.h
File Source/core/dom/DOMTokenList.h (right):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/DOMTokenList...
Source/core/dom/DOMTokenList.h:74: static AtomicString addToken(const
AtomicString&, const AtomicString&);
Why are these atomic?

[Inactive, 2013-12-16 14:26:26]

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (left):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp#old...
Source/core/dom/Attr.cpp:144: void Attr::setNodeValue(const String& v)
On 2013/12/15 16:51:26, eseidel wrote:
> Does this eventually change to AtomicString too?

The reason I did not update setNodeValue() to take an AtomicString is that this
is a virtual method and not all class implementing it expect an AtomicString.
For e.g. CharacterData::setNodeValue() expects a String.

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/ClassNodeList.h
File Source/core/dom/ClassNodeList.h (right):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/ClassNodeLis...
Source/core/dom/ClassNodeList.h:42: static PassRefPtr<ClassNodeList>
create(PassRefPtr<Node> rootNode, CollectionType type, const AtomicString&
classNames)
On 2013/12/15 16:51:26, eseidel wrote:
> Odd that this would be AtomicString?  I guess it's stored in the DOM as the
> class= attribute?

Yes, I found it a bit weird as well. However, it does get converted to an
AtomicString anyway when we initialize the m_classNames member.
It is a SpaceSplitString and its constructor is defined as follows:
SpaceSplitString(const AtomicString& string, bool shouldFoldCase)

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/ClassNodeLis...
Source/core/dom/ClassNodeList.h:57: SpaceSplitString m_classNames;
See this member.

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/DOMTokenList.h
File Source/core/dom/DOMTokenList.h (right):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/DOMTokenList...
Source/core/dom/DOMTokenList.h:66: virtual void setValue(const AtomicString&) =
0;
This takes an AtomicString.

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/DOMTokenList...
Source/core/dom/DOMTokenList.h:74: static AtomicString addToken(const
AtomicString&, const AtomicString&);
On 2013/12/15 16:51:26, eseidel wrote:
> Why are these atomic?

Because all the callers expect an AtomicString. The returned value is always
passed to DOMTokenList::setValue(const AtomicString&) defined above (see
DOMTokenList.cpp).

These methods use a StringBuilder internally so they can easily generate either
a String or an AtomicString. Since all callers expect an AtomicString, it is
more efficient to construct directly an AtomicString.

[Inactive, 2013-12-30 14:09:13]

Eric, do you still think it is not valuable to go forward with this?

I still think there are a lot of mistakes that could be easily avoided if the
AtomicString(const String&) constructor were marked as explicit.
I can be more conservative and call the AtomicString() constructor explicitly
instead of updating the type from String to AtomicString in cases where you
don't think changing the type is a good idea (or valuable).

[eseidel, 2013-12-30 17:19:26]

lgtm

Please add comments to the code as to *why* the vairous classes chose atomic
sting vs. string, as you understand it, and if you believe those choices to be
correct.  That will save the next person reading the code from having my same
confusion.

Otherwise I'm OK with this as a compromise step towards making explicit
AtomicString(STring) viable.

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (left):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp#old...
Source/core/dom/Attr.cpp:144: void Attr::setNodeValue(const String& v)
On 2013/12/16 14:26:27, Chris Dumez wrote:
> On 2013/12/15 16:51:26, eseidel wrote:
> > Does this eventually change to AtomicString too?
> 
> The reason I did not update setNodeValue() to take an AtomicString is that
this
> is a virtual method and not all class implementing it expect an AtomicString.
> For e.g. CharacterData::setNodeValue() expects a String.

But why are values atomic in the first place?  What are typical node values?

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/ClassNodeList.h
File Source/core/dom/ClassNodeList.h (right):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/ClassNodeLis...
Source/core/dom/ClassNodeList.h:42: static PassRefPtr<ClassNodeList>
create(PassRefPtr<Node> rootNode, CollectionType type, const AtomicString&
classNames)
On 2013/12/16 14:26:27, Chris Dumez wrote:
> On 2013/12/15 16:51:26, eseidel wrote:
> > Odd that this would be AtomicString?  I guess it's stored in the DOM as the
> > class= attribute?
> 
> Yes, I found it a bit weird as well. However, it does get converted to an
> AtomicString anyway when we initialize the m_classNames member.
> It is a SpaceSplitString and its constructor is defined as follows:
> SpaceSplitString(const AtomicString& string, bool shouldFoldCase)

I guess it's common for many elements to share the same set of class names, and
thus same m_classnames string.  So it's OK for it to be atomic.

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/DOMTokenList.h
File Source/core/dom/DOMTokenList.h (right):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/DOMTokenList...
Source/core/dom/DOMTokenList.h:74: static AtomicString addToken(const
AtomicString&, const AtomicString&);
On 2013/12/16 14:26:27, Chris Dumez wrote:
> On 2013/12/15 16:51:26, eseidel wrote:
> > Why are these atomic?
> 
> Because all the callers expect an AtomicString. The returned value is always
> passed to DOMTokenList::setValue(const AtomicString&) defined above (see
> DOMTokenList.cpp).
> 
> These methods use a StringBuilder internally so they can easily generate
either
> a String or an AtomicString. Since all callers expect an AtomicString, it is
> more efficient to construct directly an AtomicString.

You should add comments whenever you're changing these to explain why various
lasses use Atmic vs. Stirng and if they should IYO.

[Inactive, 2013-12-30 19:53:33]

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp
File Source/core/dom/Attr.cpp (left):

https://codereview.chromium.org/112843002/diff/1/Source/core/dom/Attr.cpp#old...
Source/core/dom/Attr.cpp:144: void Attr::setNodeValue(const String& v)
On 2013/12/30 17:19:27, eseidel wrote:
> On 2013/12/16 14:26:27, Chris Dumez wrote:
> > On 2013/12/15 16:51:26, eseidel wrote:
> > > Does this eventually change to AtomicString too?
> > 
> > The reason I did not update setNodeValue() to take an AtomicString is that
> this
> > is a virtual method and not all class implementing it expect an
AtomicString.
> > For e.g. CharacterData::setNodeValue() expects a String.
> 
> But why are values atomic in the first place?  What are typical node values?

Node values are not atomic, they are Strings and I did not change that.
setNodeValue() virtual method is implemented by both Attr and CharacterData.
Using AtomicString for the CharacterData text does not seem to make much sense.
However, Attr uses AtomicString internally to store its value (The value of an
Element's attribute). 

I assume there is a lot of duplication among Element attributes values on a Web
page. Therefore, using AtomicString for them seems to make sense to save memory.
I think calling AtomicString() explicitly here is the right thing to do as I
don't think we want to move CharacterData's data to AtomicString type.

[Inactive, 2013-12-30 20:29:09]

I added the comments as requested. I am going to wait a little bit before
landing to give someone a chance to go over them (as I am not a native speaker).

[eseidel, 2013-12-30 22:31:10]

lgtm

https://codereview.chromium.org/112843002/diff/50001/Source/core/dom/NamedFlo...
File Source/core/dom/NamedFlowCollection.cpp (right):

https://codereview.chromium.org/112843002/diff/50001/Source/core/dom/NamedFlo...
Source/core/dom/NamedFlowCollection.cpp:73: // Takes an AtomicString in argument
because RenderStyle::flowThread() returns an AtomicString.
It would be fine for this to take a String in that case.  This function isn't
asking for an atomized name, only happens to be given one.

When you grab a String or StringImpl out of an AtomicString the underlying impl
is still atomized and caries the isAtomic bit, so converting back into an
AtomicString is cheap.

[Inactive, 2013-12-30 22:39:34]

https://codereview.chromium.org/112843002/diff/50001/Source/core/dom/NamedFlo...
File Source/core/dom/NamedFlowCollection.cpp (right):

https://codereview.chromium.org/112843002/diff/50001/Source/core/dom/NamedFlo...
Source/core/dom/NamedFlowCollection.cpp:73: // Takes an AtomicString in argument
because RenderStyle::flowThread() returns an AtomicString.
On 2013/12/30 22:31:11, eseidel wrote:
> It would be fine for this to take a String in that case.  This function isn't
> asking for an atomized name, only happens to be given one.
> 
> When you grab a String or StringImpl out of an AtomicString the underlying
impl
> is still atomized and caries the isAtomic bit, so converting back into an
> AtomicString is cheap.

Yes, I understand that. However, in this particular case, knowing this is an
AtomicString allows us to use AtomicStringHash instead of StringHash (see
changes below in the same file). I am assuming AtomicStringHash is faster?

[commit-bot: I haz the power, 2013-12-31 14:20:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/112843002/50001

[commit-bot: I haz the power, 2013-12-31 14:27:30]

Change committed as 164398