Fix a crash when getElementsByClassName() is called twice with the same argument including capital …

Fix a crash when getElementsByClassName() is called twice with the same argument including capital letters.

This fixes a regression by crrev.com/474217, which deleted a protector AtomicString
accidentally. This CL removes a tricky StringImpl* hash key in NodeListsNodeData.

BUG=725929
R=yosin@chromium.org

Review-Url: https://codereview.chromium.org/2903373002 .
Cr-Commit-Position: refs/heads/master@{#474870}
Committed: https://chromium.googlesource.com/chromium/src/+/9cf04000e65705ebced49e4bc7e04ae0d2c7d895

[tkent, 2017-05-25 23:43:26]

The CQ bit was checked by tkent@chromium.org to run a CQ dry run

[tkent, 2017-05-25 23:43:56]

Description was changed from

==========
Fix a crash when getElementsByClassName() is called twice with the same argument
including capital letters.

This is a regression by crrev.com/474217, which deletes a protector AtomicString
accidentally. This CL removes a tricky StringImpl* hash key in
NodeListsNodeData.

BUG=725929
==========

to

==========
Fix a crash when getElementsByClassName() is called twice with the same argument
including capital letters.

This fixes a regression by crrev.com/474217, which deleted a protector
AtomicString
accidentally. This CL removes a tricky StringImpl* hash key in
NodeListsNodeData.

BUG=725929
==========

[commit-bot: I haz the power, 2017-05-25 23:44:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-05-26 00:43:16]

tkent@chromium.org changed reviewers:
+ yosin@chromium.org

[tkent, 2017-05-26 00:43:17]

yosin@, would you review this please? This fixes the top crasher on Canary.

[yosin_UTC9, 2017-05-26 01:35:41]

lgtm

We need to keep |String| member of |AtomicString| in |std::pair<unsigned char,
AtomicString>| in
key of |HashMap<K, V>| in |NodeListNodeData|.

BTW, it is better to use |std::pair<CollectionType, AtomicString>| instead of
|unsigned char|
with declaring |enum CollectionType : uint8_t|.

However, I don't think using |unsigned char| can save memory since there are
padding to
align to |sizeof(void*)| between |unsigned char| and |AtomicString == String ==
RefPtr<StringImpl>
== StringImpl*|

[tkent, 2017-05-26 01:40:49]

On 2017/05/26 at 01:35:41, yosin wrote:
> BTW, it is better to use |std::pair<CollectionType, AtomicString>| instead of
|unsigned char|
> with declaring |enum CollectionType : uint8_t|.

Yeah, I agree that |unsigned char| here doesn't make sense.  I'll change it in a
separated CL.

[tkent, 2017-05-26 01:42:55]

Description was changed from

==========
Fix a crash when getElementsByClassName() is called twice with the same argument
including capital letters.

This fixes a regression by crrev.com/474217, which deleted a protector
AtomicString
accidentally. This CL removes a tricky StringImpl* hash key in
NodeListsNodeData.

BUG=725929
==========

to

==========
Fix a crash when getElementsByClassName() is called twice with the same argument
including capital letters.

This fixes a regression by crrev.com/474217, which deleted a protector
AtomicString
accidentally. This CL removes a tricky StringImpl* hash key in
NodeListsNodeData.

BUG=725929
R=yosin@chromium.org

Review-Url: https://codereview.chromium.org/2903373002 .
Cr-Commit-Position: refs/heads/master@{#474870}
Committed:
https://chromium.googlesource.com/chromium/src/+/9cf04000e65705ebced49e4bc7e0...
==========

[tkent, 2017-05-26 01:42:56]

Committed patchset #1 (id:1) manually as
9cf04000e65705ebced49e4bc7e04ae0d2c7d895 (presubmit successful).

[tkent, 2017-05-26 07:52:13]

On 2017/05/26 at 01:40:49, tkent wrote:
> On 2017/05/26 at 01:35:41, yosin wrote:
> > BTW, it is better to use |std::pair<CollectionType, AtomicString>| instead
of |unsigned char|
> > with declaring |enum CollectionType : uint8_t|.
> 
> Yeah, I agree that |unsigned char| here doesn't make sense.  I'll change it in
a separated CL.

I found we needed to add a dedicated HashTraits for CollectionType in that case.
So it's not worth changing it from |unsigned char|.

[yosin_UTC9, 2017-05-26 09:42:32]

On 2017/05/26 at 07:52:13, tkent wrote:
> On 2017/05/26 at 01:40:49, tkent wrote:
> > On 2017/05/26 at 01:35:41, yosin wrote:
> > > BTW, it is better to use |std::pair<CollectionType, AtomicString>| instead
of |unsigned char|
> > > with declaring |enum CollectionType : uint8_t|.
> > 
> > Yeah, I agree that |unsigned char| here doesn't make sense.  I'll change it
in a separated CL.
> 
> I found we needed to add a dedicated HashTraits for CollectionType in that
case. So it's not worth changing it from |unsigned char|.

It seems we don't have HashTrait<T> for |enum| and |enum class|.
Once we have them, we can use enum as hash key easier.
Can be done with std::is_enum<T> and std::underlying_type<T>?