script: Generate custom fields also, for interfaces inheriting from PerformanceEntry, from impl_performance_entry_struct!#44289
Conversation
servo-highfive
added
the
S-awaiting-review
shubhamg13
changed the title
impl_performance_entry_struct! impl_performance_entry_struct!
|
|
||
| [Exposed=(Window,Worker)] | ||
| interface PerformanceMeasure : PerformanceEntry { | ||
| readonly attribute any detail; |
There was a problem hiding this comment.
Right now, we are passing default value.
Will keep separate PR for serialization logic of same. And we have separate WPT tests to verify that too.
shubhamg13
changed the title
impl_performance_entry_struct!impl_performance_entry_struct!
xiaochengh
left a comment
xiaochengh
left a comment
There was a problem hiding this comment.
Looks OK but I got a question.
| /// DOM struct implementation for simple interfaces inheriting from PerformanceEntry. | ||
| macro_rules! impl_performance_entry_struct( | ||
| ($binding:ident, $struct:ident, $type:path) => ( | ||
| ($binding:ident, $struct:ident, $type:path, { $($field_name:ident : $field_type:ty),* $(,)? }) => ( |
There was a problem hiding this comment.
What's the consideration for leaving a general custom field here instead of just detail?
Are there more custom fields expected here?
There was a problem hiding this comment.
Yes, There are quite a list of interfaces inheriting from PerformanceEntry. Like LargestContentfulPaint, PerformanceLongAnimationFrameTiming e.t.c. This can be very useful.
Very few Implemented in Servo yet. Probably port existing ones to use this macro sooner.
There was a problem hiding this comment.
I would prefer to go the other way and remove this macro. It already is confusing because the implementations look different than every other DOM interface, and I think extending the macro to support additional fields does not improve the situation.
There was a problem hiding this comment.
I don't have a strong opinion on this but starting with something straightforward usually doesn't hurt.
shubhamg13
changed the title
impl_performance_entry_struct!impl_performance_entry_struct!
shubhamg13
changed the title
impl_performance_entry_struct!PerformanceEntry, from impl_performance_entry_struct!
| EntryType::Measure | ||
| EntryType::Measure, | ||
| { | ||
| detail: RootedTraceableBox<Heap<JSVal>>, |
There was a problem hiding this comment.
We should only use Heap<JSVal> here; the RootedTraceableBox in a member like this can lead to unbreakable GC cycles.
There was a problem hiding this comment.
Updated the attribute.
Signed-off-by: Shubham Gupta <[email protected]>
Signed-off-by: Shubham Gupta <[email protected]>
shubhamg13
force-pushed
the
impl_performance_entry_struct
branch
from
b1b10ff to
b0d6942
Compare
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
4 participants
Add definition for generation of custom fields for interfaces inheriting from PerformanceEntry.
Testing: More WPT Tests Passed