Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3069028:
EULA screen enabling/disabling crash/metrics reporting renovated.... (Closed)
|
Created:
10 years, 4 months ago by glotov Modified:
9 years, 7 months ago CC:
chromium-reviews, nkostylev+cc_chromium.org, davemoore+watch_chromium.org, ben+cc_chromium.org, Nikita (slow) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionEULA screen enabling/disabling crash/metrics reporting renovated.
BUG=chromium-os:2884
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55061
Patch Set 1 #
Total comments: 1
Messages
Total messages: 13 (0 generated)
Hello Mattias, Lei! I noted you have updated crash sending logic resently. I have updated it on login wizard side as well. Please double-check that I got your ideas right. In breif about this code. This is EULA screen shown to user only initially. This screen has a checkbox (usage_statistics_checkbox_) that user sets/unsets to enable/disable crash and statistics reporting accordingly. Please let me know if you need more info.
LGTM Do we intend to change the Linux first time startup code as well?
Thanks for the review! What do you mean: Linux first time startup code? browser_main.cc - it has been modified 2 days ago, no? On Thu, Aug 5, 2010 at 12:08 AM, <thestig@chromium.org> wrote: > LGTM > > Do we intend to change the Linux first time startup code as well? > > > http://codereview.chromium.org/3069028/show > -- Thank you, Denis
I'm referring to the similar code in chrome/browser/gtk/first_run_dialog.cc. On Wed, Aug 4, 2010 at 1:52 PM, Denis Glotov <glotov@chromium.org> wrote: > Thanks for the review! > What do you mean: Linux first time startup code? browser_main.cc - it has > been modified 2 days ago, no? > > On Thu, Aug 5, 2010 at 12:08 AM, <thestig@chromium.org> wrote: >> >> LGTM >> >> Do we intend to change the Linux first time startup code as well? >> >> http://codereview.chromium.org/3069028/show > > > > -- > Thank you, > Denis >
LGTM. The first run dialog also needs work to disable the checkbox if the setting is controlled through policy btw. http://codereview.chromium.org/3069028/diff/1/2 File chrome/browser/chromeos/login/eula_view.cc (right): http://codereview.chromium.org/3069028/diff/1/2#newcode284 chrome/browser/chromeos/login/eula_view.cc:284: prefs->SetBoolean(prefs::kMetricsReportingEnabled, enable_reporting); I'm not current on the plans for ChromeOS w.r.t. configuration policy. When ChromeOS starts supporting it, you also need to make sure that you disable or hide the checkbox if the admin has already taken the decision through policy and then not write the preference, since that'll fail.
Mattias, thank you for the comment. What do you call "policy"? Is it already implemented in Chrome or just a notion or idea? On the other hand, in ChromeOS, there is flag-file "/etc/send_metrics". It is created (or not created) by the ChromeOS build process to allow sending crash/statistics to Goole. If the file is present - ChromeOS may send data to Google on user's discretion. If not present - sending is forbidden. Am I right that currently /etc/send_metrics is not honored by Chrome? So I fix it in the nearest future. Lei, regarding first_run_dialog.cc, I'll update it (or file a bug) as I finish this issue, hopefully the next week. Thank you, Denis
On Thu, Aug 5, 2010 at 4:19 PM, Denis Glotov <glotov@chromium.org> wrote: > Mattias, thank you for the comment. What do you call "policy"? Is it > already implemented in Chrome or just a notion or idea? It is already there. We have a team in Munich that implemented support for windows group policy (and their equivalents on Linux and Mac). It basically works by allowing policy to override values in the pref_service. See pref_value_store.cc for details. On Linux, we read a policy directory under /etc to get the policy. On the other hand, in ChromeOS, there is flag-file "/etc/send_metrics". It > is created (or not created) by the ChromeOS build process to allow sending > crash/statistics to Goole. If the file is present - ChromeOS may send data > to Google on user's discretion. If not present - sending is forbidden. > Provided ChromeOS uses the standard linux policy loader (there's been discussion to produce a specialized version, don't know the state of that), you could just have it put a file /etc/opt/google/policy/managed/send_metrics and put the following in it: { "MetricsReportingEnabled": false } > > Am I right that currently /etc/send_metrics is not honored by Chrome? So I > fix it in the nearest future. > I guess so, I haven't seen any code that checks that file. Lei, regarding first_run_dialog.cc, I'll update it (or file a bug) as I > finish this issue, hopefully the next week. > Thanks, Mattias
Mattias, is the group policy (PrefValueStore) currently used for prefs::kMetricsReportingEnabled? I haven't noticed it in linux. Only seen it in windows code. And to double-check, is prefs::kMetricsReportingEnabled used only to speed up the query to "Consent To Send Stats" file inside Chrome process? I mean, if I need to change the setting, I need to both change the pref and update the "Consent To Send Stats" file. BTW, prefs::kMetricsReportingEnabled is initialized with the presence of the "Consent To Send Stats" file ( https://cs.corp.google.com/p#chrome/trunk/src/chrome/browser/browser_main.cc&...). So there is no need to call prefs->SavePersistentPrefs() after modifying it, what do you think? On Thu, Aug 5, 2010 at 6:37 PM, Mattias Nissler <mnissler@chromium.org>wrote: > On Thu, Aug 5, 2010 at 4:19 PM, Denis Glotov <glotov@chromium.org> wrote: > >> Mattias, thank you for the comment. What do you call "policy"? Is it >> already implemented in Chrome or just a notion or idea? > > > It is already there. We have a team in Munich that implemented support for > windows group policy (and their equivalents on Linux and Mac). It basically > works by allowing policy to override values in the pref_service. See > pref_value_store.cc for details. On Linux, we read a policy directory under > /etc to get the policy. > > On the other hand, in ChromeOS, there is flag-file "/etc/send_metrics". It >> is created (or not created) by the ChromeOS build process to allow sending >> crash/statistics to Goole. If the file is present - ChromeOS may send data >> to Google on user's discretion. If not present - sending is forbidden. >> > > Provided ChromeOS uses the standard linux policy loader (there's been > discussion to produce a specialized version, don't know the state of that), > you could just have it put a file > /etc/opt/google/policy/managed/send_metrics and put the following in it: > > { > "MetricsReportingEnabled": false > } > > >> >> Am I right that currently /etc/send_metrics is not honored by Chrome? So I >> fix it in the nearest future. >> > > I guess so, I haven't seen any code that checks that file. > > Lei, regarding first_run_dialog.cc, I'll update it (or file a bug) as I >> finish this issue, hopefully the next week. >> > > Thanks, > > Mattias > -- Thank you, Denis
5 августа 2010 г. 19:22 пользователь Denis Glotov <glotov@chromium.org>написал: > Mattias, is the group policy (PrefValueStore) currently used for > prefs::kMetricsReportingEnabled? I haven't noticed it in linux. Only seen it > in windows code. > > And to double-check, is prefs::kMetricsReportingEnabled used only to speed > up the query to "Consent To Send Stats" file inside Chrome process? I mean, > if I need to change the setting, I need to both change the pref and update > the "Consent To Send Stats" file. > > BTW, prefs::kMetricsReportingEnabled is initialized with the presence of > the "Consent To Send Stats" file ( > https://cs.corp.google.com/p#chrome/trunk/src/chrome/browser/browser_main.cc&...). > So there is no need to call prefs->SavePersistentPrefs() after modifying it, > what do you think? > This call local_state->RegisterBooleanPref(prefs::kMetricsReportingEnabled, GoogleUpdateSettings::GetCollectStatsConsent()); will initialize default value of kMetricsReportingEnabled based on consent file presence. If somewhere in code this preference is changed later than it's new value will be user for subsequent users of preference. So the real question is what happens when UMA/crash sending option is toggled in Chrome options? Will it change kMetricsReportingEnabled? > > On Thu, Aug 5, 2010 at 6:37 PM, Mattias Nissler <mnissler@chromium.org>wrote: > >> On Thu, Aug 5, 2010 at 4:19 PM, Denis Glotov <glotov@chromium.org> wrote: >> >>> Mattias, thank you for the comment. What do you call "policy"? Is it >>> already implemented in Chrome or just a notion or idea? >> >> >> It is already there. We have a team in Munich that implemented support for >> windows group policy (and their equivalents on Linux and Mac). It basically >> works by allowing policy to override values in the pref_service. See >> pref_value_store.cc for details. On Linux, we read a policy directory under >> /etc to get the policy. >> >> On the other hand, in ChromeOS, there is flag-file "/etc/send_metrics". It >>> is created (or not created) by the ChromeOS build process to allow sending >>> crash/statistics to Goole. If the file is present - ChromeOS may send data >>> to Google on user's discretion. If not present - sending is forbidden. >>> >> >> Provided ChromeOS uses the standard linux policy loader (there's been >> discussion to produce a specialized version, don't know the state of that), >> you could just have it put a file >> /etc/opt/google/policy/managed/send_metrics and put the following in it: >> >> { >> "MetricsReportingEnabled": false >> } >> >> >>> >>> Am I right that currently /etc/send_metrics is not honored by Chrome? So >>> I fix it in the nearest future. >>> >> >> I guess so, I haven't seen any code that checks that file. >> >> Lei, regarding first_run_dialog.cc, I'll update it (or file a bug) as I >>> finish this issue, hopefully the next week. >>> >> >> Thanks, >> >> Mattias >> > > > > -- > Thank you, > Denis > -- Nikita
On Thu, Aug 5, 2010 at 5:22 PM, Denis Glotov <glotov@chromium.org> wrote: > Mattias, is the group policy (PrefValueStore) currently used for > prefs::kMetricsReportingEnabled? I haven't noticed it in linux. Only seen it > in windows code. It's also used on Linux. The code uses a table that maps policy to preferences that's shared between platforms. > And to double-check, is prefs::kMetricsReportingEnabled used only to speed > up the query to "Consent To Send Stats" file inside Chrome process? I mean, > if I need to change the setting, I need to both change the pref and update > the "Consent To Send Stats" file. > Unfortunately, no. For example, at startup the file is available but the PrefService is not, so the crash reporter initialization can only look at the file. Furthermore, the metrics reporting part only checks the preference and not the file. And note that on Windows there is no file, but a registry key. And that is set through the installer. And finally note that I'm not an expert but only worked my way through this last week :) If you have ideas on how to clean it up though, I'm happy to help. To answer your question: My current understanding is that you should always write both the file and the preference. > > BTW, prefs::kMetricsReportingEnabled is initialized with the presence of > the "Consent To Send Stats" file ( > https://cs.corp.google.com/p#chrome/trunk/src/chrome/browser/browser_main.cc&...). > So there is no need to call prefs->SavePersistentPrefs() after modifying it, > what do you think? > > On Thu, Aug 5, 2010 at 6:37 PM, Mattias Nissler <mnissler@chromium.org>wrote: > >> On Thu, Aug 5, 2010 at 4:19 PM, Denis Glotov <glotov@chromium.org> wrote: >> >>> Mattias, thank you for the comment. What do you call "policy"? Is it >>> already implemented in Chrome or just a notion or idea? >> >> >> It is already there. We have a team in Munich that implemented support for >> windows group policy (and their equivalents on Linux and Mac). It basically >> works by allowing policy to override values in the pref_service. See >> pref_value_store.cc for details. On Linux, we read a policy directory under >> /etc to get the policy. >> >> On the other hand, in ChromeOS, there is flag-file "/etc/send_metrics". It >>> is created (or not created) by the ChromeOS build process to allow sending >>> crash/statistics to Goole. If the file is present - ChromeOS may send data >>> to Google on user's discretion. If not present - sending is forbidden. >>> >> >> Provided ChromeOS uses the standard linux policy loader (there's been >> discussion to produce a specialized version, don't know the state of that), >> you could just have it put a file >> /etc/opt/google/policy/managed/send_metrics and put the following in it: >> >> { >> "MetricsReportingEnabled": false >> } >> >> >>> >>> Am I right that currently /etc/send_metrics is not honored by Chrome? So >>> I fix it in the nearest future. >>> >> >> I guess so, I haven't seen any code that checks that file. >> >> Lei, regarding first_run_dialog.cc, I'll update it (or file a bug) as I >>> finish this issue, hopefully the next week. >>> >> >> Thanks, >> >> Mattias >> > > > > -- > Thank you, > Denis >
On Thu, Aug 5, 2010 at 5:31 PM, Nikita Kostylev <nkostylev@chromium.org>wrote: > > > 5 августа 2010 г. 19:22 пользователь Denis Glotov <glotov@chromium.org>написал: > > Mattias, is the group policy (PrefValueStore) currently used for >> prefs::kMetricsReportingEnabled? I haven't noticed it in linux. Only seen it >> in windows code. >> >> And to double-check, is prefs::kMetricsReportingEnabled used only to speed >> up the query to "Consent To Send Stats" file inside Chrome process? I mean, >> if I need to change the setting, I need to both change the pref and update >> the "Consent To Send Stats" file. >> >> BTW, prefs::kMetricsReportingEnabled is initialized with the presence of >> the "Consent To Send Stats" file ( >> https://cs.corp.google.com/p#chrome/trunk/src/chrome/browser/browser_main.cc&...). >> So there is no need to call prefs->SavePersistentPrefs() after modifying it, >> what do you think? >> > > This call > > local_state->RegisterBooleanPref(prefs::kMetricsReportingEnabled, > GoogleUpdateSettings::GetCollectStatsConsent()); > > will initialize default value of kMetricsReportingEnabled based on consent > file presence. > If somewhere in code this preference is changed later than it's new value > will be user for subsequent users of preference. > > So the real question is what happens when UMA/crash sending option is > toggled in Chrome options? > Will it change kMetricsReportingEnabled? > It should. I haven't checked though. > > >> >> On Thu, Aug 5, 2010 at 6:37 PM, Mattias Nissler <mnissler@chromium.org>wrote: >> >>> On Thu, Aug 5, 2010 at 4:19 PM, Denis Glotov <glotov@chromium.org>wrote: >>> >>>> Mattias, thank you for the comment. What do you call "policy"? Is it >>>> already implemented in Chrome or just a notion or idea? >>> >>> >>> It is already there. We have a team in Munich that implemented support >>> for windows group policy (and their equivalents on Linux and Mac). It >>> basically works by allowing policy to override values in the pref_service. >>> See pref_value_store.cc for details. On Linux, we read a policy directory >>> under /etc to get the policy. >>> >>> On the other hand, in ChromeOS, there is flag-file "/etc/send_metrics". >>>> It is created (or not created) by the ChromeOS build process to allow >>>> sending crash/statistics to Goole. If the file is present - ChromeOS may >>>> send data to Google on user's discretion. If not present - sending is >>>> forbidden. >>>> >>> >>> Provided ChromeOS uses the standard linux policy loader (there's been >>> discussion to produce a specialized version, don't know the state of that), >>> you could just have it put a file >>> /etc/opt/google/policy/managed/send_metrics and put the following in it: >>> >>> { >>> "MetricsReportingEnabled": false >>> } >>> >>> >>>> >>>> Am I right that currently /etc/send_metrics is not honored by Chrome? So >>>> I fix it in the nearest future. >>>> >>> >>> I guess so, I haven't seen any code that checks that file. >>> >>> Lei, regarding first_run_dialog.cc, I'll update it (or file a bug) as I >>>> finish this issue, hopefully the next week. >>>> >>> >>> Thanks, >>> >>> Mattias >>> >> >> >> >> -- >> Thank you, >> Denis >> > > > > -- > Nikita >
Thank you Mattias! I simultaneously found where we use policy. Will send you tiny patch tomorrow. BTW, you say that at startup PrefService is not available. Why? -- Thank you, Denis
On Thu, Aug 5, 2010 at 7:04 PM, Denis Glotov <glotov@chromium.org> wrote: > Thank you Mattias! I simultaneously found where we use policy. Will send > you tiny patch tomorrow. > > BTW, you say that at startup PrefService is not available. Why? > The local_state PrefService instance in BrowserProcess gets initialized during BrowserMain(). The profile PrefService instance (the one containing most user prefs) even later. There is code that runs before that, and on win (and maybe mac also, cannot remember exactly), the crash reporter is even initialized by code in chrome/app, which runs very early and cannot use stuff from chrome/browser (that's in chrome.dll which is not loaded at that point). In general, it makes sense to run as little code as possible before initializing the crash reporter, since you want to catch all crashs you can, including those that happen during startup. Mattias |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
