Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2253953003:
Disable AutoImport: experiment and histogram for analysis. (Closed)
Description1. Add an experiment that disables AutoImport on first run
(Waterfall Testing will be requested separately). The features keeps
running for enterprise.
2. Create a histogram that measures the size of the histogram
when AutoImport is run. This histogram should be helpful in analyzing
the prospect of disabling AutoImport on first run.
BUG=555550
Committed: https://crrev.com/65bfd343efd7d5e8e84452f9244920cc43371490
Cr-Commit-Position: refs/heads/master@{#415442}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Restricted experiment to autoImport from IE #
Total comments: 1
Patch Set 3 : Aggregate all data imported #
Total comments: 19
Patch Set 4 #
Total comments: 6
Patch Set 5 : Restricted to IE #Patch Set 6 : Merge #
Total comments: 6
Patch Set 7 : Counts empty histories as well #
Total comments: 2
Messages
Total messages: 51 (30 generated)
Description was changed from ========== Add experiment to disable AutoImport on first run and histogram to measure history length on first run AutoImport. 1. Add an experiement that disables AutoImport on first run (Waterfall Testing will be requested separately). The features keeps running for enterprise. 2. Create a histogram that measures the size of the histogram when AutoImport is run. This histogram should be helpful in analyzing the prospect of disabling AutoImport on first run. BUG=555550 ========== to ========== 1. Add an experiment that disables AutoImport on first run (Waterfall Testing will be requested separately). The features keeps running for enterprise. 2. Create a histogram that measures the size of the histogram when AutoImport is run. This histogram should be helpful in analyzing the prospect of disabling AutoImport on first run. BUG=555550 ==========
gcomanici@chromium.org changed reviewers: + manzagop@chromium.org
This issue addresses the points raised in comment #37 in BUG=555550.
This first CL is Looking great! I've put a few uninformed comments, but there's little of substance aside from reducing the formatting diff. Once you've addressed that, you can send to an OWNER. gab@ is OOO so you can try isherman@ who also is a metrics owner. https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:296: const base::Feature kFirstRunAutoImport{"FirstRunAutoImport", I'd call it DisableFirstRunAutoImport, so it's accurate (although confusing) that when the feature is disabled, the import is happening. https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1956: base::Bind(nacl::NaClProcessHost::EarlyStartup)); Yes, this is too big a diff. I really think git cl format only is supposed to edit changed lines, but it may be the line endings switcharoos we suffered. If it's easy enough, I'd check out the master version of the file and manually reapply your changes before sending to the next reviewer. https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... chrome/browser/importer/profile_writer.cc:96: PasswordStoreFactory::GetForProfile(profile_, Did git cl format do this? Typically you don't want to reformat lines you don't change to avoid claming the "blame". Perhaps it's because of the line endings conversions that happened. I wouldn't worry about it this time. Actually, looking below, there's quite a few lines. It seems you've only added l.114-117, so you could checkout the file from master and paste your changes back in, to get a cleaner diff. https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... chrome/browser/importer/profile_writer.cc:116: UMA_HISTOGRAM_COUNTS("Import.SizeImportedHistory.AutoImport", page.size()); Is this called once per importer? Eg if I import both from firefox and IE. Also, do we care about differentiating which importers are providing actual value (ie visit_source)? Does the Import.ImporterType. histogram give you enough info. Basically, depending on the channel, there's a high turn around time to get the data, so you want the metrics to be right from the get go. https://codereview.chromium.org/2253953003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2253953003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:18999: + <summary>The size of the history imported on AutoImport.</summary> IIUC this is only logged on successful import (ie no logging if firefox lock was in use?). Perhaps mention that?
On 2016/08/18 15:59:56, manzagop wrote: > This first CL is Looking great! > > I've put a few uninformed comments, but there's little of substance aside from > reducing the formatting diff. > > Once you've addressed that, you can send to an OWNER. gab@ is OOO so you can try > isherman@ who also is a metrics owner. > > https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... > chrome/browser/chrome_browser_main.cc:296: const base::Feature > kFirstRunAutoImport{"FirstRunAutoImport", > I'd call it DisableFirstRunAutoImport, so it's accurate (although confusing) > that when the feature is disabled, the import is happening. Fixed. > > https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... > chrome/browser/chrome_browser_main.cc:1956: > base::Bind(nacl::NaClProcessHost::EarlyStartup)); > Yes, this is too big a diff. I really think git cl format only is supposed to > edit changed lines, but it may be the line endings switcharoos we suffered. If > it's easy enough, I'd check out the master version of the file and manually > reapply your changes before sending to the next reviewer. > I will do that. > https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... > File chrome/browser/importer/profile_writer.cc (right): > > https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... > chrome/browser/importer/profile_writer.cc:96: > PasswordStoreFactory::GetForProfile(profile_, > Did git cl format do this? Typically you don't want to reformat lines you don't > change to avoid claming the "blame". Perhaps it's because of the line endings > conversions that happened. > > I wouldn't worry about it this time. > > Actually, looking below, there's quite a few lines. It seems you've only added > l.114-117, so you could checkout the file from master and paste your changes > back in, to get a cleaner diff. > > https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... > chrome/browser/importer/profile_writer.cc:116: > UMA_HISTOGRAM_COUNTS("Import.SizeImportedHistory.AutoImport", page.size()); > Is this called once per importer? Eg if I import both from firefox and IE. Also, > do we care about differentiating which importers are providing actual value (ie > visit_source)? Does the Import.ImporterType. histogram give you enough info. > > Basically, depending on the channel, there's a high turn around time to get the > data, so you want the metrics to be right from the get go. This is a very good point. I tried to see if there is an easy way to aggregate the size of all imported histories. From what I gather that is not an easy task. Import.ImporterType suggests that 80% of the time people import from IE and 15% from EDGE (which is not in the enum history::VisitSourse - I assume it gets labelled as IE). Do you think it is correct to restrict the metric to IE-imports only? If you are OK with this, I will take care to modify the UMA details and the description accordingly. > > https://codereview.chromium.org/2253953003/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2253953003/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:18999: + <summary>The size of the > history imported on AutoImport.</summary> > IIUC this is only logged on successful import (ie no logging if firefox lock was > in use?). Perhaps mention that? I will change the description to add that detail.
https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... chrome/browser/importer/profile_writer.cc:116: UMA_HISTOGRAM_COUNTS("Import.SizeImportedHistory.AutoImport", page.size()); On 2016/08/18 15:59:56, manzagop wrote: > Is this called once per importer? Eg if I import both from firefox and IE. Also, > do we care about differentiating which importers are providing actual value (ie > visit_source)? Does the Import.ImporterType. histogram give you enough info. > > Basically, depending on the channel, there's a high turn around time to get the > data, so you want the metrics to be right from the get go. This is a very good point. I tried to see if there is an easy way to aggregate the size of all imported histories. From what I gather that is not an easy task. Import.ImporterType suggests that 80% of the time people import from IE and 15% from EDGE (which is not in the enum history::VisitSourse - I assume it gets labelled as IE). Do you think it is correct to restrict the metric to IE-imports only? If you are OK with this, I will take care to modify the UMA details and the description accordingly.
Forgot to tell you: in terms of workflow, we typically send reply to comments only after having uploaded a new patch (unless there are questions that need answering before going forward). That way the reviewer can check the new code while reading the replies. It's very common to iterate a few times on comments/new patch/more comments... https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... chrome/browser/importer/profile_writer.cc:116: UMA_HISTOGRAM_COUNTS("Import.SizeImportedHistory.AutoImport", page.size()); On 2016/08/18 19:40:00, gcomanici wrote: > On 2016/08/18 15:59:56, manzagop wrote: > > Is this called once per importer? Eg if I import both from firefox and IE. > Also, > > do we care about differentiating which importers are providing actual value > (ie > > visit_source)? Does the Import.ImporterType. histogram give you enough info. > > > > Basically, depending on the channel, there's a high turn around time to get > the > > data, so you want the metrics to be right from the get go. > > This is a very good point. I tried to see if there is an easy way to aggregate > the size of all imported histories. From what I gather that is not an easy task. > > Import.ImporterType suggests that 80% of the time people import from IE and 15% > from EDGE (which is not in the enum history::VisitSourse - I assume it gets > labelled as IE). > > Do you think it is correct to restrict the metric to IE-imports only? If you are > OK with this, I will take care to modify the UMA details and the description > accordingly. This sounds like a discussion for the bug, where you'll find the relevant stakeholders (eg PMs). ;)
https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:296: const base::Feature kFirstRunAutoImport{"FirstRunAutoImport", On 2016/08/18 15:59:56, manzagop wrote: > I'd call it DisableFirstRunAutoImport, so it's accurate (although confusing) > that when the feature is disabled, the import is happening. Done. https://codereview.chromium.org/2253953003/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1956: base::Bind(nacl::NaClProcessHost::EarlyStartup)); On 2016/08/18 15:59:56, manzagop wrote: > Yes, this is too big a diff. I really think git cl format only is supposed to > edit changed lines, but it may be the line endings switcharoos we suffered. If > it's easy enough, I'd check out the master version of the file and manually > reapply your changes before sending to the next reviewer. Done. https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... chrome/browser/importer/profile_writer.cc:96: PasswordStoreFactory::GetForProfile(profile_, On 2016/08/18 15:59:56, manzagop wrote: > Did git cl format do this? Typically you don't want to reformat lines you don't > change to avoid claming the "blame". Perhaps it's because of the line endings > conversions that happened. > > I wouldn't worry about it this time. > > Actually, looking below, there's quite a few lines. It seems you've only added > l.114-117, so you could checkout the file from master and paste your changes > back in, to get a cleaner diff. Done. https://codereview.chromium.org/2253953003/diff/1/chrome/browser/importer/pro... chrome/browser/importer/profile_writer.cc:116: UMA_HISTOGRAM_COUNTS("Import.SizeImportedHistory.AutoImport", page.size()); On 2016/08/18 20:35:29, manzagop wrote: > On 2016/08/18 19:40:00, gcomanici wrote: > > On 2016/08/18 15:59:56, manzagop wrote: > > > Is this called once per importer? Eg if I import both from firefox and IE. > > Also, > > > do we care about differentiating which importers are providing actual value > > (ie > > > visit_source)? Does the Import.ImporterType. histogram give you enough info. > > > > > > Basically, depending on the channel, there's a high turn around time to get > > the > > > data, so you want the metrics to be right from the get go. > > > > This is a very good point. I tried to see if there is an easy way to aggregate > > the size of all imported histories. From what I gather that is not an easy > task. > > > > Import.ImporterType suggests that 80% of the time people import from IE and > 15% > > from EDGE (which is not in the enum history::VisitSourse - I assume it gets > > labelled as IE). > > > > Do you think it is correct to restrict the metric to IE-imports only? If you > are > > OK with this, I will take care to modify the UMA details and the description > > accordingly. > > This sounds like a discussion for the bug, where you'll find the relevant > stakeholders (eg PMs). ;) Acknowledged. https://codereview.chromium.org/2253953003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2253953003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:18999: + <summary>The size of the history imported on AutoImport.</summary> On 2016/08/18 15:59:56, manzagop wrote: > IIUC this is only logged on successful import (ie no logging if firefox lock was > in use?). Perhaps mention that? Done.
This is looking good! You can now add a reviewer. ;) https://codereview.chromium.org/2253953003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2253953003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19000: + The size of the history on AutoImport. This measure is only logged on a nit: weird number of whitespace?
gcomanici@chromium.org changed reviewers: + isherman@chromium.org
Thanks =) https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:297: "DisableFirstRunAutoImport", base::FEATURE_DISABLED_BY_DEFAULT}; Please define this feature in https://cs.chromium.org/chromium/src/chrome/common/chrome_features.h https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1807: // By default, autoImport is performed on first run. nit: Please try to be consist about how autoImport vs. "AutoImport" vs. "auto-import" (vs. any other variants) is spelled. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1808: bool perform_autoImport_on_first_run = true; nit: s/autoImport/auto_import https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1808: bool perform_autoImport_on_first_run = true; nit: I'd drop the "_on_first_run" suffix, since this code is already within the body of an IsChromeFirstRun() if-stmt. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1812: #if defined(OS_WIN) Please control the platform via the field trial's server-side config, rather than via a #define, unless you have a really compelling reason to use a #define. If you do, please document it here =) https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1814: base::FieldTrialList::Find("DisableFirstRunAutoImport"); There's no need to call this when using the Feature API. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1817: base::win::IsEnrolledToDomain(); Please make the IsEnrolledToDomain() check first. That way, users who are in enterprise domains won't be considered as "active" in the field trial, which will reduce noise when viewing the metrics. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1818: #endif The indentation looks off for these lines. Please run git cl format over this CL -- it's a handy tool =) (Apologies if you've already run it, and I'm misunderstanding something.) https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/importer... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/importer... chrome/browser/importer/profile_writer.cc:13: #include "base/metrics/histogram.h" nit: Please import histogram_macros.h instead. https://codereview.chromium.org/2253953003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2253953003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18997: +<histogram name="Import.SizeImportedHistory.AutoImport"> nit: I'd tweak this name to "Import.ImportedHistorySize.AutoImport". https://codereview.chromium.org/2253953003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18997: +<histogram name="Import.SizeImportedHistory.AutoImport"> Please add a units attribute. Specifically, it's not entirely clear whether this histogram is measuring history entries, or bytes.
Addressed helpful comments by Ilya. Thank you! https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:297: "DisableFirstRunAutoImport", base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/08/22 21:10:45, Ilya Sherman wrote: > Please define this feature in > https://cs.chromium.org/chromium/src/chrome/common/chrome_features.h Done. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1808: bool perform_autoImport_on_first_run = true; On 2016/08/22 21:10:45, Ilya Sherman wrote: > nit: I'd drop the "_on_first_run" suffix, since this code is already within the > body of an IsChromeFirstRun() if-stmt. Done. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1812: #if defined(OS_WIN) On 2016/08/22 21:10:45, Ilya Sherman wrote: > Please control the platform via the field trial's server-side config, rather > than via a #define, unless you have a really compelling reason to use a #define. > If you do, please document it here =) Done. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1814: base::FieldTrialList::Find("DisableFirstRunAutoImport"); On 2016/08/22 21:10:45, Ilya Sherman wrote: > There's no need to call this when using the Feature API. Acknowledged. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1817: base::win::IsEnrolledToDomain(); On 2016/08/22 21:10:45, Ilya Sherman wrote: > Please make the IsEnrolledToDomain() check first. That way, users who are in > enterprise domains won't be considered as "active" in the field trial, which > will reduce noise when viewing the metrics. Done. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1818: #endif On 2016/08/22 21:10:45, Ilya Sherman wrote: > The indentation looks off for these lines. Please run git cl format over this > CL -- it's a handy tool =) (Apologies if you've already run it, and I'm > misunderstanding something.) Acknowledged. https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/importer... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/40001/chrome/browser/importer... chrome/browser/importer/profile_writer.cc:13: #include "base/metrics/histogram.h" On 2016/08/22 21:10:45, Ilya Sherman wrote: > nit: Please import histogram_macros.h instead. Done. https://codereview.chromium.org/2253953003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2253953003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18997: +<histogram name="Import.SizeImportedHistory.AutoImport"> On 2016/08/22 21:10:45, Ilya Sherman wrote: > nit: I'd tweak this name to "Import.ImportedHistorySize.AutoImport". Done.
Yay, much cleaner =) https://codereview.chromium.org/2253953003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1805: // The feature keeps running for enterprise. nit: I'd phrase this as something like: "Auto Import might be disabled via a field trial. However, this field trial is not intended to affect enterprise users." https://codereview.chromium.org/2253953003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1806: if (base::win::IsEnrolledToDomain() || Hmm, I just noticed that you're calling a "base::win" function here. First of all, this probably won't compile on non-Windows OSes. Secondly, it's probably not the correct notion of "enterprise enrolled" -- that ought to be a platform-neutral concept, AFAIK. I'm not sure what the "correct" method is, but I bet it would be from something within //components/policy. So, I'd recommend either scanning through that code to find something appropriate, or checking with the folks listed in https://cs.chromium.org/chromium/src/components/policy/OWNERS https://codereview.chromium.org/2253953003/diff/60001/chrome/common/chrome_fe... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2253953003/diff/60001/chrome/common/chrome_fe... chrome/common/chrome_features.h:40: #endif Again, no need to restrict this to just Windows -- indeed, since chrome_browser_main.cc calls this on other platforms, I'd expect that code to currently not compile on, e.g., Mac.
The CQ bit was checked by gcomanici@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by gcomanici@chromium.org to run a CQ dry run
The CQ bit was unchecked by gcomanici@chromium.org
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 checked by gcomanici@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gcomanici@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by gcomanici@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...
I looked into finding the right notion for "enterprise enrolled" for all devices and there is no simple solution for that. Also, in the BUG discussion it was settled that measuring the size of Auto Import from IE only and running the experiment only for Windows would be enough. I have made the necessary adjustments to the code. https://codereview.chromium.org/2253953003/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1805: // The feature keeps running for enterprise. On 2016/08/23 21:05:33, Ilya Sherman wrote: > nit: I'd phrase this as something like: > > "Auto Import might be disabled via a field trial. However, this field trial is > not intended to affect enterprise users." Done. https://codereview.chromium.org/2253953003/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1806: if (base::win::IsEnrolledToDomain() || On 2016/08/23 21:05:33, Ilya Sherman wrote: > Hmm, I just noticed that you're calling a "base::win" function here. First of > all, this probably won't compile on non-Windows OSes. Secondly, it's probably > not the correct notion of "enterprise enrolled" -- that ought to be a > platform-neutral concept, AFAIK. > > I'm not sure what the "correct" method is, but I bet it would be from something > within //components/policy. So, I'd recommend either scanning through that code > to find something appropriate, or checking with the folks listed in > https://cs.chromium.org/chromium/src/components/policy/OWNERS From what I gather in BUG=555550, the trial is intended to run on OS-Windows only (i.e. use base::win::IsEnrolledToDomain() to determine enterprise enrolled). Indeed, there were a few issues with compilation on other platforms. I addressed that in the latest patch. https://codereview.chromium.org/2253953003/diff/60001/chrome/common/chrome_fe... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2253953003/diff/60001/chrome/common/chrome_fe... chrome/common/chrome_features.h:40: #endif On 2016/08/23 21:05:33, Ilya Sherman wrote: > Again, no need to restrict this to just Windows -- indeed, since > chrome_browser_main.cc calls this on other platforms, I'd expect that code to > currently not compile on, e.g., Mac. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks.
gcomanici@chromium.org changed reviewers: + sky@chromium.org
Asking for an Owners Review on the following files: chrome/browser/chrome_browser_main.cc chrome/browser/importer/profile_writer.cc chrome/common/chrome_features.h chrome/common/chrome_features.cc Thank you!
Asking for an Owners Review on the following files: chrome/browser/chrome_browser_main.cc chrome/browser/importer/profile_writer.cc chrome/common/chrome_features.h chrome/common/chrome_features.cc Thank you!
https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1836: first_run::DoPostImportTasks(profile_, Did you investigate what this does to ensure we want to run it in the !auto_import case? https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/importe... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/importe... chrome/browser/importer/profile_writer.cc:107: void ProfileWriter::AddHistoryPage(const history::URLRows& page, It looks like this isn't called if no rows are found. It seems like that should be captured. https://codereview.chromium.org/2253953003/diff/160001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2253953003/diff/160001/chrome/common/chrome_f... chrome/common/chrome_features.h:40: extern const base::Feature kDisableFirstRunAutoImportWin; If possible, use Enable instead of disable. That way code looks like: if (isEnabled... vs if (!isDisabled), which is easier to get wrong.
Addressed issues raised by sky. Thank you! https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1836: first_run::DoPostImportTasks(profile_, On 2016/08/29 16:08:56, sky wrote: > Did you investigate what this does to ensure we want to run it in the > !auto_import case? This is a very good point. The remaining lines are dealing with the following issues: 1. Should Chrome be the default browser? 2. Show Welcome Page? 3. In Windows: notifies PDM that this is the first run. 4. Other Platform specific tasks unrelated to Auto Import. None of these are affected by Auto Import. https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/importe... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/160001/chrome/browser/importe... chrome/browser/importer/profile_writer.cc:107: void ProfileWriter::AddHistoryPage(const history::URLRows& page, On 2016/08/29 16:08:56, sky wrote: > It looks like this isn't called if no rows are found. It seems like that should > be captured. The new patch modifies the code in src/chrome/utility/importer/ie_importer_win.cc to resolve the issue. I attempted to move the UMA_HISTOGRAM_COUNTS call in ie_importer_win.cc, but I was not allowed to import first_run.h since it would create some dependency conflicts. https://codereview.chromium.org/2253953003/diff/160001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2253953003/diff/160001/chrome/common/chrome_f... chrome/common/chrome_features.h:40: extern const base::Feature kDisableFirstRunAutoImportWin; On 2016/08/29 16:08:56, sky wrote: > If possible, use Enable instead of disable. That way code looks like: > > if (isEnabled... > > vs > > if (!isDisabled), which is easier to get wrong. I think the name should stay kDisableFirstRunAutoImportWin to avoid confusion in the list of features. The default behavior is to run Auto Import on a first browser run, so the feature would disable it. Also, code in chrome_browser_main.cc indeed has the line if(IsEnabled(features::kDisableFirstRunAutoImportWin)).
LGTM https://codereview.chromium.org/2253953003/diff/180001/chrome/browser/importe... File chrome/browser/importer/profile_writer.cc (right): https://codereview.chromium.org/2253953003/diff/180001/chrome/browser/importe... chrome/browser/importer/profile_writer.cc:109: if (!page.empty()) use {} as the body is more than one line. https://codereview.chromium.org/2253953003/diff/180001/chrome/utility/importe... File chrome/utility/importer/ie_importer_win.cc (right): https://codereview.chromium.org/2253953003/diff/180001/chrome/utility/importe... chrome/utility/importer/ie_importer_win.cc:563: if (!cancelled()) { no {}
The CQ bit was checked by gcomanici@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2253953003/#ps180001 (title: "Counts empty histories as well")
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 ========== 1. Add an experiment that disables AutoImport on first run (Waterfall Testing will be requested separately). The features keeps running for enterprise. 2. Create a histogram that measures the size of the histogram when AutoImport is run. This histogram should be helpful in analyzing the prospect of disabling AutoImport on first run. BUG=555550 ========== to ========== 1. Add an experiment that disables AutoImport on first run (Waterfall Testing will be requested separately). The features keeps running for enterprise. 2. Create a histogram that measures the size of the histogram when AutoImport is run. This histogram should be helpful in analyzing the prospect of disabling AutoImport on first run. BUG=555550 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== 1. Add an experiment that disables AutoImport on first run (Waterfall Testing will be requested separately). The features keeps running for enterprise. 2. Create a histogram that measures the size of the histogram when AutoImport is run. This histogram should be helpful in analyzing the prospect of disabling AutoImport on first run. BUG=555550 ========== to ========== 1. Add an experiment that disables AutoImport on first run (Waterfall Testing will be requested separately). The features keeps running for enterprise. 2. Create a histogram that measures the size of the histogram when AutoImport is run. This histogram should be helpful in analyzing the prospect of disabling AutoImport on first run. BUG=555550 Committed: https://crrev.com/65bfd343efd7d5e8e84452f9244920cc43371490 Cr-Commit-Position: refs/heads/master@{#415442} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/65bfd343efd7d5e8e84452f9244920cc43371490 Cr-Commit-Position: refs/heads/master@{#415442}
Message was sent while issue was closed.
On 2016/08/18 14:19:49, gcomanici wrote: > This issue addresses the points raised in comment #37 in BUG=555550. When I worked on my task I noticed that you used first_run::IsChromeFirstRun() in this issue. There are one problem with this. User can make manual import during first run. I mean he launched browser first time. Browser could get some history from another browser. The user goes to settings and make manual import. So statistics for manual import will merge with statistics about autoimport. It's not big issue for this task because you made restriction for browser type. But in general it's not good idea to use first_run::IsChromeFirstRun() in profile_writer.cc. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
