Implement Profile Reset for Proxy Settings [chromium/src : main]
Scott Violet (Gerrit)
Scott Violet added 2 comments![]( "Open in Gerrit")
// These flags are using in conjunction withusing->used
MarkAsDone(DNS_CONFIGURATIONS);What ensures dns is reset after extensions are done?
Related details
- Steven Bennetts
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
// These flags are using in conjunction withThomas Cedenousing->used
Done
MarkAsDone(DNS_CONFIGURATIONS);What ensures dns is reset after extensions are done?
Using the sequence_checker ensures that every method launched in ProfileResetter is launched in sequential order in ResetSettingsImpl, I also manually confirmed this behavior through some debug logging, technically DCHECK_CALLED_ON_VALID_SEQUENCE would fail before this check.
Related details
- Scott Violet
- Steven Bennetts
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet added 1 comment![]( "Open in Gerrit")
MarkAsDone(DNS_CONFIGURATIONS);What ensures dns is reset after extensions are done?
Using the sequence_checker ensures that every method launched in ProfileResetter is launched in sequential order in ResetSettingsImpl, I also manually confirmed this behavior through some debug logging, technically DCHECK_CALLED_ON_VALID_SEQUENCE would fail before this check.
The sequence_checker_ doesn't really impact that. What matters is that extensions are cleared first *and* extensions completes synchronously. AFAICT this is what the code does. Given these two constraints, it shouldn't be possible to hit this code. By that I mean pending_reset_flags_ should never contain EXTENSIONS by the time this function is called. Unless I'm missing something I recommend adding a dcheck to that effect.
Related details
- Steven Bennetts
- Thomas Cedeno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
MarkAsDone(DNS_CONFIGURATIONS);Thomas CedenoWhat ensures dns is reset after extensions are done?
Scott VioletUsing the sequence_checker ensures that every method launched in ProfileResetter is launched in sequential order in ResetSettingsImpl, I also manually confirmed this behavior through some debug logging, technically DCHECK_CALLED_ON_VALID_SEQUENCE would fail before this check.
The sequence_checker_ doesn't really impact that. What matters is that extensions are cleared first *and* extensions completes synchronously. AFAICT this is what the code does. Given these two constraints, it shouldn't be possible to hit this code. By that I mean pending_reset_flags_ should never contain EXTENSIONS by the time this function is called. Unless I'm missing something I recommend adding a dcheck to that effect.
IIUC CHECK is preferred over DCHECK; change is reflected otherwise
Related details
- Scott Violet
- Steven Bennetts
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet added 1 comment![]( "Open in Gerrit")
MarkAsDone(DNS_CONFIGURATIONS);Thomas CedenoWhat ensures dns is reset after extensions are done?
Scott VioletUsing the sequence_checker ensures that every method launched in ProfileResetter is launched in sequential order in ResetSettingsImpl, I also manually confirmed this behavior through some debug logging, technically DCHECK_CALLED_ON_VALID_SEQUENCE would fail before this check.
Thomas CedenoThe sequence_checker_ doesn't really impact that. What matters is that extensions are cleared first *and* extensions completes synchronously. AFAICT this is what the code does. Given these two constraints, it shouldn't be possible to hit this code. By that I mean pending_reset_flags_ should never contain EXTENSIONS by the time this function is called. Unless I'm missing something I recommend adding a dcheck to that effect.
IIUC CHECK is preferred over DCHECK; change is reflected otherwise
Sorry if I wasn't clear. Based on my second comment in this thread I think lines 450-453 should change to:
```
// DNS must be reset after extensions. Extensions are reset first, so by
// the time we get here we should not be waiting on extensions.
CHECK(!(pending_reset_flags_ & EXTENSIONS));
```
Related details
- Steven Bennetts
- Thomas Cedeno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +1 |
1 comment
MarkAsDone(DNS_CONFIGURATIONS);Thomas CedenoWhat ensures dns is reset after extensions are done?
Scott VioletUsing the sequence_checker ensures that every method launched in ProfileResetter is launched in sequential order in ResetSettingsImpl, I also manually confirmed this behavior through some debug logging, technically DCHECK_CALLED_ON_VALID_SEQUENCE would fail before this check.
Thomas CedenoThe sequence_checker_ doesn't really impact that. What matters is that extensions are cleared first *and* extensions completes synchronously. AFAICT this is what the code does. Given these two constraints, it shouldn't be possible to hit this code. By that I mean pending_reset_flags_ should never contain EXTENSIONS by the time this function is called. Unless I'm missing something I recommend adding a dcheck to that effect.
Scott VioletIIUC CHECK is preferred over DCHECK; change is reflected otherwise
Sorry if I wasn't clear. Based on my second comment in this thread I think lines 450-453 should change to:
```
// DNS must be reset after extensions. Extensions are reset first, so by
// the time we get here we should not be waiting on extensions.
CHECK(!(pending_reset_flags_ & EXTENSIONS));
```
- Scott Violet
- Steven Bennetts
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet voted Code-Review+1![]( "Open in Gerrit")
- Steven Bennetts
- Thomas Cedeno
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Behnood Momenzadeh (Gerrit)
Behnood Momenzadeh added 1 comment![]( "Open in Gerrit")
} flagToMethod[] = {Just a quick nit, now that we rely on the order of the methods here for the dns and proxy resets, leave a comment here as well, so no one accidentally changes the order of the methods here.
Related details
- Steven Bennetts
- Thomas Cedeno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +1 |
1 comment
Just a quick nit, now that we rely on the order of the methods here for the dns and proxy resets, leave a comment here as well, so no one accidentally changes the order of the methods here.
- Behnood Momenzadeh
- Kyle Horimoto
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Horimoto (Gerrit)
Kyle Horimoto added 1 comment![]( "Open in Gerrit")
- Andreea Costinas
- Behnood Momenzadeh
- Thomas Cedeno
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreea Costinas (Gerrit)
Andreea Costinas added 2 comments![]( "Open in Gerrit")
Hi Thomas,
The approach LGTM but would it be possible to add a browser test that sets the proxy "from the settings UI" (like ProxyConfigServiceImplTest.NetworkProxy), run PlatfromSanitizeSettings and then check that the proxy is ignored?
"performSanitizeSettings",Sorry if the question is silly, but why remove the callback registration? Isn't this callback still being called from os_sanitize_dialog.ts?
Related details
- Behnood Momenzadeh
- Thomas Cedeno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +1 |
1 comment
"performSanitizeSettings",Sorry if the question is silly, but why remove the callback registration? Isn't this callback still being called from os_sanitize_dialog.ts?
This function was re-implemented at the request of an external team, this code removal is just come proactive clean-up on the side: https://chromium-review.googlesource.com/c/chromium/src/+/5649400/27/chrome/browser/ash/sanitize/chrome_sanitize_ui_delegate.cc.
Related details
- Andreea Costinas
- Behnood Momenzadeh
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +0 |
1 comment
content::EvalJsResult result =
EvalJs(app_browser->tab_strip_model()->GetActiveWebContents(),
"onPerformSanitize");@acos...@google.com, this is the "non-working" part I'm trying to figure out; after the Sanitize SWA opens it only presents one button that performs this Javascript function (https://source.corp.google.com/h/chrome-internal/codesearch/chrome/src/+/main:ash/webui/sanitize_ui/resources/sanitize_initial.ts;l=52?q=onPerformSanitize&ss=h%2Fchrome-internal%2Fcodesearch%2Fchrome%2Fsrc%2F%2B%2Frefs%2Fheads%2Fmain) in its call stack to initially begin the process. What is the best practice for executing this code for testing?
Related details
- Andreea Costinas
- Behnood Momenzadeh
- Scott Violet
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno added 1 comment![]( "Open in Gerrit")
content::EvalJsResult result =
EvalJs(app_browser->tab_strip_model()->GetActiveWebContents(),
"onPerformSanitize");@acos...@google.com, this is the "non-working" part I'm trying to figure out; after the Sanitize SWA opens it only presents one button that performs this Javascript function (https://source.corp.google.com/h/chrome-internal/codesearch/chrome/src/+/main:ash/webui/sanitize_ui/resources/sanitize_initial.ts;l=52?q=onPerformSanitize&ss=h%2Fchrome-internal%2Fcodesearch%2Fchrome%2Fsrc%2F%2B%2Frefs%2Fheads%2Fmain) in its call stack to initially begin the process. What is the best practice for executing this code for testing?
Done
Related details
- Andreea Costinas
- Behnood Momenzadeh
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Andreea Costinas
- Behnood Momenzadeh
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Andreea Costinas
- Behnood Momenzadeh
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Andreea Costinas
- Behnood Momenzadeh
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreea Costinas (Gerrit)
Andreea Costinas voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andreea Costinas
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Behnood Momenzadeh
- Thomas Cedeno
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted Commit-Queue+1![]( "Open in Gerrit")
- Andreea Costinas
- Behnood Momenzadeh
- Kyle Horimoto
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Horimoto (Gerrit)
Kyle Horimoto added 2 comments![]( "Open in Gerrit")
"Allow proxies for shared networks" in chrome://settings and it's`its`
if (restart_chrome_) {
chrome::AttemptRestart();
}
}Is there any way that we can get the test to set some test global in `chrome::AttemptRestart()`? This CL adds a lot of extra boilerplate in several places, which adds a lot of complexity for a test-only feature.
Related details
- Andreea Costinas
- Behnood Momenzadeh
- Thomas Cedeno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
"Allow proxies for shared networks" in chrome://settings and it'sThomas Cedeno`its`
Done
Is there any way that we can get the test to set some test global in `chrome::AttemptRestart()`? This CL adds a lot of extra boilerplate in several places, which adds a lot of complexity for a test-only feature.
To be honest, the answer is mostly no, there is no global within Chrome::AttemptRestart. Other browser tests who have similar considerations (https://source.corp.google.com/search?q=file:browsertest%20AttemptRestart&ss=h%2Fchromium%2Fchromium%2Fsrc%2F%2B%2Frefs%2Fheads%2Fmain) mainly inject chrome::AttemptRestart as a functional closure during testing. I'll modify the CL since this is slightly cleaner than using bools for control.
Related details
- Andreea Costinas
- Behnood Momenzadeh
- Kyle Horimoto
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Andreea Costinas
- Behnood Momenzadeh
- Kyle Horimoto
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Violet
| Code-Review | +1 |
1 comment
const GURL urls[] = {GURL("http://foo"), GURL("http://bar")};Can you share these constants with line 78? Perhaps a function to return a vector of urls?
Related details
- Andreea Costinas
- Kyle Horimoto
- Thomas Cedeno
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Cedeno
| Auto-Submit | +1 |
1 comment
const GURL urls[] = {GURL("http://foo"), GURL("http://bar")};Can you share these constants with line 78? Perhaps a function to return a vector of urls?
I've collated the strings as constexpr - the raw values are used beyond GURL() constructors.
Related details
- Andreea Costinas
- Kyle Horimoto
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreea Costinas (Gerrit)
Andreea Costinas voted Code-Review+1![]( "Open in Gerrit")
- Kyle Horimoto
- Thomas Cedeno
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Cedeno (Gerrit)
Thomas Cedeno voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Kyle Horimoto
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Implement Profile Reset for Proxy Settings
As part of Back to Safety, implements a reset of proxy settings
of the profile tied to the default network, changes
"Allow proxies for shared networks" in chrome://settings and its
associated Shill settings.
- M ash/webui/sanitize_ui/sanitize_ui.cc
- M ash/webui/sanitize_ui/sanitize_ui.h
- M ash/webui/sanitize_ui/sanitize_ui_delegate.h
- M chrome/browser/ash/sanitize/chrome_sanitize_ui_delegate.cc
- M chrome/browser/ash/sanitize/chrome_sanitize_ui_delegate.h
- M chrome/browser/ash/sanitize/chrome_sanitize_ui_delegate_unittest.cc
- M chrome/browser/profile_resetter/profile_resetter.cc
- M chrome/browser/profile_resetter/profile_resetter.h
- M chrome/browser/ui/webui/settings/reset_settings_handler.cc
- M chrome/browser/ui/webui/settings/reset_settings_handler.h
- M chrome/test/data/webui/BUILD.gn
- A chrome/test/data/webui/chromeos/sanitize_ui/DEPS
- M chrome/test/data/webui/chromeos/sanitize_ui/sanitize_ui_browsertest.cc
- M chrome/test/data/webui/chromeos/sanitize_ui/sanitize_ui_test.ts
Code-Review: +1 by Scott Violet, +1 by Andreea Costinas
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |