Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
836703005:
Add a new function for getting a deterministic machine ID (Closed)
DescriptionAdd a new function for getting a deterministic machine ID
Currently, an RLZ machine-specific ID is used to protect and verify a
subset of user preferences. A new function was added that returns the
machine SID and can be used instead of the RLZ ID. Currently available
only on Windows.
BUG=453825
Committed: https://crrev.com/c4f3ce9f101b12a3368355a28c02c8aac21ed432
Cr-Commit-Position: refs/heads/master@{#314058}
Patch Set 1 #
Total comments: 47
Patch Set 2 : #
Total comments: 22
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #Patch Set 6 : Circumvent gcc bug in device_id_unittest.cc #Patch Set 7 : Trying a different gcc bug circumvention #
Total comments: 3
Patch Set 8 : Added link to bug report in comment and fixed include order #
Messages
Total messages: 58 (19 generated)
alito@chromium.org changed reviewers: + gab@google.com
gab: please take a look.
gab@chromium.org changed reviewers: + gab@chromium.org - gab@google.com
Great CL, thanks Ali. Digging into this code made me realize however that getting the machine SID is actually more complex than I thought and sprung a bunch of questions to my mind... I was sure this was the next step and am now debating it (sorry about that..). Let's discuss offline whether this is really what we need. Thanks for your patience and great work (and hopefully you learn something out of this even if we end up not doing it). https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id.h:10: bool GetDeterministicMachineSpecificId(std::string* machine_id); Add a method comment which starts with a verb stating the actions it results in, e.g.: // Populates |machine_id| with a deterministic ID for this machine. Returns false on failure (in which case |machine_id|'s content is undefined and should not be used). In fact since we'll need to differentiate between "unsupported on platform" and "failed to obtain ID" (easier for tests and users of the call will likely care about the difference), how about changing the return type to an enum? https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id.h:11: Now that I see it in context, I think it may make sense to wrap this in a "tracked_prefs" namespace (similar to the "chrome_prefs" namespace used in chrome_pref_service_factory.h for similar free functions). I always forget the exact guidance on namespaces in Chromium, probably because there isn't even a clear agreement among Chromium devs (latest discussion on the topic: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/E3K_K...). From the Google C++ Style Guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem... "Sometimes it is useful, or even necessary, to define a function not bound to a class instance. Such a function can be either a static member or a nonmember function. Nonmember functions should not depend on external variables, and should nearly always exist in a namespace. Rather than creating classes only to group static member functions which do not share static data, use namespaces instead." http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces "With named namespaces, choose the name based on the project, and possibly its path." Therefore it feels like a "tracked_prefs" namespace here is the right thing after all (sorry if I told you the opposite when we first discussed it, I changed my mind after seeing it..). @grt: WDYT, any opinion/guidance on namespace usage in Chromium? https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_stub.cc (left): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_stub.cc:5: package org.chromium.dummy; You can use git cl upload --similarity=100 in order to not have the upload consider this a copy w/ modifications of an other file. (no big deal, just a tip) https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_stub.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_stub.cc:13: return false; I think this should just return false (or the enum value for "not implemented"). https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_unittest.cc:15: #endif This will likely change based on other comments, but FTR here's what I had in mind: const bool kMachineIdEnabled = #if defined(OS_WIN) true; #else false; #endif https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_unittest.cc:22: EXPECT_EQ(string(""), first_machine_id); I think this test will change based on other comments, but FTR here it's slightly cleaner to do EXPECT_TRUE(first_machine_id.empty()); In fact here we also want to make sure it's not empty when one is expected, so you'd want: EXPECT_EQ(!kMachineIdEnabled, first_machine_id.empty()); https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> I don't think you need these two headers. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:10: #include <intSafe.h> // For DWORD. DWORD implicitly comes with windows.h AFAIK (I've never seen anyone include intsafe.h for it at least). https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:11: #include <sddl.h> // For ConvertSidToStringSidW. s/ConvertSidToStringSidW/ConvertSidToStringSidA https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:12: #include <windows.h> We always put windows.h first (this is more or less the only exception to "always alpha sort the headers"). We put the definitions in the following order: <interface header> <C headers> (alpha-sorted, but windows.h first if needed) (optional empty line -- I like having it but it's not required AFAICT) <C++ headers> (alpha-sorted) <project headers> (alpha-sorted) So in this case (with the headers you have here if we weren't changing them): <windows.h> <intsafe.h> <sddl.h> <cstddef> <cwchar> <base/macros.h> ... (see this article for more details: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...) https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:17: bool GetDeterministicMachineSpecificId(std::string* machine_id) { Looks like there is a GetUserSidString() method in base/win; it would probably make sense to have a general implementation of a GetMachineSidString() there as well (used by both this code and RLZ) since this is non-trivial code. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:19: return false; Instead do: DCHECK(machine_id); to enforce the contract that |machine_id| should always be provided (and mention that contract in the header). https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:21: wchar_t computer_name[MAX_COMPUTERNAME_LENGTH + 1] = {0}; {0} is the same as {} except that you explicitly set the first element to 0 and the rest is default initialized. {} is preferable here (@grt: I know you're an advocate of this, is this still the recommendation?) https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:45: domain_buffer.get(), &domain_size, |domain_buffer| is optional and we don't use its value, change it to NULL and |domain_size| to 0 This also makes it unnecessary to call this a second time if it fails the first time IIUC. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:47: return false; Wrap in {} (always wrap in {} unless both the conditional and the body are one-liners) https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:49: DCHECK_EQ(SID_NAME_USE::SidTypeComputer, sid_name_use); to confirm that we're using this API the right way and are indeed getting the machine's SID https://codereview.chromium.org/836703005/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/836703005/diff/1/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:2182: 'browser/prefs/tracked/device_id.h', Put these two alongside the other prefs/tracked in the non-OS specific section. (the _win.cc will automatically get selected out on non-Win (by filename_rules.gypi) -- these OS-specific sections are only for things where an ifdef is truly intended whereas we want our interface to be cross-platform)
Please take another look. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id.h:10: bool GetDeterministicMachineSpecificId(std::string* machine_id); On 2015/01/20 15:34:12, gab wrote: > Add a method comment which starts with a verb stating the actions it results in, > e.g.: > > // Populates |machine_id| with a deterministic ID for this machine. Returns > false on failure (in which case |machine_id|'s content is undefined and should > not be used). > > In fact since we'll need to differentiate between "unsupported on platform" and > "failed to obtain ID" (easier for tests and users of the call will likely care > about the difference), how about changing the return type to an enum? Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id.h:11: On 2015/01/20 15:34:12, gab wrote: > Now that I see it in context, I think it may make sense to wrap this in a > "tracked_prefs" namespace (similar to the "chrome_prefs" namespace used in > chrome_pref_service_factory.h for similar free functions). > > I always forget the exact guidance on namespaces in Chromium, probably because > there isn't even a clear agreement among Chromium devs (latest discussion on the > topic: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/E3K_K...). > > From the Google C++ Style Guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem... > > "Sometimes it is useful, or even necessary, to define a function not bound to a > class instance. Such a function can be either a static member or a nonmember > function. Nonmember functions should not depend on external variables, and > should nearly always exist in a namespace. Rather than creating classes only to > group static member functions which do not share static data, use namespaces > instead." > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces > > "With named namespaces, choose the name based on the project, and possibly its > path." > > > Therefore it feels like a "tracked_prefs" namespace here is the right thing > after all (sorry if I told you the opposite when we first discussed it, I > changed my mind after seeing it..). > > > @grt: WDYT, any opinion/guidance on namespace usage in Chromium? Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_stub.cc (left): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_stub.cc:5: package org.chromium.dummy; On 2015/01/20 15:34:12, gab wrote: > You can use > git cl upload --similarity=100 > in order to not have the upload consider this a copy w/ modifications of an > other file. > > (no big deal, just a tip) Acknowledged. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_stub.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_stub.cc:13: return false; On 2015/01/20 15:34:12, gab wrote: > I think this should just return false (or the enum value for "not implemented"). Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_unittest.cc:15: #endif On 2015/01/20 15:34:12, gab wrote: > This will likely change based on other comments, but FTR here's what I had in > mind: > > const bool kMachineIdEnabled = > #if defined(OS_WIN) > true; > #else > false; > #endif Acknowledged. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_unittest.cc:22: EXPECT_EQ(string(""), first_machine_id); On 2015/01/20 15:34:12, gab wrote: > I think this test will change based on other comments, but FTR here it's > slightly cleaner to do > EXPECT_TRUE(first_machine_id.empty()); > > In fact here we also want to make sure it's not empty when one is expected, so > you'd want: > > EXPECT_EQ(!kMachineIdEnabled, first_machine_id.empty()); Acknowledged. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> On 2015/01/20 15:34:12, gab wrote: > I don't think you need these two headers. <cstddef> was for NULL. But probably also defined in windows.h. But <cwchar> is for wchar_t. I don't see it being defined in any other headers directly. Should I not include it? https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:10: #include <intSafe.h> // For DWORD. On 2015/01/20 15:34:12, gab wrote: > DWORD implicitly comes with windows.h AFAIK (I've never seen anyone include > intsafe.h for it at least). Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:11: #include <sddl.h> // For ConvertSidToStringSidW. On 2015/01/20 15:34:12, gab wrote: > s/ConvertSidToStringSidW/ConvertSidToStringSidA Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:12: #include <windows.h> On 2015/01/20 15:34:12, gab wrote: > We always put windows.h first (this is more or less the only exception to > "always alpha sort the headers"). > > We put the definitions in the following order: > > <interface header> > > <C headers> (alpha-sorted, but windows.h first if needed) > (optional empty line -- I like having it but it's not required AFAICT) > <C++ headers> (alpha-sorted) > > <project headers> (alpha-sorted) > > So in this case (with the headers you have here if we weren't changing them): > > <windows.h> > <intsafe.h> > <sddl.h> > > <cstddef> > <cwchar> > > <base/macros.h> > ... > > > (see this article for more details: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...) Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:17: bool GetDeterministicMachineSpecificId(std::string* machine_id) { On 2015/01/20 15:34:13, gab wrote: > Looks like there is a GetUserSidString() method in base/win; it would probably > make sense to have a general implementation of a GetMachineSidString() there as > well (used by both this code and RLZ) since this is non-trivial code. As we discussed offline, we will wait with this refactoring until later. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:19: return false; On 2015/01/20 15:34:13, gab wrote: > Instead do: > DCHECK(machine_id); > to enforce the contract that |machine_id| should always be provided (and mention > that contract in the header). Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:21: wchar_t computer_name[MAX_COMPUTERNAME_LENGTH + 1] = {0}; On 2015/01/20 15:34:12, gab wrote: > {0} is the same as {} except that you explicitly set the first element to 0 and > the rest is default initialized. > > {} is preferable here (@grt: I know you're an advocate of this, is this still > the recommendation?) Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:45: domain_buffer.get(), &domain_size, On 2015/01/20 15:34:12, gab wrote: > |domain_buffer| is optional and we don't use its value, change it to NULL and > |domain_size| to 0 > > This also makes it unnecessary to call this a second time if it fails the first > time IIUC. The documentation does not mention this explicitly, but in fact, the call to |LookupAccountNameW| will return false and set the error code to |ERROR_INSUFFICIENT_BUFFER| if |domain_buffer| is NULL. This is despite the fact that the parameter is annotated as |_Out_opt_|. So, in order to ensure that the call was successfull and we did in fact obtain a valid SID, we need to make sure |domain_buffer| can hold the domain. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:47: return false; On 2015/01/20 15:34:12, gab wrote: > Wrap in {} > > (always wrap in {} unless both the conditional and the body are one-liners) Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:49: On 2015/01/20 15:34:12, gab wrote: > DCHECK_EQ(SID_NAME_USE::SidTypeComputer, sid_name_use); > > to confirm that we're using this API the right way and are indeed getting the > machine's SID As we discussed offline, this actually fails consistently. See comments in the updated code. Because of this confusion in the API I've added a run-time test instead and will return FAILURE if the value of |sid_name_use| is unexpected. https://codereview.chromium.org/836703005/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/836703005/diff/1/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:2182: 'browser/prefs/tracked/device_id.h', On 2015/01/20 15:34:13, gab wrote: > Put these two alongside the other prefs/tracked in the non-OS specific section. > > (the _win.cc will automatically get selected out on non-Win (by > filename_rules.gypi) -- these OS-specific sections are only for things where an > ifdef is truly intended whereas we want our interface to be cross-platform) Done.
CC grt: see question below regarding nit-picky details of C++/MSVC/wchar_t, thanks! LG, last couple nits. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> On 2015/01/26 16:32:48, alito wrote: > On 2015/01/20 15:34:12, gab wrote: > > I don't think you need these two headers. > > <cstddef> was for NULL. But probably also defined in windows.h. But <cwchar> is > for wchar_t. I don't see it being defined in any other headers directly. Should > I not include it? I've never seen anyone include <cwchar>, from what I understand wchar_t is a native type in MSVC but I may be wrong. @grt? https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:45: domain_buffer.get(), &domain_size, On 2015/01/26 16:32:48, alito wrote: > On 2015/01/20 15:34:12, gab wrote: > > |domain_buffer| is optional and we don't use its value, change it to NULL and > > |domain_size| to 0 > > > > This also makes it unnecessary to call this a second time if it fails the > first > > time IIUC. > > The documentation does not mention this explicitly, but in fact, the call to > |LookupAccountNameW| will return false and set the error code to > |ERROR_INSUFFICIENT_BUFFER| if |domain_buffer| is NULL. This is despite the fact > that the parameter is annotated as |_Out_opt_|. So, in order to ensure that the > call was successfull and we did in fact obtain a valid SID, we need to make sure > |domain_buffer| can hold the domain. I see, please add comments to that effect to avoid a future reader spending some time figuring this out yet again. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:12: enum MachineIdStatus { Use enum class (avoids polluting the namespace and is more explicit). https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:22: // std::string object. Returns |FAILURE| if a machine ID cannot be Remove "and point to a a valid std::string object" (it's implied by the type that callers shouldn't be stupid and cast some other pointer in there :-)!) https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_stub.cc (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_stub.cc:5: #include "device_id.h" Use full path. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_stub.cc:11: rm empty line (general rule is to reduce vertical space to a minimum unless it improves readability). https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include "build/build_config.h" Sort. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_unittest.cc:27: EXPECT_EQ(kExpectedStatus != SUCCESS, first_machine_id.empty()); I would flip this around (i.e. verify what should happen when success is expected -- positive conditionals are prefered when it's an option) https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:30: DWORD domain_size = 128; // Will expand if needed. s/Will expand if needed/Will expand below if needed https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:38: // required size is now found in domain_size. Resize and try s/domain_buffer/|domain_buffer| same for domain_size https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:51: // We want to ensure that the correct type of SID was obtained. The Pronouns in comments are frowned upon in Chromium. s/We want to ensure/Ensure https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:54: // is passed in as its second argument and therefore we will accept s/second argument and therefore we will accept either of the two enum values/second argument and therefore both values will be considered acceptable. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:58: if (sid_name_use != SID_NAME_USE::SidTypeComputer && Let's just assume this is correct and not handle failure, i.e.: DCHECK(sid_name_use == SID_NAME_USE::SidTypeComputer || sid_name_use == SID_NAME_USE::SidTypeDomain);
On 2015/01/26 19:40:46, gab wrote: > CC grt: see question below regarding nit-picky details of C++/MSVC/wchar_t, > thanks! Also, Greg, I had some "@grt" questions in my initial set of comments (but forgot to CC you at the time), though I don't absolutely require you to weigh in, I'd still appreciate your take on the two topics if you have time and have something to add there. Thanks! Gab > > LG, last couple nits. > > https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... > File chrome/browser/prefs/tracked/device_id_win.cc (right): > > https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... > chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> > On 2015/01/26 16:32:48, alito wrote: > > On 2015/01/20 15:34:12, gab wrote: > > > I don't think you need these two headers. > > > > <cstddef> was for NULL. But probably also defined in windows.h. But <cwchar> > is > > for wchar_t. I don't see it being defined in any other headers directly. > Should > > I not include it? > > I've never seen anyone include <cwchar>, from what I understand wchar_t is a > native type in MSVC but I may be wrong. > > @grt? > > https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... > chrome/browser/prefs/tracked/device_id_win.cc:45: domain_buffer.get(), > &domain_size, > On 2015/01/26 16:32:48, alito wrote: > > On 2015/01/20 15:34:12, gab wrote: > > > |domain_buffer| is optional and we don't use its value, change it to NULL > and > > > |domain_size| to 0 > > > > > > This also makes it unnecessary to call this a second time if it fails the > > first > > > time IIUC. > > > > The documentation does not mention this explicitly, but in fact, the call to > > |LookupAccountNameW| will return false and set the error code to > > |ERROR_INSUFFICIENT_BUFFER| if |domain_buffer| is NULL. This is despite the > fact > > that the parameter is annotated as |_Out_opt_|. So, in order to ensure that > the > > call was successfull and we did in fact obtain a valid SID, we need to make > sure > > |domain_buffer| can hold the domain. > > I see, please add comments to that effect to avoid a future reader spending some > time figuring this out yet again. > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > File chrome/browser/prefs/tracked/device_id.h (right): > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id.h:12: enum MachineIdStatus { > Use enum class (avoids polluting the namespace and is more explicit). > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id.h:22: // std::string object. Returns > |FAILURE| if a machine ID cannot be > Remove "and point to a a valid std::string object" (it's implied by the type > that callers shouldn't be stupid and cast some other pointer in there :-)!) > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > File chrome/browser/prefs/tracked/device_id_stub.cc (right): > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_stub.cc:5: #include "device_id.h" > Use full path. > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_stub.cc:11: > rm empty line (general rule is to reduce vertical space to a minimum unless it > improves readability). > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > File chrome/browser/prefs/tracked/device_id_unittest.cc (right): > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include > "build/build_config.h" > Sort. > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_unittest.cc:27: EXPECT_EQ(kExpectedStatus > != SUCCESS, first_machine_id.empty()); > I would flip this around (i.e. verify what should happen when success is > expected -- positive conditionals are prefered when it's an option) > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > File chrome/browser/prefs/tracked/device_id_win.cc (right): > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_win.cc:30: DWORD domain_size = 128; // > Will expand if needed. > s/Will expand if needed/Will expand below if needed > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_win.cc:38: // required size is now found > in domain_size. Resize and try > s/domain_buffer/|domain_buffer| > > same for domain_size > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_win.cc:51: // We want to ensure that the > correct type of SID was obtained. The > Pronouns in comments are frowned upon in Chromium. > > s/We want to ensure/Ensure > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_win.cc:54: // is passed in as its second > argument and therefore we will accept > s/second argument and therefore we will accept either of the two enum > values/second argument and therefore both values will be considered acceptable. > > https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_win.cc:58: if (sid_name_use != > SID_NAME_USE::SidTypeComputer && > Let's just assume this is correct and not handle failure, i.e.: > > DCHECK(sid_name_use == SID_NAME_USE::SidTypeComputer || sid_name_use == > SID_NAME_USE::SidTypeDomain);
Made changes according to comments by gab@. Also removed "#include <cwchar>" since codesearch showed that it is not normally included in chromium code. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> On 2015/01/26 19:40:46, gab wrote: > On 2015/01/26 16:32:48, alito wrote: > > On 2015/01/20 15:34:12, gab wrote: > > > I don't think you need these two headers. > > > > <cstddef> was for NULL. But probably also defined in windows.h. But <cwchar> > is > > for wchar_t. I don't see it being defined in any other headers directly. > Should > > I not include it? > > I've never seen anyone include <cwchar>, from what I understand wchar_t is a > native type in MSVC but I may be wrong. > > @grt? I've removed <cwchar>. It is not normally included in chromium code it seems. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:45: domain_buffer.get(), &domain_size, On 2015/01/26 19:40:45, gab wrote: > On 2015/01/26 16:32:48, alito wrote: > > On 2015/01/20 15:34:12, gab wrote: > > > |domain_buffer| is optional and we don't use its value, change it to NULL > and > > > |domain_size| to 0 > > > > > > This also makes it unnecessary to call this a second time if it fails the > > first > > > time IIUC. > > > > The documentation does not mention this explicitly, but in fact, the call to > > |LookupAccountNameW| will return false and set the error code to > > |ERROR_INSUFFICIENT_BUFFER| if |domain_buffer| is NULL. This is despite the > fact > > that the parameter is annotated as |_Out_opt_|. So, in order to ensure that > the > > call was successfull and we did in fact obtain a valid SID, we need to make > sure > > |domain_buffer| can hold the domain. > > I see, please add comments to that effect to avoid a future reader spending some > time figuring this out yet again. Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:12: enum MachineIdStatus { On 2015/01/26 19:40:46, gab wrote: > Use enum class (avoids polluting the namespace and is more explicit). Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:22: // std::string object. Returns |FAILURE| if a machine ID cannot be On 2015/01/26 19:40:46, gab wrote: > Remove "and point to a a valid std::string object" (it's implied by the type > that callers shouldn't be stupid and cast some other pointer in there :-)!) Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_stub.cc (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_stub.cc:5: #include "device_id.h" On 2015/01/26 19:40:46, gab wrote: > Use full path. Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_stub.cc:11: On 2015/01/26 19:40:46, gab wrote: > rm empty line (general rule is to reduce vertical space to a minimum unless it > improves readability). Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include "build/build_config.h" On 2015/01/26 19:40:46, gab wrote: > Sort. Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_unittest.cc:27: EXPECT_EQ(kExpectedStatus != SUCCESS, first_machine_id.empty()); On 2015/01/26 19:40:46, gab wrote: > I would flip this around (i.e. verify what should happen when success is > expected -- positive conditionals are prefered when it's an option) Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:30: DWORD domain_size = 128; // Will expand if needed. On 2015/01/26 19:40:46, gab wrote: > s/Will expand if needed/Will expand below if needed Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:38: // required size is now found in domain_size. Resize and try On 2015/01/26 19:40:46, gab wrote: > s/domain_buffer/|domain_buffer| > > same for domain_size Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:51: // We want to ensure that the correct type of SID was obtained. The On 2015/01/26 19:40:46, gab wrote: > Pronouns in comments are frowned upon in Chromium. > > s/We want to ensure/Ensure Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:54: // is passed in as its second argument and therefore we will accept On 2015/01/26 19:40:46, gab wrote: > s/second argument and therefore we will accept either of the two enum > values/second argument and therefore both values will be considered acceptable. Done. https://codereview.chromium.org/836703005/diff/20001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_win.cc:58: if (sid_name_use != SID_NAME_USE::SidTypeComputer && On 2015/01/26 19:40:46, gab wrote: > Let's just assume this is correct and not handle failure, i.e.: > > DCHECK(sid_name_use == SID_NAME_USE::SidTypeComputer || sid_name_use == > SID_NAME_USE::SidTypeDomain); Done.
Awesome, lgtm :-)! @grt: Our questions were mostly instructional, we'd love your input but won't block this CL on it. Thanks! Gab
The CQ bit was checked by alito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836703005/40001
The CQ bit was unchecked by commit-bot@chromium.org
CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true in order for the CQ to process them
Hmmm looks like the remote ref is wrong (and that may also be why git cl try had trouble before). Is your local branch not set to have master as its upstream branch (the default if you did "git checkout -b foo" from master to create your branch)? This would cause that. In any case do not use NOTRY=True as the bot suggests as your are not truly trying to commit to another ref. Le lun. 26 janv. 2015 16:08, null <commit-bot@chromium.org> a écrit : > CLs for remote refs other than refs/pending/heads/master must contain > NOTRY=true > in order for the CQ to process them > > https://codereview.chromium.org/836703005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
No, I branched from lkgr. So I guess a rebase is in order at this point? Will that affect the review process and require a new lgtm? /ali On Mon, Jan 26, 2015 at 4:15 PM, Gabriel Charette <gab@google.com> wrote: > Hmmm looks like the remote ref is wrong (and that may also be why git cl > try had trouble before). > > Is your local branch not set to have master as its upstream branch (the > default if you did "git checkout -b foo" from master to create your > branch)? This would cause that. > > In any case do not use NOTRY=True as the bot suggests as your are not > truly trying to commit to another ref. > > Le lun. 26 janv. 2015 16:08, null <commit-bot@chromium.org> a écrit : > > CLs for remote refs other than refs/pending/heads/master must contain >> NOTRY=true >> in order for the CQ to process them >> >> https://codereview.chromium.org/836703005/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/26 21:19:41, alito wrote: > No, I branched from lkgr. So I guess a rebase is in order at this point? > Will that affect the review process and require a new lgtm? Ah, I see, interestingly on another thread in my inbox this morning I just learnt that support to land on master from an lkcr/lkgr ref was just added yesterday at 4:39PM, so let's try again!
The CQ bit was checked by alito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836703005/40001
The CQ bit was unchecked by commit-bot@chromium.org
CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true in order for the CQ to process them
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id.h:11: On 2015/01/20 15:34:12, gab wrote: > Now that I see it in context, I think it may make sense to wrap this in a > "tracked_prefs" namespace (similar to the "chrome_prefs" namespace used in > chrome_pref_service_factory.h for similar free functions). > > I always forget the exact guidance on namespaces in Chromium, probably because > there isn't even a clear agreement among Chromium devs (latest discussion on the > topic: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/E3K_K...). > > From the Google C++ Style Guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem... > > "Sometimes it is useful, or even necessary, to define a function not bound to a > class instance. Such a function can be either a static member or a nonmember > function. Nonmember functions should not depend on external variables, and > should nearly always exist in a namespace. Rather than creating classes only to > group static member functions which do not share static data, use namespaces > instead." > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces > > "With named namespaces, choose the name based on the project, and possibly its > path." > > > Therefore it feels like a "tracked_prefs" namespace here is the right thing > after all (sorry if I told you the opposite when we first discussed it, I > changed my mind after seeing it..). > > > @grt: WDYT, any opinion/guidance on namespace usage in Chromium? In general, I avoid them since I'm tired of having the discussion. :-) The "chrome" namespace is flat out banned, as I understand it. In this case, I think a tracked_prefs namespace to hold an enum and a single function is overkill. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> On 2015/01/26 16:32:48, alito wrote: > On 2015/01/20 15:34:12, gab wrote: > > I don't think you need these two headers. > > <cstddef> was for NULL. New code should use nullptr. > But probably also defined in windows.h. But <cwchar> is > for wchar_t. wchar_t is a fundamental type in the language, so no header is needed. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:21: wchar_t computer_name[MAX_COMPUTERNAME_LENGTH + 1] = {0}; On 2015/01/20 15:34:12, gab wrote: > {0} is the same as {} except that you explicitly set the first element to 0 and > the rest is default initialized. > > {} is preferable here (@grt: I know you're an advocate of this, is this still > the recommendation?) It's still my preference. https://codereview.chromium.org/836703005/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:21: // machine. |machine_id| must not be NULL. Returns |FAILURE| if a NULL -> null or nullptr. I prefer the former, but the latter is acceptable. https://codereview.chromium.org/836703005/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:25: MachineIdStatus GetDeterministicMachineSpecificId(std::string* machine_id); Do consumers of this function need to differentiate FAILURE from NOT_IMPLEMENTED?
Thanks for your input, see replies below. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id.h:11: On 2015/01/27 17:06:22, grt wrote: > On 2015/01/20 15:34:12, gab wrote: > > Now that I see it in context, I think it may make sense to wrap this in a > > "tracked_prefs" namespace (similar to the "chrome_prefs" namespace used in > > chrome_pref_service_factory.h for similar free functions). > > > > I always forget the exact guidance on namespaces in Chromium, probably because > > there isn't even a clear agreement among Chromium devs (latest discussion on > the > > topic: > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/E3K_K...). > > > > From the Google C++ Style Guide: > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem... > > > > "Sometimes it is useful, or even necessary, to define a function not bound to > a > > class instance. Such a function can be either a static member or a nonmember > > function. Nonmember functions should not depend on external variables, and > > should nearly always exist in a namespace. Rather than creating classes only > to > > group static member functions which do not share static data, use namespaces > > instead." > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces > > > > "With named namespaces, choose the name based on the project, and possibly its > > path." > > > > > > Therefore it feels like a "tracked_prefs" namespace here is the right thing > > after all (sorry if I told you the opposite when we first discussed it, I > > changed my mind after seeing it..). > > > > > > @grt: WDYT, any opinion/guidance on namespace usage in Chromium? > > In general, I avoid them since I'm tired of having the discussion. :-) The > "chrome" namespace is flat out banned, as I understand it. In this case, I think > a tracked_prefs namespace to hold an enum and a single function is overkill. Ok sounds good, I'm happy with none as well, especially with enum class since it doesn't pollute the (global) namespace. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> On 2015/01/27 17:06:22, grt wrote: > On 2015/01/26 16:32:48, alito wrote: > > On 2015/01/20 15:34:12, gab wrote: > > > I don't think you need these two headers. > > > > <cstddef> was for NULL. > > New code should use nullptr. Yes, but in this case it's using Windows APIs whose documentation still uses NULL (sure it's the same thing in practice). So do you think we should use nullptr even when calling into Windows APIs (i.e. for the pointer-checking benefit)? Or stick to the documentation? > > > But probably also defined in windows.h. But <cwchar> is > > for wchar_t. > > wchar_t is a fundamental type in the language, so no header is needed. https://codereview.chromium.org/836703005/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:25: MachineIdStatus GetDeterministicMachineSpecificId(std::string* machine_id); On 2015/01/27 17:06:22, grt wrote: > Do consumers of this function need to differentiate FAILURE from > NOT_IMPLEMENTED? Yes, they will, this is why we added this. i.e. settings hardening and its metrics should care IMO.
https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> On 2015/01/27 18:27:40, gab wrote: > On 2015/01/27 17:06:22, grt wrote: > > On 2015/01/26 16:32:48, alito wrote: > > > On 2015/01/20 15:34:12, gab wrote: > > > > I don't think you need these two headers. > > > > > > <cstddef> was for NULL. > > > > New code should use nullptr. > > Yes, but in this case it's using Windows APIs whose documentation still uses > NULL (sure it's the same thing in practice). > > So do you think we should use nullptr even when calling into Windows APIs (i.e. > for the pointer-checking benefit)? Or stick to the documentation? Yup, use nullptr rather than NULL when calling a Windows API that takes a pointer to something. Use nullptr everywhere possible.
Changes introduced as per comments/discussions. Also ran "git cl format" which caught a couple of spacing issues. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id.h:11: On 2015/01/27 18:27:40, gab wrote: > On 2015/01/27 17:06:22, grt wrote: > > On 2015/01/20 15:34:12, gab wrote: > > > Now that I see it in context, I think it may make sense to wrap this in a > > > "tracked_prefs" namespace (similar to the "chrome_prefs" namespace used in > > > chrome_pref_service_factory.h for similar free functions). > > > > > > I always forget the exact guidance on namespaces in Chromium, probably > because > > > there isn't even a clear agreement among Chromium devs (latest discussion on > > the > > > topic: > > > > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/E3K_K...). > > > > > > From the Google C++ Style Guide: > > > > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem... > > > > > > "Sometimes it is useful, or even necessary, to define a function not bound > to > > a > > > class instance. Such a function can be either a static member or a nonmember > > > function. Nonmember functions should not depend on external variables, and > > > should nearly always exist in a namespace. Rather than creating classes only > > to > > > group static member functions which do not share static data, use namespaces > > > instead." > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces > > > > > > "With named namespaces, choose the name based on the project, and possibly > its > > > path." > > > > > > > > > Therefore it feels like a "tracked_prefs" namespace here is the right thing > > > after all (sorry if I told you the opposite when we first discussed it, I > > > changed my mind after seeing it..). > > > > > > > > > @grt: WDYT, any opinion/guidance on namespace usage in Chromium? > > > > In general, I avoid them since I'm tired of having the discussion. :-) The > > "chrome" namespace is flat out banned, as I understand it. In this case, I > think > > a tracked_prefs namespace to hold an enum and a single function is overkill. > > Ok sounds good, I'm happy with none as well, especially with enum class since it > doesn't pollute the (global) namespace. Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... File chrome/browser/prefs/tracked/device_id_win.cc (right): https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:8: #include <cwchar> On 2015/01/27 19:48:59, grt wrote: > On 2015/01/27 18:27:40, gab wrote: > > On 2015/01/27 17:06:22, grt wrote: > > > On 2015/01/26 16:32:48, alito wrote: > > > > On 2015/01/20 15:34:12, gab wrote: > > > > > I don't think you need these two headers. > > > > > > > > <cstddef> was for NULL. > > > > > > New code should use nullptr. > > > > Yes, but in this case it's using Windows APIs whose documentation still uses > > NULL (sure it's the same thing in practice). > > > > So do you think we should use nullptr even when calling into Windows APIs > (i.e. > > for the pointer-checking benefit)? Or stick to the documentation? > > Yup, use nullptr rather than NULL when calling a Windows API that takes a > pointer to something. Use nullptr everywhere possible. Done. https://codereview.chromium.org/836703005/diff/1/chrome/browser/prefs/tracked... chrome/browser/prefs/tracked/device_id_win.cc:21: wchar_t computer_name[MAX_COMPUTERNAME_LENGTH + 1] = {0}; On 2015/01/27 17:06:22, grt wrote: > On 2015/01/20 15:34:12, gab wrote: > > {0} is the same as {} except that you explicitly set the first element to 0 > and > > the rest is default initialized. > > > > {} is preferable here (@grt: I know you're an advocate of this, is this still > > the recommendation?) > > It's still my preference. Acknowledged. https://codereview.chromium.org/836703005/diff/40001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/40001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:21: // machine. |machine_id| must not be NULL. Returns |FAILURE| if a On 2015/01/27 17:06:22, grt wrote: > NULL -> null or nullptr. I prefer the former, but the latter is acceptable. Done.
lgtm, thanks!
The CQ bit was checked by alito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836703005/60001
lgtm w/ nits below https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:13: NOT_IMPLEMENTED // Returned if the method for obtaining a wrap at 80 cols: NOT_IMPLEMENTED // Returned if the method for obtaining a machine-specific ID // is not implemented for the system. https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:18: // Populates |machine_id| with a deterministic ID for this 80 cols: // Populates |machine_id| with a deterministic ID for this machine. |machine_id| // must not be null. Returns |FAILURE| if a machine ID cannot be obtained or // |NOT_IMPLEMENTED| on systems for which this feature is not supported (in both // cases |machine_id| is left untouched). https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_stub.cc (right): https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_stub.cc:8: DCHECK(machine_id); #include "base/logging.h" for this https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include "chrome/browser/prefs/tracked/device_id.h" move above line 5 with a blank line after it
The CQ bit was unchecked by grt@chromium.org
The CQ bit was unchecked by alito@chromium.org
Alexei, question for you below, thanks! https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include "chrome/browser/prefs/tracked/device_id.h" On 2015/01/27 21:08:45, grt wrote: > move above line 5 with a blank line after it Interesting, I've heard the opposite (and am the one who suggested moving it in here), but from https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/test$2... it seems you're right. I remember Alexei had asked me to move one way or another with a link to a chromium-dev thread at some point, but I can't find the recommendation, Alexei do you have an opinion on this?
On 2015/01/27 21:20:30, gab wrote: > Alexei, question for you below, thanks! > > https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... > File chrome/browser/prefs/tracked/device_id_unittest.cc (right): > > https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... > chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include > "chrome/browser/prefs/tracked/device_id.h" > On 2015/01/27 21:08:45, grt wrote: > > move above line 5 with a blank line after it > > Interesting, I've heard the opposite (and am the one who suggested moving it in > here), but from > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/test$2... > it seems you're right. The Google style guide is pretty clear: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...
On 2015/01/27 21:23:20, grt wrote: > On 2015/01/27 21:20:30, gab wrote: > > Alexei, question for you below, thanks! > > > > > https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... > > File chrome/browser/prefs/tracked/device_id_unittest.cc (right): > > > > > https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... > > chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include > > "chrome/browser/prefs/tracked/device_id.h" > > On 2015/01/27 21:08:45, grt wrote: > > > move above line 5 with a blank line after it > > > > Interesting, I've heard the opposite (and am the one who suggested moving it > in > > here), but from > > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/test$2... > > it seems you're right. > > The Google style guide is pretty clear: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord... Right okay, sgtm :-)! (thanks a lot for the nits, trying to find the appropriate resolution for all the corner cases so that Ali starts off with the correct style rule basis)
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_unittest.cc:6: #include "chrome/browser/prefs/tracked/device_id.h" On 2015/01/27 21:20:30, gab wrote: > On 2015/01/27 21:08:45, grt wrote: > > move above line 5 with a blank line after it > > Interesting, I've heard the opposite (and am the one who suggested moving it in > here), but from > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/test$2... > it seems you're right. > > I remember Alexei had asked me to move one way or another with a link to a > chromium-dev thread at some point, but I can't find the recommendation, Alexei > do you have an opinion on this? I believe the recommended style in Chromium is now to have it at the top. (Both are OK according to style guide, but Chromium has been moving to that one.)
The CQ bit was checked by alito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836703005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
I have no idea as to why this test is failing on android. From what I can see, the complaint concerns a call to EXPECT_EQ that is passed two boolean values: EXPECT_EQ(kExpectedStatus == MachineIdStatus::SUCCESS, !first_machine_id.empty()); where first_machine_id is a std::string. But the error message refers to conversion of boolean value to pointer type (could it still be a bug in gcc https://code.google.com/p/chromium/issues/detail?id=150812?): FAILED: /b/build/goma/gomacc /b/build/slave/android/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/chrome/browser/prefs/tracked/unit_tests.device_id_unittest.o.d [... output shortened ...] In file included from ../../testing/gtest/include/gtest/gtest.h:1967:0, from ../../chrome/browser/prefs/tracked/device_id_unittest.cc:7: ../../chrome/browser/prefs/tracked/device_id_unittest.cc: In member function 'virtual void GetDeterministicMachineSpecificIdTest_IsDeterministic_Test::TestBody()': ../../testing/gtest/include/gtest/internal/gtest-internal.h:135:55: error: converting 'false' to pointer type for argument 1 of 'char testing::internal::IsNullLiteralHelper(testing::internal::Secret*)' [-Werror=conversion-null] (sizeof(::testing::internal::IsNullLiteralHelper(x)) == 1) ^ ../../testing/gtest/include/gtest/gtest_pred_impl.h:77:52: note: in definition of macro 'GTEST_ASSERT_' if (const ::testing::AssertionResult gtest_ar = (expression)) \ ^ ../../testing/gtest/include/gtest/gtest_pred_impl.h:162:3: note: in expansion of macro 'GTEST_PRED_FORMAT2_' GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_) ^ ../../testing/gtest/include/gtest/gtest.h:2016:3: note: in expansion of macro 'EXPECT_PRED_FORMAT2' EXPECT_PRED_FORMAT2(::testing::internal:: \ ^ ../../testing/gtest/include/gtest/gtest.h:2017:32: note: in expansion of macro 'GTEST_IS_NULL_LITERAL_' EqHelper<GTEST_IS_NULL_LITERAL_(expected)>::Compare, \ ^ ../../chrome/browser/prefs/tracked/device_id_unittest.cc:25:3: note: in expansion of macro 'EXPECT_EQ' EXPECT_EQ(kExpectedStatus == MachineIdStatus::SUCCESS, ^ cc1plus: all warnings being treated as errors ninja: build stopped: subcommand failed. On Wed, Jan 28, 2015 at 3:53 PM, <commit-bot@chromium.org> wrote: > Try jobs failed on following builders: > android_dbg_tests_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/ > builders/android_dbg_tests_recipe/builds/49639) > > https://codereview.chromium.org/836703005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
asvitkine@chromium.org changed reviewers: - asvitkine@chromium.org
Reviewers please take a look. I tried to circumvent the GCC bug by static_cast:ing the first argument. That did not help. I have now replaced EXPECT_EQ(), which uses two arguments, with EXPECT_TRUE(), which uses only one. Any opinions on better ways to circumvent this issue? /ali https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id.h (right): https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:13: NOT_IMPLEMENTED // Returned if the method for obtaining a On 2015/01/27 21:08:45, grt wrote: > wrap at 80 cols: > NOT_IMPLEMENTED // Returned if the method for obtaining a machine-specific ID > // is not implemented for the system. Done. https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id.h:18: // Populates |machine_id| with a deterministic ID for this On 2015/01/27 21:08:45, grt wrote: > 80 cols: > // Populates |machine_id| with a deterministic ID for this machine. |machine_id| > // must not be null. Returns |FAILURE| if a machine ID cannot be obtained or > // |NOT_IMPLEMENTED| on systems for which this feature is not supported (in both > // cases |machine_id| is left untouched). Done. https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... File chrome/browser/prefs/tracked/device_id_stub.cc (right): https://codereview.chromium.org/836703005/diff/60001/chrome/browser/prefs/tra... chrome/browser/prefs/tracked/device_id_stub.cc:8: DCHECK(machine_id); On 2015/01/27 21:08:45, grt wrote: > #include "base/logging.h" for this Done.
Wow, even static_cast isn't enough to tell gcc what we want here :-(... well lgtm then (I find it ugly but hey, not your fault!). Thanks for pushing through with this. One last thing: open a bug about the meta goal of this CL and follow-up CLs and add its ID under the BUG= section of this CL's description (and also add the EXPECT_EQ bug as a reference). (we can do this tomorrow if you're unsure, no rush) Cheers! Gab
https://codereview.chromium.org/836703005/diff/120001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/120001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/device_id_unittest.cc:30: EXPECT_TRUE((kExpectedStatus == MachineIdStatus::SUCCESS) == how about getting rid of the hack? if (kExpectedStatus == MachineIdStatus::SUCCESS) EXPECT_TRUE(!first_machine_id.empty()); else EXPECT_TRUE(first_machine_id.empty();
https://codereview.chromium.org/836703005/diff/120001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/120001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/device_id_unittest.cc:30: EXPECT_TRUE((kExpectedStatus == MachineIdStatus::SUCCESS) == On 2015/01/30 14:39:08, grt wrote: > how about getting rid of the hack? > if (kExpectedStatus == MachineIdStatus::SUCCESS) > EXPECT_TRUE(!first_machine_id.empty()); > else > EXPECT_TRUE(first_machine_id.empty(); I considered that syntax, but found it just as weird (i.e. still requires the comment to justify it). In that light I prefer the current syntax because it's 2 lines rather than 4. In practice if anyone ever reads this code, they'll find it weird, then read the comment and understand. The syntax used currently doesn't make it hacky or unreadable, it just makes it feel a little overkill until one reads the comment above IMO.
https://codereview.chromium.org/836703005/diff/120001/chrome/browser/prefs/tr... File chrome/browser/prefs/tracked/device_id_unittest.cc (right): https://codereview.chromium.org/836703005/diff/120001/chrome/browser/prefs/tr... chrome/browser/prefs/tracked/device_id_unittest.cc:30: EXPECT_TRUE((kExpectedStatus == MachineIdStatus::SUCCESS) == On 2015/01/30 17:37:16, gab wrote: > On 2015/01/30 14:39:08, grt wrote: > > how about getting rid of the hack? > > if (kExpectedStatus == MachineIdStatus::SUCCESS) > > EXPECT_TRUE(!first_machine_id.empty()); > > else > > EXPECT_TRUE(first_machine_id.empty(); > > I considered that syntax, but found it just as weird (i.e. still requires the > comment to justify it). It seems pretty clear to me, and I don't think it requires a comment about a GCC bug. That said, do as you wish.
The CQ bit was checked by alito@chromium.org
The CQ bit was unchecked by alito@chromium.org
The CQ bit was checked by alito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836703005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_gn_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)
On 2015/01/31 07:54:24, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win8_chromium_gn_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...) @Pawel: any idea what's up with this? The bot looks green AFAICT...
The CQ bit was checked by alito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836703005/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c4f3ce9f101b12a3368355a28c02c8aac21ed432 Cr-Commit-Position: refs/heads/master@{#314058} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
