Mutation observer API #16190

srivassumit · 2017-03-30T08:47:36Z

Changes for implementing the Initial Steps of the Mutation Observer API as described here

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #6633 (initial steps).

There are tests for these changes OR
These changes do not require tests because _____

This change is


        Initial Files for MutationObserverAPI


        add a vector of MutationObserver objects as a member of ScriptThread


        Merge branch 'master' of https://github.com/servo/servo into Mutation…

…Observer-dev merging changes from upstream Merge branch 'master' of https://github.com/servo/servo into MutationObserver-dev merging changes from upstream


        Merge branch 'master' of https://github.com/servo/servo into Mutation…

…Observer-dev merge from upstream


        incomplete code for mutationobserver and mutationrecord


        fixed build errors, Constructor is pending


        Resolving merge conflicts


        added Mutation Observer Constructor


        Merge branch 'MutationObserver-dev' of https://github.com/srivassumit…

…/servo into MutationObserver-dev merged


        Merge branch 'master' of https://github.com/servo/servo into Mutation…

…Observer-dev merged from upstream


        changes for constructor


        Merge branch 'master' of https://github.com/servo/servo into Mutation…

…Observer-dev merge upstream

highfive · 2017-03-30T08:47:42Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

highfive · 2017-03-30T08:47:42Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/mod.rs, components/script/dom/webidls/MutationObserver.webidl, components/script/dom/bindings/codegen/Bindings.conf, components/script/dom/mutationobserver.rs, components/script/script_thread.rs, components/script/dom/mutationrecord.rs, components/script/dom/webidls/MutationRecord.webidl
@KiChjang: components/script/dom/mod.rs, components/script/dom/webidls/MutationObserver.webidl, components/script/dom/bindings/codegen/Bindings.conf, components/script/dom/mutationobserver.rs, components/script/script_thread.rs, components/script/dom/mutationrecord.rs, components/script/dom/webidls/MutationRecord.webidl

highfive · 2017-03-30T08:47:44Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2017-03-30T08:54:19Z

components/script/dom/bindings/codegen/Bindings.conf

@@ -18,6 +18,16 @@ DOMInterfaces = {
    'weakReferenceable': True,
 },

