-
Notifications
You must be signed in to change notification settings - Fork 1.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
browser: Fix options races p5 #4533
Conversation
06767b8
to
520cfef
Compare
ea76798
to
e92ec0e
Compare
520cfef
to
67e4019
Compare
@@ -1478,16 +1474,9 @@ func (f *Frame) selectOption(selector string, values sobek.Value, opts *FrameSel | |||
} | |||
|
|||
// SetContent replaces the entire HTML document content. | |||
func (f *Frame) SetContent(html string, opts sobek.Value) error { | |||
func (f *Frame) SetContent(html string, _ *FrameSetContentOptions) error { |
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.
Sounds like here (and maybe upstream) we can clean up the opts completely? 🤔
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.
I don't know, for this reason, I would prefer if we defer this to the next release. In my opinion, it's better to coordinate with @inancgumus on similar refactors.
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.
@inancgumus do you have any plan in mind for it? Can we remove it or do we expect to add features where it is needed?
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.
It's better to keep it and add a TODO comment instead because here, we (the early-browser team) forgot to respect the options.
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.
@inancgumus Unfortunately, I don't have the knowledge to add a meaningful TODO. Can you add one, please? Happy to review the pull request.
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.
What?
Continue #4524