Closed Bug 973638 Opened 6 years ago Closed 5 years ago

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

Categories

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

30 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: crimsteam, Assigned: smaug, Mentored)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

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:)
Summary: Update MutationObserver.observe() method → Update MutationObserver.observe() method - more flexible options
Summary: Update MutationObserver.observe() method - more flexible options → Update MutationObserver.observe() method - more flexible options and throw a JavaScript TypeError
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).
Attached patch v1 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=e944cda6cf9e
Assignee: nobody → bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
Attachment #8508930 - Flags: review?(amarchesini)
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?
Attachment #8508930 - Flags: review?(amarchesini) → review+
> 
> 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
https://hg.mozilla.org/mozilla-central/rev/dc8d30fd13e7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Duplicate of this bug: 933862
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.
You need to log in before you can comment on or make changes to this bug.