Closed Bug 790978 Opened 12 years ago Closed 11 years ago

Convert MutationObserver to webidl

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached patch backup (obsolete) — Splinter Review
      No description provided.
Attached patch better (obsolete) — Splinter Review
Attachment #660847 - Attachment is obsolete: true
Depends on: 793000
No longer depends on: 793000
Attached patch patchSplinter Review
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
Attachment #692674 - Flags: review?(bzbarsky)
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?  :(
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch unint32_t (obsolete) — Splinter Review
Attachment #693167 - Attachment is obsolete: true
Attached patch patchSplinter Review
Attachment #693172 - Attachment is obsolete: true
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.  :(
Depends on: 822826
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: