Closed Bug 1186265 Opened 9 years ago Closed 6 years ago

Remove DOMPoint{,ReadOnly}(DOMPointInit) constructor, implement DOMPoint{,ReadOnly}.fromPoint

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Ms2ger, Assigned: mrbkap)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files, 3 obsolete files)

This apparently got changed around.

https://drafts.fxtf.org/geometry/#dompoint

Since we already shipped this starting in 31, we should probably revert the spec change, though. Roc, I believe you worked on this?
Flags: needinfo?(roc)
I did. But I don't know whether this is used in the wild enough to defeat the spec change.
Flags: needinfo?(roc)
Perhaps not, but it seems very gratuitous compat risk.
Keywords: site-compat
Some data on the Web compat situation...

In httparchive I see 0 instances of `new DOMPoint(...)`.

SELECT * FROM (
SELECT page, url, REGEXP_EXTRACT(body, r'(\bnew\s+DOMPoint\s*\([^\)]*\))') AS match
FROM [httparchive:har.2016_12_15_chrome_requests_bodies]
) WHERE match != "null"

Query returned zero records.

In GitHub search there are 25 results for "new DOMPoint", none of them use dictionary argument as far as I can tell.

https://github.com/search?l=JavaScript&q="new+dompoint"&ref=searchresults&type=Code&utf8=✓

Test case:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4800

Dictionary constructor is currently supported in Gecko, Chromium, WebKit. DOMPoint is not supported in Edge.
It appears the dictionary constructor was removed from Chromium in https://bugs.chromium.org/p/chromium/issues/detail?id=675437#c7

Also, per that issue DOMPoint is behind a runtime flag, so DOMPoint is not supported by default in Chrome/Opera.
I ran into this randomly and it annoyed me, so I mostly* updated our implementation to the current draft of the spec given in comment 0. Glancing through the GitHub search that Simon mentioned in comment 3, I didn't see any more uses of the constructor taking a DOMPointInit, so it seems mostly reasonable to remove it.

That being said, WebKit is literally sitting on the fence between the latest CR and the draft: it has the fromPoint methods added in the draft as well as the constructors taking DOMPointInit.

