PWA: VIewer as an ampdoc service #5300

dvoytenko · 2016-09-29T02:22:18Z

Partial for #3742.
Closes #3406.

dvoytenko · 2016-09-30T02:14:30Z

/to @erwinmombay @aghassemi for review. This change caused a big ripple effect, so please review thoroughly.

erwinmombay · 2016-10-03T17:11:51Z

@dvoytenko LGTM, is the overlay code completely gone or has the mechanism moved somewhere?

aghassemi

nothing major except a possible race in canAnimate_ of vsyn since viewerService is initialized asynchronously there.

aghassemi · 2016-10-03T15:29:56Z

ads/google/a4a/test/test-google-ads-a4a-config.js

@@ -41,8 +42,10 @@ const INTERNAL_BRANCHES = {
 describe('a4a_config', () => {
  let sandbox;
  let win;
+  let ampdoc;


I don't see this actually used outside of beforeEach, maybe inline as installViewerServiceForDoc(ampdocService.getAmpDoc());

aghassemi · 2016-10-03T15:32:31Z

ads/google/a4a/test/test-google-ads-a4a-config.js

    events = {};
    installPlatformService(win);
    documentStateFor(win);
+    const attrs = {};
+    element = {


This element is mocked but the one used above isn't. Can we use the same approach for both?

Yes. This is unfortunate. I'm independently working on test rules to avoid this discrepancy. But it's out of scope for this PR.

aghassemi · 2016-10-03T15:43:00Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+      const ampdocService = ampdocServiceFor(doc.defaultView);
+      return ampdocService.getAmpDoc(element);
+    };
+    doc.body.appendChild(element);


does it need to be appended? If so, probably should remove it in afterEach

This is tested inside of a hermetic doc. The whole document is thrown out so this is ok.

aghassemi · 2016-10-03T16:08:56Z

src/runtime.js

@@ -466,6 +466,18 @@ function prepareAndAttachShadowDoc(global, extensions, hostElement, doc, url) {
  // Instal doc services.
  installAmpdocServices(ampdoc);

+  const viewer = viewerForDoc(ampdoc);


I think this and the same code in adopt can be refactored to adoptShared

No. This part is executed independently and currently is rather different.

aghassemi · 2016-10-03T16:15:25Z

src/service/viewport-impl.js

   */
-  constructor(win) {
+  constructor(win, ampdoc) {


why both? can we do ampdoc.win and remove win param?

I wanted to highlight that win is the main argument. ampdoc is a less important thing thing here for now. ampdoc is only used to fix an iOS bug. So as soon as that code gets cleaned up, I will remove it.

aghassemi · 2016-10-03T16:19:58Z

src/service/vsync-impl.js

+    /** @private @const {!./ampdoc-impl.AmpDocService} */
+    this.ampdocService_ = ampdocServiceFor(this.win);
+
+    /** @const {!../document-state.DocumentState} */


aghassemi · 2016-10-03T16:44:13Z

src/service/vsync-impl.js

-        this.forceSchedule_();
-      }
-    });
+    const boundOnVisibilityChanged = this.onVisibilityChanged_.bind(this);


This is confusing me. I was thrown off by docState being a window-level state and not a per-doc state, but that aside, I don't understand why viewer.onVisibilityChanged is more efficient than docState_.onVisibilityChanged in the singleDoc mode.

Basically, when window == ampdoc, viewer's visibility property can be used to determine visibility for both. In this case it's more efficient to disable animations when ampdoc is not visible. In multi-doc mode, we can only use window signal.

aghassemi · 2016-10-03T16:58:45Z

src/service/vsync-impl.js

+      return false;
+    }
+
+    // Single doc: animations allowed when single doc is visible.


This may have race issue since singleDocViewer_ is initialized asynchronously so it may or may not be null based on timing. Is it guaranteed to have viewerService instantiated by this point? if not, this may either return true or fail if there is a contextNode and viewerForDoc returns null because viewerService is not instantiated yet. If we are guaranteed to have a viewerService instance by this point, can we simplify the code to:

if (this.docState_.isHidden()) { return false; } // getAmpDoc() handles opt_contextNode and caching the singleDoc just fine itself. const ampdoc = this.ampdocService_.getAmpDoc(opt_contextNode); // is there really a need to cache the viewer instance for single viewer case? I assume `viewerForDoc` and therefore `getServiceForDoc` do proper caching of instances anyway. return viewerForDoc(ampdoc).isVisible();

This will be fine. When doc is available, the viewer is guaranteed to be available. The risk condition is ok as well - it will simply yield the same instance in this case. The only place where viewer might not be available is the constructor above.

dvoytenko · 2016-10-03T18:21:01Z

@erwinmombay The overlay code as used to be defined in BaseElement has been deprecated couple of month ago. It's never provided by the Viewport class. This is simply a cleanup on that deprecation.

aghassemi · 2016-10-03T19:43:34Z

LGTM!

* Make Viewer an ampdoc service * review fixes * lints

dvoytenko force-pushed the pwa13-viewer3 branch 2 times, most recently from 98f7c59 to 4dbab6a Compare September 30, 2016 01:44

dvoytenko added the NEEDS REVIEW label Sep 30, 2016

dvoytenko assigned erwinmombay and aghassemi Sep 30, 2016

dvoytenko changed the title ~~WIP PWA: VIewer as an ampdoc service~~ PWA: VIewer as an ampdoc service Sep 30, 2016

dvoytenko force-pushed the pwa13-viewer3 branch from d88809b to 23549d0 Compare September 30, 2016 20:37

erwinmombay approved these changes Oct 3, 2016

View reviewed changes

erwinmombay removed the NEEDS REVIEW label Oct 3, 2016

aghassemi approved these changes Oct 3, 2016

View reviewed changes

Make Viewer an ampdoc service

4cf76eb

dvoytenko force-pushed the pwa13-viewer3 branch from 23549d0 to 4cf76eb Compare October 3, 2016 18:26

Dima Voytenko added 2 commits October 3, 2016 11:44

review fixes

2a60bc8

lints

547e994

dvoytenko merged commit 1ba0160 into ampproject:master Oct 3, 2016

dvoytenko deleted the pwa13-viewer3 branch October 3, 2016 20:42

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Oct 4, 2016

PWA: VIewer as an ampdoc service (ampproject#5300)

26ed02c

* Make Viewer an ampdoc service * review fixes * lints

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Oct 4, 2016

PWA: VIewer as an ampdoc service (ampproject#5300)

3999d13

* Make Viewer an ampdoc service * review fixes * lints

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Oct 12, 2016

PWA: VIewer as an ampdoc service (ampproject#5300)

7afc9aa

* Make Viewer an ampdoc service * review fixes * lints

mityaha pushed a commit to ooyala/amphtml that referenced this pull request Nov 30, 2016

PWA: VIewer as an ampdoc service (ampproject#5300)

880fe8a

* Make Viewer an ampdoc service * review fixes * lints

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PWA: VIewer as an ampdoc service #5300

PWA: VIewer as an ampdoc service #5300

dvoytenko commented Sep 29, 2016

dvoytenko commented Sep 30, 2016

erwinmombay commented Oct 3, 2016

aghassemi left a comment

aghassemi Oct 3, 2016

dvoytenko Oct 3, 2016

aghassemi Oct 3, 2016

dvoytenko Oct 3, 2016

aghassemi Oct 3, 2016

dvoytenko Oct 3, 2016

aghassemi Oct 3, 2016

dvoytenko Oct 3, 2016

aghassemi Oct 3, 2016

dvoytenko Oct 3, 2016

aghassemi Oct 3, 2016

dvoytenko Oct 3, 2016

aghassemi Oct 3, 2016

dvoytenko Oct 3, 2016

aghassemi Oct 3, 2016 •

edited

Loading

dvoytenko Oct 3, 2016

dvoytenko commented Oct 3, 2016

aghassemi commented Oct 3, 2016

PWA: VIewer as an ampdoc service #5300

PWA: VIewer as an ampdoc service #5300

Conversation

dvoytenko commented Sep 29, 2016

dvoytenko commented Sep 30, 2016

erwinmombay commented Oct 3, 2016

aghassemi left a 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

Choose a reason for hiding this comment

aghassemi Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Oct 3, 2016

aghassemi commented Oct 3, 2016

aghassemi Oct 3, 2016 •

edited

Loading