Defer Layout Assertions till Upgrade (ampproject#8229)

* Remove attached and upgrade duplication

They had very tangled state depending on the state of the element:

- Stubbed
- `#attachedCallback`
- sync `#firstAttachedCallback`  (on a stub!)
- sync `amp:stubbed`
- `#upgrade`
- deferred "full" upgrade
- deferred `#firstAttachedCallback`
- deferred `amp:attached`
- deferred `Resources#build`
- BaseElement, sync "full" upgrade
- `#attachedCallback`
- sync "full" upgrade
- sync `#firstAttachedCallback`
- sync `amp:attached`
- sync `#firstAttachedCallback` (twice!)
- sync `Resources#build`
- BaseElement, async "full" upgrade
- `#attachedCallback`
- sync `#firstAttachedCallback` (on nothing useful!)
- sync `amp:stubbed`
- `#upgrade`
- deferred "full" upgrade
- deferred `#firstAttachedCallback`
- deferred `amp:attached`
- deferred `Resources#build`

This moves all that code into the `#completeUpgrade`, which is either
called sync (if upgrade can be done sync) or async (if not). Thus we
get all of the same behavior without mixing code between the two
methods.

* Remove bogus first `#firstAttachedCallback`

It's either called on a stubbed element (useless), a sync upgraded
element twice(!), or a deferred upgraded element (useless).

It will be called a second time after the upgrade is complete.

* Remove layout assertions from attachedCallback

We must wait until the element is fully upgraded.

* Make sure resources is added before it's upgraded

* Make sure resources is rebuilt if needed after being reparented

* Only mark the resoruce as upgraded when element is upgraded

* Fix tests

* Fix more tests

extensions/amp-social-share/0.1/test/test-amp-social-share.js

@@ -88,31 +88,32 @@ describe('amp-social-share', () => {
     return createIframePromise().then(iframe => {
       const share = iframe.doc.createElement('amp-social-share');
       share.setAttribute('type', 'unknown-provider');
-      expect(() => {
-        share.tryUpgrade_();
-        share.build(true);
-      }).to.throw('data-share-endpoint attribute is required');
+      iframe.doc.body.appendChild(share);
+      return expect(share.whenBuilt())
+          .to.be.eventually.rejectedWith(
+            /data-share-endpoint attribute is required/
+          );
     });
   });
 
   it('errors if type is missing', () => {
     return createIframePromise().then(iframe => {
       const share = iframe.doc.createElement('amp-social-share');
-      expect(() => {
-        share.tryUpgrade_();
-        share.build(true);
-      }).to.throw('type attribute is required');
+      iframe.doc.body.appendChild(share);
+      return expect(share.whenBuilt())
+          .to.be.eventually.rejectedWith(/type attribute is required/);
     });
   });
 
   it('errors if type has space characters', () => {
     return createIframePromise().then(iframe => {
       const share = iframe.doc.createElement('amp-social-share');
       share.setAttribute('type', 'hello world');
-      expect(() => {
-        share.tryUpgrade_();
-        share.build(true);
-      }).to.throw('Space characters are not allowed in type attribute value');
+      iframe.doc.body.appendChild(share);
+      return expect(share.whenBuilt())
+          .to.be.eventually.rejectedWith(
+            /Space characters are not allowed in type attribute value/
+          );
     });
   });
 

extensions/amp-user-notification/0.1/test/test-amp-user-notification.js

@@ -67,7 +67,7 @@ describe('amp-user-notification', () => {
     button.setAttribute('on', 'tap:' + elem.getAttribute('id') + 'dismiss');
     elem.appendChild(button);
 
-    elem.tryUpgrade_();
+    doc.body.appendChild(elem);
     const impl = elem.implementation_;
     impl.storagePromise_ = Promise.resolve(storage);
     impl.userNotificationManager_ = {

src/custom-element.js

@@ -619,6 +619,10 @@ function createBaseCustomElementClass(win) {
       }
       this.implementation_ = new newImplClass(this);
       if (this.everAttached) {
+        // Usually, we do an implementation upgrade when the element is
+        // attached to the DOM. But, if it hadn't yet upgraded from
+        // ElementStub, we couldn't. Now that it's upgraded from a stub, go
+        // ahead and do the full upgrade.
         this.tryUpgrade_();
       }
     }
@@ -637,13 +641,9 @@ function createBaseCustomElementClass(win) {
       this.assertLayout_();
       this.implementation_.layout_ = this.layout_;
       this.implementation_.layoutWidth_ = this.layoutWidth_;
-      if (this.everAttached) {
-        this.implementation_.firstAttachedCallback();
-        this.dispatchCustomEventForTesting('amp:attached');
-        // For a never-added resource, the build will be done automatically
-        // via `resources.add` on the first attach.
-        this.getResources().upgraded(this);
-      }
+      this.implementation_.firstAttachedCallback();
+      this.dispatchCustomEventForTesting('amp:attached');
+      this.getResources().upgraded(this);
     }
 
     /* @private */
@@ -933,37 +933,38 @@ function createBaseCustomElementClass(win) {
         // Resources can now be initialized since the ampdoc is now available.
         this.resources_ = resourcesForDoc(this.ampdoc_);
       }
-      if (!this.everAttached) {
-        if (!isStub(this.implementation_)) {
-          this.tryUpgrade_();
+      this.getResources().add(this);
+
+      if (this.everAttached) {
+        const reconstruct = this.reconstructWhenReparented();
+        if (reconstruct) {
+          this.reset_();
         }
-        if (!this.isUpgraded()) {
-          this.classList.add('amp-unresolved');
-          this.classList.add('i-amphtml-unresolved');
+        if (this.isUpgraded()) {
+          if (reconstruct) {
+            this.getResources().upgraded(this);
+          }
+          this.dispatchCustomEventForTesting('amp:attached');
         }
+      } else {
+        this.everAttached = true;
+
         try {
           this.layout_ = applyLayout_(this);
-          this.assertLayout_();
-          this.implementation_.layout_ = this.layout_;
-          this.implementation_.firstAttachedCallback();
-          if (!this.isUpgraded()) {
-            // amp:attached is dispatched from the ElementStub class when it
-            // replayed the firstAttachedCallback call.
-            this.dispatchCustomEventForTesting('amp:stubbed');
-          } else {
-            this.dispatchCustomEventForTesting('amp:attached');
-          }
         } catch (e) {
           reportError(e, this);
         }
-
-        // It's important to have this flag set in the end to avoid
-        // `resources.add` called twice if upgrade happens immediately.
-        this.everAttached = true;
-      } else if (this.reconstructWhenReparented()) {
-        this.reset_();
+        if (!isStub(this.implementation_)) {
+          this.tryUpgrade_();
+        }
+        if (!this.isUpgraded()) {
+          this.classList.add('amp-unresolved');
+          this.classList.add('i-amphtml-unresolved');
+          // amp:attached is dispatched from the ElementStub class when it
+          // replayed the firstAttachedCallback call.
+          this.dispatchCustomEventForTesting('amp:stubbed');
+        }
       }
-      this.getResources().add(this);
     }
 
     /** The Custom Elements V0 sibling to `connectedCallback`. */

src/service/resources-impl.js

@@ -468,7 +468,6 @@ export class Resources {
     } else {
       // Create and add a new resource.
       resource = new Resource((++this.resourceIdCounter_), element, this);
-      this.buildOrScheduleBuildForResource_(resource);
       dev().fine(TAG_, 'resource added:', resource.debugid);
     }
     this.resources_.push(resource);