Bug 101298 – Improve the use of AtomicString with literals

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Benjamin Poulain]

More AtomicString cleanup of WebCore.

[Benjamin Poulain]

Created attachment 172484 [details]
Patch

[Build Bot]

Comment on attachment 172484 [details]
Patch

Attachment 172484 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14724906

[Eric Seidel (no email)]

Comment on attachment 172484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172484&action=review

> Source/WebCore/Modules/battery/BatteryController.cpp:94
> +    DEFINE_STATIC_LOCAL(AtomicString, name, ("BatteryController", AtomicString::ConstructFromLiteral));

Could we remove the AtomicString:: prefix for all callers (just share this constant with String?)

Are compiler constants ever a different type from char* in c++?  Could we detect this w/o an explicit enum?

[Adam Barth]

Comment on attachment 172484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172484&action=review

> Source/WebCore/html/FormController.cpp:275
> -        if (AtomicString(key.type()) != AtomicString("file"))
> +        if (AtomicString(key.type()) != "file")

Is there a reason to use an AtomicString on the left hand side of this operator?

[Benjamin Poulain]

> > Source/WebCore/Modules/battery/BatteryController.cpp:94
> > +    DEFINE_STATIC_LOCAL(AtomicString, name, ("BatteryController", AtomicString::ConstructFromLiteral));
> 
> Could we remove the AtomicString:: prefix for all callers (just share this constant with String?)

Yep, I guess that could be done, that enum is just a compile time flag.
I like the enum being namespaced by its class, but I don't really mind if you want to move it out.
 
> Are compiler constants ever a different type from char* in c++?  Could we detect this w/o an explicit enum?

I really wish.
As far as I know, C and C++ do not make any difference between a pointer from the literal pool, and a pointer from the stack; both can be char[].

> > Source/WebCore/html/FormController.cpp:275
> > -        if (AtomicString(key.type()) != AtomicString("file"))
> > +        if (AtomicString(key.type()) != "file")
> 
> Is there a reason to use an AtomicString on the left hand side of this operator?

IIRC key.type() is a AtomicStringImpl. The other option is if (!equal(key.type(), file)). I only kept the AtomicString on the left hand side for readability but there is no good reason to ref() here.

[Benjamin Poulain]

Created attachment 172661 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 172661 [details]
Patch

My only complaint about this patch is the massive amount of duplicated text. :)

Does AtomicString have a special constructor for ASCIILiteral()?  Could we just wrap these strings in ASCIILiteral instead?  It would be a shorter way to annotate the desired behavior instead of this very long enum.

[Benjamin Poulain]

> Does AtomicString have a special constructor for ASCIILiteral()?  Could we just wrap these strings in ASCIILiteral instead?  It would be a shorter way to annotate the desired behavior instead of this very long enum.

I could extend ASCIILiteral and make it works.
But that would be shooting myself in the foot for the long term plans. At some point in the future, memory access on ARM will get faster and ASCIILiteral will have the string size. On the other side, in a nearer future, AtomicString will compute the hash value at compile time and may also encode things a little differently. They were split at birth for performance reason, and the split will widen with those future change.

Note that at least half of the existing initialisation already use AtomicString::ConstructFromLiteral. In particular, all the generated code already use that.

[Eric Seidel (no email)]

AtomicString::ConstructFromLiteral is just 34 redundant characters, that's my only complaint. :)  34 * a few hundred uses, is kinda silly. :)

[Benjamin Poulain]

(In reply to comment #9)
> AtomicString::ConstructFromLiteral is just 34 redundant characters, that's my only complaint. :)  34 * a few hundred uses, is kinda silly. :)

Can we put our differences behind us, for science?

We can easily mass rename that later.

[Darin Adler]

Comment on attachment 172661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172661&action=review

> Source/WebCore/Modules/battery/BatteryController.cpp:94
> +    DEFINE_STATIC_LOCAL(AtomicString, name, ("BatteryController", AtomicString::ConstructFromLiteral));

Why does this have to be different syntax than initializing a String? For a String we construct with an argument of type ASCIILiteral, right? Why did you chose a different approach for AtomicString?

[Darin Adler]

(In reply to comment #8)
> > Does AtomicString have a special constructor for ASCIILiteral()?  Could we just wrap these strings in ASCIILiteral instead?  It would be a shorter way to annotate the desired behavior instead of this very long enum.
> 
> I could extend ASCIILiteral and make it works.
> But that would be shooting myself in the foot for the long term plans. At some point in the future, memory access on ARM will get faster and ASCIILiteral will have the string size. On the other side, in a nearer future, AtomicString will compute the hash value at compile time and may also encode things a little differently. They were split at birth for performance reason, and the split will widen with those future change.
> 
> Note that at least half of the existing initialisation already use AtomicString::ConstructFromLiteral. In particular, all the generated code already use that.

I don’t really understand these arguments. Can’t the ASCIILiteral syntax be made to generate exactly the same code that the ConstructFromLiteral syntax does?

[Darin Adler]

Comment on attachment 172661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172661&action=review

>> Source/WebCore/Modules/battery/BatteryController.cpp:94
>> +    DEFINE_STATIC_LOCAL(AtomicString, name, ("BatteryController", AtomicString::ConstructFromLiteral));
> 
> Why does this have to be different syntax than initializing a String? For a String we construct with an argument of type ASCIILiteral, right? Why did you chose a different approach for AtomicString?

I’m going to say review+ but I think this is way to awkward syntax for something so basic, and I’d like us to investigate cleaner syntax that can generate identical code.

[Benjamin Poulain]

> I’m going to say review+ but I think this is way to awkward syntax for something so basic, and I’d like us to investigate cleaner syntax that can generate identical code.

Thanks for the review. Long term I will look into unify the syntax.

[Benjamin Poulain]

Committed r133976: <http://trac.webkit.org/changeset/133976>