Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1223953003:
Introduce UpdateActiveSetupVersionWorkItem (Closed)
|
Created:
5 years, 5 months ago by gab Modified:
5 years, 5 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@a1_active_setup_onosup_addCB_API Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce UpdateActiveSetupVersionWorkItem
A cleaner solution coming out of an attempt to support two similar use
cases differently in https://codereview.chromium.org/1213913002/ and
https://codereview.chromium.org/1214163008/.
Actual use cases implemented in a follow-up CL (https://codereview.chromium.org/1223933005/).
BUG=488247, 502363
TEST=setup_unittests.exe --gtest_filter=UpdateActiveSetupVersionWorkItemTest*
Committed: https://crrev.com/3861555fdf3bd6238cb177b66a83ac34fe06752e
Cr-Commit-Position: refs/heads/master@{#338528}
Patch Set 1 #Patch Set 2 : Do not support negative values (operate on unsigned ints) #
Total comments: 52
Patch Set 3 : nits #Patch Set 4 : Move the files to installer/setup as-is #Patch Set 5 : Composition instead of inheritance #
Total comments: 4
Patch Set 6 : merge to r338517 add GN support for setup_unittests #
Dependent Patchsets: Messages
Total messages: 18 (6 generated)
gab@chromium.org changed reviewers: + grt@chromium.org
Sir Greg PTAL, opted to introduce this WorkItem and its tests in a precursor CL to the one actually using it (https://codereview.chromium.org/1223933005/). I think this will make for an easier merge as it's easy to see that the bigger chunk is this part while the implementation hooks are trivial. The tests were highly inspired from the augmented InstallWorkerTests I'd written for https://codereview.chromium.org/1213913002/ and I think with the new ones the coverage is just as good and I'm thus no longer adding InstallWorkerTests at all (with the WorkItem being fully tested, the only value of such a test would be to confirm that the WorkItem is indeed executed and this wasn't tested before so it's no worse not to test it now). Cheers, Gab
i like it! https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... File chrome/chrome_installer.gypi (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... chrome/chrome_installer.gypi:144: 'installer/util/update_active_setup_version_work_item_unittest.cc', move to setup_unittests as per comment in chrome/chrome_installer_util.gypi. https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... File chrome/chrome_installer_util.gypi (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... chrome/chrome_installer_util.gypi:91: 'installer/util/update_active_setup_version_work_item.cc', since this is only used by setup.exe, please move it into installer/setup. it's nice that your design allows for this. yay! https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:39: #include "chrome/installer/util/update_active_setup_version_work_item.h" nit: move into 1223933005 https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:50: #include "chrome/installer/util/update_active_setup_version_work_item.h" same nit https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/B... File chrome/installer/util/BUILD.gn (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/B... chrome/installer/util/BUILD.gn:153: "update_active_setup_version_work_item.cc", move to installer/setup/BUILD.gn for the unittest. i don't think setup.exe is built yet for the GN build. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/B... chrome/installer/util/BUILD.gn:261: "update_active_setup_version_work_item_unittest.cc", move to installer/setup/BUILD.gn https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.cc:53: unsigned previous_value; please use uint32_t here even though StringToUint (wrongly) uses "unsigned". https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.cc:56: << "version component: " << version_components[OS_UPGRADES]; nit: remove first << so that the compiler can concatenate the string literals. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.h (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:15: // TODO(gab): This would be cleaner if UpdateActiveSetupVersionWorkItem since you're modifying SetRegValueWorkItem to make this inheritance possible, why not friend UpdateActiveSetupVersionWorkItem instead? i think it's cleaner to use composition rather than inheritance here, even if it means poking an ugly hole in a strange design. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:21: enum Operation { why not enum class? https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:31: // |operation_| on the |active_setup_path| key in the registry. This key needs nit: |operation| https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:33: UpdateActiveSetupVersionWorkItem(const base::string16& active_setup_path, nit: make order of params and order of description in the doc comment match? https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:43: // Number of OS upgrades handled since original install. should this warp back to zero each time the major version is incremented? is there value in making this eternally increase rather than only being incremental blips when MAJOR doesn't change? https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item_unittest.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:39: *os << "Initial value: " #include <ostream> https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:46: class UpdateActiveSetupVersionWorkItemTest i don't see an advantage to having this in an unnamed namespace. it makes it harder to set breakpoints in the fixture in windbg, and makes it impossible to friend the fixture as mentioned in the recent thread here: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/wzmsu.... although it isn't mandated, i have a strong preference for Kent's style (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/dY_73...) in which the test fixture and individual tests are not in an unnamed namespace. so, unless you have a compelling reason to leave it here, please take it out. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:63: TEST_P(UpdateActiveSetupVersionWorkItemTest, Execute) { this test is beautiful! https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:77: EXPECT_EQ(ERROR_FILE_NOT_FOUND, should this be ASSERT_EQ? is there a reason to continue the test if this fails? https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:88: EXPECT_EQ(ERROR_SUCCESS, ASSERT_EQ? https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:91: EXPECT_EQ(ERROR_SUCCESS, ASSERT_EQ https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:95: active_setup_work_item.Do(); EXPECT_TRUE? https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:122: const UpdateActiveSetupVersionWorkItemTestCase can you put this up with the struct definition? it'd make it easier to see what the values mean.
PTAL, nits addressed, replies to some of the other comments below. Thanks, Gab https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... File chrome/chrome_installer.gypi (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... chrome/chrome_installer.gypi:144: 'installer/util/update_active_setup_version_work_item_unittest.cc', On 2015/07/09 14:23:53, grt wrote: > move to setup_unittests as per comment in chrome/chrome_installer_util.gypi. Hmm, which comment? All other WorkItems are here.. And also, until setup_unittests are on the waterfall I'd much rather not use them. https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... File chrome/chrome_installer_util.gypi (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... chrome/chrome_installer_util.gypi:91: 'installer/util/update_active_setup_version_work_item.cc', On 2015/07/09 14:23:53, grt wrote: > since this is only used by setup.exe, please move it into installer/setup. it's > nice that your design allows for this. yay! I debated this, but this is true of all WorkItems AFAICT, a nice follow-up cleanup, but I don't think this CL should introduce an outlier in the mean time. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... chrome/installer/setup/install.cc:39: #include "chrome/installer/util/update_active_setup_version_work_item.h" On 2015/07/09 14:23:53, grt wrote: > nit: move into 1223933005 Oops, done. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/setup/... chrome/installer/setup/install_worker.cc:50: #include "chrome/installer/util/update_active_setup_version_work_item.h" On 2015/07/09 14:23:53, grt wrote: > same nit Oops again :-). https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/B... File chrome/installer/util/BUILD.gn (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/B... chrome/installer/util/BUILD.gn:153: "update_active_setup_version_work_item.cc", On 2015/07/09 14:23:53, grt wrote: > move to installer/setup/BUILD.gn for the unittest. i don't think setup.exe is > built yet for the GN build. Same argument as the other ones. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/B... chrome/installer/util/BUILD.gn:261: "update_active_setup_version_work_item_unittest.cc", On 2015/07/09 14:23:53, grt wrote: > move to installer/setup/BUILD.gn Same. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.cc:53: unsigned previous_value; On 2015/07/09 14:23:53, grt wrote: > please use uint32_t here even though StringToUint (wrongly) uses "unsigned". Done, but curious what's the reasoning behind this? https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.cc:56: << "version component: " << version_components[OS_UPGRADES]; On 2015/07/09 14:23:53, grt wrote: > nit: remove first << so that the compiler can concatenate the string literals. Done. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.h (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:15: // TODO(gab): This would be cleaner if UpdateActiveSetupVersionWorkItem On 2015/07/09 14:23:53, grt wrote: > since you're modifying SetRegValueWorkItem to make this inheritance possible, > why not friend UpdateActiveSetupVersionWorkItem instead? i think it's cleaner to > use composition rather than inheritance here, even if it means poking an ugly > hole in a strange design. The reason I didn't do this is that friend'ing gives full access to the class (i.e. there is no such thing as a "protected-only friend"), therefore I thought this approach gave better encapsulation by only allowing the constructors to be used. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:21: enum Operation { On 2015/07/09 14:23:53, grt wrote: > why not enum class? Only because it makes constructor calls even more wordy (though I generally like the explicitness of enum class in general, happy either way, your pick!). https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:31: // |operation_| on the |active_setup_path| key in the registry. This key needs On 2015/07/09 14:23:53, grt wrote: > nit: |operation| Done. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:33: UpdateActiveSetupVersionWorkItem(const base::string16& active_setup_path, On 2015/07/09 14:23:53, grt wrote: > nit: make order of params and order of description in the doc comment match? I think the callsites read better in this order yet semantically I find the comment reads better this way... Let me know if you feel otherwise. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:43: // Number of OS upgrades handled since original install. On 2015/07/09 14:23:53, grt wrote: > should this warp back to zero each time the major version is incremented? is > there value in making this eternally increase rather than only being incremental > blips when MAJOR doesn't change? Interesting, no major reason other than that this would make the code slightly more complex for no added benefit I think (plus in the event we want to know the absolute number of OS major upgrades a user went through one day for whatever reason, we have it..) https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item_unittest.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:39: *os << "Initial value: " On 2015/07/09 14:23:54, grt wrote: > #include <ostream> Done. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:46: class UpdateActiveSetupVersionWorkItemTest On 2015/07/09 14:23:54, grt wrote: > i don't see an advantage to having this in an unnamed namespace. it makes it > harder to set breakpoints in the fixture in windbg, and makes it impossible to > friend the fixture as mentioned in the recent thread here: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/wzmsu.... > although it isn't mandated, i have a strong preference for Kent's style > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/dY_73...) > in which the test fixture and individual tests are not in an unnamed namespace. > so, unless you have a compelling reason to leave it here, please take it out. Thanks for the reference, makes sense, I had always previously followed this paradigm with only the tests outside the anonymous namespace, but I now see why it's desired to have the fixture there as well. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:63: TEST_P(UpdateActiveSetupVersionWorkItemTest, Execute) { On 2015/07/09 14:23:54, grt wrote: > this test is beautiful! :-) https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:77: EXPECT_EQ(ERROR_FILE_NOT_FOUND, On 2015/07/09 14:23:54, grt wrote: > should this be ASSERT_EQ? is there a reason to continue the test if this fails? Agreed, anything after a failure here is meaningless. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:88: EXPECT_EQ(ERROR_SUCCESS, On 2015/07/09 14:23:53, grt wrote: > ASSERT_EQ? Agreed, anything after a failure here is meaningless. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:91: EXPECT_EQ(ERROR_SUCCESS, On 2015/07/09 14:23:54, grt wrote: > ASSERT_EQ Agreed, anything after a failure here is meaningless. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:95: active_setup_work_item.Do(); On 2015/07/09 14:23:54, grt wrote: > EXPECT_TRUE? Done. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item_unittest.cc:122: const UpdateActiveSetupVersionWorkItemTestCase On 2015/07/09 14:23:54, grt wrote: > can you put this up with the struct definition? it'd make it easier to see what > the values mean. Done.
https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.h (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:43: // Number of OS upgrades handled since original install. On 2015/07/09 20:36:38, gab wrote: > On 2015/07/09 14:23:53, grt wrote: > > should this warp back to zero each time the major version is incremented? is > > there value in making this eternally increase rather than only being > incremental > > blips when MAJOR doesn't change? > > Interesting, no major reason other than that this would make the code slightly > more complex for no added benefit I think (plus in the event we want to know the > absolute number of OS major upgrades a user went through one day for whatever > reason, we have it..) Actually just thought of a reason, in my upcoming CL to restore shortcuts on-os-upgrade I keep a cache of the last OS_UPGRADES handled by this user in order to know when Active Setup kicks in whether it's kicking in response to an OS_UPGRADES bump or something else. If the major version gets updated between an OS upgrade and the next time a user logs in, we still need to handle the OS upgrade.
https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... File chrome/chrome_installer.gypi (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... chrome/chrome_installer.gypi:144: 'installer/util/update_active_setup_version_work_item_unittest.cc', On 2015/07/09 20:36:37, gab wrote: > On 2015/07/09 14:23:53, grt wrote: > > move to setup_unittests as per comment in chrome/chrome_installer_util.gypi. > > Hmm, which comment? > > All other WorkItems are here.. By accident. Only the ones needed by chrome.exe belong here. > And also, until setup_unittests are on the waterfall I'd much rather not use > them. They're almost there! I have them compiling on trybots and some or all of the waterfall builders now. They'll be running very, very soon. https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... File chrome/chrome_installer_util.gypi (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... chrome/chrome_installer_util.gypi:91: 'installer/util/update_active_setup_version_work_item.cc', On 2015/07/09 20:36:37, gab wrote: > On 2015/07/09 14:23:53, grt wrote: > > since this is only used by setup.exe, please move it into installer/setup. > it's > > nice that your design allows for this. yay! > > I debated this, but this is true of all WorkItems Some of them are used by chrome.exe (shell_util.cc uses them for the registry stuff), so they can't all be moved into setup. > AFAICT, a nice follow-up > cleanup, but I don't think this CL should introduce an outlier in the mean time. I don't see this as a compelling reason to follow a bad trend. In the worst case, chrome.exe will be bloated with dead code leading to larger downloads and more bits in the binary. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.cc (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.cc:53: unsigned previous_value; On 2015/07/09 20:36:38, gab wrote: > On 2015/07/09 14:23:53, grt wrote: > > please use uint32_t here even though StringToUint (wrongly) uses "unsigned". > > Done, but curious what's the reasoning behind this? http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types: Explicitly-sized ints are preferred when that's what you really need. Also, "Of the C integer types, only int should be used", so "unsigned" by itself is clearly wrong. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.h (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:15: // TODO(gab): This would be cleaner if UpdateActiveSetupVersionWorkItem On 2015/07/09 20:36:38, gab wrote: > On 2015/07/09 14:23:53, grt wrote: > > since you're modifying SetRegValueWorkItem to make this inheritance possible, > > why not friend UpdateActiveSetupVersionWorkItem instead? i think it's cleaner > to > > use composition rather than inheritance here, even if it means poking an ugly > > hole in a strange design. > > The reason I didn't do this is that friend'ing gives full access to the class > (i.e. there is no such thing as a "protected-only friend"), therefore I thought > this approach gave better encapsulation by only allowing the constructors to be > used. I see your point, but I think that's less bad than inheritance here. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:21: enum Operation { On 2015/07/09 20:36:38, gab wrote: > On 2015/07/09 14:23:53, grt wrote: > > why not enum class? > > Only because it makes constructor calls even more wordy (though I generally like > the explicitness of enum class in general, happy either way, your pick!). Nah, I was just curious. Enum class does require a lot of typing, especially for nested types like this. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:33: UpdateActiveSetupVersionWorkItem(const base::string16& active_setup_path, On 2015/07/09 20:36:38, gab wrote: > On 2015/07/09 14:23:53, grt wrote: > > nit: make order of params and order of description in the doc comment match? > > I think the callsites read better in this order yet semantically I find the > comment reads better this way... Let me know if you feel otherwise. Only saying because I think I've been asked to do this (perhaps by you :-)). I don't feel strongly.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
comments addressed PTAL https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... File chrome/chrome_installer_util.gypi (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/chrome_installer... chrome/chrome_installer_util.gypi:91: 'installer/util/update_active_setup_version_work_item.cc', On 2015/07/10 15:37:27, grt wrote: > On 2015/07/09 20:36:37, gab wrote: > > On 2015/07/09 14:23:53, grt wrote: > > > since this is only used by setup.exe, please move it into installer/setup. > > it's > > > nice that your design allows for this. yay! > > > > I debated this, but this is true of all WorkItems > > Some of them are used by chrome.exe (shell_util.cc uses them for the registry > stuff), so they can't all be moved into setup. > > > AFAICT, a nice follow-up > > cleanup, but I don't think this CL should introduce an outlier in the mean > time. > > I don't see this as a compelling reason to follow a bad trend. In the worst > case, chrome.exe will be bloated with dead code leading to larger downloads and > more bits in the binary. Another bad trend this forces me to follow now is the setup_unittests target actually doesn't depend on "setup" and redeclares inclusions twice: """ # TODO(robertshield): Move the items marked with "Move to lib" # below into a separate lib and then link both setup.exe and # setup_unittests.exe against that. """ But oh well, I don't think this matters enough to block this much more important CL. (also, since I want to merge this I'm leery of not having the tests on the waterfall NOW, but I'll run them locally and hope for the best..). https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... File chrome/installer/util/update_active_setup_version_work_item.h (right): https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:15: // TODO(gab): This would be cleaner if UpdateActiveSetupVersionWorkItem On 2015/07/10 15:37:27, grt wrote: > On 2015/07/09 20:36:38, gab wrote: > > On 2015/07/09 14:23:53, grt wrote: > > > since you're modifying SetRegValueWorkItem to make this inheritance > possible, > > > why not friend UpdateActiveSetupVersionWorkItem instead? i think it's > cleaner > > to > > > use composition rather than inheritance here, even if it means poking an > ugly > > > hole in a strange design. > > > > The reason I didn't do this is that friend'ing gives full access to the class > > (i.e. there is no such thing as a "protected-only friend"), therefore I > thought > > this approach gave better encapsulation by only allowing the constructors to > be > > used. > > I see your point, but I think that's less bad than inheritance here. In that case, I think moving the constructors to public is better than friend'ing. https://codereview.chromium.org/1223953003/diff/20001/chrome/installer/util/u... chrome/installer/util/update_active_setup_version_work_item.h:33: UpdateActiveSetupVersionWorkItem(const base::string16& active_setup_path, On 2015/07/10 15:37:27, grt wrote: > On 2015/07/09 20:36:38, gab wrote: > > On 2015/07/09 14:23:53, grt wrote: > > > nit: make order of params and order of description in the doc comment match? > > > > I think the callsites read better in this order yet semantically I find the > > comment reads better this way... Let me know if you feel otherwise. > > Only saying because I think I've been asked to do this (perhaps by you :-)). I > don't feel strongly. Possible :-), it's kind of arbitrary, but here I prefer it this way I think..
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1223953003/120001
lgtm w/ two nits https://codereview.chromium.org/1223953003/diff/120001/chrome/chrome_installe... File chrome/chrome_installer.gypi (right): https://codereview.chromium.org/1223953003/diff/120001/chrome/chrome_installe... chrome/chrome_installer.gypi:359: 'installer/setup/update_active_setup_version_work_item.cc', # Move to lib add to chrome/installer/setup/BUILD.gn https://codereview.chromium.org/1223953003/diff/120001/chrome/installer/setup... File chrome/installer/setup/update_active_setup_version_work_item.h (right): https://codereview.chromium.org/1223953003/diff/120001/chrome/installer/setup... chrome/installer/setup/update_active_setup_version_work_item.h:16: // TODO(gab): This would be cleaner if UpdateActiveSetupVersionWorkItem nit: stale comment
Thanks https://codereview.chromium.org/1223953003/diff/120001/chrome/chrome_installe... File chrome/chrome_installer.gypi (right): https://codereview.chromium.org/1223953003/diff/120001/chrome/chrome_installe... chrome/chrome_installer.gypi:359: 'installer/setup/update_active_setup_version_work_item.cc', # Move to lib On 2015/07/13 16:47:04, grt wrote: > add to chrome/installer/setup/BUILD.gn Ah, had checked but looks like chrome/installer/setup/ support for GN (7 days old) is more recent than my checkout (June 22nd!), so I didn't have this file locally, synced and done. https://codereview.chromium.org/1223953003/diff/120001/chrome/installer/setup... File chrome/installer/setup/update_active_setup_version_work_item.h (right): https://codereview.chromium.org/1223953003/diff/120001/chrome/installer/setup... chrome/installer/setup/update_active_setup_version_work_item.h:16: // TODO(gab): This would be cleaner if UpdateActiveSetupVersionWorkItem On 2015/07/13 16:47:04, grt wrote: > nit: stale comment Oops, done.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/1223953003/#ps140001 (title: "merge to r338517 add GN support for setup_unittests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1223953003/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3861555fdf3bd6238cb177b66a83ac34fe06752e Cr-Commit-Position: refs/heads/master@{#338528} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
