Whitelist of Variable Substitutions #3

hamousavi · 2018-02-05T17:36:30Z

If a whitelist of variables is provided using meta tags for example as follows
<meta name="amp-variable-substitution-whitelist" content="RANDOM,TIMEZONE">
then the runtime should respect it. In order to do this we parse the meta tag in GlobalVariableSource.

…s not contain functional testing

hamousavi · 2018-02-05T17:38:41Z

@raywainman @zhangsu

I managed to add some functional tests. But still have difficulty in adding functional tests to test-url-replacements.js. For some reason the test case cannot find the "meta tag" when I embed it to the window.

* document batching features * add reportWindow * minor fixes * add refer to * fix test

* 📖 add AMP emoji conventional changelogs * apply recommendations

…t#13263)

…project#13266) * Rename lightbox-viewer to lightbox-gallery * Rename lightbox css from viewer to gallery * Remove dead manual test pages

* Return play promise from playFn to ensure media playback is not interrupted. * Refactor playFn to always return a promise. Improve documentation. * Use closure so the proper context is set on the distance function. * Add debug messages * Remove muted property change, since this is in a separate PR * Fix merge

* Component skeleton * move files into amp-story * clean up example * remove bad link * bad mock test * clean up

* fix dist with alias * change minimal_set usage * fix documentation * refactor print logic * rename * update function description

zhangsu · 2018-02-05T22:34:13Z

src/service/url-replacements-impl.js