My inclination is to simply implement the draft spec for consistency with Chromium (and as we don't have evidence of the extra constructors being necessary, avoiding the redundancy with fromPoint would be nice).

Boris, does this seem reasonable to you?

* I didn't add DOMPointReadOnly.matrixTransform because our DOMMatrix hasn't grown to include a DOMMatrixInit yet and that seemed like a large enough change to deserve its own bug.
Assignee: nobody → mrbkap
Flags: needinfo?(bzbarsky)
Sorry for the lag here...

Implementing consistently with Chromium seems fine to me, I think.  Just watch out for breakage from people who assume that if it _is_ exposed it can be called with the dictionary.  :(
Flags: needinfo?(bzbarsky)
Comment on attachment 8967588 [details]
Bug 1186265 - Update wpt test expectations now that DOMPoint is updated.

https://reviewboard.mozilla.org/r/236290/#review242140

::: testing/web-platform/meta/css/geometry/DOMPoint-001.html.ini:3
(Diff revision 1)
>  [DOMPoint-001.html]
> -  [testConstructor2undefined]
> +  [testConstructor1]
> +    expected: FAIL

Fix the test instead?
Comment on attachment 8967586 [details]
Bug 1186265 - Remove DOMPoint constructor taking a DOMPointInit.

https://reviewboard.mozilla.org/r/236286/#review242386

r=me, though the spec is a bit weird (why can you pass DOMPointInit to the DOMQuad ctor but not the DOMPoint ctor?).  I assume there are Reasons for it... but it sure would be nice to know what they are.

Please make sure to send an intent to ship/unship for this change.
Attachment #8967586 - Flags: review?(bzbarsky) → review+
Comment on attachment 8967587 [details]
Bug 1186265 - Resurrect DOMQuad.bounds (deprecated) and count its uses.

https://reviewboard.mozilla.org/r/236288/#review242388

r=me, though it looks like there's more stuff to fix around this spec, given the remaining wpt failures....  Please file followups?

::: dom/webidl/DOMPoint.webidl:23
(Diff revision 1)
> +
>      readonly attribute unrestricted double x;
>      readonly attribute unrestricted double y;
>      readonly attribute unrestricted double z;
>      readonly attribute unrestricted double w; 
>  };

Want to throw in the "jsonifier;" that would match the "[Default] object toJSON();" bit in the spec?
Attachment #8967587 - Flags: review?(bzbarsky) → review+
Comment on attachment 8967588 [details]
Bug 1186265 - Update wpt test expectations now that DOMPoint is updated.

https://reviewboard.mozilla.org/r/236290/#review242390

::: commit-message-4501b:4
(Diff revision 1)
> +Bug 1186265 - Add failure data for this test. r=bz
> +
> +I don't understand this test, most of the tests here are wrong per the current
> +spec and Web IDL. It seems like it should be removed, maybe?

I think you should remove the parts of this test that test a constructor with a dictionary, maybe replacing them with a single test that checks that a dictionary converts to NaN or whatever.

And fix the expectations for the tests that pass "not enought numbers".

In any case, we shouldn't need to add new failure annotations here.  Just fix the broken test instead.

r=me if you do that.
Attachment #8967588 - Flags: review?(bzbarsky) → review+
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3a0b2686f45
Remove DOMPoint constructor taking a DOMPointInit. r=bz
https://hg.mozilla.org/integration/autoland/rev/2ba85ec7c9da
Update DOMPointReadOnly to the most recent spec. r=bz
https://hg.mozilla.org/integration/autoland/rev/d8986aead3e0
Update DOMPoint-001.html to the current spec. r=bz
Backed out 3 changesets (bug 1186265) for wpt8 failures in /css/geometry/interfaces.html on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d8986aead3e05be3a7f15252c098e7da173c42aa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&filter-searchStr=wpt8
Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&fromchange=7656585eb84000aa0fcdd663eea73fb86b2931a0&filter-searchStr=wpt8&selectedJob=173933192
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=50aeb30fd283f1f245a04e1e2861ebf028008d61&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&filter-searchStr=wpt8
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=173933192&repo=autoland&lineNumber=2742

task 2018-04-16T18:17:11.171Z] 18:17:11     INFO - @http://web-platform.test:8000/css/geometry/support/interfaces.js:28:1
[task 2018-04-16T18:17:11.171Z] 18:17:11     INFO - TEST-PASS | /css/geometry/interfaces.html | Unscopable handled correctly for matrixTransform(DOMMatrixInit) on DOMPointReadOnly 
[task 2018-04-16T18:17:11.172Z] 18:17:11     INFO - TEST-UNEXPECTED-PASS | /css/geometry/interfaces.html | DOMPointReadOnly interface: operation toJSON() - expected FAIL
[task 2018-04-16T18:17:11.172Z] 18:17:11     INFO - TEST-INFO | expected FAIL
[task 2018-04-16T18:17:11.180Z] 18:17:11     INFO -
Flags: needinfo?(mrbkap)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> Want to throw in the "jsonifier;" that would match the "[Default] object
> toJSON();" bit in the spec?

Because of the way the wpt tests are written (if one of these objects supports toJSON, they fail if others don't), this change requires updating a bunch of related classes (DOMRect, DOMQuad, and more changes to DOMPoint). I'll file a followup bug to do that.
I made part of the changes to DOMPoint-001.html upstream without realizing that there was an existing patch for this (the bug tracking import is bug 1456309). I'm not sure if the changes would apply cleanly to upstream (since I changed two test names) and I didn't remove the tests for constructing from a dictionary; it might be best to try to resolve this before landing? Not entirely sure, sorry about the extra work!
I don't know what to do about comment 23, but this bug turned into a bit of a monster. Fixing just the bits I did seems to have allowed the interfaces.html tests to proceed further than before, causing many more failures. I've fixed all of them by delving all the way into DOMQuad (!) but haven't fully fixed everything (I believe that the DOMMatrix constructors are all that's left needing updating). As I'd like to get this in eventually, I'm going to stop where I am and try to land just the changes that were "required". That being said, I think I've also run into a bug in our Web IDL parser: it doesn't consider an interface as being serializable if it inherits a jsonifier from a parent interface. I'll file another separate bug to investigate that (do note that in this try push, I've worked around the problem by sticking a jsonifier directly on DOMPoint).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccd8aedea815b2949d5d628960f21fbe960d785e
> I don't know what to do about comment 23

The simplest thing to do is to import the upstream changes into these patches, then merge with them.  You can do that via waiting for bug 1456309 to land or just grab the diff and apply it.

> As I'd like to get this in eventually, I'm going to stop where I am and try to land just the changes that were "required"

Yep.  Annotate whatever failures remain as expected fail for now and land.

> I've also run into a bug in our Web IDL parser: it doesn't consider an interface as being serializable
> if it inherits a jsonifier from a parent interface.

Well, so....  I guess we could special-case this in the case when the descendant interface has no attributes.  But really, there is no behavior definition for "jsonifier" past "whatever our codegen implements".  We should really update to the spec serialization stuff now that it's not insanely overcomplicated.  Bug 1375829 tracks that.
Comment on attachment 8979798 [details]
Bug 1186265 - Partially update DOMPoint, DOMQuad, DOMRect, DOMMatrix.

https://reviewboard.mozilla.org/r/245946/#review252278

::: dom/base/DOMMatrix.cpp:63
(Diff revision 1)
> +      return nullptr;
> +    }
> +    rval->SetMatrixValue(arg.GetAsString(), aRv);
> +  } else {
> +    const auto& sequence = arg.GetAsUnrestrictedDoubleSequence();
> +    SetDataInMatrix(rval, sequence.Elements(), sequence.Length(), aRv);

So this is moderately anoying in that in the 3d case it will allocate a 2d matrix and mutate it to a 3d one partway through the assignments, right?  I guess that's ok for the moment.

::: dom/base/DOMRect.h:54
(Diff revision 1)
> +
>    virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
>  
> -  virtual double X() const = 0;
> -  virtual double Y() const = 0;
> -  virtual double Width() const = 0;
> +  static already_AddRefed<DOMRectReadOnly>
> +  Constructor(const GlobalObject& aGlobal, double aX, double aY,
> +              double aWidth, double aHeight, ErrorResult& aRV);

aRv?

::: dom/base/DOMRect.h:56
(Diff revision 1)
>  
> -  virtual double X() const = 0;
> -  virtual double Y() const = 0;
> -  virtual double Width() const = 0;
> -  virtual double Height() const = 0;
> +  static already_AddRefed<DOMRectReadOnly>
> +  Constructor(const GlobalObject& aGlobal, double aX, double aY,
> +              double aWidth, double aHeight, ErrorResult& aRV);
> +
> +  virtual double X() const

Why do these X/Y/width/height getters need to be virtual?  I think you can make them nonvirtual.

::: dom/bindings/Errors.msg:106
(Diff revision 1)
>  MSG_DEF(MSG_TIME_VALUE_OUT_OF_RANGE, 1, JSEXN_TYPEERR, "{0} is outside the supported range for time values.")
>  MSG_DEF(MSG_ONLY_IF_CACHED_WITHOUT_SAME_ORIGIN, 1, JSEXN_TYPEERR, "Request mode '{0}' was used, but request cache mode 'only-if-cached' can only be used with request mode 'same-origin'.")
>  MSG_DEF(MSG_THRESHOLD_RANGE_ERROR, 0, JSEXN_RANGEERR, "Threshold values must all be in the range [0, 1].")
>  MSG_DEF(MSG_WORKER_THREAD_SHUTTING_DOWN, 0, JSEXN_TYPEERR, "The Worker thread is shutting down.")
>  MSG_DEF(MSG_CACHE_OPEN_FAILED, 0, JSEXN_TYPEERR, "CacheStorage.open() failed to access the storage system.")
> +MSG_DEF(MSG_MATRIX_INIT_LENGTH_WRONG, 0, JSEXN_TYPEERR, "Matrix init sequence must have a length of 6 or 16")

I guess if we really cared we could include the actual passed-in length in the message... I am not sure we care.

::: dom/webidl/DOMQuad.webidl:22
(Diff revision 1)
>  interface DOMQuad {
>      [SameObject] readonly attribute DOMPoint p1;
>      [SameObject] readonly attribute DOMPoint p2;
>      [SameObject] readonly attribute DOMPoint p3;
>      [SameObject] readonly attribute DOMPoint p4;
>      [SameObject] readonly attribute DOMRectReadOnly bounds;

So the reason this is not an issue in the spec in terms of liveness is that in the spec getBounds() returns a new object each time.   That object is created by the getBounds() call, initialized with some values, and hence has nothing to do with the original points involved after that point.

Fixing bug 1454622 will fix this issue.

::: dom/webidl/DOMQuad.webidl:22
(Diff revision 1)
>  interface DOMQuad {
>      [SameObject] readonly attribute DOMPoint p1;
>      [SameObject] readonly attribute DOMPoint p2;
>      [SameObject] readonly attribute DOMPoint p3;
>      [SameObject] readonly attribute DOMPoint p4;
>      [SameObject] readonly attribute DOMRectReadOnly bounds;

Do you want to include a toJSON here too, while you're here?
Attachment #8979798 - Flags: review?(bzbarsky) → review+
Comment on attachment 8979799 [details]
Bug 1186265 - Check the current interface's members.

https://reviewboard.mozilla.org/r/245948/#review252290

Feels like this should be rolled into one of the earlier patches, but r=me
Attachment #8979799 - Flags: review?(bzbarsky) → review+
Comment on attachment 8979800 [details]
Bug 1186265 - DOMQuad now fails further along in this test.

https://reviewboard.mozilla.org/r/245950/#review252302

::: commit-message-bf778:5
(Diff revision 1)
> +Bug 1186265 - DOMQuad now fails further along in this test. r=bz
> +
> +DOMQuad needs to be updated so that the bounds are returned via a function
> +instead of an attribute (which will allow us to use the default toJSON
> +function for it). That can happen in a followup bug.

That's tracked in bug 1454622.
Attachment #8979800 - Flags: review?(bzbarsky) → review+
Attachment #8979800 - Attachment is obsolete: true
Flags: needinfo?(mrbkap)
Attachment #8967587 - Flags: review+ → review?(bzbarsky)
Comment on attachment 8979799 [details]
Bug 1186265 - Check the current interface's members.

bz wrote this patch on IRC. It has r=me.
Attachment #8979799 - Flags: review?(mrbkap) → review+
Comment on attachment 8979798 [details]
Bug 1186265 - Partially update DOMPoint, DOMQuad, DOMRect, DOMMatrix.

mozreview's interdiff between versions 1 and 2 works pretty well on this patch. My plan is to squash these even more before pushing.
Attachment #8979798 - Flags: review+ → review?(bzbarsky)
Comment on attachment 8979798 [details]
Bug 1186265 - Partially update DOMPoint, DOMQuad, DOMRect, DOMMatrix.

https://reviewboard.mozilla.org/r/245946/#review252450
Attachment #8979798 - Flags: review?(bzbarsky) → review+
Comment on attachment 8967587 [details]
Bug 1186265 - Resurrect DOMQuad.bounds (deprecated) and count its uses.

https://reviewboard.mozilla.org/r/236288/#review252452

::: dom/base/DOMQuad.cpp:110
(Diff revision 5)
> +DOMRectReadOnly*
> +DOMQuad::Bounds()
> +{
> +  if (nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(mParent)) {
> +    if (nsIDocument* doc = win->GetExtantDoc()) {
> +      doc->WarnOnceAbout(nsIDocument::eDOMQuadBoundsAttr);

This is already handled by the [Deprecated] thing in the IDL, no?

::: dom/webidl/DOMQuad.webidl:24
(Diff revision 5)
>      [SameObject] readonly attribute DOMPoint p2;
>      [SameObject] readonly attribute DOMPoint p3;
>      [SameObject] readonly attribute DOMPoint p4;
>      [NewObject] DOMRectReadOnly getBounds();
>  
> -    [Default] object toJSON();
> +    [SameObject, Deprecated=DOMQuadBoundsAttr] readonly attribute DOMRectReadOnly bounds;

So the first read of .bounds will snapshot and then it will never change even if the DOMQuad is mutated, right?

That might be OK depending on how people are using this stuff (in particular whether they ever mutate the points of the quad).  I guess given that Blink doesn't even update getBounds() in that case it should be ok.
Attachment #8967587 - Flags: review?(bzbarsky) → review+
Attachment #8967586 - Attachment is obsolete: true
Attachment #8967588 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #43)
> This is already handled by the [Deprecated] thing in the IDL, no?

Oops, yes.

> So the first read of .bounds will snapshot and then it will never change
> even if the DOMQuad is mutated, right?

Yeah.

> That might be OK depending on how people are using this stuff (in particular
> whether they ever mutate the points of the quad).  I guess given that Blink
> doesn't even update getBounds() in that case it should be ok.

That was my reasoning as well. I'm really curious to see what the results of the telemetry are. There aren't many interfaces that even return DOMQuad, so I suspect usage will be very low for both the interface and .bounds.
Attachment #8979799 - Flags: review?(mrbkap)
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00fefd3eb634
Partially update DOMPoint, DOMQuad, DOMRect, DOMMatrix. r=bz
https://hg.mozilla.org/integration/autoland/rev/55bd80764231
Resurrect DOMQuad.bounds (deprecated) and count its uses. r=bz
https://hg.mozilla.org/integration/autoland/rev/4a9965aeeef3
Check the current interface's members. r=bz
Backed out for devtools chrome failures on browser_webconsole_visibility_messages.js

Backout: https://hg.mozilla.org/integration/autoland/rev/7d10c4c82719fa1b820b9ae29cd878aecccd422a
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=180142379&repo=autoland&lineNumber=13808
Flags: needinfo?(mrbkap)
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/11156 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Devtools is the only code in our tree that refers to .bounds, but there are a bunch of references. I'm converting it to getBounds.
Comment on attachment 8979799 [details]
Bug 1186265 - Check the current interface's members.

https://reviewboard.mozilla.org/r/245948/#review255930
Attachment #8979799 - Flags: review?(mrbkap) → review+
Comment on attachment 8983890 [details]
Bug 1186265 - Move devtools over to getBounds().

https://reviewboard.mozilla.org/r/249736/#review256094

Thanks Blake, that looks good, and in line with our discussion on IRC. Thank you for adding this comment in getAdjustedQuads too. That is helpful.
Attachment #8983890 - Flags: review?(pbrosset) → review+
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5eeebfcad8c
Partially update DOMPoint, DOMQuad, DOMRect, DOMMatrix. r=bz
https://hg.mozilla.org/integration/autoland/rev/f485f658d19c
Resurrect DOMQuad.bounds (deprecated) and count its uses. r=bz
https://hg.mozilla.org/integration/autoland/rev/3dc087521307
Check the current interface's members. r=bz
https://hg.mozilla.org/integration/autoland/rev/4321a242f5f7
Move devtools over to getBounds(). r=pbro
Documentation updated/created:

https://developer.mozilla.org/en-US/docs/Web/API/DOMPointInit
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointInit/x
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointInit/y
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointInit/z
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointInit/w
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointReadOnly
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointReadOnly/x
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointReadOnly/y
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointReadOnly/z
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointReadOnly/w
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointReadOnly/toJSON
https://developer.mozilla.org/en-US/docs/Web/API/DOMPointReadOnly/fromPoint
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint/DOMPoint
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint/x
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint/y
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint/z
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint/w
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint/toJSON
https://developer.mozilla.org/en-US/docs/Web/API/DOMPoint/fromPoint

Also added mentions to Firefox 62 for developers.

Submitted PRs as well:

Add DOMPointInit dictionary to BCD (https://github.com/mdn/browser-compat-data/pull/2437)
Add DOMPointReadOnly and DOMPointInit to API group data (https://github.com/mdn/kumascript/pull/733)

That should complete work on documentation for this change (as well as some other work that I did along the way). Let me know if I’m mistaken.

Those compatibility tables for toJSON method are not correct. Firefox 31 doesn't support it. If I'm reading this right, it came with FF 62, right? (it doesn't work in ESR 60 and it works in current 65)

Updated the compatibility data for a few places where information was wrong or missing (including that toJSON() issue mentioned in comment 69). This is included in BCD PR 4828.

You need to log in before you can comment on or make changes to this bug.