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

Use proc macro to only check against applicable event handler content attributes. #25760

Open
Manishearth opened this issue Feb 14, 2020 · 4 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Feb 14, 2020

See #25274 for the first attempt

Currently we compile any content attribute that starts with "on" as an event handler.

What we should do is still check for "on", but then only compile it as a handler if it belongs to the list of event handlers allowed on the element (see global_event_handlers and window_event_handlers)

A macro based approach was tried, but it slowed things down (see #25274 ). We should instead use a custom proc macro to generate the match statement, or perhaps a phf_map

cc @jdm

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Feb 14, 2020

Are you sure 25274 was slow? I haven't given it a close inspection, but as I said there, a lot of these event tests will report TIMEOUT if the callback with step_func_done() in it never fires.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Feb 14, 2020

I think the lazy_static caused an initial expensive setup, but also I'd be surprised if there was a behavior change that caused those timeouts. Feel free to investigate if you'd like.

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Feb 14, 2020

I looked into it (it took me a little while to get the old branch built). ELEMENT_EVENT_HANDLERS contained atoms like "blur" and "click" without the "on". You had elements looking up the full attribute name including the "on", and thus never found a match. I didn't observe any performance problem distinguishable from the usual slight sluggishness of Servo on my VM.

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Feb 15, 2020

Something else went wrong elsewhere in my attempts to use #25274 (and I've closed #25249), but the only code changes needed to get it mostly working were to have the make_array macro use concat!("on", ...) and to put "on" in the elements of in the handwritten array of ["cut", "copy", "paste"]. I still hit a problem where an "onx" content attribute of a document body seemed to be firing for a dispatched "x" event, but it wasn't the macro part that was at fault.

@jdm jdm added this to To do in web-platform-test failures via automation Feb 15, 2020
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.

None yet
2 participants
You can’t perform that action at this time.