Implement Object.getNotifier(obj).performChange()

src/object-observe.js

Index: src/object-observe.js
diff --git a/src/object-observe.js b/src/object-observe.js
index 77409b95748198194764d20322d954b559161bd6..fe4640e10e667e5fe6580c1978710c7d5c25c7a0 100644
--- a/src/object-observe.js
+++ b/src/object-observe.js
@@ -66,18 +66,148 @@ function CreateObjectInfo(object) {
   var info = {
     changeObservers: new InternalArray,
     notifier: null,
+    inactiveObservers: new InternalArray,
+    performing: { __proto__: null },
+    performingCount: 0,
   };
   objectInfoMap.set(object, info);
   return info;
 }
 
-function ObjectObserve(object, callback) {
+var defaultAcceptTypes = {
+  __proto__: null,
+  'new': true,
+  'updated': true,
+  'deleted': true,
+  'prototype': true,
+  'reconfigured': true
+};
+
+function createObserver(callback, accept) {
+  var observer = {
+    __proto__: null,
+    callback: callback,
+    accept: defaultAcceptTypes
+  };
+
+  if (IS_UNDEFINED(accept))
+    return observer;
+
+  var acceptMap = { __proto__: null };
+  for (var i = 0; i < accept.length; i++)
+    acceptMap[accept[i]] = true;
+
+  observer.accept = acceptMap;
+  return observer;
+}
+
+function observerIsActive(observer, objectInfo) {
+  if (objectInfo.performingCount === 0)
+    return true;
+
+  var performing = objectInfo.performing;
+  for (var type in performing) {
+    if (performing[type] > 0 && observer.accept[type])
+      return false;
+  }
+
+  return true;
+}
+
+function observerIsInactive(observer, objectInfo) {
+  return !observerIsActive(observer, objectInfo);
+}
+
+function removeNullElements(from) {
+  var i = 0;
+  var j = 0;
+  for (; i < from.length; i++, j++) {
+    if (i === j)
+      continue;
+
+    if (from[i] === null) {
+      j--;
+    } else {
+      from[j] = from[i];
+    }
+  }
+
+  if (i !== j)
+    from.length = from.length - (i - j);
+}
+
+function moveObserversWhichAre(conditionFn, from, to, objectInfo) {
+  var anyRemoved = false;
+  for (var i = 0; i < from.length; i++) {
+    var observer = from[i];
+    if (conditionFn(observer, objectInfo)) {
+      anyRemoved = true;
+      from[i] = null;
+      to.push(observer);
+    }
+  }
+
+  if (anyRemoved)
+    removeNullElements(from);
+}
+
+function BeginPerformChange(objectInfo, type) {
+  if (IS_UNDEFINED(objectInfo.performing[type]))
+    objectInfo.performing[type] = 1
+  else
+    objectInfo.performing[type]++;
+
+  objectInfo.performingCount++;
+  moveObserversWhichAre(observerIsInactive,
+                        objectInfo.changeObservers,
+                        objectInfo.inactiveObservers,
+                        objectInfo);
+}
+
+function EndPerformChange(objectInfo, type) {
+  objectInfo.performing[type]--;
+  objectInfo.performingCount--;
+  moveObserversWhichAre(observerIsActive,
+                        objectInfo.inactiveObservers,
+                        objectInfo.changeObservers,
+                        objectInfo);
+}
+
+function ensureObserverRemoved(observerList, callback) {
+  for (var i = 0; i < observerList.length; i++) {
+    if (observerList[i].callback === callback) {
+      observerList.splice(i, 1);
+      return;
+    }
+  }
+}
+
+function AcceptArgIsValid(arg) {

[rafaelw, 2013/05/09 15:39:50]

Arv,

what do you think about 

a) Accepting Array-likes?
b) Not accepting anything other than strings within the Array-like?

[arv (Not doing code reviews), 2013/05/09 20:23:50]

This looks right to me. We should just iterate over 0 - length - 1 and verify
that they are strings. Anything else is irrelevant.

+  if (IS_UNDEFINED(arg))
+    return true;
+
+  if (!IS_SPEC_OBJECT(arg) ||
+      !IS_NUMBER(arg.length) ||
+      arg.length < 0)
+    return false;
+
+  var length = arg.length;
+  for (var i = 0; i < length; i++) {
+    if (!IS_STRING(arg[i]))
+      return false;
+  }
+  return true;
+}
+
+function ObjectObserve(object, callback, accept) {
   if (!IS_SPEC_OBJECT(object))
     throw MakeTypeError("observe_non_object", ["observe"]);
   if (!IS_SPEC_FUNCTION(callback))
     throw MakeTypeError("observe_non_function", ["observe"]);
   if (ObjectIsFrozen(callback))
     throw MakeTypeError("observe_callback_frozen");
+  if (!AcceptArgIsValid(accept))
+    throw MakeTypeError("observe_accept_invalid");
 
   if (!observerInfoMap.has(callback)) {
     observerInfoMap.set(callback, {
@@ -90,8 +220,14 @@ function ObjectObserve(object, callback) {
   if (IS_UNDEFINED(objectInfo)) objectInfo = CreateObjectInfo(object);
   %SetIsObserved(object, true);
 
-  var changeObservers = objectInfo.changeObservers;
-  if (changeObservers.indexOf(callback) < 0) changeObservers.push(callback);
+  ensureObserverRemoved(objectInfo.changeObservers, callback);
+  ensureObserverRemoved(objectInfo.inactiveObservers, callback);
+
+  var observer = createObserver(callback, accept);
+  if (observerIsActive(observer, objectInfo))
+    objectInfo.changeObservers.push(observer);
+  else
+    objectInfo.inactiveObservers.push(observer);
 
   return object;
 }
@@ -106,11 +242,12 @@ function ObjectUnobserve(object, callback) {
   if (IS_UNDEFINED(objectInfo))
     return object;
 
-  var changeObservers = objectInfo.changeObservers;
-  var index = changeObservers.indexOf(callback);
-  if (index >= 0) {
-    changeObservers.splice(index, 1);
-    if (changeObservers.length === 0) %SetIsObserved(object, false);
+  ensureObserverRemoved(objectInfo.changeObservers, callback);
+  ensureObserverRemoved(objectInfo.inactiveObservers, callback);

[adamk, 2013/05/09 16:37:07]

This pair of calls happens twice. Seems like it should just be a single function
that takes an objectInfo and removed the observer from whichever list it's in
(that'd also be a very minor optimization, as you wouldn't need to walk through
the inactiveObservers if it was found in changeObservers).

[rafaelw, 2013/05/09 18:47:47]

Done.

+
+  if (objectInfo.changeObservers.length === 0 &&
+      objectInfo.inactiveObservers.length === 0) {
+    %SetIsObserved(object, false);
   }
 
   return object;
@@ -122,8 +259,12 @@ function EnqueueChangeRecord(changeRecord, observers) {
 
   for (var i = 0; i < observers.length; i++) {
     var observer = observers[i];
-    var observerInfo = observerInfoMap.get(observer);
-    observationState.pendingObservers[observerInfo.priority] = observer;
+    if (IS_UNDEFINED(observer.accept[changeRecord.type]))
+      continue;
+
+    var callback = observer.callback;
+    var observerInfo = observerInfoMap.get(callback);
+    observationState.pendingObservers[observerInfo.priority] = callback;
     %SetObserverDeliveryPending();
     if (IS_NULL(observerInfo.pendingChangeRecords)) {
       observerInfo.pendingChangeRecords = new InternalArray(changeRecord);
@@ -135,6 +276,9 @@ function EnqueueChangeRecord(changeRecord, observers) {
 
 function NotifyChange(type, object, name, oldValue) {
   var objectInfo = objectInfoMap.get(object);
+  if (objectInfo.changeObservers.length === 0)
+    return;
+
   var changeRecord = (arguments.length < 4) ?
       { type: type, object: object, name: name } :
       { type: type, object: object, name: name, oldValue: oldValue };
@@ -173,6 +317,36 @@ function ObjectNotifierNotify(changeRecord) {
   EnqueueChangeRecord(newRecord, objectInfo.changeObservers);
 }
 
+function ObjectNotifierPerformChange(changeType, changeFn, receiver) {
+  if (!IS_SPEC_OBJECT(this))
+    throw MakeTypeError("called_on_non_object", ["performChange"]);
+
+  var target = notifierTargetMap.get(this);
+  if (IS_UNDEFINED(target))
+    throw MakeTypeError("observe_notify_non_notifier");
+  if (!IS_STRING(changeType))
+    throw MakeTypeError("observe_perform_non_string");
+  if (!IS_SPEC_FUNCTION(changeFn))
+    throw MakeTypeError("observe_perform_non_function");
+
+  if (IS_NULL_OR_UNDEFINED(receiver)) {
+    receiver = %GetDefaultReceiver(changeFn) || receiver;
+  } else if (!IS_SPEC_OBJECT(receiver) && %IsClassicModeFunction(changeFn)) {
+    receiver = ToObject(receiver);
+  }
+
+  var objectInfo = objectInfoMap.get(target);
+  if (IS_UNDEFINED(objectInfo))
+    return;
+
+  BeginPerformChange(objectInfo, changeType);
+  try {

[rafaelw, 2013/05/09 15:39:50]

Andreas,

Is crankshaft going to ignore this try/finally? If so, how should be approach
this?

We need to be sure to clean-up in the case that the the changeFn throws?

[rossberg, 2013/05/13 16:39:17]

Yes, Crankshaft won't be able to handle it, hence will never optimise this
function. I'm not sure how performance-critical it actually is, though. In any
case, you can always pull out the 'try' into an auxiliary function.

+    %_CallFunction(receiver, changeFn);
+  } finally {
+    EndPerformChange(objectInfo, changeType);
+  }
+}
+
 function ObjectGetNotifier(object) {
   if (!IS_SPEC_OBJECT(object))
     throw MakeTypeError("observe_non_object", ["getNotifier"]);
@@ -235,7 +409,8 @@ function SetupObjectObserve() {
     "unobserve", ObjectUnobserve
   ));
   InstallFunctions(notifierPrototype, DONT_ENUM, $Array(
-    "notify", ObjectNotifierNotify
+    "notify", ObjectNotifierNotify,
+    "performChange", ObjectNotifierPerformChange
   ));
 }