Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285483002:
Move UMA to opt-out (Closed)
|
Created:
4 years, 3 months ago by gayane -on leave until 09-2017 Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove UMA to opt-out for all the platforms. This mainly
affects MAC OSX for channels other than canary and Linux for
all the channels. Other platforms don't go through the same
first run flow.
BUG=600391
Committed: https://crrev.com/bf1f2adb1a184c294cefcc5c411a1ce09934f120
Cr-Commit-Position: refs/heads/master@{#414770}
Patch Set 1 #
Total comments: 2
Patch Set 2 : removed the constant #
Total comments: 1
Patch Set 3 : fix comment #Messages
Total messages: 18 (9 generated)
gayane@chromium.org changed reviewers: + jwd@chromium.org
Please take a look
Description was changed from ========== Move UMA to opt-out BUG=600391 ========== to ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary BUG=600391 ==========
Description was changed from ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary BUG=600391 ========== to ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary and Linux for all the channels. Other platforms don't go through the same first run flow. BUG=600391 ==========
Description was changed from ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary and Linux for all the channels. Other platforms don't go through the same first run flow. BUG=600391 ========== to ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary and Linux for all the channels. Other platforms don't go through the same first run flow. BUG=600391 ==========
lgtm
gayane@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please take a look.
https://codereview.chromium.org/2285483002/diff/1/chrome/browser/first_run/fi... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2285483002/diff/1/chrome/browser/first_run/fi... chrome/browser/first_run/first_run.cc:659: return !kDefaultMetricsReportingState; The negative is always confusing. Is the reason you didn't want to go with a constant that matches the function name? kIsMetricsReportingOptIn? Or just have this return false with a comment?
I have removed the constant. Please have one more look. https://codereview.chromium.org/2285483002/diff/1/chrome/browser/first_run/fi... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2285483002/diff/1/chrome/browser/first_run/fi... chrome/browser/first_run/first_run.cc:659: return !kDefaultMetricsReportingState; On 2016/08/26 00:00:57, sky wrote: > The negative is always confusing. Is the reason you didn't want to go with a > constant that matches the function name? kIsMetricsReportingOptIn? Or just have > this return false with a comment? Done.
LGTM https://codereview.chromium.org/2285483002/diff/20001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2285483002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:657: // FRE. FRE may be obvious to you, but I suspect most engineers won't have a clue. Please avoid the acronym and spell out what FRE is.
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2285483002/#ps40001 (title: "fix comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary and Linux for all the channels. Other platforms don't go through the same first run flow. BUG=600391 ========== to ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary and Linux for all the channels. Other platforms don't go through the same first run flow. BUG=600391 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary and Linux for all the channels. Other platforms don't go through the same first run flow. BUG=600391 ========== to ========== Move UMA to opt-out for all the platforms. This mainly affects MAC OSX for channels other than canary and Linux for all the channels. Other platforms don't go through the same first run flow. BUG=600391 Committed: https://crrev.com/bf1f2adb1a184c294cefcc5c411a1ce09934f120 Cr-Commit-Position: refs/heads/master@{#414770} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bf1f2adb1a184c294cefcc5c411a1ce09934f120 Cr-Commit-Position: refs/heads/master@{#414770} |
