Refactor FileVersionInfo into an interface with platform implementations.

Refactor FileVersionInfo into an interface with platform implementations.
This allows us to move the chrome specific version informaton used by
Linux into src/chrome.

Add a GetChromeVersionInfo() for Linux in src/chrome/app/ and make sure to use this in src/chrome.

In src/webkit/glue, add a new glue method for getting the product version.  When compiling chrome, use an implementation in src/chrome/renderer (which uses GetChromeVersionInfo()) and a stub implementation for test_shell.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44435

[Evan Martin, 2010-04-13 03:59:20]

LGTM

http://codereview.chromium.org/1560027/diff/7001/8002
File base/base.gypi (right):

http://codereview.chromium.org/1560027/diff/7001/8002#newcode305
base/base.gypi:305: }, { #
flesh out comment?

http://codereview.chromium.org/1560027/diff/7001/8003
File base/file_version_info.h (right):

http://codereview.chromium.org/1560027/diff/7001/8003#newcode14
base/file_version_info.h:14: // Provides an interface for to accessing the
version information for a file.
typo: "for to"

http://codereview.chromium.org/1560027/diff/7001/8004
File base/file_version_info_linux.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8004#newcode11
base/file_version_info_linux.cc:11: class FileVersionInfoLinux : public
FileVersionInfo {
You ifdef FileVersionInfo for Linux in the header; why do you need this?

http://codereview.chromium.org/1560027/diff/7001/8006
File base/file_version_info_mac.mm (right):

http://codereview.chromium.org/1560027/diff/7001/8006#newcode114
base/file_version_info_mac.mm:114: bool FileVersionInfoMac::GetValue(const
wchar_t* name, std::wstring* value_str) {
80

http://codereview.chromium.org/1560027/diff/7001/8010
File chrome/app/chrome_version_info.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8010#newcode14
chrome/app/chrome_version_info.cc:14: class ChromeVersionInfoPosix : public
FileVersionInfo {
doc this (e.g. "all our info comes from the above header"

http://codereview.chromium.org/1560027/diff/7001/8011
File chrome/app/chrome_version_info.h (right):

http://codereview.chromium.org/1560027/diff/7001/8011#newcode12
chrome/app/chrome_version_info.h:12: // Creates a FileVersionInfo for Chrome.
Returns NULL in case of error. The
s/Chrome/something like "Chrome, as in the binary we're trying to build here" or
something/

http://codereview.chromium.org/1560027/diff/7001/8013
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8013#newcode191
chrome/browser/automation/automation_provider.cc:191:
scoped_ptr<FileVersionInfo> file_version_info(
s/file_version_info/version_info/  ?

http://codereview.chromium.org/1560027/diff/7001/8032
File chrome/test/reliability/page_load_test.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8032#newcode166
chrome/test/reliability/page_load_test.cc:166:
file_info.reset(chrome_app::GetChromeVersionInfo());
Is the comment above this still correct?  I guess so, huh.
I wonder if they ever filed a bug.

http://codereview.chromium.org/1560027/diff/7001/8034
File chrome_frame/test/util_unittests.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8034#newcode32
chrome_frame/test/util_unittests.cc:32:
static_cast<FileVersionInfoWin*>(base_info.get());
I wonder if you'd have been better off with just concrete definitions of
FileVersionInfo rather than this subclassing business.

http://codereview.chromium.org/1560027/diff/7001/8039
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8039#newcode805
webkit/tools/test_shell/test_shell.cc:805: return std::string("Chrome/0.0.0.0");
Maybe better to use an identifiable string like "TestShell/1.2.3.4" in case it
ever shows up in test output (?).

[tony, 2010-04-13 05:28:03]

http://codereview.chromium.org/1560027/diff/7001/8002
File base/base.gypi (right):

http://codereview.chromium.org/1560027/diff/7001/8002#newcode305
base/base.gypi:305: }, { #
On 2010/04/13 03:59:20, Evan Martin wrote:
> flesh out comment?

Meh, just removed the file.

http://codereview.chromium.org/1560027/diff/7001/8003
File base/file_version_info.h (right):

http://codereview.chromium.org/1560027/diff/7001/8003#newcode14
base/file_version_info.h:14: // Provides an interface for to accessing the
version information for a file.
On 2010/04/13 03:59:20, Evan Martin wrote:
> typo: "for to"

Done.

http://codereview.chromium.org/1560027/diff/7001/8004
File base/file_version_info_linux.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8004#newcode11
base/file_version_info_linux.cc:11: class FileVersionInfoLinux : public
FileVersionInfo {
On 2010/04/13 03:59:20, Evan Martin wrote:
> You ifdef FileVersionInfo for Linux in the header; why do you need this?

You're right!  Deleted this file.

http://codereview.chromium.org/1560027/diff/7001/8006
File base/file_version_info_mac.mm (right):

http://codereview.chromium.org/1560027/diff/7001/8006#newcode114
base/file_version_info_mac.mm:114: bool FileVersionInfoMac::GetValue(const
wchar_t* name, std::wstring* value_str) {
On 2010/04/13 03:59:20, Evan Martin wrote:
> 80

Done.

http://codereview.chromium.org/1560027/diff/7001/8010
File chrome/app/chrome_version_info.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8010#newcode14
chrome/app/chrome_version_info.cc:14: class ChromeVersionInfoPosix : public
FileVersionInfo {
On 2010/04/13 03:59:20, Evan Martin wrote:
> doc this (e.g. "all our info comes from the above header"

Done.

http://codereview.chromium.org/1560027/diff/7001/8011
File chrome/app/chrome_version_info.h (right):

http://codereview.chromium.org/1560027/diff/7001/8011#newcode12
chrome/app/chrome_version_info.h:12: // Creates a FileVersionInfo for Chrome.
Returns NULL in case of error. The
On 2010/04/13 03:59:20, Evan Martin wrote:
> s/Chrome/something like "Chrome, as in the binary we're trying to build here"
or
> something/

Re-worded a bit.

http://codereview.chromium.org/1560027/diff/7001/8013
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8013#newcode191
chrome/browser/automation/automation_provider.cc:191:
scoped_ptr<FileVersionInfo> file_version_info(
On 2010/04/13 03:59:20, Evan Martin wrote:
> s/file_version_info/version_info/  ?

Done.

http://codereview.chromium.org/1560027/diff/7001/8032
File chrome/test/reliability/page_load_test.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8032#newcode166
chrome/test/reliability/page_load_test.cc:166:
file_info.reset(chrome_app::GetChromeVersionInfo());
On 2010/04/13 03:59:20, Evan Martin wrote:
> Is the comment above this still correct?  I guess so, huh.
> I wonder if they ever filed a bug.

I updated the comment so it only refers to mac.

http://codereview.chromium.org/1560027/diff/7001/8034
File chrome_frame/test/util_unittests.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8034#newcode32
chrome_frame/test/util_unittests.cc:32:
static_cast<FileVersionInfoWin*>(base_info.get());
On 2010/04/13 03:59:20, Evan Martin wrote:
> I wonder if you'd have been better off with just concrete definitions of
> FileVersionInfo rather than this subclassing business.

That's how it was before, but there were lots of extra #ifs in the header file
(and lots of stuff just not implemented on linux).  This at least makes it
obvious what features are OS specific.

http://codereview.chromium.org/1560027/diff/7001/8039
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/1560027/diff/7001/8039#newcode805
webkit/tools/test_shell/test_shell.cc:805: return std::string("Chrome/0.0.0.0");
On 2010/04/13 03:59:20, Evan Martin wrote:
> Maybe better to use an identifiable string like "TestShell/1.2.3.4" in case it
> ever shows up in test output (?).

I considered changing it to TestShell, but I'm worried about regressions in
layout tests :(