Base: Fix FileVersionInfo::CreateFileVersionInfoForCurrentModule so that

Base: Fix FileVersionInfo::CreateFileVersionInfoForCurrentModule so that
when base is built as a dll, the module that is inspected for the version
info is the one that makes the call instead of base.dll

BUG=76996
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82754

[wtc, 2011-04-22 20:00:09]

LGTM.

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h
File base/file_version_info.h (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode14
base/file_version_info.h:14: #endif  // OS_WIN
Nit: the current Chromium Coding Style guide suggests
listing "build/build_config.h" among the regular headers,
and listing the platform-specific headers after the regular
headers:
http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode53
base/file_version_info.h:53: HMODULE module =
reinterpret_cast<HMODULE>(&__ImageBase);
What is __ImageBase?

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode55
base/file_version_info.h:55: }
If we don't inline CreateFileVersionInfoForCurrentModule,
we won't need to declare and document
CreateFileVersionInfoForCurrentModule twice here.

But I guess this method MUST be inlined.  If so, please
document that.  Should we use the MSVC __forceinline keyword
instead?

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.cc
File base/file_version_info_win.cc (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.cc#n...
base/file_version_info_win.cc:40: GetModuleFileName(module, system_buffer,
MAX_PATH);
Should we check if GetModuleFileName fails (returns 0)?

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.h
File base/file_version_info_win.h (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.h#ne...
base/file_version_info_win.h:19: class FileVersionInfoWin : public
FileVersionInfo {
Why do you change from exporting the class to exporting
individual methods?

[rvargas (doing something else), 2011-04-22 21:28:20]

Brett, it looks like base now has set no parent :(

[rvargas (doing something else), 2011-04-22 21:40:05]

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h
File base/file_version_info.h (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode14
base/file_version_info.h:14: #endif  // OS_WIN
On 2011/04/22 20:00:09, wtc wrote:
> Nit: the current Chromium Coding Style guide suggests
> listing "build/build_config.h" among the regular headers,
> and listing the platform-specific headers after the regular
> headers:
> http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code

Sure, but that doesn't cover the case of having to include <windows.h> itself.

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode53
base/file_version_info.h:53: HMODULE module =
reinterpret_cast<HMODULE>(&__ImageBase);
On 2011/04/22 20:00:09, wtc wrote:
> What is __ImageBase?

Explained at line 12

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode55
base/file_version_info.h:55: }
On 2011/04/22 20:00:09, wtc wrote:
> If we don't inline CreateFileVersionInfoForCurrentModule,
> we won't need to declare and document
> CreateFileVersionInfoForCurrentModule twice here.
> 
> But I guess this method MUST be inlined.  If so, please
> document that.  Should we use the MSVC __forceinline keyword
> instead?

Yes, the whole point is that we have to inline the method. We could use
__forceinline; I'm fine either way.

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.cc
File base/file_version_info_win.cc (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.cc#n...
base/file_version_info_win.cc:40: GetModuleFileName(module, system_buffer,
MAX_PATH);
On 2011/04/22 20:00:09, wtc wrote:
> Should we check if GetModuleFileName fails (returns 0)?

Done.

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.h
File base/file_version_info_win.h (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info_win.h#ne...
base/file_version_info_win.h:19: class FileVersionInfoWin : public
FileVersionInfo {
On 2011/04/22 20:00:09, wtc wrote:
> Why do you change from exporting the class to exporting
> individual methods?

Because I cannot derive from FileVersionInfo unless that class is also fully
exported... and if we do that, the methods are not inlined, because they have to
be exported.

[brettw, 2011-04-22 21:43:34]

LGTM

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h
File base/file_version_info.h (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode55
base/file_version_info.h:55: }
On 2011/04/22 20:00:09, wtc wrote:
> If we don't inline CreateFileVersionInfoForCurrentModule,
> we won't need to declare and document
> CreateFileVersionInfoForCurrentModule twice here.
> 
> But I guess this method MUST be inlined.  If so, please
> document that.  Should we use the MSVC __forceinline keyword
> instead?

Force inline would make me feel better, and I think we should document this
clearly.

http://codereview.chromium.org/6897016/diff/5001/base/file_version_info.h#new...
base/file_version_info.h:10: #if defined(OS_WIN)
Can you put a blank line above this?

[rvargas (doing something else), 2011-04-22 22:06:43]

Thanks

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h
File base/file_version_info.h (right):

http://codereview.chromium.org/6897016/diff/1/base/file_version_info.h#newcode55
base/file_version_info.h:55: }
On 2011/04/22 21:43:34, brettw wrote:
> On 2011/04/22 20:00:09, wtc wrote:
> > If we don't inline CreateFileVersionInfoForCurrentModule,
> > we won't need to declare and document
> > CreateFileVersionInfoForCurrentModule twice here.
> > 
> > But I guess this method MUST be inlined.  If so, please
> > document that.  Should we use the MSVC __forceinline keyword
> > instead?
> 
> Force inline would make me feel better, and I think we should document this
> clearly.

Done. Note that I'm not adding an additional guard for the compiler being
MSVS... the only other equivalent instance that I can find also assumes that
this is true for windows.

http://codereview.chromium.org/6897016/diff/5001/base/file_version_info.h#new...
base/file_version_info.h:10: #if defined(OS_WIN)
On 2011/04/22 21:43:34, brettw wrote:
> Can you put a blank line above this?

Done.

[wtc, 2011-04-22 22:15:03]

LGTM.

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info.h
File base/file_version_info.h (right):

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info.h#new...
base/file_version_info.h:56: CreateFileVersionInfoForCurrentModule() {
Nit: this line probably doesn't need to be indented.

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info_win.cc
File base/file_version_info_win.cc (right):

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info_win.c...
base/file_version_info_win.cc:41: system_buffer[0] = 0;
Should we just return NULL here, as the original code does?

[rvargas (doing something else), 2011-04-22 23:08:23]

Thanks again

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info.h
File base/file_version_info.h (right):

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info.h#new...
base/file_version_info.h:56: CreateFileVersionInfoForCurrentModule() {
On 2011/04/22 22:15:04, wtc wrote:
> Nit: this line probably doesn't need to be indented.

Done.

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info_win.cc
File base/file_version_info_win.cc (right):

http://codereview.chromium.org/6897016/diff/1004/base/file_version_info_win.c...
base/file_version_info_win.cc:41: system_buffer[0] = 0;
On 2011/04/22 22:15:04, wtc wrote:
> Should we just return NULL here, as the original code does?

Sure... the effect was the same because the next call would fail, but this
version is simpler :)