Refactor and test custom marshalling logic #15113

pcardune · 2017-05-15T21:31:34Z

Here's what happened in the following commits:

Renamed PatchedInterpreter to CustomMarshalingInterpreter to better reflect that it's really an extension of the interpreter with customizations (which happen to include patches/overrides of the interpreter)
Converted monkey patches of get/set/hasProperty in JSInterpreter into regular method overrides in CustomMarhsalingInterpreter
Moved custom marshaling configuration and related logic into a new CustomMarshaler class
Added unit tests for:
- the custom marshaling bit of codegen.marshalNativeToInterpreter
- how properties are set on objects with different custom marshaling configurations
- how objects actually get marshaled

There are probably still some more tests to write, but I think this represents a good checkpoint that is worth merging into staging.

islemaster · 2017-05-16T16:58:58Z

apps/src/codegen.js

@@ -300,8 +239,12 @@ export function marshalNativeToInterpreter(interpreter, nativeVar, nativeParentO
  if (typeof maxDepth === "undefined") {
    maxDepth = Infinity; // default to infinite levels of depth
  }
-  if (shouldCustomMarshalObject(nativeVar, nativeParentObj)) {
-    return createCustomMarshalObject(interpreter, nativeVar, nativeParentObj);
+  // TODO (pcardune): remove circular dependency


TODO: Address now, or in a near-future PR?

This TODO is still around after your last batch of changes. I assume you'd like to address it in the future?

islemaster · 2017-05-16T17:00:47Z

apps/src/lib/tools/jsinterpreter/CustomMarshaler.js

@@ -0,0 +1,69 @@
+
+module.exports = class CustomMarshaler {


Why not export default here?

good question, will add a comment explaining. tl;dr: to make it work natively with node.js

islemaster · 2017-05-16T17:01:07Z

apps/src/lib/tools/jsinterpreter/CustomMarshaler.js

+            nativeParentObj[marshalObj.requiredMethod] !== undefined) {
+          return marshalObj.methodOpts || {};
+        } else {
+          return {};


Having just extracted this helper, it'd be nice for it to have 100% test coverage.

islemaster · 2017-05-16T17:02:34Z

apps/src/lib/tools/jsinterpreter/CustomMarshaler.js

+      isPrimitive: false,
+      isCustomMarshal: true,
+      type: typeof nativeObj,
+      parent: nativeParentObj, // TODO (cpirich): replace with interpreter object?


Is this TODO worth keeping around anymore? (@cpirich?)

I think this might break in strange ways with the latest interpreter version, which gets rid of the parent property entirely in favor of something a bit more complicated for which nativeParentObj probably doesn't make any sense.

islemaster · 2017-05-16T17:04:16Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+
+  constructor(code, customMarshaler, opt_initFunc) {
+    super(code);
+    if (!customMarshaler) {


Maybe better? if (!(customMarshaler instanceof CustomMarshaler))

islemaster · 2017-05-16T17:05:09Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+  constructor(code, customMarshaler, opt_initFunc) {
+    super(code);
+    if (!customMarshaler) {
+      throw new Error("You must provide a CustomMarshaler to CustomMarshalingInterpreter");


It'd be nice to cover this error case.

islemaster · 2017-05-16T17:06:03Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+    // this work from the parent class's constructor because it needs
+    // customMarshaler to have been set before-hand. Unfortunately, javascript
+    // does not allow us to set customMarshaler until after the parent constructor
+    // has finished.


This TODO does not actually describe an action item. It should maybe be updated to describe a desired outcome (refactor constructors to do less work, deduplicate code with parent constructor by extracting a helper, etc).

islemaster · 2017-05-16T17:09:15Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+          obj === this.globalScope &&
+          !!(nativeParent = this.customMarshaler.globalProperties[name]) &&
+          !this.shouldBlockCustomMarshalling_(name, obj, nativeParent)) {
+        customMarshalValue = nativeParent[name];


😆 I love that this one case of this method is not covered, especially because I can't really tell what this case is. Something about getting a custom-marshaled property from the interpreter's global scope?

islemaster · 2017-05-16T17:10:38Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+      if (!hasProperty &&
+          obj === this.globalScope &&
+          !!(nativeParent = this.customMarshaler.globalProperties[name]) &&
+          !this.shouldBlockCustomMarshalling_(name, obj, nativeParent)) {


This complex conditional gets reused in getProperty, hasProperty and setProperty. Let's extract a helper.

islemaster · 2017-05-16T17:11:57Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+        }
+        return;
+      }
+      scope = scope.parentScope;


It really seems like we should have a test over this method walking up the scope chain.

islemaster · 2017-05-16T17:14:11Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+  set customMarshalObjectList(customMarshalObjectList) {
+    // TODO: perform validation on this
+    this._customMarshalObjectList = customMarshalObjectList;
+  }


Can these go away yet? They appear to get/set a property that's not used at all within the class, meaning anything calling them is probably broken anyway.

islemaster

Hooray for better naming and new test coverage! 🎉 I've made some comments on cleanup and tightening up tests, but I see that you'd like to use this as a checkpoint. My only real concern before merge is whether the vestigial _customMarshalObjectList property in the interpreter is going to cause problems.

…behavior

pcardune · 2017-05-17T01:06:19Z

I think this got better since the last time you looked at this. PTAL?

islemaster · 2017-05-17T15:57:04Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+       * 4. If the property being looked up has been explicitly blocked from custom
+       * marshalling, then don't return the native parent.
+       */
+      !this.shouldBlockCustomMarshalling_(name, obj, nativeParent)


🤣 Great commenting here. Scary to even have a conditional this complicated and subtle, but great explanation.

islemaster · 2017-05-17T16:05:08Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+    } else {
+      const nativeParent = this.getNativeParent_(obj, name);
+      if (nativeParent) {
+        customMarshalValue = nativeParent[name];


I'm not seeing coverage for these new files from the browser extension, not sure why. Anyway, it looks like this line still isn't covered. Have trouble tracking down this case?

islemaster

This looks great!

islemaster · 2017-05-19T17:57:44Z

@pcardune This change shipped yesterday. We got a Zendesk ticket this morning saying that the edges group (somehow added to the global scope by the createEdgeSprites() function in gamelab) has stopped working, and we repro. My suspicion is that it has something to do with this change. @caleybrock and I are investigating. Any idea what might have happened?

islemaster · 2017-05-19T17:58:45Z

Looks like edges and some others are listed as "custom marshal global properties" for GameLabP5, even though some of them don't exist at initialization time.

caleybrock · 2017-05-19T18:36:47Z

apps/src/lib/tools/jsinterpreter/CustomMarshalingInterpreter.js

+      }
+    } else {
+      if (this.getNativeParent_()) {
+        return true;


Looks like getNativeParent should be called with parameters, like this: this.getNativeParent_(obj, name)
@islemaster is working on a fix

pcardune added 4 commits May 11, 2017 14:45

Add customMarshalObjectList to PatchedInterpreter instance

13f4acb

Rename PatchedInterpreter to CustomMarshalingInterpreter

e7f0c1b

Break out custom marshaling into separate helper object

a02c7f4

Start writing tests for CustomMarshalingInterpreter functionality

05230a9

pcardune requested review from islemaster, cpirich and joshlory May 15, 2017 22:35

islemaster reviewed May 16, 2017

View reviewed changes

More tests and refactoring to increase overall coverage and document …

195acf7

…behavior

islemaster reviewed May 17, 2017

View reviewed changes

islemaster approved these changes May 17, 2017

View reviewed changes

pcardune merged commit ff413c4 into staging May 17, 2017

pcardune deleted the pcardune-custom-marshaling-interpreter branch May 17, 2017 20:47

caleybrock reviewed May 19, 2017

View reviewed changes

islemaster mentioned this pull request May 19, 2017

Bugfix: Interpreter should call hasProperty with arguments #15222

Merged

islemaster mentioned this pull request May 19, 2017

Regression tests for interpreter hasProperty bugfix #15229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and test custom marshalling logic #15113

Refactor and test custom marshalling logic #15113

pcardune commented May 15, 2017

islemaster May 16, 2017

islemaster May 17, 2017

pcardune May 17, 2017

islemaster May 16, 2017

pcardune May 16, 2017

islemaster May 16, 2017

islemaster May 16, 2017

pcardune May 16, 2017

islemaster May 16, 2017

islemaster May 16, 2017

islemaster May 16, 2017

islemaster May 16, 2017

islemaster May 16, 2017

islemaster May 16, 2017

islemaster May 16, 2017

islemaster left a comment

pcardune commented May 17, 2017

islemaster May 17, 2017

islemaster May 17, 2017

islemaster left a comment

islemaster commented May 19, 2017

islemaster commented May 19, 2017

caleybrock May 19, 2017

Refactor and test custom marshalling logic #15113

Refactor and test custom marshalling logic #15113

Conversation

pcardune commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

pcardune commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

islemaster commented May 19, 2017

islemaster commented May 19, 2017

Choose a reason for hiding this comment