grit complete CL

grit complete CL

BUG=
TEST=

(got landed in pieces)

[Mark Mentovai, 2011-05-16 16:42:54]

http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium
File build/gyp_chromium (right):

http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode10
build/gyp_chromium:10: import cProfile
Unneeded?

http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode22
build/gyp_chromium:22: sys.path.insert(0, os.path.join(chrome_src, 'tools',
'grit'))
I find this hard to follow on a quick glance. You’re putting tools/grit into
sys.path after tools/gyp/pylib, but you’re doing the insertions backwards. I
think you should put this sys.path.insert after the existing one for
tools/gyp/pylib, and insert it at position 1. Nobody says you need to use 0 for
each sys.path.insert.

http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode174
build/gyp_chromium:174: 
This file seems to use only one blank line between things.

http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode176
build/gyp_chromium:176: #cProfile.run('main()', 'profile')
Unneeded?

http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode177
build/gyp_chromium:177: main()
sys.exit(main(sys.argv[1:])) instead, and adjust main accordingly?

Might make it easier in the future if someone wants to wrap gyp_chromium in the
same way you’re now trying to wrap grit_info.

http://codereview.chromium.org/7015061/diff/2001/tools/grit/grit_info.py
File tools/grit/grit_info.py (right):

http://codereview.chromium.org/7015061/diff/2001/tools/grit/grit_info.py#newc...
tools/grit/grit_info.py:9: import cProfile
Unneeded?

http://codereview.chromium.org/7015061/diff/2001/tools/grit/grit_info.py#newc...
tools/grit/grit_info.py:132: #cProfile.run('main(sys.argv)', 'grit_profile')
Unneeded? (Revert the changes in this file?)

[Nico, 2011-05-16 16:46:36]

This CL is not for review, it's just there to give an overview of stuff I
did. (I updated the link in the gyp CL description to point somewhere else.)

On Mon, May 16, 2011 at 9:42 AM, <mark@chromium.org> wrote:

>
> http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium
> File build/gyp_chromium (right):
>
>
> http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode10
> build/gyp_chromium:10: import cProfile
> Unneeded?
>
>
> http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode22
> build/gyp_chromium:22: sys.path.insert(0, os.path.join(chrome_src,
> 'tools', 'grit'))
> I find this hard to follow on a quick glance. You’re putting tools/grit
> into sys.path after tools/gyp/pylib, but you’re doing the insertions
> backwards. I think you should put this sys.path.insert after the
> existing one for tools/gyp/pylib, and insert it at position 1. Nobody
> says you need to use 0 for each sys.path.insert.
>
>
> http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode174
> build/gyp_chromium:174:
> This file seems to use only one blank line between things.
>
>
> http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode176
> build/gyp_chromium:176: #cProfile.run('main()', 'profile')
> Unneeded?
>
>
> http://codereview.chromium.org/7015061/diff/2001/build/gyp_chromium#newcode177
> build/gyp_chromium:177: main()
> sys.exit(main(sys.argv[1:])) instead, and adjust main accordingly?
>
> Might make it easier in the future if someone wants to wrap gyp_chromium
> in the same way you’re now trying to wrap grit_info.
>
> http://codereview.chromium.org/7015061/diff/2001/tools/grit/grit_info.py
> File tools/grit/grit_info.py (right):
>
>
>
http://codereview.chromium.org/7015061/diff/2001/tools/grit/grit_info.py#newc...
> tools/grit/grit_info.py:9: import cProfile
> Unneeded?
>
>
>
http://codereview.chromium.org/7015061/diff/2001/tools/grit/grit_info.py#newc...
> tools/grit/grit_info.py:132: #cProfile.run('main(sys.argv)',
> 'grit_profile')
> Unneeded? (Revert the changes in this file?)
>
> http://codereview.chromium.org/7015061/
>