-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
fix: Introduce LocationProxy for BrowserWindowProxy #15019
Conversation
@Anrock did you test this locally? i'm seeing 10 test failures for most builds that all look related |
@codebytere no, i didn't - once again However i'm interested in top-level review if this approach is viable and if it's a welcomed change in regards of electron architecture.. |
Removing WIP: works for me, usual 4-5 test fails as on master. |
Is there relevant unit tests for location object i should change? |
@codebytere ping |
https://circleci.com/gh/electron/electron/82004
Can you please fix them? |
@alexeykuzmin i'll take a look at it as soon as i can. Don't have much free time for it, unfortunately. |
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 looks good to me, I have fixed the failing tests by adding a setter for window.location
.
Release Notes Persisted
|
Fixes #15017
Description of Change
Implements proxy Location object for BrowserWindowProxy
Checklist
npm test
passesRelease Notes
Notes: Implement proper Location object for BrowserWindowProxy