Bug 163534 – Implement DOMQuad

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[Simon Fraser (smfr)]

Implement DOMQuad

[Simon Fraser (smfr)]

Created attachment 291793 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291793 [details]
Patch

Patch needs patches from bug 163464 and bug 133916

[Simon Fraser (smfr)]

Created attachment 291905 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291905 [details]
Patch

Need some more clarify on this API before I continue: https://lists.w3.org/Archives/Public/public-fx/2016OctDec/0016.html

[Simon Fraser (smfr)]

Created attachment 311929 [details]
Patch

[Simon Fraser (smfr)]

Current discussion is in https://github.com/w3c/fxtf-drafts/issues/124

[Simon Fraser (smfr)]

Created attachment 311930 [details]
Patch

[Chris Dumez]

Comment on attachment 311930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311930&action=review

> Source/WebCore/dom/DOMQuad.h:40
> +    static Ref<DOMQuad> create(const DOMPointInit& p1, const DOMPointInit& p2, const DOMPointInit& p3, const DOMPointInit& p4) { return adoptRef(*new DOMQuad(p1, p2, p3, p4)); }

Can probably take DOMPointInit&& parameters to allow moving.

> Source/WebCore/dom/DOMQuad.h:41
> +    static Ref<DOMQuad> fromRect(const DOMRectInit& init) { return adoptRef(*new DOMQuad(init)); }

This can probably take a DOMRectInit&& init so we can move it into the constructor.

> Source/WebCore/dom/DOMQuad.h:42
> +    static Ref<DOMQuad> fromQuad(const DOMQuadInit& init) { return create(init.p1, init.p2, init.p3, init.p4); }

This can probably take a DOMQuadInit&& init so we can move its members into the constructor.

> Source/WebCore/dom/DOMQuad.h:49
> +    DOMRectInit getBounds() const;

I think this should return a Ref<DOMRect>.

