From f62755d4b5a5f06655f9fa6844861501265d91ef Mon Sep 17 00:00:00 2001
From: Steven Orvell <sorvell@google.com>
Date: Tue, 6 Nov 2018 12:27:47 -0800
Subject: [PATCH] Changed based on review feedback.

---
 lib/legacy/class.html          | 180 +++++++++++++++++----------------
 test/unit/behaviors.html       |  47 ++++++++-
 test/unit/mixin-behaviors.html | 130 ++++++++++++++++++++----
 3 files changed, 248 insertions(+), 109 deletions(-)

diff --git a/lib/legacy/class.html b/lib/legacy/class.html
index f99679f3a9..99fa627085 100644
--- a/lib/legacy/class.html
+++ b/lib/legacy/class.html
@@ -24,20 +24,20 @@
       attributeChanged: true,
     };
 
-    const noBehaviorCopyProps = Object.assign({
+    const noCopyProps = Object.assign({
       behaviors: true
     }, metaProps);
 
-    const memoizedProps = Object.assign({
+    const filteredProps = Object.assign({
       listeners: true,
       hostAttributes: true
     }, metaProps);
 
     function copyProperties(source, target) {
       for (let p in source) {
-        // NOTE: cannot copy `noBehaviorCopyProps` methods onto prototype at least because
+        // NOTE: cannot copy `noCopyProps` methods onto prototype at least because
         // `super.ready` must be called and is not included in the user fn.
-        if (!(p in noBehaviorCopyProps)) {
+        if (!(p in noCopyProps)) {
           let pd = Object.getOwnPropertyDescriptor(source, p);
           if (pd) {
             Object.defineProperty(target, p, pd);
@@ -95,24 +95,19 @@
     // If lifecycle is called (super then me), order is
     // (1) C.created, (2) A.created, (3) B.created, (4) element.created
     // (again same as 1.x)
-    function copyBehaviorProperties(behaviors, klass) {
-      const meta = {};
-      const superMeta = klass.prototype.__behaviorMetaProps;
-      if (behaviors) {
-        klass.prototype.__behaviorMetaProps = meta;
-        for (let i=0; i<behaviors.length; i++) {
-          copyProperties(behaviors[i], klass.prototype);
-          memoizeBehaviorMetaProps(meta, behaviors[i], superMeta);
-        }
+    function copyAndFilterBehaviors(proto, behaviors, lifecycle) {
+      for (let i=0; i<behaviors.length; i++) {
+        copyAndFilterProperties(proto, behaviors[i], lifecycle);
       }
-      klass.prototype.__behaviorMetaProps = meta;
+      return lifecycle;
     }
 
-    function memoizeBehaviorMetaProps(meta, behavior, superMeta) {
-      for (let p in memoizedProps) {
-        if (behavior[p]) {
-          meta[p] = meta[p] || (superMeta && superMeta[p] ? superMeta[p].slice() : []);
-          meta[p].push(behavior[p]);
+    function copyAndFilterProperties(proto, infoOrBehavior, lifecycle) {
+      copyProperties(infoOrBehavior, proto);
+      for (let p in filteredProps) {
+        if (infoOrBehavior[p]) {
+          lifecycle[p] = lifecycle[p] || [];
+          lifecycle[p].push(infoOrBehavior[p]);
         }
       }
     }
@@ -182,13 +177,42 @@
       }
     }
 
-    function mergeProperties(a, b) {
+    // Note, the properties in `b` are normalized before being merged
+    // into `a`. This means that given `a == {}` and `b == {foo: String}`,
+    // the result is `a == {foo: {type: String}}`.
+    function mergeElementProperties(a, b) {
       for (let p in b) {
         a[p] = a[p] || {};
         mergePropertyInfo(a[p], b[p]);
       }
     }
 
+    /* Note about construction and extension of legacy classes.
+      [Changed in Q4 2018 to optimize performance.]
+
+      When calling `Polymer` or `mixinBehaviors`, the generated class below is
+      made. The list of behaviors was previously made into one generated class per
+      behavior, but this is no longer the case as behaviors are now called
+      manually. Note, there may *still* be multiple generated classes in the
+      element's prototype chain if extension is used with `mixinBehaviors`.
+
+      The generated class is directly tied to the info object and behaviors
+      used to create it. That list of behaviors is filtered so it's only the
+      behaviors not active on the superclass. In order to call through to the
+      entire list of lifecycle methods, it's important to call `super`.
+
+      The element's `properties`, `observers`, and the `_registered` method
+      are controlled via the finalization mechanism provided by `Properties-Mixin`.
+
+      `Properties` and `observers` are collected by manually traversing the prototype
+      chain and merging.
+
+      The `_registered` method is called via `LegacyElementMixin._finalizeClass`
+      and is called on each prototype in the element's chain. Because a non-legacy
+      element may extend a legacy one, it's important that work in `_registered`
+      carefully act only once.
+
+    */
     /**
      * @param {!PolymerInit} info Polymer info object
      * @param {function(new:HTMLElement)} Base base class to extend with info object
@@ -199,26 +223,31 @@
      */
     function GenerateClassFromInfo(info, Base, behaviors) {
 
+      // manages behavior and lifecycle processing (filled in after class definition)
+      let registered = false;
+      let activeBehaviors;
+      const lifecycle = {};
+
       /** @private */
       class PolymerGenerated extends Base {
 
         static get properties() {
           const properties = {};
-          if (this.prototype.__behaviors) {
-            for (let i=0, b; i < this.prototype.__behaviors.length; i++) {
-              b = this.prototype.__behaviors[i];
-              mergeProperties(properties, b.properties);
+          if (activeBehaviors) {
+            for (let i=0, b; i < activeBehaviors.length; i++) {
+              b = activeBehaviors[i];
+              mergeElementProperties(properties, b.properties);
             }
           }
-          mergeProperties(properties, info.properties);
+          mergeElementProperties(properties, info.properties);
           return properties;
         }
 
         static get observers() {
           let observers = [];
-          if (this.prototype.__behaviors) {
-            for (let i=0, b; i < this.prototype.__behaviors.length; i++) {
-              b = this.prototype.__behaviors[i];
+          if (activeBehaviors) {
+            for (let i=0, b; i < activeBehaviors.length; i++) {
+              b = activeBehaviors[i];
               if (b.observers) {
                 observers = observers.concat(b.observers);
               }
@@ -234,15 +263,13 @@
          * @return {void}
          */
         created() {
-          const list = this.__behaviorMetaProps.created;
+          super.created();
+          const list = lifecycle.created;
           if (list) {
             for (let i=0; i < list.length; i++) {
               list[i].call(this);
             }
           }
-          if (info.created) {
-            info.created.call(this);
-          }
         }
 
         /**
@@ -258,29 +285,28 @@
            `is` in `beforeRegister` as you could in 1.x.
           */
           const proto = this;
-          if (proto.hasOwnProperty('__behaviors')) {
-            copyBehaviorProperties(proto.__behaviors, proto.constructor);
-          }
-          proto.__behaviorMetaProps = proto.__behaviorMetaProps || {};
-          copyProperties(info, proto);
-          // Note, previously these were interleaved.
-          let list = proto.__behaviorMetaProps.beforeRegister;
-          if (list) {
-            for (let i=0; i < list.length; i++) {
-              list[i].call(proto);
+          // NOTE: this `registered` flag is required so that extensions
+          // that do not override `_registered` do not try to "re-register"
+          // this data. Only extensions that use `mixinBehaviors` will normally
+          // have this implementation.
+          if (!registered) {
+            registered = true;
+            if (activeBehaviors) {
+              copyAndFilterBehaviors(proto, activeBehaviors, lifecycle);
             }
-          }
-          list = proto.__behaviorMetaProps.registered;
-          if (list) {
-            for (let i=0; i < list.length; i++) {
-              list[i].call(proto);
+            copyAndFilterProperties(proto, info, lifecycle);
+            let list = lifecycle.beforeRegister;
+            if (list) {
+              for (let i=0; i < list.length; i++) {
+                list[i].call(proto);
+              }
+            }
+            list = lifecycle.registered;
+            if (list) {
+              for (let i=0; i < list.length; i++) {
+                list[i].call(proto);
+              }
             }
-          }
-          if (info.beforeRegister) {
-            info.beforeRegister.call(proto);
-          }
-          if (info.registered) {
-            info.registered.call(proto);
           }
         }
 
@@ -288,7 +314,8 @@
          * @return {void}
          */
         _applyListeners() {
-          const list = this.__behaviorMetaProps.listeners;
+          super._applyListeners();
+          const list = lifecycle.listeners;
           if (list) {
             for (let i=0; i < list.length; i++) {
               const listeners = list[i];
@@ -299,11 +326,6 @@
               }
             }
           }
-          if (info.listeners) {
-            for (let l in info.listeners) {
-              this._addMethodEventListenerToNode(this, l, info.listeners[l]);
-            }
-          }
         }
 
         // note: exception to "super then me" rule;
@@ -313,12 +335,7 @@
          * @return {void}
          */
         _ensureAttributes() {
-          if (info.hostAttributes) {
-            for (let a in info.hostAttributes) {
-              this._ensureAttribute(a, info.hostAttributes[a]);
-            }
-          }
-          const list = this.__behaviorMetaProps.hostAttributes;
+          const list = lifecycle.hostAttributes;
           if (list) {
             for (let i=list.length-1; i >= 0; i--) {
               const hostAttributes = list[i];
@@ -327,6 +344,7 @@
                 }
             }
           }
+          super._ensureAttributes();
         }
 
         /**
@@ -334,45 +352,38 @@
          */
         ready() {
           super.ready();
-          let list = this.__behaviorMetaProps.ready;
+          let list = lifecycle.ready;
           if (list) {
             for (let i=0; i < list.length; i++) {
               list[i].call(this);
             }
           }
-          if (info.ready) {
-            info.ready.call(this);
-          }
         }
 
         /**
          * @return {void}
          */
         attached() {
-          let list = this.__behaviorMetaProps.attached;
+          super.attached();
+          let list = lifecycle.attached;
           if (list) {
             for (let i=0; i < list.length; i++) {
               list[i].call(this);
             }
           }
-          if (info.attached) {
-            info.attached.call(this);
-          }
         }
 
         /**
          * @return {void}
          */
         detached() {
-          let list = this.__behaviorMetaProps.detached;
+          super.detached();
+          let list = lifecycle.detached;
           if (list) {
             for (let i=0; i < list.length; i++) {
               list[i].call(this);
             }
           }
-          if (info.detached) {
-            info.detached.call(this);
-          }
         }
 
         /**
@@ -385,19 +396,17 @@
          * @return {void}
          */
         attributeChanged(name, old, value) {
-          let list = this.__behaviorMetaProps.attributeChanged;
+          super.attributeChanged();
+          let list = lifecycle.attributeChanged;
           if (list) {
             for (let i=0; i < list.length; i++) {
               list[i].call(this, name, old, value);
             }
           }
-          if (info.attributeChanged) {
-            info.attributeChanged.call(this, name, old, value);
-          }
         }
       }
 
-      // apply behaviors
+      // apply behaviors, note actual copying is done lazily at first instance creation
       if (behaviors) {
         // NOTE: ensure the behavior is extending a class with
         // legacy element api. This is necessary since behaviors expect to be able
@@ -405,12 +414,11 @@
         if (!Array.isArray(behaviors)) {
           behaviors = [behaviors];
         }
-        let superBehaviors = PolymerGenerated.prototype.behaviors;
+        let superBehaviors = Base.prototype.behaviors;
         // get flattened, deduped list of behaviors *not* already on super class
-        behaviors = flattenBehaviors(behaviors, null, superBehaviors);
+        activeBehaviors = flattenBehaviors(behaviors, null, superBehaviors);
         PolymerGenerated.prototype.behaviors = superBehaviors ?
-          superBehaviors.concat(behaviors) : behaviors;
-        PolymerGenerated.prototype.__behaviors = behaviors;
+          superBehaviors.concat(behaviors) : activeBehaviors;
       }
 
       PolymerGenerated.generatedFrom = info;
diff --git a/test/unit/behaviors.html b/test/unit/behaviors.html
index 98cf74dd7d..d7f23ea09a 100644
--- a/test/unit/behaviors.html
+++ b/test/unit/behaviors.html
@@ -340,6 +340,37 @@
       ]
     });
 
+    window.ModifyObserversBehavior = {
+
+      __barChangedCalled: 0,
+
+      beforeRegister: function() {
+        const observers = this.constructor.generatedFrom.observers;
+        this.constructor.generatedFrom.observers = observers.concat([
+          '_barChanged(bar)'
+        ]);
+      },
+
+      _barChanged: function() {
+        this.__barChangedCalled++;
+      }
+
+    };
+
+    Polymer({
+      is: 'modify-observers-via-behavior',
+      __zonkChangedCalled: 0,
+      observers: [
+        '_zonkChanged(zonk)'
+      ],
+      behaviors: [
+        window.ModifyObserversBehavior
+      ],
+      _zonkChanged: function() {
+        this.__zonkChangedCalled++;
+      }
+    });
+
   });
   </script>
 
@@ -396,7 +427,13 @@
       <template-from-behavior-registered></template-from-behavior-registered>
     </template>
   </test-fixture>
-  
+
+  <test-fixture id="modify-observers-via-behavior">
+    <template>
+      <modify-observers-via-behavior></modify-observers-via-behavior>
+    </template>
+  </test-fixture>
+
 <script>
 
 suite('single behavior element', function() {
@@ -479,6 +516,14 @@
     assert.equal(el.beforeRegisterBehaviors, el.behaviors);
   });
 
+  test('modify element observers', function() {
+    var el = fixture('modify-observers-via-behavior');
+    el.bar = 1;
+    assert.equal(el.__barChangedCalled, 1);
+    el.zonk = 1;
+    assert.equal(el.__zonkChangedCalled, 1);
+  });
+
 });
 
 
diff --git a/test/unit/mixin-behaviors.html b/test/unit/mixin-behaviors.html
index 6f4dc50428..e00218dc25 100644
--- a/test/unit/mixin-behaviors.html
+++ b/test/unit/mixin-behaviors.html
@@ -19,9 +19,6 @@
   <script>
   HTMLImports.whenReady(function() {
     window.LifeCycleBehavior1 = {
-      _calledCreated: 0,
-      _calledAttached: 0,
-      _calledAttributeChanged: 0,
 
       properties: {
         foo: {
@@ -30,27 +27,45 @@
       },
 
       created() {
-        this._calledCreated++;
+        this.__createdList = this.__createdList || [];
+        this.__createdList.push('1');
       },
 
       attached() {
-        this._calledAttached++;
+        this.__attachedList = this.__attachedList || [];
+        this.__attachedList.push('1');
       },
 
       attributeChanged() {
-        this._calledAttributeChanged++;
+        this.__attributeChangedList = this.__attributeChangedList || [];
+        this.__attributeChangedList.push('1');
       }
     };
 
     window.LifeCycleBehavior2 = {
       created() {
-        this._calledCreated++;
+        this.__createdList = this.__createdList || [];
+        this.__createdList.push('2');
       },
 
       attached() {
-        this._calledAttached++;
-      },
+        this.__attachedList = this.__attachedList || [];
+        this.__attachedList.push('2');
+      }
+    };
 
+    window.LifeCycleBehavior3 = {
+      created() {
+        this.__createdList = this.__createdList || [];
+        this.__createdList.push('3');
+      }
+    };
+
+    window.LifeCycleBehavior4 = {
+      created() {
+        this.__createdList = this.__createdList || [];
+        this.__createdList.push('4');
+      }
     };
 
     window.BehaviorA = {
@@ -252,18 +267,40 @@
     </script>
   </dom-module>
 
-  <dom-module id="nested-behaviors">
     <script>
     HTMLImports.whenReady(function() {
+
       customElements.define('nested-behaviors',
         class extends Polymer.mixinBehaviors([window.BehaviorD, window.LifeCycleBehavior1], Polymer.mixinBehaviors(
           [
             [window.BehaviorB, [window.BehaviorC, window.BehaviorB], window.BehaviorA, window.LifeCycleBehavior2],
           ], Polymer.Element)) {
       });
+
+      var base = Polymer({
+        is: 'sup-element',
+        behaviors: [
+          window.LifeCycleBehavior1,
+          window.LifeCycleBehavior2
+        ],
+        created: function() {
+          this.__createdList.push('sup');
+        }
+      });
+
+      class extended extends Polymer.mixinBehaviors([window.LifeCycleBehavior3,
+          window.LifeCycleBehavior4], base) {
+
+        created() {
+          super.created();
+          this.__createdList.push('sub');
+        }
+      }
+
+      customElements.define('extended-behaviors', extended);
+
     });
     </script>
-  </dom-module>
 
   <dom-module id="behavior-registered">
     <template>
@@ -273,6 +310,7 @@
     HTMLImports.whenReady(function() {
 
       window.registerBehavior1 ={
+        registeredCount: 0,
         registered: function() {
           this._createPropertyObserver('prop', 'propChanged1');
           this._createMethodObserver('propChanged2(prop)');
@@ -320,21 +358,37 @@
         }
       }
 
-      BehaviorRegistered.prototype.registeredCount = 0;
-
       customElements.define(BehaviorRegistered.is, BehaviorRegistered);
 
       class BehaviorRegisteredExt extends BehaviorRegistered {
         static get is() { return 'behavior-registered-ext';}
       }
 
-      BehaviorRegisteredExt.prototype.registeredCount = 0;
-
       customElements.define(BehaviorRegisteredExt.is, BehaviorRegisteredExt);
     });
     </script>
   </dom-module>
 
+  <script>
+    HTMLImports.whenReady(function() {
+      var klass = Polymer.mixinBehaviors([
+        {
+          __fooChangedCalled: 0,
+          beforeRegister: function() {
+            this.constructor.generatedFrom.observers = [
+              '_fooChanged(foo)'
+            ];
+          },
+          _fooChanged: function() {
+            this.__fooChangedCalled++;
+          }
+        }
+      ], Polymer.Element);
+
+      customElements.define('before-register-observers', klass);
+    });
+  </script>
+
   <test-fixture id="single">
     <template>
       <single-behavior></single-behavior>
@@ -359,12 +413,24 @@
     </template>
   </test-fixture>
 
+  <test-fixture id="extended-behaviors">
+    <template>
+      <extended-behaviors></extended-behaviors>
+    </template>
+  </test-fixture>
+
   <test-fixture id="registered">
     <template>
       <behavior-registered></behavior-registered>
     </template>
   </test-fixture>
 
+  <test-fixture id="before-register-observers">
+    <template>
+      <before-register-observers></before-register-observers>
+    </template>
+  </test-fixture>
+
   <test-fixture id="registered-ext">
     <template>
       <behavior-registered-ext></behavior-registered-ext>
@@ -409,7 +475,7 @@
 
 });
 
-suite('behavior.registered', function() {
+suite('behavior.registered/beforeRegister', function() {
   test('can install dynamic properties', function() {
     var el = fixture('registered');
     assert.ok(el.$.content);
@@ -435,6 +501,12 @@
     assert.deepEqual(el.registeredProps, [true, true, true]);
   });
 
+  test('add observers via behavior in beforeRegister', function() {
+    var el = fixture('before-register-observers');
+    el.foo = 1;
+    assert.equal(el.__fooChangedCalled, 1);
+  });
+
 });
 
 suite('behavior lifecycle', function() {
@@ -446,9 +518,9 @@
     });
 
     test('lifecycle', function() {
-      assert.equal(el._calledCreated, 2, 'created call count wrong');
-      assert.equal(el._calledAttached, 2, 'attached call count wrong');
-      assert.equal(el._calledAttributeChanged, 1,  'attributeChanged call count wrong');
+      assert.deepEqual(el.__createdList, ['1', '2'], 'created list wrong');
+      assert.deepEqual(el.__attachedList, ['1', '2'], 'attached list wrong');
+      assert.deepEqual(el.__attributeChangedList, ['1'],  'attributeChanged list wrong');
     });
 
   });
@@ -555,9 +627,9 @@
   });
 
   test('nested-behavior lifecycle', function() {
-    assert.equal(el._calledCreated, 2, 'created call count wrong');
-    assert.equal(el._calledAttached, 2, 'attached call count wrong');
-    assert.equal(el._calledAttributeChanged, 1,  'attributeChanged call count wrong');
+    assert.deepEqual(el.__createdList, ['2', '1'], 'created list wrong');
+    assert.deepEqual(el.__attachedList, ['2', '1'], 'attached list wrong');
+    assert.deepEqual(el.__attributeChangedList, ['1'],  'attributeChanged list wrong');
   });
 
   test('nested-behavior overrides ordering', function() {
@@ -570,6 +642,20 @@
 
 });
 
+suite('extended-behaviors', function() {
+
+  var el;
+
+  setup(function() {
+    el = fixture('extended-behaviors');
+  });
+
+  test('lifecycle', function() {
+    assert.deepEqual(el.__createdList, ['1', '2', 'sup', '3', '4', 'sub'], 'created list wrong');
+  });
+
+});
+
 </script>
 
 </body>