GritResourceMap: output sorted by keys (issue 1312723006 by ljagielski@opera.com)

[ljagi...@opera.com]

Reviewers: tony,

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

Description:
GritResourceMap: output sorted by keys

This enables quicker lookup.

BUG=

Please review this at https://codereview.chromium.org/1312723006/

Base URL: https://chromium.googlesource.com/external/grit-i18n.git@master

Affected files (+4, -0 lines):
M grit/format/resource_map.py


Index: grit/format/resource_map.py
diff --git a/grit/format/resource_map.py b/grit/format/resource_map.py
index
37ac54ad3986d73363c3bc306f8828be02128617..a41932a3d6bf7784782bc5715e59a7cba7e632be
100644
--- a/grit/format/resource_map.py
+++ b/grit/format/resource_map.py
@@ -108,11 +108,15 @@ def _FormatSource(get_key, root, lang, output_dir):
yield _FormatSourceHeader(root)
tids = rc_header.GetIds(root)
seen = set()
+ items = []
active_descendants = [item for item in root.ActiveDescendants()]
output_all_resource_defines = root.ShouldOutputAllResourceDefines()
for item in root:
if not item.IsResourceMapSource():
continue
+ items.append(item)
+ items = sorted(items, key=get_key)
+ for item in items:
key = get_key(item)
tid = item.attrs['name']
if tid not in tids or key in seen:

[to...@chromium.org]

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


https://codereview.chromium.org/1312723006/

[ljagi...@opera.com]

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

https://codereview.chromium.org/1312723006/

[fla...@chromium.org]

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_map.py
File grit/format/resource_map.py (right):

https://codereview.chromium.org/1312723006/diff/20001/grit/format/resource_map.py#newcode118
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_map.py#newcode123
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_map.py#newcode127
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.

https://codereview.chromium.org/1312723006/

[fla...@chromium.org]

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

https://codereview.chromium.org/1312723006/

[tha...@chromium.org]

i haven't yet

https://codereview.chromium.org/1312723006/