Convert MutationObserver to webidl

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Created attachment 692674 [details] [diff] [review]
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).
Attachment #660853 - Attachment is obsolete: true
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?
Ah, I need to remove stuff from test_interfaces.html.json
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)
(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.
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.
> 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> >& ?
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.
nsTArray<OwningNonNull<nsDOMMutationRecord> >& as out param would be just fine.
But that doesn't seem to be what codegen expects.
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.
(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 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
Attachment #692674 - Flags: review?(bzbarsky) → review+
(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.
> 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?  :(
MutationCallback used to be interface. I don't recall why it was changed to be just callback.
(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.
Created attachment 693437 [details] [diff] [review]
change dictionary names

Apparently dictionary properties need now m-prefix.
> Apparently dictionary properties need now m-prefix.

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

Updated

6 years ago
Depends on: 822826
Depends on: 823009
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla20
(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
(I disagree with that deprecation ;) Callback interfaces are good.)
(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.
You need to log in before you can comment on or make changes to this bug.