GritResourceMap: output sorted by keys

GritResourceMap: output sorted by keys

This enables quicker lookup.

BUG=

[Lukasz Jagielski, 2015-08-24 19:54:12]

ljagielski@opera.com changed reviewers:
+ tony@chromium.org

[Lukasz Jagielski, 2015-08-24 19:54:48]

Hi,
Could you please take a look at this?
Regards,
Łukasz

[tony, 2015-08-24 20:08:27]

Can you write a unittest and verify that the existing unittests pass?

Hi, I don't work on Chromium anymore.  Can you find a different reviewer?

Here are some recent committers:
https://chromium.googlesource.com/external/grit-i18n

[Lukasz Jagielski, 2015-08-24 20:32:08]

ljagielski@opera.com changed reviewers:
+ flackr@chromium.org
- tony@chromium.org

[Lukasz Jagielski, 2015-08-24 20:32:53]

Hi,
PTAL at my CL proposition.
Regards,
Łukasz

[flackr, 2015-08-27 22:03:15]

Makes sense looks good.

It might be nice to have an explicit test to the effect of this being a plain
character sort (e.g. 'B' < 'a' so maybe a test with resource names: 'A', 'B',
'a') even though realistically we wouldn't have those names but just to
reinforce that the keys are not dictionary sorted.

https://codereview.chromium.org/1312723006/diff/20001/grit/format/resource_ma...
File grit/format/resource_map.py (right):

https://codereview.chromium.org/1312723006/diff/20001/grit/format/resource_ma...
grit/format/resource_map.py:118: items.append(item)
nit: Prefer list comprehension over looping to build lists:
e.g.
items = sorted([item for item in root if not item.IsResourceMapSource()],
key=get_key)

https://codereview.chromium.org/1312723006/diff/20001/grit/format/resource_ma...
grit/format/resource_map.py:123: if tid not in tids or key in seen:
now that we're sorted by key we should just compare against the last key rather
than build a set of seen keys.

As an aside, skipping duplicate keys where key is defined by the get_key
function (in our cases to be either item name or path) seems dangerous. I'd
prefer that we either output duplicate entries (Which seems to be the case for
some resourecs: https://code.google.com/p/grit-i18n/issues/detail?id=27 ) or
raise an exception to enforce that you can't generate a lossy map file. I filed
https://code.google.com/p/grit-i18n/issues/detail?id=33 for this and it can be
done later.

https://codereview.chromium.org/1312723006/diff/20001/grit/format/resource_ma...
grit/format/resource_map.py:127: item in active_descendants):
nit: looks like active_descendants should be a set as I'm pretty sure we can't
answer in for a python list efficiently.

[flackr, 2015-08-27 22:05:46]

flackr@chromium.org changed reviewers:
+ thakis@chromium.org

[flackr, 2015-08-27 22:05:47]

+thakis, have you moved the grit code to chromium? This review may need to get
moved to that repo.

[Nico, 2015-08-27 23:17:59]

i haven't yet