Skip to content
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 based Edge to the Ruby bindings #7257

Merged
merged 4 commits into from
Jun 10, 2019
Merged

Add Chrome based Edge to the Ruby bindings #7257

merged 4 commits into from
Jun 10, 2019

Conversation

twalpole
Copy link
Contributor

@twalpole twalpole commented Jun 3, 2019

This is the start of adding Chrome based Edge to the Ruby bindings. We need to decide whether to implement this separately from legacy edge, or to accept some sort of configuration option to switch between legacy and chrome based - thoughts?

@twalpole twalpole requested review from titusfortner and p0deje June 3, 2019 20:49
@twalpole twalpole added the C-rb label Jun 3, 2019
Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Copy link
Member

@titusfortner titusfortner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is great for now.
Before we release 4.0, if Edge Chrome is production released, I think it should be what we provide with Selenium::WebDriver.for :edge and we should rename existing convenience method to :edge_legacy. It won't be backward compatible, but for a major version release, I think that's fine. We could also theoretically do some kind of handshake on Windows platforms as well, but that might be too complicated.
What do you think @p0deje ?

@twalpole
Copy link
Contributor Author

twalpole commented Jun 5, 2019

I've updated this to move the edge 18 into EdgeLegacy module, with Edge being an alias for it. Swapping from Edge/:edge being Legacy to Chrome based should be simple if/when chrome based edge goes release

@p0deje
Copy link
Member

p0deje commented Jun 6, 2019

This is fine by me. I know that Java calls legacy EdgeHtml while Python keeps old name and calls a new one ChromiumEdge (#7253). We might want to make the naming consistent when other bindings support lands, but that's for the future.

@twalpole twalpole marked this pull request as ready for review June 9, 2019 19:47
@twalpole
Copy link
Contributor Author

twalpole commented Jun 9, 2019

I swapped the legacy edge namespace to EdgeHtml since that is what the engine is called -- thanks for pointing that out @p0deje. Anyone have issues with me squashing and merging this now?

Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

end
end

# it 'should be able to run in headless mode with #headless!' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It currently crashes when trying to start headless - not sure if they plan to support a headless mode or not

@p0deje
Copy link
Member

p0deje commented Jun 10, 2019

Anyone have issues with me squashing and merging this now?

No issues from my side. Thanks for tackling this!

@twalpole twalpole changed the title Start Adding Chrome based Edge to the Ruby bindings Add Chrome based Edge to the Ruby bindings Jun 10, 2019
@twalpole twalpole merged commit 7a72023 into master Jun 10, 2019
@twalpole twalpole deleted the edge_chrome_rb branch June 14, 2019 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants