WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED CONFIGURATION CHANGED
163534
Implement DOMQuad
https://bugs.webkit.org/show_bug.cgi?id=163534
Summary
Implement DOMQuad
Simon Fraser (smfr)
Reported
2016-10-16 21:57:02 PDT
Implement DOMQuad
Attachments
Patch
(43.49 KB, patch)
2016-10-16 22:00 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(52.27 KB, patch)
2016-10-17 17:57 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(41.27 KB, patch)
2017-06-03 09:17 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(38.57 KB, patch)
2017-06-03 09:26 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(41.04 KB, patch)
2017-06-12 22:01 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(51.84 KB, patch)
2017-06-17 10:15 PDT
,
Simon Fraser (smfr)
sam
: review+
Details
Formatted Diff
Diff
Patch
(46.18 KB, patch)
2017-06-17 11:02 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(46.52 KB, patch)
2017-06-17 11:45 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(46.75 KB, patch)
2017-06-17 12:45 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Patch
(46.80 KB, patch)
2017-06-17 13:05 PDT
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1
2016-10-16 22:00:19 PDT
Created
attachment 291793
[details]
Patch
Simon Fraser (smfr)
Comment 2
2016-10-16 22:01:39 PDT
Comment on
attachment 291793
[details]
Patch Patch needs patches from
bug 163464
and
bug 133916
Simon Fraser (smfr)
Comment 3
2016-10-17 17:57:11 PDT
Created
attachment 291905
[details]
Patch
Simon Fraser (smfr)
Comment 4
2016-10-18 18:00:54 PDT
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)
Comment 5
2017-06-03 09:17:48 PDT
Created
attachment 311929
[details]
Patch
Simon Fraser (smfr)
Comment 6
2017-06-03 09:18:03 PDT
Current discussion is in
https://github.com/w3c/fxtf-drafts/issues/124
Simon Fraser (smfr)
Comment 7
2017-06-03 09:26:26 PDT
Created
attachment 311930
[details]
Patch
Chris Dumez
Comment 8
2017-06-04 10:45:59 PDT
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
Comment 9
2017-06-08 16:30:17 PDT
I think this would be much simpler if DOMQuad had four Ref<DOMPoint> members.
Simon Fraser (smfr)
Comment 10
2017-06-11 11:00:01 PDT
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
Comment 11
2017-06-11 11:37:20 PDT
(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
Comment 12
2017-06-11 12:25:01 PDT
(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)
Comment 13
2017-06-12 22:01:00 PDT
Created
attachment 312748
[details]
Patch
Sam Weinig
Comment 14
2017-06-13 05:52:48 PDT
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 15
2017-06-13 09:32:58 PDT
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
Comment 16
2017-06-13 09:34:41 PDT
(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
Comment 17
2017-06-13 09:35:07 PDT
(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 18
2017-06-13 15:39:34 PDT
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)
Comment 19
2017-06-14 11:08:26 PDT
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
Comment 20
2017-06-14 17:51:27 PDT
(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)
Comment 21
2017-06-17 10:15:23 PDT
Created
attachment 313199
[details]
Patch
Sam Weinig
Comment 22
2017-06-17 10:54:18 PDT
Comment on
attachment 313199
[details]
Patch r=me (though you should figure out why it doesn't apply).
Simon Fraser (smfr)
Comment 23
2017-06-17 11:02:50 PDT
Created
attachment 313201
[details]
Patch
Simon Fraser (smfr)
Comment 24
2017-06-17 11:45:05 PDT
Created
attachment 313203
[details]
Patch
Simon Fraser (smfr)
Comment 25
2017-06-17 12:45:39 PDT
Created
attachment 313206
[details]
Patch
Chris Dumez
Comment 26
2017-06-17 12:48:07 PDT
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 27
2017-06-17 12:53:24 PDT
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 28
2017-06-17 12:54:42 PDT
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)
Comment 29
2017-06-17 13:05:36 PDT
Created
attachment 313210
[details]
Patch
WebKit Commit Bot
Comment 30
2017-06-17 15:34:31 PDT
Comment on
attachment 313210
[details]
Patch Clearing flags on attachment: 313210 Committed
r218458
: <
http://trac.webkit.org/changeset/218458
>
Chris Dumez
Comment 31
2017-06-17 16:56:58 PDT
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
Comment 32
2017-06-17 17:33:11 PDT
(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)
Comment 33
2018-11-10 11:05:27 PST
Done.
Radar WebKit Bug Importer
Comment 34
2018-11-10 11:06:25 PST
<
rdar://problem/45968896
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug