Implement DOMQuad
Created attachment 291793 [details] Patch
Comment on attachment 291793 [details] Patch Patch needs patches from bug 163464 and bug 133916
Created attachment 291905 [details] Patch
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
Created attachment 311929 [details] Patch
Current discussion is in https://github.com/w3c/fxtf-drafts/issues/124
Created attachment 311930 [details] Patch
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 :(
I think this would be much simpler if DOMQuad had four Ref<DOMPoint> members.
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.
(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.
(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.
Created attachment 312748 [details] Patch
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.
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?
(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.
(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.
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.
So JSDOMQuad::visitAdditionalChildren should do something like: visitor.addOpaqueRoot(root(wrapped().p1())); or visitor.addOpaqueRoot(wrapped().p1()); ? I don't know what root() is.
(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()); }
Created attachment 313199 [details] Patch
Comment on attachment 313199 [details] Patch r=me (though you should figure out why it doesn't apply).
Created attachment 313201 [details] Patch
Created attachment 313203 [details] Patch
Created attachment 313206 [details] Patch
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?
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;
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>.
Created attachment 313210 [details] Patch
Comment on attachment 313210 [details] Patch Clearing flags on attachment: 313210 Committed r218458: <http://trac.webkit.org/changeset/218458>
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?
(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
Done.
<rdar://problem/45968896>