Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1146843003:
Beacons for tracking default browser status. (Closed)
|
Created:
5 years, 7 months ago by grt (UTC plus 2) Modified:
5 years, 6 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. |
DescriptionBeacons for tracking default browser status.
BUG=488247
R=gab@chromium.org,wfh@chromium.org
Committed: https://crrev.com/53cc88da9a258bc4a34c4bff50025ee044c2e64d
Cr-Commit-Position: refs/heads/master@{#332423}
Patch Set 1 #Patch Set 2 : unittest, and update default beacon on os update for user-level installs #Patch Set 3 : revise active setup version #
Total comments: 61
Patch Set 4 : gab comments #Patch Set 5 : sync to position 331965 #Patch Set 6 : gab comments #
Total comments: 8
Patch Set 7 : gab comments #Patch Set 8 : sync to position 332247 #Patch Set 9 : fix using in unittest #Patch Set 10 : take three #
Messages
Total messages: 36 (14 generated)
Patchset #1 (id:1) has been deleted
PTAL
The CQ bit was checked by grt@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/1146843003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/05/22 17:24:04, grt wrote: > PTAL gab, wfh: ping
Looks very good, neat code and tests :-). Bunch of nits/questions/comments below, but otherwise love the way this is structured. (sorry about the delay, couldn't find long enough heads down time yesterday) Cheers, Gab https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/setup/... chrome/installer/setup/install.cc:629: UpdateOsUpgradeBeacon(installer_state.system_install(), Do we get the OS upgrade notification for user-level installs? For some reason I'm having second thoughts and thinking this is perhaps system-level only though I don't see why it would be..? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:15: ignore_result(ShellUtil::GetChromeDefaultStateFromPath(chrome_exe)); Not done reading this CL but I assume this means that this ends up calling back into UpdateDefaultBrowserBeacon()? This is non-obvious to a reader at this point (and also fragile to the implementation of an external component :-(...). https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:76: const REGSAM kAccess = KEY_WOW64_32KEY | KEY_QUERY_VALUE | KEY_SET_VALUE; Haven't seen much installer code since you and Will made those changes, can you refresh my mind? KEY_WOW64_32KEY means all Chromes (32 or 64) will look at the 32-bit hive? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:89: FILETIME now(base::Time::Now().ToFileTime()); Do we really need that precise of a timestamp? Feels seconds should be good enough (i.e. like the install date timestamp in local state) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:101: if (key.Open(root_, key_path_.c_str(), kAccess) == ERROR_SUCCESS) Feels weird to use key.Open() here and the RegKey() constructor in Update() above. They are logically the same, aren't they? Can we be consistent amongst implementations in this file? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:129: root_(system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER), Remind me how ClientStateMedium works again? It's in HKLM but is writeable without admin rights? (and below we key off the SID to split each user in it? -- shouldn't we instead use HKCU?) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.h (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:25: // appropriate beacon (last was default or first not default). Reorder sentence to begin with the actual action this method performs? i.e.: Updates the (last was default or first not default) beacon based on the default state of the browser for the current user. ? (I'm asking specifically because it confused me when first glancing at definitions that the two first methods were "Update...()" yet their descriptions began differently by "Checks" and "Updates) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:30: void UpdateDefaultBrowserBeacon(const base::FilePath& chrome_exe, s/UpdateDefaultBrowserBeacon/UpdateDefaultBrowserBeaconWithState ? i.e., make it more obvious what the difference between this one and the other is? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:41: class Beacon { I think this should either be in the install_util namespace or have a more specific name than simply "Beacon". https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:43: ~Beacon(); I think you can use ~Beacon() = default; here instead of having the definition in the .cc (I'm not sure whether that's the same as inlining it, but either way you should be fine as your class is only made of POD types (+ strings which IIRC are also considered POD in some way)) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons_unittest.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:23: class TestBeacon : public Beacon { So a TestBeacon is a Beacon with a public constructor essentially? Feels like a weird way to "friend" a test. I think I'd almost prefer a private Beacon constructor which explicitly friends the test, no? (rather than having this weird "inherit me and you shall be my friend" paradigm) Maybe Erik has a better idea. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:40: bool>> { I think you can name the parameters here (doesn't change anything just like naming parameters in a header, but it helps the reader -- especially for the bool) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:63: registry_util::RegistryOverrideManager registry_override_manager_; This member should be private IMO, no reason the tests should need to override further. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:69: // Nothing in the regsitry, so the becon should not exist. s/becon/beacon https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:86: beacon_.Update(); Should there be some kind of sleep before this Update to make sure the timestamps saved do differ and make sure this test fails rather than become flaky if it is ever broken. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:126: ASSERT_TRUE(key.HasValue(value_name.c_str())) << value_name; Should also test that we can reconstruct a reasonable timestamp from the value (i.e. one that is <= Now() - test_timeout)? (to confirm that our timestamp math is correct -- ideally testing the Time returned by Beacon's logic). https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:141: CHROME_SXS, I'd remove CHROME from all of these (since enum class already enforces an explicit namespace prefix). https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:151: chrome_sxs_(GetParam() == DistributionVariant::CHROME_SXS), Curious, why is Chrome SxS different than regular user-level Chrome for the Beacon code? It should behave the same, no? I guess you're confirming that it does behave the same given it's in practice not all the same code in practice? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:186: base::FilePath GetChromePath() const { Move to private section. And I'd call this GetChromePathForParams() or something (at first I thought this was an install_util method per its generic name and was slightly confused) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:219: registry_util::RegistryOverrideManager registry_override_manager_; Move these two member to private: https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:224: TEST_P(DefaultBrowserBeaconTest, Test) { s/Test/All ? (feels weird to have a test named "Test"...) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:259: ASSERT_FALSE(first_not_default->Get().is_null()); Shouldn't this test also check that the timestamps are as expected? (e.g., bounded between two timestamps taken in this test's logic perhaps?) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:265: INSTANTIATE_TEST_CASE_P(UserLevelChrome, Oh cool, didn't know we could split various parametrized test instances like that (now I see why the macro takes an "instance name" parameter!) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:269: // Disabled for now since InstallUtil::IsChromeSxSProcess makes this impossible. Why? Can't we override DIR_EXE to fake this? i.e. with chrome_exe_.DirName(): IsChromeSxSProcess() seems to look at DIR_EXE rather than FILE_EXE Or even override the command-line with --chrome-sxs (although this switch is really only meant for the installer)
Thanks! https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/setup/... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/setup/... chrome/installer/setup/install.cc:629: UpdateOsUpgradeBeacon(installer_state.system_install(), On 2015/05/26 18:54:34, gab wrote: > Do we get the OS upgrade notification for user-level installs? Yes, the feature is agnostic to the install level. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:15: ignore_result(ShellUtil::GetChromeDefaultStateFromPath(chrome_exe)); On 2015/05/26 18:54:34, gab wrote: > Not done reading this CL but I assume this means that this ends up calling back > into UpdateDefaultBrowserBeacon()? Yes. The doc comment explains that it makes a check for defaultness. This function is simply a wrapper around Chrome's existing "am I default?" check. > This is non-obvious to a reader at this point > (and also fragile to the implementation of an external component :-(...). I chose to put the beacon update into the ShellUtil code that does Chrome's default check since the beacon's reason for existence is to record the last/first time Chrome saw that it was/was not default. Putting it anywhere else would, in my opinion, put it too high up in the stack and require slapping "update the beacon" code at each point where Chrome checks for defaultness (settings page, startup, etc). What function name/comments would you like to see to make this more clear? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:76: const REGSAM kAccess = KEY_WOW64_32KEY | KEY_QUERY_VALUE | KEY_SET_VALUE; On 2015/05/26 18:54:34, gab wrote: > Haven't seen much installer code since you and Will made those changes, can you > refresh my mind? KEY_WOW64_32KEY means all Chromes (32 or 64) will look at the > 32-bit hive? Yes. This is where Google Update keeps its state regardless of the bitness of the app. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:89: FILETIME now(base::Time::Now().ToFileTime()); On 2015/05/26 18:54:34, gab wrote: > Do we really need that precise of a timestamp? Feels seconds should be good > enough (i.e. like the install date timestamp in local state) FILETIME is a simple, well-documented platform type. What are the arguments against it? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:101: if (key.Open(root_, key_path_.c_str(), kAccess) == ERROR_SUCCESS) On 2015/05/26 18:54:34, gab wrote: > Feels weird to use key.Open() here and the RegKey() constructor in Update() > above. > > They are logically the same, aren't they? Can we be consistent amongst > implementations in this file? Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:129: root_(system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER), On 2015/05/26 18:54:34, gab wrote: > Remind me how ClientStateMedium works again? It's in HKLM but is writeable > without admin rights? Yes. > (and below we key off the SID to split each user in it? -- > shouldn't we instead use HKCU?) No. Google Update does not maintain keys in HKCU for per-machine installs. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.h (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:25: // appropriate beacon (last was default or first not default). On 2015/05/26 18:54:34, gab wrote: > Reorder sentence to begin with the actual action this method performs? It is in the right order: it checks the default state and updates the beacons. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:30: void UpdateDefaultBrowserBeacon(const base::FilePath& chrome_exe, On 2015/05/26 18:54:34, gab wrote: > s/UpdateDefaultBrowserBeacon/UpdateDefaultBrowserBeaconWithState ? > > i.e., make it more obvious what the difference between this one and the other > is? Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:41: class Beacon { On 2015/05/26 18:54:34, gab wrote: > I think this should either be in the install_util namespace or have a more > specific name than simply "Beacon". I've been steered away from namespaces by other reviewers. Could you suggest a name you'd be happy with? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:43: ~Beacon(); On 2015/05/26 18:54:34, gab wrote: > I think you can use > ~Beacon() = default; > here instead of having the definition in the .cc (I'm not sure whether that's > the same as inlining it, but either way you should be fine as your class is only > made of POD types (+ strings which IIRC are also considered POD in some way)) A string16 isn't a POD since it has a dtor, but maybe it's considered trivial enough to not call for an out-of-line dtor here. I find the rules around this somewhat arbitrary, and I'm loath to change it without knowing that it isn't going to set off an alarm somewhere. I'm trying to build with clang, but clang itself isn't building for me at the moment. I'll leave this as-is for now. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons_unittest.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:23: class TestBeacon : public Beacon { On 2015/05/26 18:54:35, gab wrote: > So a TestBeacon is a Beacon with a public constructor essentially? Feels like a > weird way to "friend" a test. > > I think I'd almost prefer a private Beacon constructor which explicitly friends > the test, no? (rather than having this weird "inherit me and you shall be my > friend" paradigm) I've seen and used this from time to time. I find friend distasteful. No one is going to subclass it in Chrome, so the protected ctor is not a risk. I think keeping the test names out of the public header is a good thing. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:40: bool>> { On 2015/05/26 18:54:35, gab wrote: > I think you can name the parameters here (doesn't change anything just like > naming parameters in a header, but it helps the reader -- especially for the > bool) Do you mean BeaconType, BeaconScope, and bool? Those are template arguments. I don't think there any other ways to name them. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:63: registry_util::RegistryOverrideManager registry_override_manager_; On 2015/05/26 18:54:35, gab wrote: > This member should be private IMO, no reason the tests should need to override > further. Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:69: // Nothing in the regsitry, so the becon should not exist. On 2015/05/26 18:54:35, gab wrote: > s/becon/beacon Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:86: beacon_.Update(); On 2015/05/26 18:54:35, gab wrote: > Should there be some kind of sleep before this Update to make sure the > timestamps saved do differ and make sure this test fails rather than become > flaky if it is ever broken. Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:126: ASSERT_TRUE(key.HasValue(value_name.c_str())) << value_name; On 2015/05/26 18:54:35, gab wrote: > Should also test that we can reconstruct a reasonable timestamp from the value > (i.e. one that is <= Now() - test_timeout)? > > (to confirm that our timestamp math is correct -- ideally testing the Time > returned by Beacon's logic). Done in UpdateAndGet. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:141: CHROME_SXS, On 2015/05/26 18:54:35, gab wrote: > I'd remove CHROME from all of these (since enum class already enforces an > explicit namespace prefix). Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:151: chrome_sxs_(GetParam() == DistributionVariant::CHROME_SXS), On 2015/05/26 18:54:35, gab wrote: > Curious, why is Chrome SxS different than regular user-level Chrome for the > Beacon code? It should behave the same, no? I guess you're confirming that it > does behave the same given it's in practice not all the same code in practice? It should be the same, yes. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:186: base::FilePath GetChromePath() const { On 2015/05/26 18:54:35, gab wrote: > Move to private section. > > And I'd call this GetChromePathForParams() or something (at first I thought this > was an install_util method per its generic name and was slightly confused) Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:219: registry_util::RegistryOverrideManager registry_override_manager_; On 2015/05/26 18:54:35, gab wrote: > Move these two member to private: Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:224: TEST_P(DefaultBrowserBeaconTest, Test) { On 2015/05/26 18:54:35, gab wrote: > s/Test/All ? (feels weird to have a test named "Test"...) "All" is more descriptive? Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:259: ASSERT_FALSE(first_not_default->Get().is_null()); On 2015/05/26 18:54:35, gab wrote: > Shouldn't this test also check that the timestamps are as expected? (e.g., > bounded between two timestamps taken in this test's logic perhaps?) BeaconTest::UpdateAndGet and UpdateTwice cover everything that testing the timestamps here would cover. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:265: INSTANTIATE_TEST_CASE_P(UserLevelChrome, On 2015/05/26 18:54:35, gab wrote: > Oh cool, didn't know we could split various parametrized test instances like > that (now I see why the macro takes an "instance name" parameter!) Acknowledged. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:269: // Disabled for now since InstallUtil::IsChromeSxSProcess makes this impossible. On 2015/05/26 18:54:35, gab wrote: > Why? Can't we override DIR_EXE to fake this? i.e. with chrome_exe_.DirName(): > IsChromeSxSProcess() seems to look at DIR_EXE rather than FILE_EXE That was what I hoped, except that it doesn't work due to the local static in IsChromeSxSProcess. I do not wish to refactor that for testing today.
More thinking. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.h (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:41: class Beacon { On 2015/05/26 20:54:01, grt wrote: > On 2015/05/26 18:54:34, gab wrote: > > I think this should either be in the install_util namespace or have a more > > specific name than simply "Beacon". > > I've been steered away from namespaces by other reviewers. Could you suggest a > name you'd be happy with? Thinking about it more, this class is pretty much an implementation detail for the functions above, so I think it does make sense to move it into the installer_util namespace (leaving the functions outside of the namespace). SGTY?
lg, some more comments/replies. Cheers, Gab https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:15: ignore_result(ShellUtil::GetChromeDefaultStateFromPath(chrome_exe)); On 2015/05/26 20:54:01, grt wrote: > On 2015/05/26 18:54:34, gab wrote: > > Not done reading this CL but I assume this means that this ends up calling > back > > into UpdateDefaultBrowserBeacon()? > > Yes. The doc comment explains that it makes a check for defaultness. This > function is simply a wrapper around Chrome's existing "am I default?" check. > > > This is non-obvious to a reader at this point > > (and also fragile to the implementation of an external component :-(...). > > I chose to put the beacon update into the ShellUtil code that does Chrome's > default check since the beacon's reason for existence is to record the > last/first time Chrome saw that it was/was not default. Putting it anywhere else > would, in my opinion, put it too high up in the stack and require slapping > "update the beacon" code at each point where Chrome checks for defaultness > (settings page, startup, etc). > > What function name/comments would you like to see to make this more clear? How about s/GetChromeDefaultStateFromPath/GetChromeDefaultStateFromPathAndCacheResult/ and updating the ShellUtil method's description (i.e. it feels wrong to have a GetFoo() method have a permanent side-effect and even more so to call ignore_result(GetFoo()) from an external component solely to trigger that side-effect. With the above change we could then call this method ForceDefaultBrowserBeaconUpdateForPath() which would make it more reasonable I think to then force the check and ignore the result. In all cases I don't really like this weird dependency: how can we test that this dependency on the side-effect triggered by GetChromeDefaultStateFromPathAndCacheResult() at least remains healthy? https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:89: FILETIME now(base::Time::Now().ToFileTime()); On 2015/05/26 20:54:01, grt wrote: > On 2015/05/26 18:54:34, gab wrote: > > Do we really need that precise of a timestamp? Feels seconds should be good > > enough (i.e. like the install date timestamp in local state) > > FILETIME is a simple, well-documented platform type. What are the arguments > against it? My argument against it is you need multiple lines of code to break it down when writing it and to reconstruct it when reading it. It's also more complex to read (i.e. I can't tell whether this is done correctly without reading the spec). So I don't see a reason for using it unless the extra precision (over Time::(From|To)InternalValue() which gives you a serialized value with millisecond accuracy) is needed and I don't think it is. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.h (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:25: // appropriate beacon (last was default or first not default). On 2015/05/26 20:54:01, grt wrote: > On 2015/05/26 18:54:34, gab wrote: > > Reorder sentence to begin with the actual action this method performs? > > It is in the right order: it checks the default state and updates the beacons. Well the fact that it "checks" is a side-effect of what the implementation has to do, but what it really does is ensures the update has occurred (in fact the check is such an implementation detail that the result is ignored). https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:41: class Beacon { On 2015/05/28 14:05:42, grt wrote: > On 2015/05/26 20:54:01, grt wrote: > > On 2015/05/26 18:54:34, gab wrote: > > > I think this should either be in the install_util namespace or have a more > > > specific name than simply "Beacon". > > > > I've been steered away from namespaces by other reviewers. Could you suggest a > > name you'd be happy with? > > Thinking about it more, this class is pretty much an implementation detail for > the functions above, so I think it does make sense to move it into the > installer_util namespace (leaving the functions outside of the namespace). SGTY? I like a namespace here; otherwise you need a more Installer specific name for this class anyways. Either way you need a way to make this name more specific, a namespace seems right to me. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons_unittest.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:23: class TestBeacon : public Beacon { On 2015/05/26 20:54:02, grt wrote: > On 2015/05/26 18:54:35, gab wrote: > > So a TestBeacon is a Beacon with a public constructor essentially? Feels like > a > > weird way to "friend" a test. > > > > I think I'd almost prefer a private Beacon constructor which explicitly > friends > > the test, no? (rather than having this weird "inherit me and you shall be my > > friend" paradigm) > > I've seen and used this from time to time. I find friend distasteful. No one is > going to subclass it in Chrome, so the protected ctor is not a risk. I think > keeping the test names out of the public header is a good thing. Well at least friend puts the code owner of Beacon in control of who can build it, whereas this paradigm allows anyone to essentially backdoor friend (without any other use for inheritance). I understand why you find friend distasteful, but to me this is just a hidden friend which IMO is even worse. Friend'ing tests no matter how distasteful is at least a more common paradigm I'd say (i.e. 528 instances of FRIEND_TEST_ALL_PREFIXES() in chromium). https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:40: bool>> { On 2015/05/26 20:54:02, grt wrote: > On 2015/05/26 18:54:35, gab wrote: > > I think you can name the parameters here (doesn't change anything just like > > naming parameters in a header, but it helps the reader -- especially for the > > bool) > > Do you mean BeaconType, BeaconScope, and bool? Those are template arguments. I > don't think there any other ways to name them. Ah ok, I guess I was recalling something like base::Callback<void(Foo foo, Bar bar)> where you can give unused method parameters names, perhaps this doesn't work on template arguments directly, if so, ignore me :-) https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:224: TEST_P(DefaultBrowserBeaconTest, Test) { On 2015/05/26 20:54:02, grt wrote: > On 2015/05/26 18:54:35, gab wrote: > > s/Test/All ? (feels weird to have a test named "Test"...) > > "All" is more descriptive? Done. I find so, i.e. it tests "everything" rather than just it "tests" (something?). Perhaps, "ConfirmBeaconsAsDesiredWhenToggling" is even more descriptive?!
Patchset #6 (id:120001) has been deleted
PTAL https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.cc:89: FILETIME now(base::Time::Now().ToFileTime()); On 2015/05/29 13:37:36, gab wrote: > On 2015/05/26 20:54:01, grt wrote: > > On 2015/05/26 18:54:34, gab wrote: > > > Do we really need that precise of a timestamp? Feels seconds should be good > > > enough (i.e. like the install date timestamp in local state) > > > > FILETIME is a simple, well-documented platform type. What are the arguments > > against it? > > My argument against it is you need multiple lines of code to break it down when > writing it and to reconstruct it when reading it. > > It's also more complex to read (i.e. I can't tell whether this is done correctly > without reading the spec). > > So I don't see a reason for using it unless the extra precision (over > Time::(From|To)InternalValue() which gives you a serialized value with > millisecond accuracy) is needed and I don't think it is. Changed to storing using base::Time's internal value as discussed. As it happens, this is defined in terms of a Windows FILETIME, so it will be easy to convert the value to a FILETIME in a program that doesn't have access to base/ if needed. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons.h (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:25: // appropriate beacon (last was default or first not default). On 2015/05/29 13:37:36, gab wrote: > On 2015/05/26 20:54:01, grt wrote: > > On 2015/05/26 18:54:34, gab wrote: > > > Reorder sentence to begin with the actual action this method performs? > > > > It is in the right order: it checks the default state and updates the beacons. > > Well the fact that it "checks" is a side-effect of what the implementation has > to do, The only way that the beacons can be updated to reflect the defaultness of Chrome is to check whether or not Chrome is the default browser. This is orthogonal to the specific way it is implemented. Since the beacon update is buried at the lowest layer of Chrome's "am I default?" code, the implementation calls down to that code and ignores the result. It still remains that, semantically speaking, this function must a) somehow check to see if Chrome is the default browser, and b) update the beacons accordingly. It just so happens that b is a consequence of the implementation of a. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons.h:41: class Beacon { On 2015/05/29 13:37:36, gab wrote: > On 2015/05/28 14:05:42, grt wrote: > > On 2015/05/26 20:54:01, grt wrote: > > > On 2015/05/26 18:54:34, gab wrote: > > > > I think this should either be in the install_util namespace or have a more > > > > specific name than simply "Beacon". > > > > > > I've been steered away from namespaces by other reviewers. Could you suggest > a > > > name you'd be happy with? > > > > Thinking about it more, this class is pretty much an implementation detail for > > the functions above, so I think it does make sense to move it into the > > installer_util namespace (leaving the functions outside of the namespace). > SGTY? > > I like a namespace here; otherwise you need a more Installer specific name for > this class anyways. Either way you need a way to make this name more specific, a > namespace seems right to me. Done. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... File chrome/installer/util/beacons_unittest.cc (right): https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:23: class TestBeacon : public Beacon { On 2015/05/29 13:37:36, gab wrote: > On 2015/05/26 20:54:02, grt wrote: > > On 2015/05/26 18:54:35, gab wrote: > > > So a TestBeacon is a Beacon with a public constructor essentially? Feels > like > > a > > > weird way to "friend" a test. > > > > > > I think I'd almost prefer a private Beacon constructor which explicitly > > friends > > > the test, no? (rather than having this weird "inherit me and you shall be my > > > friend" paradigm) > > > > I've seen and used this from time to time. I find friend distasteful. No one > is > > going to subclass it in Chrome, so the protected ctor is not a risk. I think > > keeping the test names out of the public header is a good thing. > > Well at least friend puts the code owner of Beacon in control of who can build > it, whereas this paradigm allows anyone to essentially backdoor friend (without > any other use for inheritance). I understand why you find friend distasteful, > but to me this is just a hidden friend which IMO is even worse. Friend'ing tests > no matter how distasteful is at least a more common paradigm I'd say (i.e. 528 > instances of FRIEND_TEST_ALL_PREFIXES() in chromium). Rejiggered as discussed. https://codereview.chromium.org/1146843003/diff/60001/chrome/installer/util/b... chrome/installer/util/beacons_unittest.cc:224: TEST_P(DefaultBrowserBeaconTest, Test) { On 2015/05/29 13:37:36, gab wrote: > On 2015/05/26 20:54:02, grt wrote: > > On 2015/05/26 18:54:35, gab wrote: > > > s/Test/All ? (feels weird to have a test named "Test"...) > > > > "All" is more descriptive? Done. > > I find so, i.e. it tests "everything" rather than just it "tests" (something?). > > Perhaps, "ConfirmBeaconsAsDesiredWhenToggling" is even more descriptive?! I'm okay with "All".
The CQ bit was checked by grt@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/1146843003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/05/29 15:31:11, grt wrote: > PTAL gab: ping
lgtm w/ nits https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/beacons.cc (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/beacons.cc:15: ignore_result(ShellUtil::GetChromeDefaultStateFromPath(chrome_exe)); Add a comment that GetChromeDefaultStateFromPath() updates the state after obtaining it (as its API mentions) so that the reader can really on this comment rather than having to jump to another file to understand how this works. https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/beacons.h (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/beacons.h:55: scoped_ptr<Beacon> MakeFirstNotDefaultBeacon( s/Make/Get ? To me "Make" sounds like you'll actually set the state whereas you're only getting an item representing the state. https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/beacons_unittest.cc (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/beacons_unittest.cc:30: ::testing::tuple<Beacon::BeaconType, Beacon::BeaconScope, bool>> { I think it makes sense to have using Beacon::BeaconType; using Beacon::BeaconScope; at the top of this file (IIRC using works with enum classes). https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/shell_util.h:413: // Returns the DefaultState of Chrome for HTTP and HTTPS, updating the default s/, updating/and updates/ ? To me: "Foos bar, updating X." Means that the update of X is the result of foo'ing bar. Which in this case is incorrect (i.e. it's not the return that causes the update). Feel free to tell me I can't English ;-).
Thanks. https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/beacons.cc (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/beacons.cc:15: ignore_result(ShellUtil::GetChromeDefaultStateFromPath(chrome_exe)); On 2015/06/01 18:43:12, gab wrote: > Add a comment that GetChromeDefaultStateFromPath() updates the state after > obtaining it (as its API mentions) so that the reader can really on this comment > rather than having to jump to another file to understand how this works. Done. https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/beacons.h (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/beacons.h:55: scoped_ptr<Beacon> MakeFirstNotDefaultBeacon( On 2015/06/01 18:43:12, gab wrote: > s/Make/Get ? > > To me "Make" sounds like you'll actually set the state whereas you're only > getting an item representing the state. I chose Make since it returns an instance distinct from any other (Create would also work, but Make is shorter :-) ). Get doesn't sound right to me for a factory function since it sounds like it returns the one Beacon instance. https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/beacons_unittest.cc (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/beacons_unittest.cc:30: ::testing::tuple<Beacon::BeaconType, Beacon::BeaconScope, bool>> { On 2015/06/01 18:43:12, gab wrote: > I think it makes sense to have > using Beacon::BeaconType; > using Beacon::BeaconScope; > at the top of this file (IIRC using works with enum classes). Done. https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1146843003/diff/140001/chrome/installer/util/... chrome/installer/util/shell_util.h:413: // Returns the DefaultState of Chrome for HTTP and HTTPS, updating the default On 2015/06/01 18:43:12, gab wrote: > s/, updating/and updates/ ? > > To me: > "Foos bar, updating X." > Means that the update of X is the result of foo'ing bar. > > Which in this case is incorrect (i.e. it's not the return that causes the > update). > > Feel free to tell me I can't English ;-). Done.
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1146843003/#ps180001 (title: "sync to position 332247")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146843003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1146843003/#ps200001 (title: "fix using in unittest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146843003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1146843003/#ps220001 (title: "take three")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146843003/220001
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/53cc88da9a258bc4a34c4bff50025ee044c2e64d Cr-Commit-Position: refs/heads/master@{#332423}
Message was sent while issue was closed.
great success! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
