Add chrome launching code to browser_launcher #4
Conversation
lib/src/chrome.dart
Outdated
| static Future<Chrome> startWithPort( | ||
| List<String> urls, { | ||
| String userDataDir, | ||
| int remoteDebuggingPort, |
There was a problem hiding this comment.
I don't think we want to be in the business of keeping track of all Chrome configuration. There are a ton of possible flags. We should probably make this more generic and just use an iterable of strings.
There was a problem hiding this comment.
I made these named params per Nate's comment on the proposal doc:
"if there are common args that multiple usages need to have knowledge of it would be cool to have named args or something here."
@natebosch am I misunderstanding your request? Or do you think there are some that should be named and some that should not?
There was a problem hiding this comment.
Changed it to a list of strings. If we want to make some named in the future, we can do that as needed.
There was a problem hiding this comment.
If there are args that multiple clients need to know about I'd be in favor of encoding knowledge here. @grouma what is your concern? Why do you prefer to duplicate this knowledge in other clients?
There was a problem hiding this comment.
My concerns don't need to be blocking - as you mention we can always add new named args in the future.
There was a problem hiding this comment.
Nate and I discussed offline.
My concern was having a significant list of unused optional arguments and trying to keep that in sync with the available Chrome flags. Right now we have three potential clients of this package, package:test, package:webdev and DevTools. From what I can tell the only arguments that are really configured / changed are --headless (which includes others by default) and --remote-debugging-port.
We agreed that the best approach is to provide optional arguments for those two options only. We will still pass the other options and just document their use. If and when other clients need to configure more options we can easily add another optional argument in a non-breaking way.
Ignore my suggestion of providing a mechanism to add arbitrary arguments. We can revisit that if necessary but it won't be likely given the low number of users for this package.
Sorry for the churn!
There was a problem hiding this comment.
A general meta-comment but i wonder if the whole singleton instance stuff makes sense generally or not?
If multiple packages want to launch chrome it will end up just failing I think, and any package can implement their own canonical instance logic on their own?
|
removed the completer logic requiring there to be only once connected instance of Chrome at a time. Discussed offline with Jake. It makes more sense for Webdev to implement something on top of the browser_launcher code if this is necessary. |
…auncher#4) * Add chrome launching code to browser_launcher
Pulled code from https://github.com/dart-lang/webdev/blob/master/webdev/lib/src/serve/chrome.dart with some slight modifications like named parameters and a method to launch chrome without a debug port
Pulled test code from https://github.com/dart-lang/webdev/blob/master/webdev/test/serve/chrome_test.dart