Fix generate_profile.

Fix generate_profile and allow it to be built on Linux as well as Windows.




Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174024

[Randy Smith (Not in Mondays), 2012-12-18 18:53:07]

Ok, I'm at a point where I'd like initial feedback on this change.  

Scott: Could you decide if you're comfortable with my changes to
generate_profile?  I think they are at the level of cleanliness and tight
architecture set by the rest of the code in that file (i.e. not that high), so
feel free to poke me.  This was mostly done by faulting in things as I
discovered I needed them.

Brad: I'd like you to review chrome_tests.gypi.  I'd specifically like your
input on a problem that I know I have with that change, which is that
generate_profile is dependent on having an up-to-date out\Debug\locales
directory, and I don't know how to express that dependency in the .gypi file. 
Can you give me any guidance on that?  (Alternatively, I could just put a README
file in saying you need to compile chrome at the same time as you compile
generate_profile to make sure the locales directory is that, but that's pretty
ugly.)

I am testing by running the executable to replace the profile in
chrome/test/data/profiles/profile_with_default_theme, copying the History DB
from that profile to profile_with_complex_theme (according to instructions from
erg; there is no README file in that directory describing how to recreate that
profile), and running StartupTest.Warm (performance_ui_tests) and
HistoryProfileTest.TypicalProfileVersion (unit_tests).

[sky, 2012-12-18 20:54:36]

LGTM

[Randy Smith (Not in Mondays), 2012-12-19 18:30:32]

Brad's on vacation this week.  Mark, would you be a reasonable person to do the
.gypi portion of this reivew?  (See c#1)

[Mark Mentovai, 2012-12-19 18:42:01]

Well, what target is responsible for maintaining the locales directory?

[Randy Smith (Not in Mondays), 2012-12-19 18:43:04]

On 2012/12/19 18:42:01, Mark Mentovai wrote:
> Well, what target is responsible for maintaining the locales directory?

I don't know.  I spent some time looking through various .gypi files and got
confused.  I was hoping you could help :-J.

[Mark Mentovai, 2012-12-19 18:57:06]

You should look at .gyp files too, not just .gypi.

Looks like chrome/chrome_resources.gyp is responsible. Can you make your target
depend on the appropriate target in there?

[Randy Smith (Not in Mondays), 2012-12-19 19:05:09]

On 2012/12/19 18:57:06, Mark Mentovai wrote:
> You should look at .gyp files too, not just .gypi.
> 
> Looks like chrome/chrome_resources.gyp is responsible. Can you make your
target
> depend on the appropriate target in there?

I added a dependency on chrome_resources.gyp:packed_resources, which appears to
have done the trick.  Willing to review?

[Mark Mentovai, 2012-12-19 19:11:26]

The gypi looks fine but I have comments on the cc.

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
File chrome/tools/profiles/generate_profile.cc (right):

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
chrome/tools/profiles/generate_profile.cc:44: // This is unfortunately dependent
on the definition of FilePath::StringType.
You can use |"%" PRFilePath|, apparently. I didn’t know it was there, but I just
found it in base/file_path.h.

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
chrome/tools/profiles/generate_profile.cc:239: const CommandLine::StringVector&
args = cl->GetArgs();
Fix the indentation.

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
chrome/tools/profiles/generate_profile.cc:306: printf("Copying file "
FILE_PATH_VALUE_PRINTF " to "
Is it even necessary to use printf instead of, say, LOG(INFO)?

[Randy Smith (Not in Mondays), 2012-12-19 19:46:47]

PTAL?

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
File chrome/tools/profiles/generate_profile.cc (right):

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
chrome/tools/profiles/generate_profile.cc:44: // This is unfortunately dependent
on the definition of FilePath::StringType.
On 2012/12/19 19:11:26, Mark Mentovai wrote:
> You can use |"%" PRFilePath|, apparently. I didn’t know it was there, but I
just
> found it in base/file_path.h.

The more you know!  Thank you.  Done.

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
chrome/tools/profiles/generate_profile.cc:239: const CommandLine::StringVector&
args = cl->GetArgs();
On 2012/12/19 19:11:26, Mark Mentovai wrote:
> Fix the indentation.

Whoops; done.

https://codereview.chromium.org/11573018/diff/11002/chrome/tools/profiles/gen...
chrome/tools/profiles/generate_profile.cc:306: printf("Copying file "
FILE_PATH_VALUE_PRINTF " to "
On 2012/12/19 19:11:26, Mark Mentovai wrote:
> Is it even necessary to use printf instead of, say, LOG(INFO)?

Hmmm.  I'm torn on this.  That would be simpler from a coding point of view
because of the variation in type for FilePath::value() between Windows and
non-Windows.  But it seriously changes the output format.  Also, if I want to
keep the visibility of this printing (dunno if that's important, just trying to
minimize changes) I need to use LOG(WARNING), which may scare users in normal
operation.  So I'd rather not do this change unless you feel strongly about it.

[Mark Mentovai, 2012-12-19 19:51:12]

LGTM

[Randy Smith (Not in Mondays), 2012-12-19 19:55:45]

On 2012/12/19 19:51:12, Mark Mentovai wrote:
> LGTM

Thank for the help!

[commit-bot: I haz the power, 2012-12-19 19:57:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/11573018/18001

[commit-bot: I haz the power, 2012-12-19 22:58:24]

Change committed as 174024