Chromium Code Reviews
[email protected] (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(797)

Issue 18215: Add data pack file format to the grit output type and enable it for (Closed)

Created:
11 years, 11 months ago by tony
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Add data pack file format to the grit output type and enable it for webkit_resources.grd. This only adds support for <include> nodes. <message> nodes are forthcoming. The design doc describing the format is here: http://dev.chromium.org/developers/design-documents/linuxresourcesandlocalizedstrings I hooked into the <release> tag because we need to generate an index at the beginning of the file. The index contains the position of all the resources, so we walk the file at that point. This currently only handles <include> tags, but will be extended to handle <message> as well.

Patch Set 1 #

Patch Set 2 : less changes #

Patch Set 3 : review comments #

Patch Set 4 : recursion #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -3 lines) Patch
A tools/grit/grit/format/data_pack.py View 1 2 3 1 chunk +67 lines, -0 lines 0 comments Download
A tools/grit/grit/format/data_pack_unittest.py View 1 chunk +29 lines, -0 lines 0 comments Download
M tools/grit/grit/node/include.py View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M tools/grit/grit/node/misc.py View 1 chunk +6 lines, -0 lines 0 comments Download
M tools/grit/grit/test_suite_all.py View 2 chunks +2 lines, -0 lines 0 comments Download
M tools/grit/grit/tool/build.py View 1 1 chunk +4 lines, -3 lines 0 comments Download
M webkit/glue/webkit_resources.grd View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
tony
Joi, this is the first cut at having GRIT generate a generic data file for ...
11 years, 11 months ago (2009-01-14 02:24:07 UTC) #1
tony
Joi emailed me comments, I'm including them here for record: Suggest indicating in the change ...
11 years, 11 months ago (2009-01-15 22:38:02 UTC) #2
tony
11 years, 11 months ago (2009-01-15 23:01:52 UTC) #3
I addressed all the comments except passing in a list to avoid list resizes.  If
it turns out to be slow, we can optimize.

Take another look?

On 2009/01/15 22:38:02, tony wrote:
> Joi emailed me comments, I'm including them here for record:
> Suggest indicating in the change description that this adds support only for  
 
> <include> nodes and that string tables or other formats are not yet           
 
> supported.                                                                    
 
>                                                                               
 
> Strongly suggest adding a unit test for data_pack.py                          
 
>                                                                               
 
> include.py                                                                    
 
> line 56: you should get the file handle to a temporary variable and           
 
> explicitly close it after you're done with it                                 
 
>                                                                               
 
> data_pack.py                                                                  
 
>                                                                               
 
> Two blank lines between top-level elements per Google style guide (e.g.       
 
> between functions, a function and a class, and around the kPackFileVersion    
 
> constant)                                                                     
 
>                                                                               
 
> Suggest making GetDataNotes and WriteDataPack into static methods of the      
 
> DataPack class (example follows):                                             
 
>                                                                               
 
> class DataPack(interface.ItemFormatter):                                      
 
>   ...                                                                         
 
>   @staticmethod                                                               
 
>   def WriteDataPack(resources):                                               
 
>     ...                                                                       
 
>                                                                               
 
> line 16: kPackFileConstant -> PACK_FILE_CONSTANT (per style guide)            
 
>                                                                               
 
> line 25: I doubt there's any need to change this, but for future reference    
 
> note it would be more efficient to pass a list down via the recursive         
 
> function and have it append to that list than to create a new empty list,     
 
> build it, and then concatenate it to the other lists                          
 
>                                                                               
 
> A separate performance note is that it would be more efficient in terms of    
 
> memory use to walk the tree once to get all the IDs and the size of the data  
 
> items, then again to get the individual data items, keeping only one in       
 
> memory at a time, only until it is written to the pack file.  The current     
 
> approach is probably fine, but if you start seeing longer 'grit build'        
 
> times, you might want to consider this.                                       
 
>                                                                               
 
> line 48: Just yet another performance note; it is much less expensive to      
 
> build 'ret' up as a list of strings and then [ return ''.join(ret) ] at the   
 
> very end rather than concatenating again and again to the same string.        
 
>                                                                               
 
> line 36: This is a constant so strictly speaking should be named              
 
> INDEX_LENGTH.  Or you could just use [ len(ret) ].                            
 
>                                                                               
 
> Cheers,                                                                       
 
> J�i

Powered by Google App Engine
This is Rietveld 408576698