Convert MediaQueryTokenizer's codepoints array to be static data

Convert MediaQueryTokenizer's codepoints array to be static data.

Since there's no reason that the MediaQueryTokenizer's codepoints array would be created dynamically, I've created it here as an initialized array.
I used the build scripts to do that.



BUG=

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

[Yoav Weiss, 2014-03-23 09:07:43]

As we discussed, I've turned the codepoints array to an initialized one, to
avoid dynamically allocating it for each renderer.

Since I usde the build scripts to create it, I've included it as a cpp file. I
hope that's OK. (Couldn't find any precedents for that in the codebase)

[eseidel, 2014-03-23 15:35:03]

I wonder if there is template technology that we should use here.

Overall this looks OK, but the python is not particularly easy to read.  Could
you attach what the final generated file looks like somewhere?

[eseidel, 2014-03-23 15:35:58]

I really appreciate you doing this btw.  Every time we have one of these
heap-allocated structures we just make multiple-renderer systems that much
worse.

[Yoav Weiss, 2014-03-23 15:39:56]

On 2014/03/23 15:35:03, eseidel wrote:
> I wonder if there is template technology that we should use here.
> 
> Overall this looks OK, but the python is not particularly easy to read.  Could
> you attach what the final generated file looks like somewhere?

Sure. I'm currently slightly re-writing the array initialization part, since it
doesn't compile in g++.
I'll re-upload once that's done, with a link to the output file.

[Yoav Weiss, 2014-03-23 17:19:22]

On 2014/03/23 15:39:56, Yoav Weiss wrote:
> On 2014/03/23 15:35:03, eseidel wrote:
> > I wonder if there is template technology that we should use here.
> > 
> > Overall this looks OK, but the python is not particularly easy to read. 
Could
> > you attach what the final generated file looks like somewhere?
> 
> Sure. I'm currently slightly re-writing the array initialization part, since
it
> doesn't compile in g++.
> I'll re-upload once that's done, with a link to the output file.

I've uploaded the latest version that builds on g++.
The generated array file can be found on
http://ubuntuone.com/4zmpTKNYFiB0j7HgZ2F4Di

(Does Rietveld have a way to upload attachments that are not part of the patch
that I couldn't find?)

[Nils Barth (inactive), 2014-03-24 01:34:23]

Pythonicness tips to make the code a bit more legible.

https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):

https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:54: if c in ['\n',
'\r', '\t', '\f', ' ']:
This would be rather clearer and less redundant if the content was factored out
into a function:
def token_type(i):
    c = chr(i)
    if c in ...:
        return 'whiteSpace'
    ...
    return None

...and then for the main loop:
token_type = token(i)
value = '&MediaQueryTokenizer::whiteSpace%s' % token_type if token_type else '0'
output_string += '        %s,\n' % value

Also, slightly slicker than header/footer templates would be a single format,
call it CPP_TEMPLATE (with {array_size} and {token_lines}),
and then fill it in with
return CPP_TEMPLATE.format(array_size=array_size,
token_lines='\n'.join(token_lines))
...and indeed make token_lines a list comprehension.

Indeed, the overall output file would be clearer if this were a Jinja template,
though perhaps that's overkill.

[Nils Barth (inactive), 2014-03-24 01:48:37]

On 2014/03/23 17:19:22, Yoav Weiss wrote:
> (Does Rietveld have a way to upload attachments that are not part of the patch
> that I couldn't find?)

Not as far as I know.
For code dumps or uploading files, one in principle can use the writable git
repo
as described at:
[blink-dev] A writable git repo for collaboration and code dumps
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/ytcz72nXJ5s
...but that seems pretty heavy-duty for just uploading code for viewing.

Alternatively, you can include the generated file in one patch set
(for ease of reviewing), then remove it later,
or just post it literally into a comment.

[Timothy Loh, 2014-03-24 03:22:19]

https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):

https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:54: if c in ['\n',
'\r', '\t', '\f', ' ']:
On 2014/03/24 01:34:23, Nils Barth wrote:
> This would be rather clearer and less redundant if the content was factored
out
> into a function:
> def token_type(i):
>     c = chr(i)
>     if c in ...:
>         return 'whiteSpace'
>     ...
>     return None

Also I'll add that I'd write the function as:

if c.isspace():
    return "whiteSpace"
if c.isdigit():
    return "asciiDigit"
if c.isalpha() or c == '_':
    return "nameStart"

return {
    '(': leftParenthesis',
    ')': 'rightParenthesis',
    '+': 'plusOrFullStop',
    '.': 'plusOrFullStop',
    '-': 'hyphenMinus',
    ',': 'comma',
    '/': 'solidus',
    ':': 'colon',
    ';': 'semiColon',
    '\\': 'reverseSolidus',
}.get(c)

