Use ConstructFromLiteral instead of implicit conversion

Use ConstructFromLiteral instead of implicit conversion

Since we are supposed to end up with an AtomicString, don't rely
on implicit conversion, but provide it as static local variable
while using ConstructFromLiteral to avoid using strlen.

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

[rwlbuis, 2014-09-15 17:38:45]

rob.buis@samsung.com changed reviewers:
+ pdr@chromium.org

[rwlbuis, 2014-09-15 17:38:46]

PTAL.

[pdr., 2014-09-16 01:55:58]

We have roughly 300 DEFINE_STATIC_LOCAL's for strings, 2/3 use
ConstructFromLiteral and 1/3 don't. Doing these two-at-a-time isn't scalable.
Can we fix this at a deeper level using an assert in the constructor, a
bulk-conversion script, and/or a presubmit check?

[rwlbuis, 2014-09-16 15:54:11]

On 2014/09/16 01:55:58, pdr wrote:
> We have roughly 300 DEFINE_STATIC_LOCAL's for strings, 2/3 use
> ConstructFromLiteral and 1/3 don't. Doing these two-at-a-time isn't scalable.
> Can we fix this at a deeper level using an assert in the constructor, a
> bulk-conversion script, and/or a presubmit check?

Can you share how you find those 300? When I grep for DEFINE_STATIC_LOCAL and
AtomicString usage I see about 200 and not many don't use ConstructFromLiteral.
Agreed if there are 100 then bulk makes sense but I don't see them.

[pdr., 2014-09-16 18:27:41]

On 2014/09/16 15:54:11, rwlbuis wrote:
> On 2014/09/16 01:55:58, pdr wrote:
> > We have roughly 300 DEFINE_STATIC_LOCAL's for strings, 2/3 use
> > ConstructFromLiteral and 1/3 don't. Doing these two-at-a-time isn't
scalable.
> > Can we fix this at a deeper level using an assert in the constructor, a
> > bulk-conversion script, and/or a presubmit check?
> 
> Can you share how you find those 300? When I grep for DEFINE_STATIC_LOCAL and
> AtomicString usage I see about 200 and not many don't use
ConstructFromLiteral.
> Agreed if there are 100 then bulk makes sense but I don't see them.

This is the search I was using:
git grep DEFINE_STATIC_LOCAL | grep \" | grep -v ConstructFromLiteral | wc -l

[rwlbuis, 2014-09-16 18:39:24]

On 2014/09/16 18:27:41, pdr wrote:
> On 2014/09/16 15:54:11, rwlbuis wrote:
> > On 2014/09/16 01:55:58, pdr wrote:
> > > We have roughly 300 DEFINE_STATIC_LOCAL's for strings, 2/3 use
> > > ConstructFromLiteral and 1/3 don't. Doing these two-at-a-time isn't
> scalable.
> > > Can we fix this at a deeper level using an assert in the constructor, a
> > > bulk-conversion script, and/or a presubmit check?
> > 
> > Can you share how you find those 300? When I grep for DEFINE_STATIC_LOCAL
and
> > AtomicString usage I see about 200 and not many don't use
> ConstructFromLiteral.
> > Agreed if there are 100 then bulk makes sense but I don't see them.
> 
> This is the search I was using:
> git grep DEFINE_STATIC_LOCAL | grep \" | grep -v ConstructFromLiteral | wc -l

Aha, thanks! I think the confusion comes from me assuming you meant strictly
AtomicString.

[rwlbuis, 2014-09-16 20:59:20]

On 2014/09/16 18:39:24, rwlbuis wrote:
> On 2014/09/16 18:27:41, pdr wrote:
> > On 2014/09/16 15:54:11, rwlbuis wrote:
> > > On 2014/09/16 01:55:58, pdr wrote:
> > > > We have roughly 300 DEFINE_STATIC_LOCAL's for strings, 2/3 use
> > > > ConstructFromLiteral and 1/3 don't. Doing these two-at-a-time isn't
> > scalable.
> > > > Can we fix this at a deeper level using an assert in the constructor, a
> > > > bulk-conversion script, and/or a presubmit check?
> > > 
> > > Can you share how you find those 300? When I grep for DEFINE_STATIC_LOCAL
> and
> > > AtomicString usage I see about 200 and not many don't use
> > ConstructFromLiteral.
> > > Agreed if there are 100 then bulk makes sense but I don't see them.
> > 
> > This is the search I was using:
> > git grep DEFINE_STATIC_LOCAL | grep \" | grep -v ConstructFromLiteral | wc
-l
> 
> Aha, thanks! I think the confusion comes from me assuming you meant strictly
> AtomicString.

I noticed two more instances of that git grep for the AtomicString case, which I
added in patch set #3, that should be all regarding AtomicString.

How about keeping this CL for AtomicString (that was my intention anyway) and
doing a new CL for String?
I already have an idea for the latter, but it will be a bigger patch.

[pdr., 2014-09-17 00:25:57]

On 2014/09/16 20:59:20, rwlbuis wrote:
> On 2014/09/16 18:39:24, rwlbuis wrote:
> > On 2014/09/16 18:27:41, pdr wrote:
> > > On 2014/09/16 15:54:11, rwlbuis wrote:
> > > > On 2014/09/16 01:55:58, pdr wrote:
> > > > > We have roughly 300 DEFINE_STATIC_LOCAL's for strings, 2/3 use
> > > > > ConstructFromLiteral and 1/3 don't. Doing these two-at-a-time isn't
> > > scalable.
> > > > > Can we fix this at a deeper level using an assert in the constructor,
a
> > > > > bulk-conversion script, and/or a presubmit check?
> > > > 
> > > > Can you share how you find those 300? When I grep for
DEFINE_STATIC_LOCAL
> > and
> > > > AtomicString usage I see about 200 and not many don't use
> > > ConstructFromLiteral.
> > > > Agreed if there are 100 then bulk makes sense but I don't see them.
> > > 
> > > This is the search I was using:
> > > git grep DEFINE_STATIC_LOCAL | grep \" | grep -v ConstructFromLiteral | wc
> -l
> > 
> > Aha, thanks! I think the confusion comes from me assuming you meant strictly
> > AtomicString.
> 
> I noticed two more instances of that git grep for the AtomicString case, which
I
> added in patch set #3, that should be all regarding AtomicString.
> 
> How about keeping this CL for AtomicString (that was my intention anyway) and
> doing a new CL for String?
> I already have an idea for the latter, but it will be a bigger patch.

That sounds great. LGTM

[pdr., 2014-09-17 00:26:06]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-09-17 00:27:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/566373002/40001

[commit-bot: I haz the power, 2014-09-17 02:12:35]

Committed patchset #3 (id:40001) as 182133