Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2078913005:
Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not … (Closed)
DescriptionAdd a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll
This test only runs in release builds as it does not make sense in debug
builds which is componentized by default.
Changes in this patch:
1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc
The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies
on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests.
2. We have a static method on the CommandLine class on Windows called InitUsingArgvForTesting.
This allows consumers of the CommandLine class who want to avoid going to shell32 to
retrieve the command line arguments and instead want to honor the passed in arguments.
3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is
similar to the list for chrome_elf.
4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the
base MessagePumpWin constructor. Reason being that class is also the base class for
the MessagePumpForIO class which may be used by consumers who don't initialize user32.
BUG=604923
Committed: https://crrev.com/d936bc1b311562fe4055556e37e841355616ae75
Cr-Commit-Position: refs/heads/master@{#401413}
Patch Set 1 #Patch Set 2 : Don't need to initialize the TestSuite instance in chrome_elf\run_all_unittests.cc #Patch Set 3 : Fix typo #
Total comments: 14
Patch Set 4 : Fix trybot redness and address comments #Patch Set 5 : Avoid dependencies on user32 functions in base\process\launch_win.cc #Patch Set 6 : Add delay loads to chrome_elf_unittests #Patch Set 7 : Fix windows redness #Patch Set 8 : Delayload user32 for chrome_elf_unittests in its gyp #Patch Set 9 : Revert changes to base\launch #
Total comments: 2
Patch Set 10 : Add a static method InitFromArgvForTesting for Windows to honor passed in arguments #
Total comments: 2
Patch Set 11 : Remove newlines #Patch Set 12 : Restore newline in CommandLine::Init #
Total comments: 2
Patch Set 13 : Remove braces #
Messages
Total messages: 40 (13 generated)
ananta@chromium.org changed reviewers: + robertshield@chromium.org, scottmg@chromium.org, thestig@chromium.org
scottmg, robertshield please review everything. thestig please review changes to base.
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. The default test harness base\test\run_all_unittest.cc which we were using till now won't work as that pulls in shell32 dependencies via base::CommandLine::Init. To avoid that I added a test harness run_all_unittests.cc to the chrome_elf directory which initializes the command line first. I had to provide a way to override the default behavior of the CommandLine class on Windows which is to use ParseFromString which pulls in shell32. I added a static setter method set_honor_command_line_args_for_testing which provides a way to honor the args coming in. By default this is set to false. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. The default test harness base\test\run_all_unittest.cc which we were using till now won't work as that pulls in shell32 dependencies via base::CommandLine::Init. To avoid that I added a test harness run_all_unittests.cc to the chrome_elf directory which sets the flag in the command line to honor passed in args We have a static setter method in the CommandLine class on Windows called set_honor_command_line_args_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. BUG=604923 ==========
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. The default test harness base\test\run_all_unittest.cc which we were using till now won't work as that pulls in shell32 dependencies via base::CommandLine::Init. To avoid that I added a test harness run_all_unittests.cc to the chrome_elf directory which sets the flag in the command line to honor passed in args We have a static setter method in the CommandLine class on Windows called set_honor_command_line_args_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. The default test harness base\test\run_all_unittest.cc which we were using till now won't work as that pulls in shell32 dependencies via base::CommandLine::Init. To avoid that I added a test harness run_all_unittests.cc to the chrome_elf directory which sets the flag in the command line to honor passed in args We have a static setter method in the CommandLine class on Windows called set_honor_command_line_args_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. BUG=604923 ==========
https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h#new... base/command_line.h:75: // CommandLineToArgVW to parse the command line and convert it back to argc CommandLineToArgvW https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h#new... base/command_line.h:79: static void set_honor_command_line_args_for_testing( Maybe "set_honor_argv_for_testing" would be more accurate, since we're still using the command line arguments either way. https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/chrome_elf.g... chrome_elf/chrome_elf.gyp:100: 'SubSystem': '2', Why /subsystem:windows? Aren't we running main()? https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/chrome_elf.g... chrome_elf/chrome_elf.gyp:107: }, Do we not need all the delayloads that are in the gn? https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/run_all_unit... File chrome_elf/run_all_unittests.cc (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/run_all_unit... chrome_elf/run_all_unittests.cc:14: // CommandLineToArgV API. The chrome_elf_unittests test suite should not CommandLineToArgvW
The CL that landed last week got reverted, plus the try bots are still red.
https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/elf_imports_... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/elf_imports_... chrome_elf/elf_imports_unittest.cc:116: EXPECT_EQ(nullptr, ::GetModuleHandle(L"user32.dll")); Can you expand the comment here to explicitly state that if this line fails, then a user32 dependency has crept into the unit test executable and should be removed? I'm worried that someone will e.g. change base::FilePath in a way that trips this and they won't know how to proceed. https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/run_all_unit... File chrome_elf/run_all_unittests.cc (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/run_all_unit... chrome_elf/run_all_unittests.cc:20: base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite))); test_suite is undefined here
On 2016/06/20 16:24:50, Lei Zhang wrote: > The CL that landed last week got reverted, plus the try bots are still red. portions of the last cl got reverted. We still want this test to be in. I will fix the try redness.
https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h File base/command_line.h (right): https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h#new... base/command_line.h:75: // CommandLineToArgVW to parse the command line and convert it back to argc On 2016/06/18 17:36:57, scottmg (slow 20june) wrote: > CommandLineToArgvW Done. https://codereview.chromium.org/2078913005/diff/40001/base/command_line.h#new... base/command_line.h:79: static void set_honor_command_line_args_for_testing( On 2016/06/18 17:36:57, scottmg (slow 20june) wrote: > Maybe "set_honor_argv_for_testing" would be more accurate, since we're still > using the command line arguments either way. Done. https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/chrome_elf.g... chrome_elf/chrome_elf.gyp:100: 'SubSystem': '2', On 2016/06/18 17:36:57, scottmg (slow 20june) wrote: > Why /subsystem:windows? Aren't we running main()? Removed. https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/chrome_elf.g... chrome_elf/chrome_elf.gyp:107: }, On 2016/06/18 17:36:57, scottmg (slow 20june) wrote: > Do we not need all the delayloads that are in the gn? No. The gn build pulls in a dependency on chrome and hence. https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/elf_imports_... File chrome_elf/elf_imports_unittest.cc (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/elf_imports_... chrome_elf/elf_imports_unittest.cc:116: EXPECT_EQ(nullptr, ::GetModuleHandle(L"user32.dll")); On 2016/06/20 16:59:54, robertshield wrote: > Can you expand the comment here to explicitly state that if this line fails, > then a user32 dependency has crept into the unit test executable and should be > removed? > > I'm worried that someone will e.g. change base::FilePath in a way that trips > this and they won't know how to proceed. Done. https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/run_all_unit... File chrome_elf/run_all_unittests.cc (right): https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/run_all_unit... chrome_elf/run_all_unittests.cc:14: // CommandLineToArgV API. The chrome_elf_unittests test suite should not On 2016/06/18 17:36:57, scottmg (slow 20june) wrote: > CommandLineToArgvW Done. https://codereview.chromium.org/2078913005/diff/40001/chrome_elf/run_all_unit... chrome_elf/run_all_unittests.cc:20: base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite))); On 2016/06/20 16:59:54, robertshield wrote: > test_suite is undefined here Done.
Is it possible to create a new test launcher that uses CommandLine::InitFromArgv() instead of CommandLine::Init() ? Then you can use that launcher instead of having to add set_honor_argv_for_testing().
On 2016/06/20 22:44:24, Lei Zhang wrote: > Is it possible to create a new test launcher that uses > CommandLine::InitFromArgv() instead of CommandLine::Init() ? Then you can use > that launcher instead of having to add set_honor_argv_for_testing(). The CommandLine::current_process_commandline_ static member is initialized in CommandLine::Init. That effectively means that it expects the Init function to get called if the caller is relying on having a global singelton. We could perhaps add another static InitFromArgv or something like that function which does stuff differently. Seems ugly as well.
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. The default test harness base\test\run_all_unittest.cc which we were using till now won't work as that pulls in shell32 dependencies via base::CommandLine::Init. To avoid that I added a test harness run_all_unittests.cc to the chrome_elf directory which sets the flag in the command line to honor passed in args We have a static setter method in the CommandLine class on Windows called set_honor_command_line_args_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. The default test harness base\test\run_all_unittest.cc which we were using till now won't work as that pulls in shell32 dependencies via base::CommandLine::Init. To avoid that I added a test harness run_all_unittests.cc to the chrome_elf directory which sets the flag in the command line to honor passed in args We have a static setter method in the CommandLine class on Windows called set_honor_command_line_args_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. Changed base\process\launch.h to take in the parent window. This is to avoid user32 dependencies in launch_win.cc BUG=604923 ==========
On 2016/06/20 23:35:17, ananta wrote: > On 2016/06/20 22:44:24, Lei Zhang wrote: > > Is it possible to create a new test launcher that uses > > CommandLine::InitFromArgv() instead of CommandLine::Init() ? Then you can use > > that launcher instead of having to add set_honor_argv_for_testing(). > > The CommandLine::current_process_commandline_ static member is initialized in > CommandLine::Init. That effectively means > that it expects the Init function to get called if the caller is relying on > having a global singelton. We could perhaps > add another static InitFromArgv or something like that function which does stuff > differently. Seems ugly as well. Please hold off on reviewing this patch. Debugging windows redness.
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. The default test harness base\test\run_all_unittest.cc which we were using till now won't work as that pulls in shell32 dependencies via base::CommandLine::Init. To avoid that I added a test harness run_all_unittests.cc to the chrome_elf directory which sets the flag in the command line to honor passed in args We have a static setter method in the CommandLine class on Windows called set_honor_command_line_args_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. Changed base\process\launch.h to take in the parent window. This is to avoid user32 dependencies in launch_win.cc BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static setter method in the CommandLine class on Windows called set_honor_argv_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. BUG=604923 ==========
On 2016/06/21 02:28:09, ananta wrote: > On 2016/06/20 23:35:17, ananta wrote: > > On 2016/06/20 22:44:24, Lei Zhang wrote: > > > Is it possible to create a new test launcher that uses > > > CommandLine::InitFromArgv() instead of CommandLine::Init() ? Then you can > use > > > that launcher instead of having to add set_honor_argv_for_testing(). > > > > The CommandLine::current_process_commandline_ static member is initialized in > > CommandLine::Init. That effectively means > > that it expects the Init function to get called if the caller is relying on > > having a global singelton. We could perhaps > > add another static InitFromArgv or something like that function which does > stuff > > differently. Seems ugly as well. > > Please hold off on reviewing this patch. Debugging windows redness. The windows bots are now green. PTAL
https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (left): https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/mess... base/message_loop/message_pump_win.cc:125: InitUser32APIs(); Can you elaborate on why MessagePumpForIO doesn't need this?
On 2016/06/20 23:35:17, ananta wrote: > On 2016/06/20 22:44:24, Lei Zhang wrote: > > Is it possible to create a new test launcher that uses > > CommandLine::InitFromArgv() instead of CommandLine::Init() ? Then you can use > > that launcher instead of having to add set_honor_argv_for_testing(). > > The CommandLine::current_process_commandline_ static member is initialized in > CommandLine::Init. That effectively means > that it expects the Init function to get called if the caller is relying on > having a global singelton. We could perhaps > add another static InitFromArgv or something like that function which does stuff > differently. Seems ugly as well. Right, InitFromArgv() is not a static method. The alternative is to create: static void CommandLine::InitOnWindowsAndReallyUseArgvForTesting() and add CommandLine::InitInternal(argv, argv, really_use_argv) to share code with CommandLine::Init(). With the alternative, there's no global flag to reset, so that feels slightly less ugly to me. You'd need a shorter name, obviously.
On 2016/06/21 20:50:50, Lei Zhang wrote: > On 2016/06/20 23:35:17, ananta wrote: > > On 2016/06/20 22:44:24, Lei Zhang wrote: > > > Is it possible to create a new test launcher that uses > > > CommandLine::InitFromArgv() instead of CommandLine::Init() ? Then you can > use > > > that launcher instead of having to add set_honor_argv_for_testing(). > > > > The CommandLine::current_process_commandline_ static member is initialized in > > CommandLine::Init. That effectively means > > that it expects the Init function to get called if the caller is relying on > > having a global singelton. We could perhaps > > add another static InitFromArgv or something like that function which does > stuff > > differently. Seems ugly as well. > > Right, InitFromArgv() is not a static method. The alternative is to create: > static void CommandLine::InitOnWindowsAndReallyUseArgvForTesting() and add > CommandLine::InitInternal(argv, argv, really_use_argv) to share code with > CommandLine::Init(). With the alternative, there's no global flag to reset, so > that feels slightly less ugly to me. You'd need a shorter name, obviously. I added a static function InitUsingArgvForTesting ifdefed for OS_WIN. There is very little common code to share with Init. I left this as is
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static setter method in the CommandLine class on Windows called set_honor_argv_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static setter method in the CommandLine class on Windows called set_honor_argv_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ==========
https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (left): https://codereview.chromium.org/2078913005/diff/160001/base/message_loop/mess... base/message_loop/message_pump_win.cc:125: InitUser32APIs(); On 2016/06/21 20:40:14, Lei Zhang wrote: > Can you elaborate on why MessagePumpForIO doesn't need this? Thanks. I updated the description on the patch set. This is what I added Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32.
Sure, CommandLine::Init() looks cleaner then. https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc#n... base/command_line.cc:203: DCHECK(current_process_commandline_); missing '!'
On 2016/06/21 21:57:07, ananta wrote: > Thanks. I updated the description on the patch set. You need to update item 2 in the CL description as well.
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static setter method in the CommandLine class on Windows called set_honor_argv_for_testing. This sets a flag which signals the CommandLine instance to initialize itself from the arguments passed in. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ==========
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static method on the CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ==========
https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2078913005/diff/180001/base/command_line.cc#n... base/command_line.cc:203: DCHECK(current_process_commandline_); On 2016/06/21 21:57:45, Lei Zhang wrote: > missing '!' Thanks. Fixed
On 2016/06/21 21:59:24, Lei Zhang wrote: > On 2016/06/21 21:57:07, ananta wrote: > > Thanks. I updated the description on the patch set. > > You need to update item 2 in the CL description as well. Thanks. Fixed
lgtm
lgtm
ananta@chromium.org changed reviewers: + grt@chromium.org
+grt for chrome/installer owners
lgtm https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/... chrome/installer/util/google_update_util.cc:180: if (base::win::UserAccountControlIsEnabled()) { uber nit: omit braces
https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/2078913005/diff/220001/chrome/installer/util/... chrome/installer/util/google_update_util.cc:180: if (base::win::UserAccountControlIsEnabled()) { On 2016/06/22 14:13:55, grt (no reviews Jun 23-Jul 11) wrote: > uber nit: omit braces Done.
lgtm
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, scottmg@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2078913005/#ps240001 (title: "Remove braces")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078913005/240001
Message was sent while issue was closed.
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static method on the CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static method on the CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static method on the CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 ========== to ========== Add a chrome_elf_unittest ChromeElfLoadSanityTest which validates that loading chrome_elf does not load user32.dll This test only runs in release builds as it does not make sense in debug builds which is componentized by default. Changes in this patch: 1. Added a test harness for chrome_elf_unittests. The file is src/chrome_elf/run_all_unittests.cc The default test harness provided by src/base/test/run_all_unittests.cc pulls in dependencies on shell32 via base::CommandLine::Init. That won't work for chrome_elf_unittests. 2. We have a static method on the CommandLine class on Windows called InitUsingArgvForTesting. This allows consumers of the CommandLine class who want to avoid going to shell32 to retrieve the command line arguments and instead want to honor the passed in arguments. 3. Added a number of dlls to the delay load list for chrome_elf_unittests. This list is similar to the list for chrome_elf. 4. Call the InitUser32APIs in MessagePumpForUI and MessagePumpForGpu constructors instead of the base MessagePumpWin constructor. Reason being that class is also the base class for the MessagePumpForIO class which may be used by consumers who don't initialize user32. BUG=604923 Committed: https://crrev.com/d936bc1b311562fe4055556e37e841355616ae75 Cr-Commit-Position: refs/heads/master@{#401413} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/d936bc1b311562fe4055556e37e841355616ae75 Cr-Commit-Position: refs/heads/master@{#401413} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
