Add "--proxy-bypass-list=<-loopback>" flag to default chrome args #3049
Conversation
- Since this is supported switch in Electron https://electronjs.org/docs/api/chrome-command-line-switches#--proxy-byp ass-listhosts
…2+ of chromium - Wrote chrome test
There was a problem hiding this comment.
@bahmutov do we have an issue for tracking installing newer versions of Chrome in CI? we need to test on multiple versions when things like this are introduced. the unit test here isn't really good enough, we need to actually run some smoke tests on various versions of chrome to ensure that the arguments do what they're supposed to.
it's debatable but want to make sure this isn't lost on our radar
|
Yes we have made an issue for this, cannot find it from the phone
…Sent from my iPhone
On Jan 19, 2019, at 18:20, Brian Mann ***@***.***> wrote:
@brian-mann requested changes on this pull request.
@bahmutov do we have an issue for tracking installing newer versions of Chrome in CI? we need to test on multiple versions when things like this are introduced. the unit test here isn't really good enough, we need to actually run some smoke tests on various versions of chrome to ensure that the arguments do what they're supposed to.
it's debatable but want to make sure this isn't lost on our radar
In packages/server/test/unit/browsers/chrome_spec.coffee:
> @@ -139,3 +139,24 @@ describe "lib/browsers/chrome", ->
disabledRootLayerScrolling("66", true)
disabledRootLayerScrolling("67", true)
disabledRootLayerScrolling("68", false)
+
+ ## #1872
+ it "adds <-loopback> proxy bypass rule in version 72+", ->
+ arg = "--proxy-bypass-list=<-loopback>"
+
+ disabledRootLayerScrolling = (version, bool) ->
wrong function name
In packages/server/lib/environment.coffee:
> @@ -19,6 +19,10 @@ try
app = require("electron").app
app.commandLine.appendSwitch("disable-renderer-backgrounding", true)
app.commandLine.appendSwitch("ignore-certificate-errors", true)
+ ## this should really only be necessary when
+ ## running Chromium versions >= 72
+ ## #1872
+ app.commandLine.appendSwitch("proxy-bypass-list", "<-loopback>")
this definitely does not need to be here.
we already handle proxy rules during BrowserWindow creation.
the only top level arguments that have to be passed to electron are ones that cannot be passed at a later time during BrowserWindow construction.
In packages/server/lib/browsers/chrome.coffee:
> @@ -147,6 +148,11 @@ module.exports = {
if majorVersion in CHROME_VERSIONS_WITH_BUGGY_ROOT_LAYER_SCROLLING
args.push("--disable-blink-features=RootLayerScrolling")
+ ## https://chromium.googlesource.com/chromium/src/+/da790f920bbc169a6805a4fb83b4c2ab09532d91
+ ## #1872
+ if majorVersion >= MIN_CHROME_VERSION_WITH_LOOPBACK_PROXY_BYPASS_RULE
would rename this to be CHROME_VERSION_INTRODUCING_PROXY_BYPASS_ON_LOOPBACK
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Issue for multiple chrome version tests: #3096 I can't commit my changes due to |
# Conflicts: # packages/server/lib/util/ci_provider.js
|
@bahmutov can you help @jennifer-shehane out with the |
|
@jennifer-shehane @brian-mann What can be done to help move this PR along? We have docker containers that are starting to roll chrome 72 and it breaks automation. Right now, we are pinning or telling folks to use electron, but if we don't have to it would be nice 😄 |
|
Hey @Aghassi, we're working on prioritizing this release at the moment. |
We may want to be more specific about only applying this to Chrome version >=
72.0.3605.0although I haven’t seen issues with having this flag added to lower Chrome versions (like 70) or older versions (like 73).