Add whitelist support to repack.

Add whitelist support to repack.

- Added ability to filter out resources by specifying a file that contains
  a list of resource id whitelist.
- Fixed lint errors in data_pack.py, data_pack_unittest.py and repack.py
- Added tests for RePack()

BUG=338759
R=newt@chromium.org, thakis@chromium.org

Committed: https://code.google.com/p/grit-i18n/source/detail?r=158

[aurimas (slooooooooow), 2014-03-28 19:15:19]

Hey Newton,

PTAL.

Aurimas

[newt (away), 2014-03-28 19:25:33]

lgtm after dict comprehension and other nits

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py
File grit/format/data_pack.py (right):

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py#new...
grit/format/data_pack.py:79: """Return a string with a map of id=>data in the
data pack format."""
nit: Returns

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py#new...
grit/format/data_pack.py:119: that should be kept in the output file.
"or None, to include all resources"

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py#new...
grit/format/data_pack.py:147: whitelisted_resources = {key:
new_content.resources[key] for key in
use something like this, instead of dict comprehension: dict([(n, n*n) for n in
range(10)])

[aurimas (slooooooooow), 2014-03-28 20:45:40]

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py
File grit/format/data_pack.py (right):

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py#new...
grit/format/data_pack.py:79: """Return a string with a map of id=>data in the
data pack format."""
On 2014/03/28 19:25:34, newt wrote:
> nit: Returns

Done.

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py#new...
grit/format/data_pack.py:119: that should be kept in the output file.
On 2014/03/28 19:25:34, newt wrote:
> "or None, to include all resources"

Done.

https://codereview.chromium.org/217273003/diff/1/grit/format/data_pack.py#new...
grit/format/data_pack.py:147: whitelisted_resources = {key:
new_content.resources[key] for key in
On 2014/03/28 19:25:34, newt wrote:
> use something like this, instead of dict comprehension: dict([(n, n*n) for n
in
> range(10)])

Done.

[aurimas (slooooooooow), 2014-03-28 20:48:03]

+thakis for another set of eyes. WDYT?

These files were owned by joi@ and benrg@ who are sadly no longer with Google.

[aurimas (slooooooooow), 2014-03-31 22:42:42]

On 2014/03/28 20:48:03, aurimas wrote:
> +thakis for another set of eyes. WDYT?
> 
> These files were owned by joi@ and benrg@ who are sadly no longer with Google.

thakis: ping?

[Nico, 2014-04-01 18:25:53]

I'm not super familiar with grit, but I think it has tests for most things,
right? Is it possible to test this new feature?

[aurimas (slooooooooow), 2014-04-01 21:36:43]

On 2014/04/01 18:25:53, Nico wrote:
> I'm not super familiar with grit, but I think it has tests for most things,
> right? Is it possible to test this new feature?

Added tests. PTAL!

[newt (away), 2014-04-02 00:19:53]

lgtm

[Nico, 2014-04-02 00:24:46]

lgtm, thanks!

https://codereview.chromium.org/217273003/diff/60001/grit/format/data_pack.py
File grit/format/data_pack.py (right):

https://codereview.chromium.org/217273003/diff/60001/grit/format/data_pack.py...
grit/format/data_pack.py:135: def RePackFromDataPackStrings(inputs, whitelist):
Thanks for splitting this into a new function!

https://codereview.chromium.org/217273003/diff/60001/grit/format/data_pack.py...
grit/format/data_pack.py:167: whitelisted_resources = dict([(key,
content.resources[key]) for key
break in front of "for" and "if" instead of after "key"

https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py
File grit/format/repack.py (right):

https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py#ne...
grit/format/repack.py:12: import optparse
(i hear argparse is where it's at these days)

[aurimas (slooooooooow), 2014-04-02 00:55:54]

https://codereview.chromium.org/217273003/diff/60001/grit/format/data_pack.py
File grit/format/data_pack.py (right):

https://codereview.chromium.org/217273003/diff/60001/grit/format/data_pack.py...
grit/format/data_pack.py:167: whitelisted_resources = dict([(key,
content.resources[key]) for key
On 2014/04/02 00:24:46, Nico wrote:
> break in front of "for" and "if" instead of after "key"

Done.

https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py
File grit/format/repack.py (right):

https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py#ne...
grit/format/repack.py:12: import optparse
On 2014/04/02 00:24:46, Nico wrote:
> (i hear argparse is where it's at these days)

argparse was only introduced in Python 2.7 and grit uses Python 2.6.

[Nico, 2014-04-02 00:57:08]

https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py
File grit/format/repack.py (right):

https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py#ne...
grit/format/repack.py:12: import optparse
On 2014/04/02 00:55:55, aurimas wrote:
> On 2014/04/02 00:24:46, Nico wrote:
> > (i hear argparse is where it's at these days)
> 
> argparse was only introduced in Python 2.7 and grit uses Python 2.6.

All of chromium's infra is on python 2.7 by now. Does grit have other clients?

[aurimas (slooooooooow), 2014-04-02 01:01:30]

On 2014/04/02 00:57:08, Nico wrote:
> https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py
> File grit/format/repack.py (right):
> 
>
https://codereview.chromium.org/217273003/diff/60001/grit/format/repack.py#ne...
> grit/format/repack.py:12: import optparse
> On 2014/04/02 00:55:55, aurimas wrote:
> > On 2014/04/02 00:24:46, Nico wrote:
> > > (i hear argparse is where it's at these days)
> > 
> > argparse was only introduced in Python 2.7 and grit uses Python 2.6.
> 
> All of chromium's infra is on python 2.7 by now. Does grit have other clients?

Yes, there are other Google internal clients. newt@ and I think it would be
great to update to Python 2.7. I will email grit users mailing list about that.

[Nico, 2014-04-02 01:04:44]

ship it!

[newt (away), 2014-04-02 23:05:09]

Committed patchset #6 manually as r158 (presubmit successful).

[Nico, 2014-04-21 18:21:03]

(zombie review nit)

https://codereview.chromium.org/217273003/diff/100001/grit/format/data_pack.py
File grit/format/data_pack.py (right):

https://codereview.chromium.org/217273003/diff/100001/grit/format/data_pack.p...
grit/format/data_pack.py:130: whitelist = map(int, whitelist)
Should this be

  whitelist = set(map(int, whitelist))

? As is, the "if key in whitelist" below is a linear list scan I think, and
there are ~5k elements in the list. A set might be faster.