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

Implement MessageEvent.initMessageEvent #25192

Closed
pshaughn opened this issue Dec 6, 2019 · 13 comments
Closed

Implement MessageEvent.initMessageEvent #25192

pshaughn opened this issue Dec 6, 2019 · 13 comments

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 6, 2019

This is expected by WPT html/webappapis/scripting/events/messageevent-constructor.https.html and has IDL in https://html.spec.whatwg.org/multipage/comms.html#the-messageevent-interface

@CYBAI CYBAI added the A-content/dom label Dec 7, 2019
@jdm jdm added this to To do in web-platform-test failures via automation Dec 9, 2019
@jdm jdm added the E-easy label Dec 9, 2019
@highfive
Copy link

@highfive highfive commented Dec 9, 2019

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@jdm
Copy link
Member

@jdm jdm commented Dec 9, 2019

Code: components/script/dom/webidls/MessageEvent.webidl, components/script/dom/messageevent.rs
Test: ./mach test-wpt tests/wpt/web-platform-tests/html/webappapis/scripting/events/messageevent-constructor.https.html

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Dec 9, 2019

I would like to try and solve this one.
@highfive: assign me

@highfive
Copy link

@highfive highfive commented Dec 9, 2019

Hey @kunalmohan! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Dec 9, 2019
@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Dec 11, 2019

I am having trouble initializing variables which are DOMString type. How should I get past it?

@jdm
Copy link
Member

@jdm jdm commented Dec 11, 2019

@kunalmohan Are you using DOMString::new() for an empty string and DOMString::from(...) to create a DOMString from an existing Rust string value?

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Dec 11, 2019

I have defined the function as

fn InitMessageEvent(
        &self,
        _cx: JSContext,
        type_: DOMString,
        bubbles: bool,
        cancelable: bool,
        data: HandleValue,
        origin: DOMString,
        lastEventId: DOMString,
        source: Option<WindowProxyOrMessagePortOrServiceWorker>,
        ports: Vec<DomRoot<MessagePort>>
    )

I am not taking origin and lastEventId as string because then I get the following error:

method `InitMessageEvent` has an incompatible type for trait
@jdm
Copy link
Member

@jdm jdm commented Dec 11, 2019

What is the trouble you are having with initialization, in that case?

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Dec 11, 2019

Actually it's not just the DOMString.
First, I cannot initialize a key like self.[key]=value because self is not mutable reference. But initializing it using *self.[key]=value gives the error doesn't have size known at compile time for lastEventId, ports and origin.
Second, I cannot figure out why the statements *self.origin=origin (and same for lastEventId) are giving the error

expected `str`, found struct `dom::bindings::str::DOMString`

for the values obtained as parameters.

@jdm
Copy link
Member

@jdm jdm commented Dec 11, 2019

Ah. You will need to put the members that need to be reinitialized inside DOMRefCell or Cell values. These allow mutating the values inside of methods that take &self instead of &mut self, like all of our DOM APIs. As for the str vs. DOMString error, that also surprises me. See if it goes away when you use the appropriate DOMRefCell/Cell wrappers?

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Dec 11, 2019

Okay, I'll use the DOMRefCell/Cell wrappers. Would there be a need to edit other functions for the MessageEvent struct after implementing the wrappers?

@jdm
Copy link
Member

@jdm jdm commented Dec 11, 2019

Any method that uses the members that are now wrapped in cells will need to be adjusted in order to compile, yes.

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Dec 11, 2019

That was quite helpful. Thanks :)
I'll get back to you about the str vs DOMString error.

@kunalmohan kunalmohan mentioned this issue Dec 13, 2019
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Dec 17, 2019
Implement MessageEvent.InitMessageEvent

<!-- Please describe your changes on the following line: -->
InitMessageEvent had to be implemented as required by wpt. For this few keys of struct `MessageEvent` are now wrapped inside DomRefCell wrapper.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [x] These changes fix #25192  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 18, 2019
Implement MessageEvent.InitMessageEvent

<!-- Please describe your changes on the following line: -->
InitMessageEvent had to be implemented as required by wpt. For this few keys of struct `MessageEvent` are now wrapped inside DomRefCell wrapper.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [x] These changes fix #25192  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 18, 2019
Implement MessageEvent.InitMessageEvent

<!-- Please describe your changes on the following line: -->
InitMessageEvent had to be implemented as required by wpt. For this few keys of struct `MessageEvent` are now wrapped inside DomRefCell wrapper.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [x] These changes fix #25192  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 18, 2019
Implement MessageEvent.InitMessageEvent

<!-- Please describe your changes on the following line: -->
InitMessageEvent had to be implemented as required by wpt. For this few keys of struct `MessageEvent` are now wrapped inside DomRefCell wrapper.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [x] These changes fix #25192  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
web-platform-test failures automation moved this from To do to Done Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.