Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814083002:
Make the GGRV brand code non-organic. (Closed)
|
Created:
3 years, 8 months ago by Roger Tawa OOO till Jul 10th Modified:
3 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the GGRV brand code non-organic.
BUG=710855
Review-Url: https://codereview.chromium.org/2814083002
Cr-Commit-Position: refs/heads/master@{#464257}
Committed: https://chromium.googlesource.com/chromium/src/+/bc40dc02825d205b8fa33c618026d4c639bb77c4
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address review comments #
Messages
Total messages: 20 (13 generated)
The CQ bit was checked by rogerta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rogerta@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, Can you please take a look? The brand code for chrome enterprise installs should be non-organic in order to enable RLZ for these installs. Thanks.
LGTM, with a couple suggestions for surrounding code https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... File chrome/browser/google/google_brand.cc (right): https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... chrome/browser/google/google_brand.cc:93: const char* const kBrands[] = { Nit: Maybe this should be kOrganicBrands? https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... chrome/browser/google/google_brand.cc:104: const char* const* found = std::find(&kBrands[0], end, brand); Nit: Could probably use binary_search() in place of find() https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... File chrome/browser/google/google_update_settings_unittest.cc (right): https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... chrome/browser/google/google_update_settings_unittest.cc:56: EXPECT_FALSE(google_brand::IsOrganic("GGRV")); Nit: Also EXPECT_TRUE for GGRsomething_else?
The CQ bit was checked by rogerta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Peter. All points addressed, see one question below. Note that "git cl format" changed the string array formatting. https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... File chrome/browser/google/google_brand.cc (right): https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... chrome/browser/google/google_brand.cc:93: const char* const kBrands[] = { On 2017/04/12 20:05:20, Peter Kasting wrote: > Nit: Maybe this should be kOrganicBrands? Done. https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... chrome/browser/google/google_brand.cc:104: const char* const* found = std::find(&kBrands[0], end, brand); On 2017/04/12 20:05:20, Peter Kasting wrote: > Nit: Could probably use binary_search() in place of find() Done. https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... File chrome/browser/google/google_update_settings_unittest.cc (right): https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... chrome/browser/google/google_update_settings_unittest.cc:56: EXPECT_FALSE(google_brand::IsOrganic("GGRV")); On 2017/04/12 20:05:20, Peter Kasting wrote: > Nit: Also EXPECT_TRUE for GGRsomething_else? Do you think I should do all the letters?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... File chrome/browser/google/google_update_settings_unittest.cc (right): https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... chrome/browser/google/google_update_settings_unittest.cc:56: EXPECT_FALSE(google_brand::IsOrganic("GGRV")); On 2017/04/12 21:51:43, Roger Tawa wrote: > On 2017/04/12 20:05:20, Peter Kasting wrote: > > Nit: Also EXPECT_TRUE for GGRsomething_else? > > Do you think I should do all the letters? Not necessarily, I was mainly thinking of catching someone flipping GGR* to false. Do whatever you think is necessary?
On 2017/04/12 22:46:04, Peter Kasting wrote: > https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... > File chrome/browser/google/google_update_settings_unittest.cc (right): > > https://codereview.chromium.org/2814083002/diff/1/chrome/browser/google/googl... > chrome/browser/google/google_update_settings_unittest.cc:56: > EXPECT_FALSE(google_brand::IsOrganic("GGRV")); > On 2017/04/12 21:51:43, Roger Tawa wrote: > > On 2017/04/12 20:05:20, Peter Kasting wrote: > > > Nit: Also EXPECT_TRUE for GGRsomething_else? > > > > Do you think I should do all the letters? > > Not necessarily, I was mainly thinking of catching someone flipping GGR* to > false. Do whatever you think is necessary? Cool. I think it's good as is now. Thanks.
The CQ bit was checked by rogerta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2814083002/#ps20001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492049036799300,
"parent_rev": "770351d1a6f67f9c9523de9b84552a14ec8b7eec", "commit_rev":
"bc40dc02825d205b8fa33c618026d4c639bb77c4"}
Message was sent while issue was closed.
Description was changed from ========== Make the GGRV brand code non-organic. BUG=710855 ========== to ========== Make the GGRV brand code non-organic. BUG=710855 Review-Url: https://codereview.chromium.org/2814083002 Cr-Commit-Position: refs/heads/master@{#464257} Committed: https://chromium.googlesource.com/chromium/src/+/bc40dc02825d205b8fa33c618026... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/bc40dc02825d205b8fa33c618026... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
