Replace big code block to build CSS pseudo name table with a loop.

Replace big code block to build CSS pseudo name table with a loop.

Save 8 KB binary size by replacing code that expanded badly with
a loop. As a bonus, the code is much more maintainable now.

BUG=352113
R=ojan@chromium.org

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

[Daniel Bratell, 2014-03-13 11:05:16]

ojan, could you please take a look at this patch or redirect it to someone else
if someone else would be better at reviewing it? (I choose you because you
reviewed the last change in nameToPseudoTypeMap).

This gets rid of a lot of static (global) variables and the overhead of doing
the same thing over and over again.

It's not a massive save but it's a save and the new version of the code is more
maintainable so I think it's a win-win.

[ojan, 2014-03-13 17:18:44]

lgtm

Feel free to ignore my nits if you disagree with them or I missed something. 

CC'ing a couple other reviewers who might want to take a look.

https://codereview.chromium.org/196983005/diff/1/Source/core/css/CSSSelector.cpp
File Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/196983005/diff/1/Source/core/css/CSSSelector....
Source/core/css/CSSSelector.cpp:367: size_t pseudoCount = sizeof(pseudoTypeMap)
/ sizeof(*pseudoTypeMap);
Nit: Does WTF_ARRAY_LENGTH work instead of doing this?

https://codereview.chromium.org/196983005/diff/1/Source/core/css/CSSSelector....
Source/core/css/CSSSelector.cpp:368: ASSERT(sizeof(pseudoTypeMap) >
sizeof(*pseudoTypeMap));
Nit: not really sure what value this assert has.

https://codereview.chromium.org/196983005/diff/1/Source/core/css/CSSSelector....
Source/core/css/CSSSelector.cpp:380: AtomicString::ConstructFromLiteral);
Nit: we don't have a line length limit in blink. Personally, I think this would
read better on one line.

[Daniel Bratell, 2014-03-13 19:31:59]

On 2014/03/13 17:18:44, ojan wrote:
> lgtm
> 
> Feel free to ignore my nits if you disagree with them or I missed something. 
> 

Thanks!

The assert was just one way to catch if the array in the future becomes a
pointer but WTF_ARRAY_SIZE will do fine.

[Daniel Bratell, 2014-03-13 19:32:05]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-03-13 19:32:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/196983005/20001

[commit-bot: I haz the power, 2014-03-13 20:14:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 20:14:20]

Try jobs failed on following builders:
  tryserver.chromium on win_blink_rel

[esprehn, 2014-03-13 20:49:38]

I would suggest switch to using std::lower_bound and lazy populating the hash
map like we do inside RenderQuote.cpp.

Then get rid of nameToPseudoTypeMap() and have an explicit getter like
PseudoType nameToPseudoType(AtomicString)

[Daniel Bratell, 2014-03-13 22:42:54]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-03-13 22:43:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/196983005/20001

[Daniel Bratell, 2014-03-13 22:50:06]

On 2014/03/13 20:49:38, esprehn wrote:
> I would suggest switch to using std::lower_bound and lazy populating the hash
> map like we do inside RenderQuote.cpp.
> 
> Then get rid of nameToPseudoTypeMap() and have an explicit getter like
> PseudoType nameToPseudoType(AtomicString)

Not a bad suggestion, and the table this patch creates is halfway there already.
I will keep 352113 open for that.

Maybe a hashtable isn't even needed. For reasonably sized sets, binary search
(or even linear search) are often surprisingly competitive in performance with
hashtables and this set isn't that large. A third option is a generated minimal
perfect hash. No standard tools available to create those but they would
probably be the optimal solution both for space and performance.

[commit-bot: I haz the power, 2014-03-13 23:33:40]

Change committed as 169175