[Nils Barth (inactive), 2014-03-24 03:24:26]

https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):

https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:54: if c in ['\n',
'\r', '\t', '\f', ' ']:
On 2014/03/24 03:22:19, Timothy Loh wrote:
> Also I'll add that I'd write the function as:
<snip>

Good point - even better!

[Yoav Weiss, 2014-03-24 09:53:30]

On 2014/03/24 03:24:26, Nils Barth wrote:
>
https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
> File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):
> 
>
https://codereview.chromium.org/209503003/diff/40001/Source/build/scripts/mak...
> Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:54: if c in
['\n',
> '\r', '\t', '\f', ' ']:
> On 2014/03/24 03:22:19, Timothy Loh wrote:
> > Also I'll add that I'd write the function as:
> <snip>
> 
> Good point - even better!

Great points! Thanks!
I've modified the python code to match your suggestions (well almost, python's
definition of a whitespace is slightly different than CSS's definition).

[Nils Barth (inactive), 2014-03-25 02:51:46]

Few nits/tips, but otherwise Python looks great!

https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):

https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:25: codepoints =
{'(': "leftParenthesis",
nit: single quote for strings throughout
(at least be consistent, single quote preferred; no char/string distinction)

https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:36: whitespace =
['\n', '\r', '\t', '\f', ' ']
You can actually make this a single string:
whitespace = '\n\r\t\f '
...as Python's |in| functions as a substring test:
http://docs.python.org/2/library/stdtypes.html#index-22

https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:75: token_lines =
["    &MediaQueryTokenizer::%s," % token_type(i) if token_type(i) else "    0,"
for i in range(array_size)]
This would be more legible with line breaks:
token_lines = ["    &MediaQueryTokenizer::%s," % token_type(i)
               if token_type(i) else "    0,"
               for i in range(array_size)]

[Yoav Weiss, 2014-03-26 09:07:31]

On 2014/03/25 02:51:46, Nils Barth wrote:
> Few nits/tips, but otherwise Python looks great!
> 
>
https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
> File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):
> 
>
https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
> Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:25: codepoints =
> {'(': "leftParenthesis",
> nit: single quote for strings throughout
> (at least be consistent, single quote preferred; no char/string distinction)
> 
>
https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
> Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:36: whitespace =
> ['\n', '\r', '\t', '\f', ' ']
> You can actually make this a single string:
> whitespace = '\n\r\t\f '
> ...as Python's |in| functions as a substring test:
> http://docs.python.org/2/library/stdtypes.html#index-22
> 
>
https://codereview.chromium.org/209503003/diff/60001/Source/build/scripts/mak...
> Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:75: token_lines =
> ["    &MediaQueryTokenizer::%s," % token_type(i) if token_type(i) else "   
0,"
> for i in range(array_size)]
> This would be more legible with line breaks:
> token_lines = ["    &MediaQueryTokenizer::%s," % token_type(i)
>                if token_type(i) else "    0,"
>                for i in range(array_size)]

fixed

[Nils Barth (inactive), 2014-03-26 09:13:38]

https://codereview.chromium.org/209503003/diff/70001/Source/build/scripts/mak...
File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):

https://codereview.chromium.org/209503003/diff/70001/Source/build/scripts/mak...
Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:15: //
Auto-generated by make_mediaquery_tokenizer_codepoints.py
This is fragile to renaming; more robust is to use:
{{module_pyname}}
...and then compute it via:

import os
module_basename = os.path.basename(__file__)
module_pyname = os.path.splitext(module_basename)[0] + '.py'

(extension b/c otherwise might get .pyc on second run)

[Yoav Weiss, 2014-03-26 09:25:36]

On 2014/03/26 09:13:38, Nils Barth wrote:
>
https://codereview.chromium.org/209503003/diff/70001/Source/build/scripts/mak...
> File Source/build/scripts/make_mediaquery_tokenizer_codepoints.py (right):
> 
>
https://codereview.chromium.org/209503003/diff/70001/Source/build/scripts/mak...
> Source/build/scripts/make_mediaquery_tokenizer_codepoints.py:15: //
> Auto-generated by make_mediaquery_tokenizer_codepoints.py
> This is fragile to renaming; more robust is to use:
> {{module_pyname}}
> ...and then compute it via:
> 
> import os
> module_basename = os.path.basename(__file__)
> module_pyname = os.path.splitext(module_basename)[0] + '.py'
> 
> (extension b/c otherwise might get .pyc on second run)

Good point, thanks! Added.

[eseidel, 2014-03-26 15:23:47]

rslgtm.

[Yoav Weiss, 2014-03-26 15:25:04]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-03-26 15:25:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/209503003/180001

[commit-bot: I haz the power, 2014-03-26 17:55:22]

Change committed as 170074