Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(121)

Issue 2046583002: Don't use ::GetFileVersionInfo() in CreateFileVersionInfoForModule() (Closed)

Created:
4 years, 6 months ago by fdoray
Modified:
4 years, 5 months ago
CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't use ::GetFileVersionInfo() in CreateFileVersionInfoForModule() Currently, base::FileVersionInfo::CreateFileVersionInfoForModule() calls ::GetModuleFileName and ::GetFileVersionInfo, grabs the loader lock and potentially touches the disk to obtain the VS_VERSION_INFO of the module. This is gratuitous for a module that is already loaded. With this CL, base::FileVersionInfo::CreateFileVersionInfoForModule() uses base::win::GetResourceFromModule() to get the VS_VERSION_INFO resource from memory. BUG=609709 Committed: https://crrev.com/f177a678814f97f0b35bd6aa678c1cf885ad1656 Cr-Commit-Position: refs/heads/master@{#402549}

Patch Set 1 #

Total comments: 3

Patch Set 2 : change base::FileVersionInfo::CreateFileVersionInfoForModule() #

Patch Set 3 : self-review #

Total comments: 11

Patch Set 4 : CR grt #6 #

Patch Set 5 : self-review #

Total comments: 8

Patch Set 6 : CR grt #8 #

Patch Set 7 : self-review #

Patch Set 8 : self-review #

Total comments: 2

Patch Set 9 : CR grt #10 #

Total comments: 10

Patch Set 10 : CR thestig #20 #

Patch Set 11 : self-review #

Total comments: 2

Patch Set 12 : nit #

Patch Set 13 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+175 lines, -258 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +1 line, -3 lines 0 comments Download
M base/base.gyp View 1 2 3 4 5 6 7 8 9 2 chunks +1 line, -4 lines 0 comments Download
D base/file_version_info_unittest.cc View 1 1 chunk +0 lines, -143 lines 0 comments Download
M base/file_version_info_win.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +22 lines, -8 lines 0 comments Download
M base/file_version_info_win.cc View 1 2 3 4 5 6 7 8 9 4 chunks +72 lines, -47 lines 0 comments Download
A + base/file_version_info_win_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +74 lines, -42 lines 0 comments Download
M chrome/browser/win/enumerate_modules_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -11 lines 0 comments Download

Messages

