script: Readily accept all event names as Atoms#43066
Conversation
Accepting the name of the event as a `DOMString` means that when events are constructed from within Servo (for instance via input event handling), the static strings are converted to an owned `String` wrapped in a `DOMString` and then finally converted to an `Atom` in `Event`. This makes it so that all non-generated `Event` constructors accept `Atom`, which can avoid a conversion entirely when the atom already exists on the Stylo atoms list and eliminate one in-memory copy in the case that it isn't on the atom list. Eventually, all event names can be on the atom list and we can eliminate all copies. This is a tiny optimization, but also makes the code much friendlier everywhere. Signed-off-by: Martin Robinson <[email protected]>
servo-highfive
added
the
S-awaiting-review
sagudev
left a comment
sagudev
left a comment
There was a problem hiding this comment.
LGTM, tho we should have more consistent style for constructing atoms.
| FocusEventType::Blur => (DOMString::from("blur"), EventBubbles::DoesNotBubble), | ||
| FocusEventType::Focus => ("focus".into(), EventBubbles::DoesNotBubble), | ||
| FocusEventType::Blur => ("blur".into(), EventBubbles::DoesNotBubble), | ||
| }; |
There was a problem hiding this comment.
I think the issue is that "blur" is not on the list of atoms in Stylo, so atom! will fail as it resolves the static string at compile-time. I think that the solution for this is probably to create servo-atoms and look for all of the places that we convert static strings to atoms and add them to the servo-atoms list.
There was a problem hiding this comment.
For future reference, discussion is happening here: https://servo.zulipchat.com/#narrow/channel/263398-general/topic/Create.20.60servo-atoms.60.3F
|
|
||
| // Send pointermove event before mousemove. | ||
| let pointer_event = mouse_event.to_pointer_event("pointermove", can_gc); | ||
| let pointer_event = mouse_event.to_pointer_event(Atom::from("pointermove"), can_gc); |
There was a problem hiding this comment.
AFAIK atom! is preferred?
| let pointer_event = mouse_event.to_pointer_event(Atom::from("pointermove"), can_gc); | |
| let pointer_event = mouse_event.to_pointer_event(atom!("pointermove"), can_gc); |
| Some(&self.window), // view | ||
| 0, // detail | ||
| &self.window, // window | ||
| "contextmenu".into(), // type |
There was a problem hiding this comment.
| "contextmenu".into(), // type | |
| atom!("contextmenu"), // type |
| let dom_event = WheelEvent::new( | ||
| &self.window, | ||
| DOMString::from(wheel_event_type_string), | ||
| "wheel".into(), |
There was a problem hiding this comment.
| "wheel".into(), | |
| atom!("wheel"), |
| PI / 2.0, // altitude_angle (perpendicular to surface) | ||
| 0.0, // azimuth_angle | ||
| DOMString::from("touch"), | ||
| "touch".into(), |
There was a problem hiding this comment.
| "touch".into(), | |
| atom!("touch"), |
servo-highfive
removed
the
S-awaiting-review
|
I've generally tried to use |
|
@sagudev Thank you for the review. I'll go ahead and land this as I think I cannot apply your suggestions. If there is any remaining issue I am happy to fix it in a followup. |
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
3 participants
Accepting the name of the event as a
DOMStringmeans that when eventsare constructed from within Servo (for instance via input event
handling), the static strings are converted to an owned
Stringwrapped in a
DOMStringand then finally converted to anAtominEvent. This makes it so that all non-generatedEventconstructorsaccept
Atom, which can avoid a conversion entirely when the atomalready exists on the Stylo atoms list and eliminate one in-memory copy
in the case that it isn't on the atom list.
Eventually, all event names can be on the atom list and we can eliminate
all copies. This is a tiny optimization, but also makes the code much
friendlier everywhere.
Testing: This should not change behavior so should be covered by existing test.