Summary: | Move CSS AtomicStrings into static AtomicStrings | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Belshe <mbelshe> | ||||
Component: | WebCore Misc. | Assignee: | Mike Belshe <mbelshe> | ||||
Status: | NEW --- | ||||||
Severity: | Normal | CC: | ap | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Mike Belshe
2009-03-24 14:31:14 PDT
Created attachment 28921 [details]
patch
Comment on attachment 28921 [details] patch > + * This file is part of the DOM implementation for KDE. You shouldn't put these in these new files. > + * Copyright (C) 2009 Apple Computer, Inc. If you're granting copyright to Apple, the correct format is Copyright (C) 2009 Apple Inc. All rights reserved. > +#include "config.h" > + #define CSS_NAMES_DECLARE(name, str) extern const AtomicString name##Atom; I think it's a little strange to use the suffix "Atom". We don't do that for event names or HTML tag names. Does this have a measurable performance effect? It doesn't make sense to make an AtomicString for the prefix "-webkit-", because atomic strings optimize equality comparisons. They don't optimize prefix checks. (In reply to comment #2) > (From update of attachment 28921 [details] [review]) > > + * This file is part of the DOM implementation for KDE. > > You shouldn't put these in these new files. > > > + * Copyright (C) 2009 Apple Computer, Inc. > > If you're granting copyright to Apple, the correct format is Copyright (C) 2009 > Apple Inc. All rights reserved. I don't care much about the copyright; I'm just trying to abide by existing style. I'll read the webkit docs and see what looks better. > > > +#include "config.h" > > > + #define CSS_NAMES_DECLARE(name, str) extern const AtomicString name##Atom; > > I think it's a little strange to use the suffix "Atom". We don't do that for > event names or HTML tag names. True; I was just trying to find a good naming scheme. Do you have an alternate proposal? I liked calling them Atoms because it is clear what we're doing by having them. > Does this have a measurable performance effect? I used a page cycler which is heavy on international pages (churns through lots of fonts) and it did have a benefit; but it is relatively small 0.5-1%. I do think this change is good, though. > It doesn't make sense to make an AtomicString for the prefix "-webkit-", > because atomic strings optimize equality comparisons. They don't optimize > prefix checks. Sort of. In this case, we have an atomic string and we're doing: atomstr.startsWith("-webkit-") The AtomicString::startsWith() method takes an AtomicString as an argument. So the "-webkit-" gets casted all the way into an AtomicString, only to be torn out as it drops out of scope. Perhaps a better solution to this problem would be to add a method to AtomicString for startWith which takes a String. Then we could either create a static String for this or just let it create the String on each invocation. Comment on attachment 28921 [details]
patch
If you went the CSSNames route, the make_names.pl script is what you'd want to use. It would do all this for you. Then you'd just add a CSsNames.in file somewhere.
But I think you probably want to use this construct instead:
DEFINE_STATIC_LOCAL(AtomicString, anyLink, ("-webkit-any-link"));
It's simpler. It's our hack around the static local problem as far as I understand it. We used to use static locals until recently that macro was added.
(In reply to comment #4) > (From update of attachment 28921 [details] [review]) > If you went the CSSNames route, the make_names.pl script is what you'd want to > use. It would do all this for you. Then you'd just add a CSsNames.in file > somewhere. Ok - I will check that out. I wasn't aware of that magic! > > But I think you probably want to use this construct instead: > DEFINE_STATIC_LOCAL(AtomicString, anyLink, ("-webkit-any-link")); Originally I used this, but I discovered that both CSSFontSelector.cpp and CSSStyleSelector.cpp used these strings. The second file was less important, but it seemed like a good idea to cover both... > > It's simpler. It's our hack around the static local problem as far as I > understand it. We used to use static locals until recently that macro was > added. > > Perhaps a better solution to this problem would
> be to add a method to AtomicString for startWith which takes a String
Yes, this definitely looks like a better solution to me (I even think that the version taking AtomicString could be removed). Of course, find(), contains() and other AtomicString methods need this treatment, too.
0.5-1% is a significant improvement by our standards.
|