> Source/WebCore/dom/DOMQuad.idl:33
> +interface DOMQuad {

Should be on previous line.

> Source/WebCore/dom/DOMQuad.idl:37
> +    [CachedAttribute, CustomGetter] readonly attribute DOMPoint p1; // FIXME: Should be [SameObject].

I do not understand why these need a CustomGetter. Doesn't CachedAttribute do the right thing by default?

> Source/WebCore/dom/DOMQuad.idl:41
> +    [NewObject, Custom] DOMRect getBounds();

Why can't the implementation return a DOMRect instead of a DOMRectInit? So that we do not need custom bindings code.

This patch adds a lot of custom bindings :(

[Sam Weinig]

I think this would be much simpler if DOMQuad had four Ref<DOMPoint> members.

[Simon Fraser (smfr)]

Mostly cleaned up but I'm not sure how to do the serialization without changing CodeGenerator.pm to add DOMPoint as a serializable type, and can't find an example of custom serialization.

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #10)
> Mostly cleaned up but I'm not sure how to do the serialization without
> changing CodeGenerator.pm to add DOMPoint as a serializable type, and can't
> find an example of custom serialization.

Since DOMPointReadOnly has a serializer, DOMPoint is already 'a serializable type'. This means you have to do a parse of each Interface type in the attribute list to figure out if they are serializable types.  See CodeGenerator.pm's GetDictionaryByType for an example of how to trigger a parse (note, it is quite important to cache the result if you do this). If this feels too involved, let me know, and I can add the support.

[Sam Weinig]

(In reply to Sam Weinig from comment #11)
> (In reply to Simon Fraser (smfr) from comment #10)
> > Mostly cleaned up but I'm not sure how to do the serialization without
> > changing CodeGenerator.pm to add DOMPoint as a serializable type, and can't
> > find an example of custom serialization.
> 
> Since DOMPointReadOnly has a serializer, DOMPoint is already 'a serializable
> type'. This means you have to do a parse of each Interface type in the
> attribute list to figure out if they are serializable types.  See
> CodeGenerator.pm's GetDictionaryByType for an example of how to trigger a
> parse (note, it is quite important to cache the result if you do this). If
> this feels too involved, let me know, and I can add the support.

I think it would also be ok to do everything but the serializer part at first.

[Simon Fraser (smfr)]

Created attachment 312748 [details]
Patch

[Sam Weinig]

Comment on attachment 312748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312748&action=review

> Source/WebCore/dom/DOMQuad.idl:31
> +    Constructor(optional DOMPointInit p1, optional DOMPointInit p2, optional DOMPointInit p3, optional DOMPointInit p4),
> +    Exposed=(Window,Worker),
> +    ImplementationLacksVTable

I think this probably needs JSCustomMarkFunction, which will require a JSDOMQuad::visitAdditionalChildren that adds the 4 points to the visit set.

> Source/WebCore/dom/DOMQuad.idl:40
> +    [CachedAttribute] readonly attribute DOMPoint p1; // FIXME: Should be [SameObject].
> +    [CachedAttribute] readonly attribute DOMPoint p2; // FIXME: Should be [SameObject].
> +    [CachedAttribute] readonly attribute DOMPoint p3; // FIXME: Should be [SameObject].
> +    [CachedAttribute] readonly attribute DOMPoint p4; // FIXME: Should be [SameObject].

You don't need the [CachedAttribute] here. The wrapper cache will handle this for you.

[Chris Dumez]

Comment on attachment 312748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312748&action=review

>> Source/WebCore/dom/DOMQuad.idl:40
>> +    [CachedAttribute] readonly attribute DOMPoint p4; // FIXME: Should be [SameObject].
> 
> You don't need the [CachedAttribute] here. The wrapper cache will handle this for you.

I am not sure this is true. For e.g. With Simon's code this would work:
quad.p1.foo = 1;
gc();
quad.p1.foo === 1; // True with Simon's code but I believe it would return false;

Sam, am I right? Or maybe the visit function will take care of keeping the point wrappers alive as long as the quad is alive?

[Sam Weinig]

(In reply to Chris Dumez from comment #15)
> Comment on attachment 312748 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312748&action=review
> 
> >> Source/WebCore/dom/DOMQuad.idl:40
> >> +    [CachedAttribute] readonly attribute DOMPoint p4; // FIXME: Should be [SameObject].
> > 
> > You don't need the [CachedAttribute] here. The wrapper cache will handle this for you.
> 
> I am not sure this is true. For e.g. With Simon's code this would work:
> quad.p1.foo = 1;
> gc();
> quad.p1.foo === 1; // True with Simon's code but I believe it would return
> false;
> 
> Sam, am I right? Or maybe the visit function will take care of keeping the
> point wrappers alive as long as the quad is alive?

No. The wrappers should be kept alive via the visit function.

[Sam Weinig]

(In reply to Sam Weinig from comment #16)
> (In reply to Chris Dumez from comment #15)
> > Comment on attachment 312748 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=312748&action=review
> > 
> > >> Source/WebCore/dom/DOMQuad.idl:40
> > >> +    [CachedAttribute] readonly attribute DOMPoint p4; // FIXME: Should be [SameObject].
> > > 
> > > You don't need the [CachedAttribute] here. The wrapper cache will handle this for you.
> > 
> > I am not sure this is true. For e.g. With Simon's code this would work:
> > quad.p1.foo = 1;
> > gc();
> > quad.p1.foo === 1; // True with Simon's code but I believe it would return
> > false;
> > 
> > Sam, am I right? Or maybe the visit function will take care of keeping the
> > point wrappers alive as long as the quad is alive?
> 
> No. The wrappers should be kept alive via the visit function.

But, we should probably have a gc test either way.

[Sam Weinig]

Comment on attachment 312748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312748&action=review

> Source/WebCore/dom/DOMQuad.h:29
> +#include "DOMPointInit.h"
> +#include "DOMQuadInit.h"

I don't think you need to include these. They can be forward declared.

> Source/WebCore/dom/DOMQuad.h:38
> +struct DOMPointInit;

No need to forward declare if you are including the header.

[Simon Fraser (smfr)]

So JSDOMQuad::visitAdditionalChildren should do something like:

    visitor.addOpaqueRoot(root(wrapped().p1()));
or
    visitor.addOpaqueRoot(wrapped().p1()); ?

I don't know what root() is.

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #19)
> So JSDOMQuad::visitAdditionalChildren should do something like:
> 
>     visitor.addOpaqueRoot(root(wrapped().p1()));
> or
>     visitor.addOpaqueRoot(wrapped().p1()); ?
> 
> I don't know what root() is.

I think the later.

Taking the implementation from JSXMLHttpRequest as model, since it has similar semantics, it does:

void JSXMLHttpRequest::visitAdditionalChildren(SlotVisitor& visitor)
{
    if (XMLHttpRequestUpload* upload = wrapped().optionalUpload())
        visitor.addOpaqueRoot(upload);

    if (Document* responseDocument = wrapped().optionalResponseXML())
        visitor.addOpaqueRoot(responseDocument);
}

So, I believe yours should do:

void JSDOMQuad::visitAdditionalChildren(SlotVisitor& visitor)
{
    visitor.addOpaqueRoot(wrapped().p1().ptr());
    visitor.addOpaqueRoot(wrapped().p2().ptr());
    visitor.addOpaqueRoot(wrapped().p3().ptr());
    visitor.addOpaqueRoot(wrapped().p4().ptr());
}

[Simon Fraser (smfr)]

Created attachment 313199 [details]
Patch

[Sam Weinig]

Comment on attachment 313199 [details]
Patch

r=me (though you should figure out why it doesn't apply).

[Simon Fraser (smfr)]

Created attachment 313201 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 313203 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 313206 [details]
Patch

[Chris Dumez]

Comment on attachment 313206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313206&action=review

> Source/WebCore/dom/DOMQuad.h:46
> +    RefPtr<DOMPoint> p1() const { return m_p1; }

Why is this returning RefPtr instead of raw pointer? It will cause unnecessary refcounting churn.

> Source/WebCore/dom/DOMQuad.h:57
> +    RefPtr<DOMPoint> m_p1;

Looks like these can be Ref?

[Chris Dumez]

Comment on attachment 313206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313206&action=review

>> Source/WebCore/dom/DOMQuad.h:46
>> +    RefPtr<DOMPoint> p1() const { return m_p1; }
> 
> Why is this returning RefPtr instead of raw pointer? It will cause unnecessary refcounting churn.

I think this should even return a C++ reference since the points cannot be null. Having members use Ref<> would make this clearer.
DOMPoint& p1() { return m_p1.get(); }

Ref<DOMPoint> m_p1;

[Chris Dumez]

Comment on attachment 313206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313206&action=review

> Source/WebCore/dom/DOMQuad.cpp:57
> +RefPtr<DOMRect> DOMQuad::getBounds() const

Should return a Ref<DOMRect>.

[Simon Fraser (smfr)]

Created attachment 313210 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 313210 [details]
Patch

Clearing flags on attachment: 313210

Committed r218458: <http://trac.webkit.org/changeset/218458>

[Chris Dumez]

Comment on attachment 313210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313210&action=review

> Source/WebCore/dom/DOMQuad.h:51
> +    RefPtr<DOMRect> getBounds() const;

Why didn't you return a Ref<> here?

[Chris Dumez]

(In reply to Chris Dumez from comment #31)
> Comment on attachment 313210 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313210&action=review
> 
> > Source/WebCore/dom/DOMQuad.h:51
> > +    RefPtr<DOMRect> getBounds() const;
> 
> Why didn't you return a Ref<> here?

https://bugs.webkit.org/show_bug.cgi?id=173517

[Simon Fraser (smfr)]

Done.

[Radar WebKit Bug Importer]

<rdar://problem/45968896>