[Settings reset prompt] Do not use the <udpatecheck> tag when downloading default settings and download settings only when needed. |
|||||||||||||||
Issue descriptionThe <updatecheck> tag is currently present when downloading default settings. See https://cs.chromium.org/chromium/src/chrome/browser/profile_resetter/brandcode_config_fetcher.cc?rcl=e2c96f726dffb1ba319a3ed7d4da8aa0c1156fb2&l=33 This unnecessarily indicates to the servers that Chrome needs to be updated. We should be able to simply remove that tag since the corresponding data that is sent back is never used. Also, the SettingsResetPromptModel class always fetches default settings when it is created. It should instead only fetch default settings when it has established that a reset prompt is required.
,
May 30 2017
+waffles, privard FYI
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/944eab08ac07c46cb5ce2406362f7e376e4c4dec commit 944eab08ac07c46cb5ce2406362f7e376e4c4dec Author: alito <alito@chromium.org> Date: Wed May 31 08:52:11 2017 Remove "updatecheck" tag when fetching default settings. The "updatecheck" tag is unnecessary when we want to fetch default settings. Additionally its presence causes the servers to infer that Chrome needs to be updated. BUG= 727829 TBR=vasilii@chromium.org Review-Url: https://codereview.chromium.org/2908073006 Cr-Commit-Position: refs/heads/master@{#475849} [modify] https://crrev.com/944eab08ac07c46cb5ce2406362f7e376e4c4dec/chrome/browser/profile_resetter/brandcode_config_fetcher.cc
,
Jun 1 2017
If we need the CL to get merged to M59, please request the same ASAP we are planning a M59 stable launch on Monday and hoping the CL is safe to get merged to M59 as stakes are high now.
,
Jun 1 2017
Adding merge request. I have tested this on an official chrome-branded build and in my tests fetching default settings works the same with and without this change. waffles@: I believe you stated that this was considered very safe from Omaha's point of view?
,
Jun 1 2017
Yes. The merge only applies to the removal of <updatecheck>, not the second issue identified in this bug, correct? ("Also, the SettingsResetPromptModel class always fetches default settings when it is created. It should instead only fetch default settings when it has established that a reset prompt is required.")
,
Jun 1 2017
This bug requires manual review: Only 4 days from stable, we might already have a stable candidate build Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 1 2017
waffles@: yes, this merge request is only for removing the update check. The second part of the bug will be fixed, but I think it is safer not to merge that up to M59 this late.
,
Jun 1 2017
Have you already confirmed/verified this fix in Canary/Dev?
,
Jun 1 2017
Yes. I have verified that default settings are fetched correctly on Canary version 61.0.3117.0, which contains that change.
,
Jun 2 2017
Thanks for confirming - approving merge for M59.
,
Jun 2 2017
Is branch 3071 the correct branch to merge this to for M59?
,
Jun 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66ae57e1a5d8c23c6dbeebdcafbf3c9ab28305b4 commit 66ae57e1a5d8c23c6dbeebdcafbf3c9ab28305b4 Author: Chris Sharp <csharp@chromium.org> Date: Fri Jun 02 16:55:43 2017 Remove "updatecheck" tag when fetching default settings. The "updatecheck" tag is unnecessary when we want to fetch default settings. Additionally its presence causes the servers to infer that Chrome needs to be updated. BUG= 727829 TBR=vasilii@chromium.org Review-Url: https://codereview.chromium.org/2908073006 Cr-Original-Commit-Position: refs/heads/master@{#475849} Review-Url: https://codereview.chromium.org/2918043004 . Cr-Commit-Position: refs/branch-heads/3071@{#737} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/66ae57e1a5d8c23c6dbeebdcafbf3c9ab28305b4/chrome/browser/profile_resetter/brandcode_config_fetcher.cc
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d801058048b2af3c21e32efe9137561ead01e5ed commit d801058048b2af3c21e32efe9137561ead01e5ed Author: alito <alito@chromium.org> Date: Mon Jun 05 17:55:45 2017 Settings reset prompt: Fetch default settings only when needed. Instead of fetching the default settings when we create the SettingsResetPromptModel object, which is some time after startup, we now fetch settings only if the settings reset dialog needs to be shown to the user. BUG= 727829 Review-Url: https://codereview.chromium.org/2918053003 Cr-Commit-Position: refs/heads/master@{#477019} [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/default_settings_fetcher.h [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.h [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.cc [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model_browsertest_win.cc [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model_unittest.cc [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_test_utils.cc [modify] https://crrev.com/d801058048b2af3c21e32efe9137561ead01e5ed/chrome/browser/ui/views/settings_reset_prompt_dialog_browsertest.cc
,
Jun 7 2017
,
Jun 7 2017
Requesting a merge of the CL in #3 to M60. This is the same change that has already been merged to M59.
,
Jun 7 2017
,
Jun 7 2017
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4987f2a8293e08081063d28356b75a23eb06c813 commit 4987f2a8293e08081063d28356b75a23eb06c813 Author: Chris Sharp <csharp@chromium.org> Date: Wed Jun 07 21:14:52 2017 Remove "updatecheck" tag when fetching default settings. The "updatecheck" tag is unnecessary when we want to fetch default settings. Additionally its presence causes the servers to infer that Chrome needs to be updated. BUG= 727829 TBR=vasilii@chromium.org Review-Url: https://codereview.chromium.org/2908073006 Cr-Original-Commit-Position: refs/heads/master@{#475849} Review-Url: https://codereview.chromium.org/2924113002 . Cr-Commit-Position: refs/branch-heads/3112@{#237} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/4987f2a8293e08081063d28356b75a23eb06c813/chrome/browser/profile_resetter/brandcode_config_fetcher.cc
,
Jun 7 2017
I have tested the CL in #14 in the current Canary version 61.0.3122.0 and verified that it works as intended: I was able to enable the settings reset prompt feature, I was prompted and my settings were restored correctly. I also tested this with a non-default brand code and confirmed that my settings were reset to the appropriate value based on the brand code. Requesting a merge to M60 for that CL.
,
Jun 7 2017
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 8 2017
alito@, Could you please let us know the exact steps to test this issue from TE end. Thanks in advance..!!
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/556e3961add350236cfb50abbe394d5959ece352 commit 556e3961add350236cfb50abbe394d5959ece352 Author: Chris Sharp <csharp@chromium.org> Date: Thu Jun 08 18:00:38 2017 Settings reset prompt: Fetch default settings only when needed. Instead of fetching the default settings when we create the SettingsResetPromptModel object, which is some time after startup, we now fetch settings only if the settings reset dialog needs to be shown to the user. BUG= 727829 Review-Url: https://codereview.chromium.org/2918053003 Cr-Original-Commit-Position: refs/heads/master@{#477019} Review-Url: https://codereview.chromium.org/2926303002 . Cr-Commit-Position: refs/branch-heads/3112@{#258} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/default_settings_fetcher.h [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.h [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.cc [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model_browsertest_win.cc [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model_unittest.cc [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_test_utils.cc [modify] https://crrev.com/556e3961add350236cfb50abbe394d5959ece352/chrome/browser/ui/views/settings_reset_prompt_dialog_browsertest.cc
,
Jun 9 2017
jmukthavaram@: There is a detailed test plan available for testing the feature: https://docs.google.com/document/d/1TlXy5iQlUESW5xlkd20paVzSBSEI3BzQyaGSKgfME5Y/edit#heading=h.ywo9tflaxbap The test plan will allow you to confirm that the feature still works as expected with the changes that fixed this bug. Those tests will not, however, verify that the bug described here is fixed. Since the issue fixes an internal and Omaha server-side issue and has no user-visible changes, I'm not sure how this can be verified. |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by abdulsyed@chromium.org
, May 30 2017