Created attributes.js#7129
Created attributes.js#7129lannka merged 16 commits intoampproject:masterfrom google:frizz-create-attributes-file
Conversation
|
These are both 3p iframe specific. Maybe put in the 3p directory? |
|
These will also be used in the A4A flow for non-amp ads, so it is not only for 3p frames. |
But that just creates a 3p frame... right? |
|
No, although it is rendering a non-amp ad, it doesn't actually use a 3p-frame. https://github.com/ampproject/amphtml/blob/master/extensions/amp-a4a/0.1/amp-a4a.js#L1077 |
| * - data-* attributes of the <amp-ad> tag with the "data-" removed. | ||
| * - A _context object for internal use. | ||
| */ | ||
| export function getContextMetadata( |
There was a problem hiding this comment.
What code can be deleted from https://github.com/ampproject/amphtml/blob/master/src/3p-frame.js?
There was a problem hiding this comment.
I can remove a fair amount (which I have done, but it's not in this pr). If you'd like that all to be in this PR, I can do that, but getting this attributes.js file in for the next release is important for us fixing an issue with a4a so I didn't want to slow down if there were any issues integrating this file into 3p-frame.js
There was a problem hiding this comment.
As long as there's a quick follow up. I hate adding duplicated code.
for the next release is important for us fixing an issue with a4a
Where's the code using this?
|
this blocks #7132 |
|
LGTM, @lannka? |
|
Would like to get this into today's release |
lannka
left a comment
There was a problem hiding this comment.
Can you also move the test here?
BTW, this code is not used, why do you care if it gets cut in this release?
I'm not sure your exact refactoring plan. Is it hard to remove the old code in the same PR? That will be easier for reviewer to see the diff.
src/iframe-attributes.js
Outdated
| attributes.src = adSrc; | ||
| } | ||
| return attributes; | ||
|
|
|
I want it in this release so that I can also get #7132 in today's release (which this is blocking) |
|
Refactored it as requested |
|
Thanks @bradfrizzell . were there any tests need to be moved as well? |
|
I think the tests in test-3p-frame provide good testing of the getContextMetadata, there was nothing in the other PR already written that wasn't moved at least. I can add more if you think getContextMetadata isn't well tested enough. In the pr that uses it for a4a, there are tests of it's use for the a4a case that will cover it more |
|
@bradfrizzell I think it make a lot sense to move those tests from |
|
reverted the last two commits that I made to break out the tests, because I was getting really weird errors with state leakage in the tests. basically, test-iframe-attributes.js was passing if I ran it alone, but if I ran it as part of the suite as a whole it was not. Getting this PR into today's cut is of very high importance to the A4A team, so I need to push off breaking this test apart if we can |
lannka
left a comment
There was a problem hiding this comment.
@bradfrizzell please add a TODO in the test-3p-frame.js to indicate the part needs to be moved to new test.
* Created attributes.js * Whitelisted src/attributes for unconfirmed referrer url * Renamed attributes.js to iframe-attributes.js * Fixed name of file in presubmit-checks * Add sentinel name change exp / add func to generate sentinel and context * Refactored to use getContextMetadata * Removed blank line * Removed unneeded import * Moved function to fix check-types and fixed docstring * Made new test file / broke out tests * Changed test name * Revert "Changed test name" This reverts commit a509951. * Revert "Made new test file / broke out tests" This reverts commit 1961923. * Added todo
* Created attributes.js * Whitelisted src/attributes for unconfirmed referrer url * Renamed attributes.js to iframe-attributes.js * Fixed name of file in presubmit-checks * Add sentinel name change exp / add func to generate sentinel and context * Refactored to use getContextMetadata * Removed blank line * Removed unneeded import * Moved function to fix check-types and fixed docstring * Made new test file / broke out tests * Changed test name * Revert "Changed test name" This reverts commit a509951. * Revert "Made new test file / broke out tests" This reverts commit 1961923. * Added todo
This new file provides the getContextMetadata and getNameAttribute functions. getContextMetadata is a generalized function for assigning the metadata needed for setting up window.context
This addition was part of #6825 but I've realized that PR had a mixture of things that could be better suited to review by breaking apart further.