Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12036047:
Add Brand Code to UMA Uploads (Closed)
|
Created:
7 years, 11 months ago by Mark P Modified:
7 years, 10 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, MAD, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd Brand Code to UMA Uploads
Can you suggest a good way to test this? I don't have a Windows machine
(these are the only types of installs with brand codes). Or do we
simply trust the code enough that we should just check this in and
see if after the next dev release that we get a person or two? (There
probably are occasional new installs of chrome dev channels.)
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179401
Patch Set 1 #
Total comments: 2
Patch Set 2 : vertical spacing. #
Total comments: 2
Patch Set 3 : add test #
Total comments: 6
Patch Set 4 : cleanup test. #Patch Set 5 : rebased #
Messages
Total messages: 22 (0 generated)
I will not check in until I've double-checked with the chrome privacy folks.
LGTM https://codereview.chromium.org/12036047/diff/1/chrome/browser/metrics/metric... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/12036047/diff/1/chrome/browser/metrics/metric... chrome/browser/metrics/metrics_log.cc:806: system_profile->set_brand_code(brand_code); nit: Please leave blank lines around this block.
Any comments on testing? (See the changelist description.) --mark https://codereview.chromium.org/12036047/diff/1/chrome/browser/metrics/metric... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/12036047/diff/1/chrome/browser/metrics/metric... chrome/browser/metrics/metrics_log.cc:806: system_profile->set_brand_code(brand_code); On 2013/01/23 05:58:21, Ilya Sherman wrote: > nit: Please leave blank lines around this block. Done.
On 2013/01/23 17:00:47, Mark P wrote: > Any comments on testing? (See the changelist description.) For programmatic testing, you follow a similar pattern to the existing tests in metrics_log_unittest.cc. Beyond that, I think just committing and seeing what flows in is fine. Do we have brand codes for Dev channel builds, though? If not, it's going to be a long wait...
https://codereview.chromium.org/12036047/diff/5001/chrome/common/metrics/prot... File chrome/common/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/12036047/diff/5001/chrome/common/metrics/prot... chrome/common/metrics/proto/system_profile.proto:43: // The brand code or distribution tag assigned to a partner, if available. nit: Please mention that these are only available on Windows.
On 2013/01/23 22:04:40, Ilya Sherman wrote: > On 2013/01/23 17:00:47, Mark P wrote: > > Any comments on testing? (See the changelist description.) > > For programmatic testing, you follow a similar pattern to the existing tests in > metrics_log_unittest.cc. Added a test. Please take another look. > Beyond that, I think just committing and seeing what > flows in is fine. Do we have brand codes for Dev channel builds, though? Good point. I'm pretty sure we don't... should we have faith in the code or try to find someone with a windows machine who can build a branded release build? > If > not, it's going to be a long wait... --mark
https://codereview.chromium.org/12036047/diff/5001/chrome/common/metrics/prot... File chrome/common/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/12036047/diff/5001/chrome/common/metrics/prot... chrome/common/metrics/proto/system_profile.proto:43: // The brand code or distribution tag assigned to a partner, if available. On 2013/01/23 22:04:54, Ilya Sherman wrote: > nit: Please mention that these are only available on Windows. Done.
> > Beyond that, I think just committing and seeing what > > flows in is fine. Do we have brand codes for Dev channel builds, though? > > Good point. I'm pretty sure we don't... > > should we have faith in the code or try to find someone with a windows machine > who can build a branded release build? Waiting for several weeks to verify the code sounds a little iffy, so let's try to find someone who can test before then if possible. https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... File chrome/browser/metrics/metrics_log_unittest.cc (right): https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... chrome/browser/metrics/metrics_log_unittest.cc:107: scoped_ptr<google_util::BrandForTesting> brand_for_testing_; nit: No need to use a pointer; just allocate as a direct member var, and init in the initializer list.
https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... File chrome/browser/metrics/metrics_log_unittest.cc (right): https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... chrome/browser/metrics/metrics_log_unittest.cc:107: scoped_ptr<google_util::BrandForTesting> brand_for_testing_; On 2013/01/23 23:32:54, Ilya Sherman wrote: > nit: No need to use a pointer; just allocate as a direct member var, and init in > the initializer list. I can do that if I make the kBrandForTesting a string; right now it's a global char *. (I need to change types because BrandForTesting's constructor is explicit.) If it's a string, I'm not supposed to have it in global scope. Hence, I can do your suggestion but then I'll have to move the definition of kBrandForTesting. I guess then it would have to be a public member variable within this class (so the TEST_F functions can access it). That seemed ickier to me than using a scoped_ptr. What do you think?
https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... File chrome/browser/metrics/metrics_log_unittest.cc (right): https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... chrome/browser/metrics/metrics_log_unittest.cc:42: const char* kBrandForTesting = "brand_for_testing"; nit: This should have type "const char kBrandForTesting[]" or else "const char* const kBrandForTesting" https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... chrome/browser/metrics/metrics_log_unittest.cc:107: scoped_ptr<google_util::BrandForTesting> brand_for_testing_; On 2013/01/23 23:49:26, Mark P wrote: > On 2013/01/23 23:32:54, Ilya Sherman wrote: > > nit: No need to use a pointer; just allocate as a direct member var, and init > in > > the initializer list. > > I can do that if I make the kBrandForTesting a string; right now it's a global > char *. (I need to change types because BrandForTesting's constructor is > explicit.) If it's a string, I'm not supposed to have it in global scope. > Hence, I can do your suggestion but then I'll have to move the definition of > kBrandForTesting. I guess then it would have to be a public member variable > within this class (so the TEST_F functions can access it). That seemed ickier > to me than using a scoped_ptr. > > What do you think? I don't think the explicit on BrandForTesting's constructor should matter; but if the code doesn't compile without it, you can always init as brand_for_testing_(std::string(kBrandForTesting));
https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... File chrome/browser/metrics/metrics_log_unittest.cc (right): https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... chrome/browser/metrics/metrics_log_unittest.cc:42: const char* kBrandForTesting = "brand_for_testing"; On 2013/01/24 00:31:46, Ilya Sherman wrote: > nit: This should have type "const char kBrandForTesting[]" or else "const char* > const kBrandForTesting" Done. https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... chrome/browser/metrics/metrics_log_unittest.cc:107: scoped_ptr<google_util::BrandForTesting> brand_for_testing_; On 2013/01/24 00:31:46, Ilya Sherman wrote: > On 2013/01/23 23:49:26, Mark P wrote: > > On 2013/01/23 23:32:54, Ilya Sherman wrote: > > > nit: No need to use a pointer; just allocate as a direct member var, and > init > > in > > > the initializer list. > > > > I can do that if I make the kBrandForTesting a string; right now it's a global > > char *. (I need to change types because BrandForTesting's constructor is > > explicit.) If it's a string, I'm not supposed to have it in global scope. > > Hence, I can do your suggestion but then I'll have to move the definition of > > kBrandForTesting. I guess then it would have to be a public member variable > > within this class (so the TEST_F functions can access it). That seemed ickier > > to me than using a scoped_ptr. > > > > What do you think? > > I don't think the explicit on BrandForTesting's constructor should matter; but > if the code doesn't compile without it, you can always init as > brand_for_testing_(std::string(kBrandForTesting)); Works now (without the explicit conversion). Dunno why it didn't work before. *shrug* I must've had something else wrong and misinterpret the error message.
On 2013/01/24 00:43:05, Mark P wrote: > https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... > File chrome/browser/metrics/metrics_log_unittest.cc (right): > > https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... > chrome/browser/metrics/metrics_log_unittest.cc:42: const char* kBrandForTesting > = "brand_for_testing"; > On 2013/01/24 00:31:46, Ilya Sherman wrote: > > nit: This should have type "const char kBrandForTesting[]" or else "const > char* > > const kBrandForTesting" > > Done. > > https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... > chrome/browser/metrics/metrics_log_unittest.cc:107: > scoped_ptr<google_util::BrandForTesting> brand_for_testing_; > On 2013/01/24 00:31:46, Ilya Sherman wrote: > > On 2013/01/23 23:49:26, Mark P wrote: > > > On 2013/01/23 23:32:54, Ilya Sherman wrote: > > > > nit: No need to use a pointer; just allocate as a direct member var, and > > init > > > in > > > > the initializer list. > > > > > > I can do that if I make the kBrandForTesting a string; right now it's a > global > > > char *. (I need to change types because BrandForTesting's constructor is > > > explicit.) If it's a string, I'm not supposed to have it in global scope. > > > Hence, I can do your suggestion but then I'll have to move the definition of > > > kBrandForTesting. I guess then it would have to be a public member variable > > > within this class (so the TEST_F functions can access it). That seemed > ickier > > > to me than using a scoped_ptr. > > > > > > What do you think? > > > > I don't think the explicit on BrandForTesting's constructor should matter; but > > if the code doesn't compile without it, you can always init as > > brand_for_testing_(std::string(kBrandForTesting)); > > Works now (without the explicit conversion). Dunno why it didn't work before. > *shrug* I must've had something else wrong and misinterpret the error message. https://chromiumcodereview.appspot.com/12051052/ is waiting for this CL to land. What is the ETA for this?
On 2013/01/28 21:26:17, szym wrote: > On 2013/01/24 00:43:05, Mark P wrote: > > > https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... > > File chrome/browser/metrics/metrics_log_unittest.cc (right): > > > > > https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... > > chrome/browser/metrics/metrics_log_unittest.cc:42: const char* > kBrandForTesting > > = "brand_for_testing"; > > On 2013/01/24 00:31:46, Ilya Sherman wrote: > > > nit: This should have type "const char kBrandForTesting[]" or else "const > > char* > > > const kBrandForTesting" > > > > Done. > > > > > https://codereview.chromium.org/12036047/diff/8002/chrome/browser/metrics/met... > > chrome/browser/metrics/metrics_log_unittest.cc:107: > > scoped_ptr<google_util::BrandForTesting> brand_for_testing_; > > On 2013/01/24 00:31:46, Ilya Sherman wrote: > > > On 2013/01/23 23:49:26, Mark P wrote: > > > > On 2013/01/23 23:32:54, Ilya Sherman wrote: > > > > > nit: No need to use a pointer; just allocate as a direct member var, and > > > init > > > > in > > > > > the initializer list. > > > > > > > > I can do that if I make the kBrandForTesting a string; right now it's a > > global > > > > char *. (I need to change types because BrandForTesting's constructor is > > > > explicit.) If it's a string, I'm not supposed to have it in global scope. > > > > > Hence, I can do your suggestion but then I'll have to move the definition > of > > > > kBrandForTesting. I guess then it would have to be a public member > variable > > > > within this class (so the TEST_F functions can access it). That seemed > > ickier > > > > to me than using a scoped_ptr. > > > > > > > > What do you think? > > > > > > I don't think the explicit on BrandForTesting's constructor should matter; > but > > > if the code doesn't compile without it, you can always init as > > > brand_for_testing_(std::string(kBrandForTesting)); > > > > Works now (without the explicit conversion). Dunno why it didn't work before. > > > *shrug* I must've had something else wrong and misinterpret the error > message. > > https://chromiumcodereview.appspot.com/12051052/ is waiting for this CL to land. > What is the ETA for this? Probably this week. I still need to get a windows machine to test it. Why are you waiting? There's no reason to wait that I can see. (Protocol buffer number do not need to be consecutive.) --mark
On 2013/01/28 21:26:17, szym wrote: > https://chromiumcodereview.appspot.com/12051052/ is waiting for this CL to land. > What is the ETA for this? I think you meant to link to [ https://chromiumcodereview.appspot.com/12086008/ ]. Szymon , you needn't wait for this CL to land; Mark can rebase on top of yours once it's landed. The field tags in a protocol buffer don't have to be contiguous; they just all need to be unique.
On 2013/01/28 21:37:17, Ilya Sherman wrote: > On 2013/01/28 21:26:17, szym wrote: > > https://chromiumcodereview.appspot.com/12051052/ is waiting for this CL to > land. > > What is the ETA for this? > > I think you meant to link to [ https://chromiumcodereview.appspot.com/12086008/ > ]. Szymon , you needn't wait for this CL to land; Mark can rebase on top of > yours once it's landed. The field tags in a protocol buffer don't have to be > contiguous; they just all need to be unique. Yes. Thanks for clarifying. I'm going to keep tag of Network = 13, so that Mark only needs to update the "// Next tag" line.
On 2013/01/28 22:09:52, szym wrote: > On 2013/01/28 21:37:17, Ilya Sherman wrote: > > On 2013/01/28 21:26:17, szym wrote: > > > https://chromiumcodereview.appspot.com/12051052/ is waiting for this CL to > > land. > > > What is the ETA for this? > > > > I think you meant to link to [ > https://chromiumcodereview.appspot.com/12086008/ > > ]. Szymon , you needn't wait for this CL to land; Mark can rebase on top of > > yours once it's landed. The field tags in a protocol buffer don't have to be > > contiguous; they just all need to be unique. > > Yes. Thanks for clarifying. I'm going to keep tag of Network = 13, so that Mark > only needs to update the "// Next tag" line. FYI, I have successfully tested this patch on a windows machine. It works. I've also got chrome privacy approval for this change. As soon as the other patch ( https://chromiumcodereview.appspot.com/12086008/ ) goes in, I'll rebase and submit this one as well.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/12036047/15005
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/12036047/15005
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/12036047/15005
Message was sent while issue was closed.
Change committed as 179401 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