+'MutationObserver': {


Is this really needed? I'd expect it to just work without this.

emilio · 2017-03-30T08:54:19Z

components/script/dom/mutationobserver.rs

+
+impl MutationObserver {
+
+   fn new(global: &dom::bindings::js::Root<dom::window::Window>, callback: MutationCallback) -> MutationObserver {


Please fix indentation in the whole file so it's indented to 4 spaces.

emilio · 2017-03-30T08:54:19Z

...s/wpt/metadata/dom/nodes/Document-contentType/contentType/contenttype_javascripturi.html.ini

@@ -1,5 +1,5 @@
 [contenttype_javascripturi.html]
  type: testharness
  [Javascript URI document.contentType === 'text/html']
-    expected: FAIL
+    expected: TIMEOUT


Are all these tests really timing out with your changes?

An easy way to check is to make a build on a branch that does not include any of the changes in this PR and run the tests again.

Those tests timed out without our changes. We ran the tests on a unmodified servo code.

emilio · 2017-03-30T08:54:32Z

r? @jdm

jdm

This is a good start! Please remove the servo submodule that was added in this PR, and if you're having trouble with compile errors then please ask specific questions. I was surprised to see code in mutationobserve.rs that can't compile.

jdm · 2017-03-30T12:45:51Z

components/script/dom/mutationobserver.rs

+        }
+    }
+
+  pub fn Constructor(global: &dom::bindings::js::Root<dom::window::Window>, callback: Rc<MutationCallback>) -> Result<Root<MutationObserver>, Error> {


This should be &Window instead of &Root<Window>.

jdm · 2017-03-30T12:45:51Z

components/script/dom/mutationobserver.rs

+  pub fn Constructor(global: &dom::bindings::js::Root<dom::window::Window>, callback: Rc<MutationCallback>) -> Result<Root<MutationObserver>, Error> {
+  	let observer = MutationObserver::new(global, &callback);
+    ScriptThread.set_mutation_observer(observer)
+  }


This can't compile right now since the return type doesn't match what the code is doing. Please use the new/new_inherited methods from other files in dom/ as a guide.

jdm · 2017-03-30T12:45:51Z

components/script/dom/mutationobserver.rs

+
+impl MutationObserverMethods for MutationObserver {
+
+  //void observe(Node target, optional MutationObserverInit options);


The comments should contain links to the DOM specification like https://dom.spec.whatwg.org/#dom-mutationobserver-observe instead.

jdm · 2017-03-30T12:45:51Z

components/script/dom/mutationrecord.rs

+pub struct MutationRecord {
+  reflector_: Reflector,
+
+//  readonly attribute DOMString record_type;


Rather than duplicating IDL constructs in the comments, we should be describing what these fields used are for in English prose instead.

jdm · 2017-03-30T12:45:51Z

components/script/script_thread.rs

@@ -480,6 +481,9 @@ pub struct ScriptThread {

    microtask_queue: MicrotaskQueue,

+    /// a vector of MutationObserver objects
+    mutation_observers: Vec<MutationObserver>,


This should be Vec<JS<MutationObserver>> instead.

jdm · 2017-03-30T12:45:51Z

...s/wpt/metadata/dom/nodes/Document-contentType/contentType/contenttype_javascripturi.html.ini

@@ -1,5 +1,5 @@
 [contenttype_javascripturi.html]
  type: testharness
  [Javascript URI document.contentType === 'text/html']
-    expected: FAIL
+    expected: TIMEOUT


An easy way to check is to make a build on a branch that does not include any of the changes in this PR and run the tests again.

jdm · 2017-03-30T12:45:51Z

components/script/dom/webidls/MutationRecord.webidl

+  readonly attribute Node? nextSibling;
+  readonly attribute DOMString? attributeName;
+  readonly attribute DOMString? attributeNamespace;
+  readonly attribute DOMString? oldValue;


Rather than implementing stubs for all of these attributes, let's comment out all of them in this file except for type and target. We can uncomment them when we give them real implementations.

jdm · 2017-03-30T12:45:51Z

components/script/dom/webidls/MutationObserver.webidl

+interface MutationObserver {
+  void observe(Node target, optional MutationObserverInit options);
+  void disconnect();
+  sequence<MutationRecord> takeRecords();


Let's comment out all of these methods in this file until we actually implement them.

jdm · 2017-03-30T12:45:51Z

components/script/dom/mutationrecord.rs

+  attributeNamespace: DOMString,
+
+//  readonly attribute DOMString? oldValue;
+  oldValue: DOMString,


Given my request to remove the methods we don't implement yet, let's remove all of these members except for record_type and target.

jdm · 2017-03-30T12:45:51Z

components/script/dom/mutationrecord.rs

+
+//  [SameObject]
+//  readonly attribute Node target;
+  target: Root<Node>,


This should be JS<Node> instead.

Hi Josh, when I make this change, it gives me the following error:

error[E0053]: method Targethas an incompatible type for trait --> /home/sumit/Dropbox/NCSU/OODD/Servo/MyWork/servo/components/script/dom/mutationrecord.rs:38:25 | 38 | fn Target(&self) -> JS<Node> { | ^^^^^^^^ expected structdom::bindings::js::Root, found struct dom::bindings::js::JS| ::: /home/sumit/Dropbox/NCSU/OODD/Servo/MyWork/servo/target/debug/build/script-bb44ff0630eb7b72/out/Bindings/MutationRecordBinding.rs | 617 | fn Target(&self) -> Root<Node>; | ---------- type in trait | = note: expected typefn(&dom::mutationrecord::MutationRecord) -> dom::bindings::js::Rootdom::node::Nodefound typefn(&dom::mutationrecord::MutationRecord) -> dom::bindings::js::JSdom::node::Node`

It seems that it is expecting a Root element in the '/servo/target/debug/build/script-bb44ff0630eb7b72/out/Bindings/MutationRecordBinding.rs' file

The field in the struct should be JS<Node>, but the return value of fn Target(&self) should be Root<Node>. Use Root::from_ref(&*self.target) to get a Root<T> from a JS<T>.

jdm · 2017-03-30T12:47:34Z

Also, I'm very confused that ./mach test-tidy does not show any errors for you, given that there are some clear style violations that I would expect it to report. Luckily, our CI picks it up just fine:

Checking files for tidiness...

./components/script/dom/mutationobserver.rs:1: incorrect license

./components/script/dom/mutationobserver.rs:31: Line is longer than 120 characters

./components/script/dom/mutationobserver.rs:32: tab on line

./components/script/dom/mutationobserver.rs:42: tab on line

./components/script/dom/mutationobserver.rs:48: tab on line

./components/script/dom/mutationobserver.rs:54: tab on line

./components/script/dom/mutationobserver.rs:4: use statement is not in alphabetical order

	expected: dom_struct::dom_struct

	found: dom::bindings::codegen::Bindings::MutationObserverBinding::MutationCallback

./components/script/dom/mutationobserver.rs:23: found an empty line following a {

./components/script/dom/mutationobserver.rs:39: found an empty line following a {

./components/script/dom/mutationobserver.rs:41: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationobserver.rs:47: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationobserver.rs:52: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:1: incorrect license

./components/script/dom/mutationrecord.rs:3: use statement is not in alphabetical order

	expected: dom_struct::dom_struct

	found: dom::bindings::codegen::Bindings::MutationRecordBinding::MutationRecordBinding::MutationRecordMethods

./components/script/dom/mutationrecord.rs:6: use statement is not in alphabetical order

	expected: dom::bindings::str::DOMString

	found: dom::bindings::reflector::Reflector

./components/script/dom/mutationrecord.rs:66: found an empty line following a {

./components/script/dom/mutationrecord.rs:67: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:73: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:77: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:81: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:85: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:95: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:105: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:115: method declared in webidl is missing a comment with a specification link

./components/script/dom/mutationrecord.rs:125: method declared in webidl is missing a comment with a specification link

./components/script/dom/webidls/MutationObserver.webidl:0: No specification link found.

./components/script/dom/webidls/MutationObserver.webidl:1: incorrect license

./components/script/dom/webidls/MutationRecord.webidl:0: No specification link found.

./components/script/dom/webidls/MutationRecord.webidl:1: incorrect license

./resources/prefs.json:None: Unordered key (found dom.mutation_observer.enabled before dom.mozbrowser.enabled)


        Update mutationrecord.rs

made the changes as per the review comments in PR

jdm · 2017-04-05T01:51:28Z

components/script/dom/mutationobserver.rs

-        let observer = MutationObserver::new(global, Rc::deref(&callback));
-        ScriptThread::add_mutation_observer(&observer)
+    pub fn Constructor(global: &Window, callback: Rc<MutationCallback>) -> Fallible<Root<MutationObserver>> {
+        let observer = MutationObserver::new(global, callback);


You should be calling reflect_dom_object here (per the documentation); follow the pattern of other code that has a constructor method like Blob.

jdm · 2017-04-05T01:51:28Z

components/script/dom/mutationobserver.rs

-        ScriptThread::add_mutation_observer(&observer)
+    pub fn Constructor(global: &Window, callback: Rc<MutationCallback>) -> Fallible<Root<MutationObserver>> {
+        let observer = MutationObserver::new(global, callback);
+        ScriptThread::add_mutation_observer(Root::from_ref(&*observer))


add_mutation_observer should accept a &MutationObserver argument instead of Root<MutationObserver>.

jdm · 2017-04-05T01:51:28Z

components/script/dom/mutationobserver.rs

-        ScriptThread::add_mutation_observer(&observer)
+    pub fn Constructor(global: &Window, callback: Rc<MutationCallback>) -> Fallible<Root<MutationObserver>> {
+        let observer = MutationObserver::new(global, callback);
+        ScriptThread::add_mutation_observer(Root::from_ref(&*observer))
    }


This method will need to return Ok(observer) once observer is a Root<MutationObserver> value.

jdm · 2017-04-05T01:52:48Z

./mach test-tidy reports:

./components/script/dom/webidls/MutationObserver.webidl:1: incorrect license
./components/script/dom/webidls/MutationRecord.webidl:1: incorrect license
./resources/prefs.json:None: Unordered key (found dom.mutation_observer.enabled before dom.mozbrowser.enabled)

jdm · 2017-04-05T01:55:52Z

components/script/dom/mutationobserver.rs

+#[dom_struct]
+pub struct MutationObserver {
+    reflector_: Reflector,
+    callback: MutationCallback,


This should be Rc<MutationCallback>. To avoid the errors about heapsize::HeapSizeOf, you can add this annotation above this field: #[ignore_heap_size_of = "can't measure Rc values"]

Thanks Josh,

I have fixed the test-tidy issues, for some reason those were not visible when I ran the command locally.
I have also made the other changes as per the comments. Now the mutation observer part is not giving any errors, but there is an error in the script thread file at line 583:
"error: the type of this value must be known in this context" I cannot figure out why is it an error, the method is exactly the same as the mark_document_with_no_blocked_loads method.


        changes as per review comments


        	Merge branch 'master' of https://github.com/servo/servo into Mutatio…

…nObserver-dev merged upstream

jdm

Only a couple small changes left! Thanks for doing this!

jdm · 2017-04-05T04:50:33Z

components/script/script_thread.rs

@@ -18,6 +18,7 @@
 //! loop.

 use bluetooth_traits::BluetoothRequest;
+use core::borrow::BorrowMut;


Doesn't this cause a warning about an unused import?

It was giving me an error when I used the borrow_mut() in the add_mutation_observer, now that I have change it to DOMRefCell, I guess this won't be needed anymore. I'll remove it.

jdm · 2017-04-05T04:50:33Z

components/script/script_thread.rs

@@ -575,10 +576,11 @@ impl ScriptThreadFactory for ScriptThread {

 impl ScriptThread {
    pub fn add_mutation_observer(observer: &MutationObserver) {
-        SCRIPT_THREAD_ROOT.with(|root| {
+       SCRIPT_THREAD_ROOT.with(|root| {


nit: reindent by one extra space.

jdm · 2017-04-05T04:50:33Z

components/script/dom/mutationrecord.rs

@@ -37,7 +37,7 @@ impl MutationRecord {
 impl MutationRecordMethods for MutationRecord {
    // https://dom.spec.whatwg.org/#dom-mutationrecord-type
    fn Type(&self) -> DOMString {
-        return self.record_type;
+        DOMString::from(self.record_type.clone())


This only needs self.record.clone(); the DOMString::from is unnecessary.

jdm · 2017-04-05T04:50:33Z

components/script/dom/mutationobserver.rs

+        reflect_dom_object(boxed_observer, global, MutationObserverBinding::Wrap)
+    }
+
+    pub fn new_inherited(callback: Rc<MutationCallback>) -> MutationObserver {


Let's not make these methods public. Only the constructor should be used here.

jdm · 2017-04-05T04:54:19Z

components/script/script_thread.rs

@@ -480,6 +482,9 @@ pub struct ScriptThread {

    microtask_queue: MicrotaskQueue,

+    /// The unit of related similar-origin browsing contexts' list of MutationObserver objects
+    mutation_observers: Vec<JS<MutationObserver>>,


This needs to be DOMRefCell<Vec<JS<MutationObserver>>> (which provides the borrow_mut() method that you're calling).


        removed unused imports and fixed review comments.


        removed unused import

jdm · 2017-04-05T05:54:33Z

I took the liberty of squashing your commits into one and removing the many merge commits that this PR included. I've pushed the squashed branch to #16268.


        Auto merge of #16268 - jdm:tmp, r=<try>

Basic MutationObserver interface stubs Rebase and squash of #16190.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16268)


        Auto merge of #16268 - jdm:tmp, r=jdm

Basic MutationObserver interface stubs Rebase and squash of #16190.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16268)


        servo: Merge #16268 - Basic MutationObserver interface stubs (from jd…

…m:tmp); r=jdm Rebase and squash of servo/servo#16190. Source-Repo: https://github.com/servo/servo Source-Revision: 8e2a1477ae800b86eae45fc9c6daf85615100854 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : febfe9bf5d1c77941bdc9a4e820a0e08741b2d57


        servo: Merge #16268 - Basic MutationObserver interface stubs (from jd…

…m:tmp); r=jdm Rebase and squash of servo/servo#16190. Source-Repo: https://github.com/servo/servo Source-Revision: 8e2a1477ae800b86eae45fc9c6daf85615100854


        servo: Merge #16268 - Basic MutationObserver interface stubs (from jd…

…m:tmp); r=jdm Rebase and squash of servo/servo#16190. Source-Repo: https://github.com/servo/servo Source-Revision: 8e2a1477ae800b86eae45fc9c6daf85615100854


        servo: Merge #16268 - Basic MutationObserver interface stubs (from jd…

…m:tmp); r=jdm Rebase and squash of servo/servo#16190. Source-Repo: https://github.com/servo/servo Source-Revision: 8e2a1477ae800b86eae45fc9c6daf85615100854 UltraBlame original commit: 2dbda4bb18ff68e7932dbe50949102cb16d9d36e


        servo: Merge #16268 - Basic MutationObserver interface stubs (from jd…

…m:tmp); r=jdm Rebase and squash of servo/servo#16190. Source-Repo: https://github.com/servo/servo Source-Revision: 8e2a1477ae800b86eae45fc9c6daf85615100854 UltraBlame original commit: 2dbda4bb18ff68e7932dbe50949102cb16d9d36e


        servo: Merge #16268 - Basic MutationObserver interface stubs (from jd…

…m:tmp); r=jdm Rebase and squash of servo/servo#16190. Source-Repo: https://github.com/servo/servo Source-Revision: 8e2a1477ae800b86eae45fc9c6daf85615100854 UltraBlame original commit: 2dbda4bb18ff68e7932dbe50949102cb16d9d36e

srivassumit and others added 12 commits Mar 16, 2017

Initial Files for MutationObserverAPI

73f2c6e

add a vector of MutationObserver objects as a member of ScriptThread

bee1340

Merge branch 'master' of https://github.com/servo/servo into Mutation…

a5af9a2

…Observer-dev merging changes from upstream Merge branch 'master' of https://github.com/servo/servo into MutationObserver-dev merging changes from upstream

Merge branch 'master' of https://github.com/servo/servo into Mutation…

4ec5d92

…Observer-dev merge from upstream

incomplete code for mutationobserver and mutationrecord

16c09d4

fixed build errors, Constructor is pending

e162f8a

Resolving merge conflicts

b0af820

added Mutation Observer Constructor

8e6d4e9

Merge branch 'MutationObserver-dev' of https://github.com/srivassumit…

4797cc3

…/servo into MutationObserver-dev merged

Merge branch 'master' of https://github.com/servo/servo into Mutation…

dd9ea19

…Observer-dev merged from upstream

changes for constructor

4db8c26

Merge branch 'master' of https://github.com/servo/servo into Mutation…

Loading status checks…

1a7e2d4

…Observer-dev merge upstream

highfive assigned emilio Mar 30, 2017

highfive added the S-awaiting-review label Mar 30, 2017

emilio reviewed Mar 30, 2017

View changes

highfive assigned jdm and unassigned emilio Mar 30, 2017

jdm requested changes Mar 30, 2017

View changes

jdm added S-fails-tidy S-needs-code-changes S-needs-squash and removed S-awaiting-review labels Mar 30, 2017

Update mutationrecord.rs

Loading status checks…

ef51869

made the changes as per the review comments in PR

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 31, 2017

jdm requested changes Apr 5, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 5, 2017

jdm requested changes Apr 5, 2017

View changes

changes as per review comments

Loading status checks…

2821def

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 5, 2017

Merge branch 'master' of https://github.com/servo/servo into Mutatio…

Loading status checks…

a82e79d

…nObserver-dev merged upstream

jdm requested changes Apr 5, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-fails-tidy labels Apr 5, 2017

jdm reviewed Apr 5, 2017

View changes

removed unused imports and fixed review comments.

Loading status checks…

3bd4438

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 5, 2017

removed unused import

Loading status checks…

c6739ef

jdm mentioned this pull request Apr 5, 2017

Basic MutationObserver interface stubs #16268

Merged

jdm closed this Apr 5, 2017

srivassumit deleted the srivassumit:MutationObserver-dev branch Apr 19, 2017


		impl MutationObserver {

		fn new(global: &dom::bindings::js::Root<dom::window::Window>, callback: MutationCallback) -> MutationObserver {


		impl MutationObserverMethods for MutationObserver {

		//void observe(Node target, optional MutationObserverInit options);

servo / servo

Join GitHub today

Mutation observer API #16190

Mutation observer API #16190

Conversation

srivassumit commented Mar 30, 2017 • edited

highfive commented Mar 30, 2017

highfive commented Mar 30, 2017

highfive commented Mar 30, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

krishnakarthick1993 Apr 1, 2017 • edited

emilio commented Mar 30, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

srivassumit Mar 31, 2017 • edited

This comment has been minimized.

jdm commented Mar 30, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Apr 5, 2017

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Apr 5, 2017

srivassumit commented Mar 30, 2017 •

edited

krishnakarthick1993 Apr 1, 2017 •

edited

srivassumit Mar 31, 2017 •

edited