-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add chrome flags used in chromedriver/puppeteer #23
Conversation
I also found Not sure about it yet though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice finds
@@ -25,4 +26,20 @@ export const DEFAULT_FLAGS = [ | |||
'--mute-audio', | |||
// Skip first run wizards | |||
'--no-first-run', | |||
|
|||
// Disable timers being throttled in background pages/tabs | |||
'--disable-background-timer-throttling', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this have any benefit for us? It seems like it could have an unrealistic effect on performance in certain corner cases of sites and it may be unnecessary for Lighthouse due to always being the foreground tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary for Lighthouse due to always being the foreground tab
yes seems correct.
however since other projects use chrome-launcher... this would affect anyone attempting to use a pool of tabs to do a bunch of work.
'--disable-popup-blocking', | ||
// Reloading a page that came from a POST normally prompts the user. | ||
'--disable-prompt-on-repost', | ||
// Disable the a few things considered not appropriate for automation, e.g. infobar animations, password saving UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the a few/a few/
// Reloading a page that came from a POST normally prompts the user. | ||
'--disable-prompt-on-repost', | ||
// Disable the a few things considered not appropriate for automation, e.g. infobar animations, password saving UI | ||
// https://docs.google.com/a/google.com/document/d/1JYj9K61UyxIYavR8_HATYIglR9T_rDwAtLLsD3fbDQg/preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this link doesn't actually reference that flag, do we have something that explains what it does? or is --enable-automation
the same as --enable-chromedriver
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah in short that design doc turned into --enable-automation during code review but the doc wasn't updated.
CL here: https://codereview.chromium.org/2564973002 where pavel recommended the flag be generalized. :)
My only comment is to call it in a generic manner: --enable-automation, AutomationInfobar, etc. It is likely that Lighthouse would also like to use it the same way ChromeDriver does.
from what i can tell...
- it disables the password saving UI
- it disables dev mode extension bubbles (?), and doesn't show some other info bars
- it means the default browser check prompt isn't shown
- it avoids showing these 3 infobars: ShowBadFlagsPrompt, GoogleApiKeysInfoBarDelegate, ObsoleteSystemInfoBarDelegate
- it adds this infobar:
I went over the differences between Puppeteer and the new chrome-launcher flags. The only differences are |
happy to address the nit comment from @patrickhulce if everyone is happy to land this? |
I think we need to have the flag disabling solution (#65) in place before landing this guy since there's a few ones in here some consumers of chrome-launcher may not want set by default. |
sources:
puppeteer/puppeteer@4af0911
https://cs.chromium.org/chromium/src/chrome/test/chromedriver/chrome_launcher.cc?type=cs&q=f:chrome_launcher++kDesktopSwitches&sq=package:chromium&l=72