dom: IntersectionObserver initialization#35314
Conversation
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
stevennovaryo
force-pushed
the
intersection-observer
branch
from
9760f71 to
3a60ad5
Compare
Signed-off-by: stevennovaryo <[email protected]>
|
I think it is quite ready for review. I am open to expand this PR's scope. I will make |
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
This was straightforward to review! Thank you!
| use crate::dom::bindings::str::DOMString; | ||
| use crate::dom::window::Window; | ||
| use crate::script_runtime::{CanGc, JSContext}; | ||
|
|
| callback: Rc<IntersectionObserverCallback>, | ||
|
|
||
| /// <https://w3c.github.io/IntersectionObserver/#dom-intersectionobserver-queuedentries-slot> | ||
| queued_entries: DomRefCell<Vec<DomRoot<IntersectionObserverEntry>>>, |
There was a problem hiding this comment.
Can we make this a Dom instead of a DomRoot?
There was a problem hiding this comment.
Sure thanks, it should make GC works more seamlessly.
| /// <https://w3c.github.io/IntersectionObserver/#intersectionobserverregistration> | ||
| #[derive(JSTraceable, MallocSizeOf)] | ||
| pub(crate) struct IntersectionObserverRegistration { | ||
| observer: DomRoot<IntersectionObserver>, |
There was a problem hiding this comment.
Can we make this a Dom and add #[cfg_attr(crown, crown::unrooted_must_root_lint::must_root] to the struct?
| /// <https://w3c.github.io/IntersectionObserver/#dom-intersectionobserver-rootmargin-slot> | ||
| #[no_trace] | ||
| #[ignore_malloc_size_of = "Defined in style"] | ||
| root_margin: DomRefCell<IntersectionObserverRootMargin>, |
There was a problem hiding this comment.
Out of curiousity, can this and scroll_margin use Cell instead?
There was a problem hiding this comment.
The current object it is compatible, as it does not implement Copy trait. Previously, I just changed it into RefCell, but I think it could use Cell<T> as well, because the underlying type is quite basic.
| }; | ||
|
|
||
| // > 6. If any value in thresholds is less than 0.0 or greater than 1.0, throw a RangeError exception. | ||
| threshold.iter().try_for_each(|num| { |
There was a problem hiding this comment.
I think this would be easier to read as a for loop:
for num in &threshold {
if **num < 0.0 || **num > 1.0 {
return Err(Error::Range("Value in thresholds should not be less than 0.0 or greater than 1.0".to_owned()));
}
}| Self::AbsolutePixelLength(0.0) | ||
| // > 11. If options.trackVisibility is true and delay is less than 100, set delay to 100. | ||
| if init.trackVisibility { | ||
| delay = delay.min(100); |
There was a problem hiding this comment.
I believe we need delay.max(100) here, otherwise we'll take the lower value.
There was a problem hiding this comment.
Yes, thanks! My bad for the silly mistake.
| match parser.parse_entirely(|p| IntersectionObserverRootMargin::parse(&context, p)) { | ||
| Ok(margin) => Ok(margin), | ||
| _ => Err(()), | ||
| } |
There was a problem hiding this comment.
parser.parse_entirely(|p| IntersectionObserverRootMargin::parse(&context, p))
.map_err(|_| ())|
|
||
| /// > The root provided to the IntersectionObserver constructor, or null if none was provided. | ||
| /// <https://w3c.github.io/IntersectionObserver/#dom-intersectionobserver-root> | ||
| #[ignore_malloc_size_of = "mozjs"] |
There was a problem hiding this comment.
We can remove this by adding an entry for the ElementOrDocument union to https://github.com/servo/servo/blob/main/components/script_bindings/codegen/Bindings.conf#L607 that derives MallocSizeOf.
There was a problem hiding this comment.
Sure, that would make the implementation much more clean. Thanks!
| } | ||
| } | ||
|
|
||
| impl Clone for ElementOrDocument { |
There was a problem hiding this comment.
Signed-off-by: stevennovaryo <[email protected]>
Signed-off-by: stevennovaryo <[email protected]>
2 participants
Depends on Stylo's Bug 1946679 - IntersectionObserverRootMargin.
Implement initialization and interfaces of IntersectionObserver following the #intersection-observer-interface and #intersection-observer-processing-model. Specifically, the processing model implementation includes:
The remaining implementation of IntersectionObserver will be more related to the Event Loop #update-the-rendering (see step 19). Which will be my next work.
Feedback are appreciated about #lifefime, fields of the objects, and overall rust codes.
cc: @xiaochengh
Try
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors