790978 - Convert MutationObserver to webidl

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: backup

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=d80554810476

Large patch but things were easy to convert.
.target is the only hack, since I want to make sure we don't crash even if
CC or GC is buggy (and we end up using record even after unlink).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

The patch gives TEST-UNEXPECTED-PASS in DOMCore/tests/approved/test_interfaces.html
However I don't understand what that test is doing.
Ms2ger, how do I make that test to accept MutationObserver?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Ah, I need to remove stuff from test_interfaces.html.json

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 692674 [details] [diff] [review]
patch

Review of attachment 692674 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDOMMutationObserver.cpp
@@ +41,5 @@
> +  return mAddedNodes;
> +}
> +
> +nsINodeList*
> +nsDOMMutationRecord::RemovedNodes() const

No need to make those const

@@ +502,4 @@
>      allAttrs = false;
> +    const mozilla::dom::Sequence<nsString>& filtersAsString =
> +      aOptions.attributeFilter.Value();
> +    int32_t len = filtersAsString.Length();

uint32_t, perhaps

@@ +561,4 @@
>  {
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal);
> +  if (!window || !window->IsInnerWindow()) {
> +    aRv.Throw(NS_ERROR_FAILURE);

You can assert that, if you get a window, it's an inner window.

::: dom/bindings/Bindings.conf
@@ +345,5 @@
>  {
>      'workers': True,
>  }],
>  
> + 

Nit: trailing whitespace (and one empty line is enough)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Ms2ger from comment #5)
> > +nsINodeList*
> > +nsDOMMutationRecord::RemovedNodes() const
> 
> No need to make those const
Ah. I relied on the example .h file


> 
> @@ +502,4 @@
> >      allAttrs = false;
> > +    const mozilla::dom::Sequence<nsString>& filtersAsString =
> > +      aOptions.attributeFilter.Value();
> > +    int32_t len = filtersAsString.Length();
> 
> uint32_t, perhaps
I explicitly wanted signed int, because nsCOMArray uses those.


> > +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal);
> > +  if (!window || !window->IsInnerWindow()) {
> > +    aRv.Throw(NS_ERROR_FAILURE);
> 
> You can assert that, if you get a window, it's an inner window.
Could do


One thing I wondered - is there an easy way to convert nsTArray to mozilla::dom::Sequence.
What I ended up doing needs to move objects from nsTArray to Sequence, though without addreffing.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I guess I'd like to be able to have nsTArray<nsRefPtr<nsDOMMutationRecord> >& as the
return (out-param) value of takeRecords() and as a parameter of the callback. If we want to let
such setup, the change could be done in a followup bug. But if this is the only
case when such param value would be useful, not worth to change the codegen.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> No need to make those const

Shouldn't the default for methods be const unless needed otherwise?  That's how I think about it, certainly!

> is there an easy way to convert nsTArray to mozilla::dom::Sequence

That depends on what sort of conversion you want... if you want to pass a mozilla::dom::Sequence to a const nsTArray argument or vice versa then once bug 819523 lands you can just do it, as long as the element types are the same.

How much pain would it be to have your out-param be nsTArray<OwningNonNull<nsDOMMutationRecord> >& ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I guess another option is to just have sequence arguments in callbacks become nsTArray in general, like variadics do... But the element type would still be OwningNonNull.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

nsTArray<OwningNonNull<nsDOMMutationRecord> >& as out param would be just fine.
But that doesn't seem to be what codegen expects.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  So yeah, I tried to make the codegen smart and use nsTArray<nsRefPtr> for return values because that's what people usually have lying around in our codebase.  But that does cause problems with calling a callback with the same array.  :(

Let's get a followup filed on that, I guess.  It's a bit of an annoyance, and I'm not sure what the best solution is.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> > No need to make those const
> 
> Shouldn't the default for methods be const unless needed otherwise?  That's
> how I think about it, certainly!

Yeah. Note the "nsDOMMutationRecord* thisObject = const_cast<nsDOMMutationRecord*>(this);"

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692674 [details] [diff] [review]
patch

>+++ b/content/base/src/nsDOMMutationObserver.cpp
>+nsDOMMutationObserver* nsDOMMutationObserver::sCurrentObserver = nullptr;

Do we still need this static?  The this-translator stuff is gone, so I'm not sure whether we reallly have consumers that still need this global state.  Do we?

>+nsDOMMutationRecord::~nsDOMMutationRecord()
>+  nsContentUtils::ReleaseWrapper(this, this);

Yikes.  Is this needed in all classes inheriting from nsWrapperCache?  I don't think I've ever seen this done manually before... and we have lots of classes that are newly inheriting from nsWrapperCache with WebIDL bindings!

>+nsDOMMutationRecord::AddedNodes() const
>+nsDOMMutationRecord::RemovedNodes() const

Ah, yeah.  Please don't make this const if it's really not const.  Just write your C++ naturally; if you can't, the WebIDL bindings are failing.  ;)

>+nsDOMMutationObserver::Observe(nsINode& aTarget,
>+  if (!(!aOptions.attributeOldValue || aOptions.attributes)) {

  if (aOptions.attributeOldValue && !aOptions.attributes) {

seems clearer...

>+  if (!(!aOptions.characterDataOldValue || aOptions.characterData)) {

  if (aOptions.characterDataOldValue && !aOptions.characterData) {

>+    if (!(len == 0 || aOptions.attributes)) {

  if (len != 0 && !aOptions.attributes) {

>+    filters.SetCapacity(len);

Is this a fallible or infallible allocation?  The "len" here is totally under caller script control; it can easily be made very large...  Please make sure this is a fallible allocation.

>+    for (int32_t i = 0; i < len; ++i) {

"uint32_t i", no?

>+nsDOMMutationObserver::Constructor(nsISupports* aGlobal,
>+  if (!window || !window->IsInnerWindow()) {

You should be able to assert window->IsInnerWindow() if window is not null.

>@@ -652,33 +579,33 @@ nsDOMMutationObserver::HandleMutation()
>+  mCallback->Call(this, mutations, *this, rv);

So just to make sure...  The second argument to the callback is the MutationObserver, and the "this" object is _also_ the same MutationObserver?  I assume the spec requires this slightly odd behavior, right?

>@@ -699,40 +626,40 @@ nsDOMMutationObserver::HandleMutationsIn

It might be nice to typedef "nsTArray<nsRefPtr<nsDOMMutationObserver> >" to something shorter, perhaps...

>+++ b/dom/webidl/MutationObserver.webidl
>+ * The origin of this IDL file is
>+ * www.w3.org/TR/2012/WD-XMLHttpRequest-20120117/

I sorta doubt that.  Please link to the right spec?

>+  // .target is not nullable per the spec, but in order to prevent crashes,
>+  // if there are GC/CC bugs in Gecko, we let the property to be null.

I'd like to understand the worry here.  Is this for cases when we unlink mTarget but don't die ourselves or something?

r=me with the above nits fixed

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Do we still need this static?
Yes


> 
> Yikes.  Is this needed in all classes inheriting from nsWrapperCache?
No. removed


> >+nsDOMMutationRecord::AddedNodes() const
> >+nsDOMMutationRecord::RemovedNodes() const
> 
> Ah, yeah.  Please don't make this const if it's really not const.
Ok. I just used whatever the example codegen gave me.

> seems clearer...
fixed

> Is this a fallible or infallible allocation?
added oom check
 
> "uint32_t i", no?
well, len is int32_t


> You should be able to assert window->IsInnerWindow() if window is not null.
done

> 
> >@@ -652,33 +579,33 @@ nsDOMMutationObserver::HandleMutation()
> >+  mCallback->Call(this, mutations, *this, rv);
> 
> So just to make sure...  The second argument to the callback is the
> MutationObserver, and the "this" object is _also_ the same MutationObserver?
> I assume the spec requires this slightly odd behavior, right?
yes, so that could can bind the callback and still get the mutationobserver.
Originally the idea was to allow { handleMutations: function(mutations, mutationobservser) {} }

> I sorta doubt that.  Please link to the right spec?
Oops

> I'd like to understand the worry here.  Is this for cases when we unlink
> mTarget but don't die ourselves or something?
Yes.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> well, len is int32_t

It should also be uint32_t or soemthing.  filtersAsString.Lenth() returns an unsigned type.

> yes, so that could can bind the callback and still get the mutationobserver.

Ah, ok.

> Yes.

How much do we need to worry about that sort of thing in other parts of the code?  :(

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

MutationCallback used to be interface. I don't recall why it was changed to be just callback.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> > well, len is int32_t
> 
> It should also be uint32_t or soemthing.  filtersAsString.Lenth() returns an
> unsigned type.
Ok, i'll change to uint32_t


> > Yes.
> 
> How much do we need to worry about that sort of thing in other parts of the
> code?  :(
Unfortunately one needs to think what can be null after unlink.
Need to discuss with mccr8 if we could find some solution for this.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: unint32_t

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: change dictionary names

Apparently dictionary properties need now m-prefix.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/e1427a1f27c4
https://hg.mozilla.org/mozilla-central/rev/9de611848111

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Apparently dictionary properties need now m-prefix.

Yeah, sorry. There were specs using C++ reserved words as dictionary property names.  :(

Comment 24

[Masatoshi Kimura [:emk]]

(In reply to Olli Pettay [:smaug] from comment #17)
> MutationCallback used to be interface. I don't recall why it was changed to
> be just callback.

The general tendency to deprecate callback interfaces?
http://dev.w3.org/2006/webapi/WebIDL/#dfn-callback-interface

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(I disagree with that deprecation ;) Callback interfaces are good.)

Comment 26

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to Olli Pettay [:smaug] from comment #2)
> .target is the only hack, since I want to make sure we don't crash even if
> CC or GC is buggy (and we end up using record even after unlink).

I filed bug 896272.