A4A: Do not set visibility hidden for iframe attached via A4A flow

[keithwrightbos]

Fixes #4212.

[keithwrightbos]

Accidentally based from a4a upgradeCallback PR. Apologies. Please only refer to commit "Do not set visibility hidden when rendering via iframe from A4A"

[jridgewell]

Inline comments go before the param.

[jridgewell]

Please only refer to commit "Do not set visibility hidden when rendering via iframe from A4A"

You can rebase with git rebase -i HEAD~3. Then delete the unwanted commit lines, and force push to origin.

[keithwrightbos]

Removed a4a upgrade callback changes. Back to @jridgewell Thanks!

[jridgewell]

LGTM. You'll need to rebase of latest master, though.

[jridgewell]

Reopening this one. @keithwrightbos, appears we have a test failure:

    TypeError: Failed to execute 'decode' on 'TextDecoder': The provided value is not of type '(ArrayBuffer or ArrayBufferView)'
        at TypeError (native)
        at utf8FromArrayBuffer (/tmp/de3d7ddf4c118c5299fb6069caeb2dae.browserify:6504:53 <- /home/travis/build/ampproject/amphtml/extensions/amp-a4a/0.1/amp-a4a.js:82:52)
        at AmpA4A.maybeRenderAmpAd_ (/tmp/de3d7ddf4c118c5299fb6069caeb2dae.browserify:7011:14 <- /home/travis/build/ampproject/amphtml/extensions/amp-a4a/0.1/amp-a4a.js:546:11)
        at /tmp/de3d7ddf4c118c5299fb6069caeb2dae.browserify:7883:20 <- /home/travis/build/ampproject/amphtml/extensions/amp-a4a/0.1/test/test-amp-a4a.js:406:19

[keithwrightbos]

@jridgewell I believe this is unrelated to the changes I made however I believe I have pushed a fix. @tdrl I believe this occurred as a result of the structure you committed recently.

[tdrl]

I don't think this is the right fix. maybeRenderAmpAd_() should be able to handle a null input. That should cause utf8FromArrayBuffer() to throw an error. It looks like the missing bit is that utf8FromArrayBuffer() is missing a .catch() clause. (Alternatively, maybeRenderAmpAd_() could just check for null on input and return Promise.resolve(false) immediately.)

I could make that change if you like, or you could on this PR. Happy either way.

What puzzles me is that this test passed when I submitted the previous PR. (Otherwise it wouldn't have gone in.) Once again, I'm suspicious of the unit test framework's reliability. :-(

[keithwrightbos]

I went this route because the current documentation indicates ArrayBuffer
input cannot be null which given the code the calls it first checks that it
is not null makes sense to me:

return creative && this.maybeRenderAmpAd_(creative);

/**

• Render a validated AMP creative directly in the parent page.
• @param {!ArrayBuffer} bytes The creative, as raw bytes.
• @return {Promise} Whether the creative was successfully

• rendered.
  

• @Private
  */
  maybeRenderAmpAd_(bytes) { ... }

On Fri, Jul 29, 2016 at 11:30 AM, Terran Lane notifications@github.com
wrote:

In extensions/amp-a4a/0.1/test/test-amp-a4a.js
#4215 (comment):

@@ -403,7 +403,9 @@ describe('amp-a4a', () => {
const doc = fixture.doc;
const a4aElement = doc.createElement('amp-a4a');
const a4a = new AmpA4A(a4aElement);

•    return a4a.maybeRenderAmpAd_(null).then(rendered => {
  

•    a4a.adUrl_ = 'http://foo.com';
  

•    a4a.maybeRenderAmpAd_ = function() { return Promise.resolve(false); }
  

I don't think this is the right fix. maybeRenderAmpAd_() should be able
to handle a null input. That should cause utf8FromArrayBuffer() to throw
an error. It looks like the missing bit is that utf8FromArrayBuffer() is
missing a .catch() clause. (Alternatively, maybeRenderAmpAd_() could just
check for null on input and return Promise.resolve(false) immediately.)

I could make that change if you like, or you could on this PR. Happy
either way.

What puzzles me is that this test passed when I submitted the previous
PR. (Otherwise it wouldn't have gone in.) Once again, I'm suspicious of the
unit test framework's reliability. :-(

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/pull/4215/files/b61b2e8d312d2b3918f6723a42c14d28ac6728ee..ede9c7390fa511bd7f1fa11e994aa43e157ba889#r72811353,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABr0LyXRo0Lfe0kYrRH8Jpizdvccvx8Gks5qahyEgaJpZM4JVYV9
.

[tdrl]

Yeah... I think that's my mistake in writing the docs. :-) The output from validateAdResponse_() can be null:

return verifySignature(
          new Uint8Array(creative), signature, publicKeyInfos).then(isValid => {
            return isValid ? creative : null;
          });

And I think the cut-out for checking creative && ... is knee-jerk more than anything.

The intended behavior is: "If the thing validates, then it should pass a valid creative ArrayBuffer along. Anything else (i.e., null) should be interpreted as invalid." But I lost track of who was responsible for handling the null case. By adding the cut-out, I prevented null from entering maybeRenderAmpAd() in the pipeline flow. But then this test forced null in. Since maybeRenderAmpAd doesn't have a check for that, it errors when trying to convert null to utf8.

I guess formerly that error was correctly interpreted as render failure, but now it's not?

[keithwrightbos]

@tdrl I agree that the output from validateAdResponse_ can be null but I think the creative && ... check is sufficient for now. As we have discussed offline, we need to rework the flow to handle empty creative (either from empty XHR response or empty creative from extract). I believe our plan was to check the state at each point within the promise chain flow as opposed to within each private function call (in other words as we are doing now as opposed to allowing empty creative state to be passed to validateAdResponse_).

[tdrl]

Oh, ok. I see what you're saying. This will end up changing in the future, as we tighten up the promise chain regardless. For the moment, though, your fix LGTM.

[jridgewell]

Need a semicolon.

[jridgewell]

One last style nit to pass lint, then LGTM.

[keithwrightbos]

@jridgewell - I fixed the remaining lint issue. Thanks