+
+      // Cache the whitelist of allowed AMP actions (if provided).
+      if (meta) {
+        this.ampVariableSubstitutionWhitelist_ =


This field needs JSDoc.

It might be better to move the initialization logic to a helper method and use null as the default value for this field. ?string is usually more idiomatic than string|undefined.

zhangsu · 2018-02-05T22:35:05Z

src/service/url-replacements-impl.js

@@ -74,6 +74,22 @@ export class GlobalVariableSource extends VariableSource {
    /** @const {!./ampdoc-impl.AmpDoc} */
    this.ampdoc = ampdoc;

+    // A meta[name="amp-action-whitelist"] tag, if present, contains,
+    // in its content attribute, a whitelist of actions on the special AMP target.
+    if (this.ampVariableSubstitutionWhitelist_ === undefined


This code is in a constructor, and there's no previous assignment to ampVariableSubstitutionWhitelist_, so this condition should always be true.

zhangsu · 2018-02-05T22:36:30Z

src/service/url-replacements-impl.js

+    // A meta[name="amp-action-whitelist"] tag, if present, contains,
+    // in its content attribute, a whitelist of actions on the special AMP target.
+    if (this.ampVariableSubstitutionWhitelist_ === undefined
+      && this.ampdoc.getRootNode() && this.ampdoc.getRootNode().head) {


The return value of getRootNode() is non-nullable and never undefined, so we shouldn't need to check this.ampdoc.getRootNode() ...

zhangsu · 2018-02-05T22:39:01Z

src/service/url-replacements-impl.js

+      if (meta) {
+        this.ampVariableSubstitutionWhitelist_ =
+          meta.getAttribute('content').split(',')
+              .map(action => action.trim());


I see that this code is repeated from the previous PR. Consider extracting this to a utility class.

zhangsu · 2018-02-05T22:45:53Z

src/service/variable-source.js

@@ -121,6 +121,9 @@ export class VariableSource {

    /** @private {boolean} */
    this.initialized_ = false;
+
+    /** @const @private {!Array<string>|undefined} */


If this is private, and if the type checker is doing the correct thing, then subclasses should never be able to access this field... I think we really want a (ideally-abstract) method.

That said, why do we implement the actual meta tag check in the base class instead of here in the base class? Are there other implementations of VariableSource that makes whitelisting using meta tags not make sense?

GlobalVariableSource is the only implementation of VariableSource. So I moved this logic to the base class as you suggested.

…d bookend. (ampproject#13270) * Preventing CSS overrides. * Preventing CSS overrides for the navigation hint, landscape orientation and unsupported browser.

…3277)

…13278) * Increase button tap target size * Fix desktop mode top padding * tweak button padding to 16px per mulptiple of 4 * Reorder media queries

…ject#13295)

) * Linear transitions for media story pages progress bar. * Unobfuscate the unobfuscated code.

* Updating to new async tag * Linter fix: imports must be alphabetical

…s not contain functional testing

…ct#13342)

* chore(package): update browserify to version 16.0.0 * chore(package): update lockfile https://npm.im/greenkeeper-lockfile

* change amp-bind verification to user warning and clarify message * fix presubmit * fix test

* Revision bump for ampproject#13036 * Revision bump for ampproject#13073 * Add DEDUPE_ON_MINIFY flags to license tags so that release process can reduce the number of identical duplicate licenses in the minified validator. * Add requires_extension to AttrSpec. * JSDoc updates. * Generated validator javascript improvements. * Add comment to ValidationError hinting at how to render. * Revision bump for ampproject#12955 * Add new error types for future CSS validation. * Revision bump for ampproject#12798 * Fix a typo. * Allow animation-timing-function for keyframes * Fix typo

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

* Development/debug setup. * Basic exp setup. * Header parsing and stuff. * Client-side changes fin. * Cleaned PR + tests. * Undoing yarn stuff. * Yarn attempt #2. * Reverting example page. * lint fix * PR feedback. * Undoing changes to test page. * Merges * Yarn * Not calling document.open/close when writing in body and not waiting for onload. Also removed accidentally merged tests. * Adding back document.open/close into default flow. * Simplified conditional. * Adsense support. * Merges/rebase. * Alphabetized imports. * Factored out common code to utils.

…s not contain functional testing

zhangsu · 2018-02-07T22:25:31Z

Could you merge upstream into your master branch? Currently your variable branch includes changes from upstream but master doesn't, so the diff is huge...

Merge conflict

…s not contain functional testing

hamousavi added 2 commits February 5, 2018 12:31

the code for whitelist of variables in url substitution. The code doe…

cdbabc6

…s not contain functional testing

Added some functional tests

c221629

rsimha and others added 13 commits February 5, 2018 12:57

Fix import ordering in amp-access-scroll (ampproject#13265)

ffbee60

<amp-facebook-*>Add data-locale attribute (ampproject#13204)

332ca05

Document batchPlugin and how vendor add their plugin (ampproject#13065)

fba9fec

* document batching features * add reportWindow * minor fixes * add refer to * fix test

📖 add AMP emoji conventional changelogs (ampproject#13037)

910d343

* 📖 add AMP emoji conventional changelogs * apply recommendations

cron job from @erwinmombay to update size.txt and size.csv (ampprojec…

e2dedbd

…t#13263)

Rename lightbox viewer to lightbox gallery and clean up examples (amp…

d69a859

…project#13266) * Rename lightbox-viewer to lightbox-gallery * Rename lightbox css from viewer to gallery * Remove dead manual test pages

Fix bookend image aspect scaling. (ampproject#13212)

5e91864

Story-Auto-Ads Skeleton (ampproject#13239)

0aad6b4

* Component skeleton * move files into amp-story * clean up example * remove bad link * bad mock test * clean up

Indicate progress during gulp lint (ampproject#13274)

68aefae

Support --extensions=minimal_set & bug fix (ampproject#13249)

c20c36e

* fix dist with alias * change minimal_set usage * fix documentation * refactor print logic * rename * update function description

Adding amp-story-analytics (ampproject#13276)

a8e4f9e

Fail build on error, and print a useful message (ampproject#13255)

515e812

zhangsu reviewed Feb 5, 2018

View reviewed changes

zhouyx and others added 13 commits February 5, 2018 15:39

return with user error (ampproject#13279)

00032a7

Run lint and presubmit on gulpfile.js (ampproject#13258)

a0f855b

🐛🖍️ Preventing publishers CSS from overriding stories system layer an…

d8df163

…d bookend. (ampproject#13270) * Preventing CSS overrides. * Preventing CSS overrides for the navigation hint, landscape orientation and unsupported browser.

Make sure polyfills.js is imported before anything else (ampproject#1…

686abe4

…3277)

Fix for broken snapping on IOS for slides (ampproject#13290)

fe9925c

Increase button tap target size and ghost image on enter (ampproject#…

79c4969

…13278) * Increase button tap target size * Fix desktop mode top padding * tweak button padding to 16px per mulptiple of 4 * Reorder media queries

Clean up wording in Media Session API section (ampproject#13273)

64d4ab3

Fix 'gulp test --a4a' and remove '--randomize' (ampproject#13292)

88d14b9

Update amp-iframe container in build callback (ampproject#13242)

4c86a54

Implement amp-byside-placeholder extension (ampproject#12798)

56a4a20

Hand icon for all lightboxed images (ampproject#13298)

7c1ce87

Delete build-system/tasks/package.json, whitelist heroku page (amppro…

99a0424

…ject#13295)

amp-fx-collection: documentation (ampproject#13268)

2ee9b80

gmajoulet and others added 20 commits February 7, 2018 09:13

Linear transitions for media story pages progress bar. (ampproject#13296

a941bda

) * Linear transitions for media story pages progress bar. * Unobfuscate the unobfuscated code.

Fix gulp test --a4a (ampproject#13331)

3d54a68

Resets and disables touch zoom for amp stories. (ampproject#13316)

b630522

Update Pubmine amp-ad to new 'async' implementation (ampproject#12855)

0de8ae2

* Updating to new async tag * Linter fix: imports must be alphabetical

the code for whitelist of variables in url substitution. The code doe…

bd9cdcc

…s not contain functional testing

Added some functional tests

6828fc4

Fix flaky Karma startup by refreshing the wd cache (ampproject#13328)

7b211a1

Clean up unnecessary dependency of ads/_config in A4A test. (ampproje…

e2c3db6

…ct#13342)

Update browserify to the latest version 🚀 (ampproject#13333)

3e41cb2

* chore(package): update browserify to version 16.0.0 * chore(package): update lockfile https://npm.im/greenkeeper-lockfile

amp-bind: Warn during verify instead of error (ampproject#13236)

537a7d9

* change amp-bind verification to user warning and clarify message * fix presubmit * fix test

Change [state] usage from warning to error (ampproject#13237)

8e92030

Add support for <amp-facebook-page> (ampproject#13261)

9673d3f

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

Revert "Update browserify to the latest version 🚀" (ampproject#13347)

a0325bc

some refactoring

9865f0a

merged conflicts

5d9b64e

the code for whitelist of variables in url substitution. The code doe…

2a691ba

…s not contain functional testing

Added some functional tests

f42b66d

the code for whitelist of variables in url substitution. The code doe…

b0df971

…s not contain functional testing

hamousavi added 9 commits February 7, 2018 17:29

Merge branch 'variable' of github.com:hamousavi/amphtml into variable

22ae973

Merge conflict

the code for whitelist of variables in url substitution. The code doe…

3fe6c9b

…s not contain functional testing

Added some functional tests

83295f2

the code for whitelist of variables in url substitution. The code doe…

15fec70

…s not contain functional testing

the code for whitelist of variables in url substitution. The code doe…

c245f55

…s not contain functional testing

Added some functional tests

8081b3c

the code for whitelist of variables in url substitution. The code doe…

a82bf51

…s not contain functional testing

rebased

5e87808

sync

d30d718

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Whitelist of Variable Substitutions #3

Whitelist of Variable Substitutions #3

hamousavi commented Feb 5, 2018

hamousavi commented Feb 5, 2018

zhangsu Feb 5, 2018

hamousavi Feb 7, 2018

zhangsu Feb 5, 2018

hamousavi Feb 7, 2018

zhangsu Feb 5, 2018

hamousavi Feb 7, 2018

zhangsu Feb 5, 2018

hamousavi Feb 7, 2018

zhangsu Feb 5, 2018

hamousavi Feb 7, 2018

zhangsu commented Feb 7, 2018

Whitelist of Variable Substitutions #3

Are you sure you want to change the base?

Whitelist of Variable Substitutions #3

Conversation

hamousavi commented Feb 5, 2018

hamousavi commented Feb 5, 2018

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

zhangsu commented Feb 7, 2018