Total messages: 40 (15 generated)
fdoray
Can you review this CL? Thanks. https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/module_util_win.cc File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/module_util_win.cc#newcode26 chrome/installer/util/module_util_win.cc:26: // This structure ...
4 years, 6 months ago (2016-06-06 14:40:58 UTC) #2
grt (UTC plus 2)
https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/module_util_win.cc File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/module_util_win.cc#newcode26 chrome/installer/util/module_util_win.cc:26: // This structure is not declared anywhere in the ...
4 years, 6 months ago (2016-06-06 15:42:56 UTC) #3
fdoray
PTAnL https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/module_util_win.cc File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2046583002/diff/1/chrome/installer/util/module_util_win.cc#newcode26 chrome/installer/util/module_util_win.cc:26: // This structure is not declared anywhere in ...
4 years, 6 months ago (2016-06-17 20:18:45 UTC) #5
grt (UTC plus 2)
cool! https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_win.cc#newcode38 base/file_version_info_win.cc:38: FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t> data, nit: move ctors down in the ...
4 years, 6 months ago (2016-06-17 20:35:00 UTC) #6
fdoray
PTAnL https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_win.cc#newcode38 base/file_version_info_win.cc:38: FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t> data, On 2016/06/17 20:34:59, grt (slow) wrote: ...
4 years, 6 months ago (2016-06-20 14:06:41 UTC) #7
grt (UTC plus 2)
https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/40001/base/file_version_info_win.cc#newcode41 base/file_version_info_win.cc:41: : FileVersionInfoWin(data.data(), language, code_page) { On 2016/06/20 14:06:41, fdoray ...
4 years, 6 months ago (2016-06-20 20:05:15 UTC) #8
fdoray
PTAnL https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/80001/base/file_version_info_win.cc#newcode29 base/file_version_info_win.cc:29: ::VerQueryValue(data, L"\\VarFileInfo\\Translation", (void**)&translate, On 2016/06/20 20:05:14, grt (no ...
4 years, 6 months ago (2016-06-21 19:31:12 UTC) #9
grt (UTC plus 2)
lgtm https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info_win.cc#newcode206 base/file_version_info_win.cc:206: fixed_file_info_(GetVsFixedFileInfo(data_)) {} should this DCHECK(!owned_data_.empty());?
4 years, 6 months ago (2016-06-22 14:09:43 UTC) #10
fdoray
https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/140001/base/file_version_info_win.cc#newcode206 base/file_version_info_win.cc:206: fixed_file_info_(GetVsFixedFileInfo(data_)) {} On 2016/06/22 14:09:43, grt (no reviews Jun ...
4 years, 6 months ago (2016-06-22 17:20:19 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2046583002/160001
4 years, 6 months ago (2016-06-22 17:20:58 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/205145)
4 years, 6 months ago (2016-06-22 17:30:41 UTC) #16
fdoray
Can you review this CL? Thanks.
4 years, 6 months ago (2016-06-22 17:33:25 UTC) #18
fdoray
thestig@: Can you review this CL? Thanks.
4 years, 6 months ago (2016-06-22 17:33:57 UTC) #19
Lei Zhang
https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info_win.cc#newcode203 base/file_version_info_win.cc:203: data_(owned_data_.data()), What happens when |owned_data_| is empty? Or does ...
4 years, 6 months ago (2016-06-23 04:33:53 UTC) #20
fdoray
thestig@: PTAnL https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info_win.cc File base/file_version_info_win.cc (right): https://codereview.chromium.org/2046583002/diff/160001/base/file_version_info_win.cc#newcode203 base/file_version_info_win.cc:203: data_(owned_data_.data()), On 2016/06/23 04:33:53, Lei Zhang (Slow) ...
4 years, 5 months ago (2016-06-27 20:51:29 UTC) #21
Lei Zhang
lgtm https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info_win_unittest.cc File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info_win_unittest.cc#newcode132 base/file_version_info_win_unittest.cc:132: } test_items[]{ nit: space between ']' and '{', ...
4 years, 5 months ago (2016-06-27 20:57:22 UTC) #22
fdoray
https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info_win_unittest.cc File base/file_version_info_win_unittest.cc (right): https://codereview.chromium.org/2046583002/diff/200001/base/file_version_info_win_unittest.cc#newcode132 base/file_version_info_win_unittest.cc:132: } test_items[]{ On 2016/06/27 20:57:22, Lei Zhang wrote: > ...
4 years, 5 months ago (2016-06-28 13:57:39 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2046583002/220001
4 years, 5 months ago (2016-06-28 14:26:23 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/27815) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, ...
4 years, 5 months ago (2016-06-28 14:29:12 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2046583002/240001
4 years, 5 months ago (2016-06-28 16:41:29 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/253950)
4 years, 5 months ago (2016-06-28 19:44:18 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2046583002/240001
4 years, 5 months ago (2016-06-28 20:00:24 UTC) #35
commit-bot: I haz the power
Committed patchset #13 (id:240001)
4 years, 5 months ago (2016-06-28 21:36:36 UTC) #37
commit-bot: I haz the power
Patchset 13 (id:??) landed as https://crrev.com/f177a678814f97f0b35bd6aa678c1cf885ad1656 Cr-Commit-Position: refs/heads/master@{#402549}
4 years, 5 months ago (2016-06-28 21:39:26 UTC) #39
Pete Williamson
4 years, 5 months ago (2016-06-29 00:35:14 UTC) #40
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2102363002/ by petewil@chromium.org.

The reason for reverting is: Sheriff reverting on suspicion of causing the Win
x64 build to fail:
https://build.chromium.org/p/chromium/builders/Win%20x64/builds/1995

From the log:
FAILED: obj/base/base.file_version_info_win.obj 
ninja -t msvc -e environment.x64 -- C:\b\build\slave\cache\cipd\goma/gomacc
"C:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\VC\bin\amd64\cl.exe"
/nologo /showIncludes /FC @obj\base\base.file_version_info_win.obj.rsp /c
..\..\base\file_version_info_win.cc /Foobj\base\base.file_version_info_win.obj
/Fdobj\base\base.cc.pdb 
c:\b\build\slave\win_x64\build\src\base\file_version_info_win.cc(81): error
C2220: warning treated as error - no 'object' file generated
c:\b\build\slave\win_x64\build\src\base\file_version_info_win.cc(81): warning
C4267: 'argument': conversion from 'size_t' to 'DWORD', possible loss of data
[8341/32887] CXX obj\base\threading\base.thread_collision_warner.obj

.

Powered by Google App Engine
This is Rietveld 408576698