973638 - Update MutationObserver.observe() method - more flexible options and throw a JavaScript TypeError

Status: RESOLVED FIXED

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

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

In observe() method for MutationObserver DOM4 make some change:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23189

Before we have:

dictionary MutationObserverInit {
	boolean childList = false;
	boolean attributes = false;
	boolean characterData = false;
	boolean subtree = false;
	boolean attributeOldValue = false;
	boolean characterDataOldValue = false;
	sequence<DOMString> attributeFilter;
};

Now options for observe is more flexible:

dictionary MutationObserverInit {
  boolean childList = false;
  boolean attributes;
  boolean characterData;
  boolean subtree = false;
  boolean attributeOldValue;
  boolean characterDataOldValue;
  sequence<DOMString> attributeFilter;
};

So at this moment, if we set some dictionary member we don't need set other dictionary member (because its set automatically), like we see in algorithm:
http://dom.spec.whatwg.org/#dom-mutationobserver-observe

This is small change and should be easy to implement. Maybe its time to correct this simple inaccuracy:)

Comment 1

[Arkadiusz Michalski (Spirit)]

Per DOM MutationObserver.observe() should throw JavaScript TypeError, not DOMException "SyntaxError":
http://dom.spec.whatwg.org/#dom-mutationobserver-observe

At now in browsers we have:
- SyntaxError (Firefox and IE)
- TypeError (Chrome)

Looks like that in Chrome 36 we have correct behaviour (including more flexible algorithm for MutationObserver.observe() method).

Comment 2

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

Want to update Gecko?
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MutationObserver.webidl and
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMMutationObserver.cpp#451

Comment 3

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

Attachment: v1

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

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8508930 [details] [diff] [review]
v1

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

::: content/base/src/nsDOMMutationObserver.cpp
@@ +471,5 @@
> +  if (!aOptions.mAttributes.WasPassed() &&
> +      (aOptions.mAttributeOldValue.WasPassed() ||
> +       aOptions.mAttributeFilter.WasPassed())) {
> +    attributes = true;
> +  }

I don't know if easier to understand, but:

bool attributes = aOptions.mAttributes.WasPassed() ?
                    aOptions.mAttributes.Value() :
                    (aOptions.mAttributeOldValue.WaPassed() || aOptions.mAttributeFilter.WasPassed());

@@ +476,5 @@
> +
> +  if (!aOptions.mCharacterData.WasPassed() &&
> +      aOptions.mCharacterDataOldValue.WasPassed()) {
> +    characterData = true;
> +  }

bool characterData = aOptions.mCharacterData.WasPassed() ?
                       aOptions.mCharacterData.Value() : aOptions.mCharacterDataOldValue.WasPassed();

@@ +487,2 @@
>      return;
>    }

if (attributeOldValue &&
    aOptions.mAttributes.WasPassed() &&
    !aOptions.mAttributes.Value()) {

@@ +496,5 @@
>    }
> +
> +  if (aOptions.mCharacterDataOldValue.WasPassed() &&
> +      aOptions.mCharacterDataOldValue.Value() &&
> +      aOptions.mCharacterData.WasPassed() &&

if (characterDataOldValue &&
    aOptions.mCharacterData.WasPassed() &&
    !aOptions.mCharacterData.Value()) {

::: content/base/test/test_mutationobservers.html
@@ +124,5 @@
>      m.observe(document, { childList: true, characterDataOldValue: true });
>    } catch (ex) {
>      e = ex;
>    }
> +  ok(!e, "Should have thrown an exception");

Shouldn't

@@ +132,5 @@
>      m.observe(document);
>    } catch (ex) {
>      e = ex;
>    }
>    ok(e, "Should have thrown an exception");  

can you remove this extra space?

Comment 5

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

> 
> I don't know if easier to understand, but:
I tried to follow the step from the spec, and would prefer to keep it that way.
The local bool variables are there only because aOptions is passes as const

Comment 6

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

Attachment: v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc8d30fd13e7

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc8d30fd13e7

Comment 9

[Eric Shepherd [:sheppy]]

This change is mentioned on Fx 61 for devs and on the main MutationObserver page, as well as to some extent on the page for observe(). I will be finishing this work when I get home from a trip.