Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make DocInfo.pageViewId64 async #23998

Merged
merged 2 commits into from Aug 16, 2019

Conversation

dreamofabear
Copy link

@dreamofabear dreamofabear commented Aug 15, 2019

Fixes #23988.

Bug introduced in: https://github.com/ampproject/amphtml/pull/23451/files#r314530969

Affects browsers that don't implement WebCrypto or Crypto.getRandomValues: https://caniuse.com/#search=getRandomValues

Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@@ -104,7 +105,15 @@ export class DocInfo {
},
canonicalUrl,
pageViewId,
pageViewId64,
get pageViewId64() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we are relying on get pageViewId64() getting called after extensions service is ready?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only after it's registered which happens very soon afterwards in runtime.adopt().

@jridgewell
Copy link
Contributor

Does this even need to be on document info?

@zhouyx
Copy link
Contributor

zhouyx commented Aug 15, 2019

@jridgewell Good point. Maybe not.

I do see one concern here. If this hasn't changed, FIE and the parent doc shares the same ampdoc instance. Maybe we should not store pageViewId within the DocInfo, as that shares the same id between FIE and parent doc.

Do you see this as the case? If so I think that's an issue we need to fix asap

@dreamofabear
Copy link
Author

Maybe we should not store pageViewId within the DocInfo, as that shares the same id between FIE and parent doc.

This would also affect PAGE_VIEW_ID then, right? PAGE_VIEW_ID_64 isn't a whitelisted A4A variable but looks like PAGE_VIEW_ID is.

const WHITELISTED_VARIABLES = [
'AMPDOC_HOST',
'AMPDOC_HOSTNAME',
'AMPDOC_URL',
'AMP_VERSION',
'AVAILABLE_SCREEN_HEIGHT',
'AVAILABLE_SCREEN_WIDTH',
'BACKGROUND_STATE',
'BROWSER_LANGUAGE',
'CANONICAL_HOST',
'CANONICAL_HOSTNAME',
'CANONICAL_PATH',
'CANONICAL_URL',
'COUNTER',
'DOCUMENT_CHARSET',
'DOCUMENT_REFERRER',
'FIRST_CONTENTFUL_PAINT',
'FIRST_VIEWPORT_READY',
'MAKE_BODY_VISIBLE',
'PAGE_VIEW_ID',
'RANDOM',
'SCREEN_COLOR_DEPTH',
'SCREEN_HEIGHT',
'SCREEN_WIDTH',
'SCROLL_HEIGHT',
'SCROLL_LEFT',
'SCROLL_TOP',
'SCROLL_WIDTH',
'SHARE_TRACKING_INCOMING',
'SHARE_TRACKING_OUTGOING',
'SOURCE_HOST',
'SOURCE_HOSTNAME',
'SOURCE_PATH',
'SOURCE_URL',
'TIMESTAMP',
'TIMEZONE',
'TIMEZONE_CODE',
'TITLE',
'TOTAL_ENGAGED_TIME',
'USER_AGENT',
'VARIANT',
'VARIANTS',
'VIEWER',
'VIEWPORT_HEIGHT',
'VIEWPORT_WIDTH',
];

@zhouyx
Copy link
Contributor

zhouyx commented Aug 16, 2019

True, it should only affect PAGE_VEW_ID. cc @lannka Should page_view_id be supported in FIE?

@zhouyx
Copy link
Contributor

zhouyx commented Aug 16, 2019

Talked to @lannka offline, we will be sharing PAGE_VIEW_ID, and maybe PAGE_VIEW_ID_64 in the future upon request. This is good now

@dreamofabear dreamofabear merged commit a2dd0d6 into ampproject:master Aug 16, 2019
@dreamofabear dreamofabear deleted the pageViewId64-async branch August 16, 2019 21:23
dreamofabear pushed a commit that referenced this pull request Aug 16, 2019
* Make DocInfo.pageViewId64 async.

* Fix types.
westonruter added a commit to westonruter/amphtml that referenced this pull request Aug 17, 2019
…cript-img-with-http-protocol

* 'master' of github.com:ampproject/amphtml: (1326 commits)
  Fix and enable e2e tests for AMPHTML ads FIE rendering mode (ampproject#23995)
  🏗 Update WorkerDOM to 0.17.0 (ampproject#24024)
  Make DocInfo.pageViewId64 async (ampproject#23998)
  🐛 Updates amp-sidebar in amp-story  (ampproject#23956)
  Revert "Revert "📖Update documentation for carousel 0.2 (ampproject#23840)" (ampproject#23967)" (ampproject#24016)
  🔥 Revert "📈 Initial StorySpec Implementation (ampproject#23030)" (ampproject#24013)
  Extension skeleton code for payment widgets (ampproject#23045)
  🏗🐛 Don't call `travisBuildNumber()` in the global scope (ampproject#24021)
  Remove suppressTypes from amp-mustache. (ampproject#23993)
  🐛 Move `terser` from `dependencies` to `devDependencies` (ampproject#24018)
  Revert "Revert "Set the new loaders experiment to 1% of traffic. (ampproject#23780)" (ampproject#23963)" (ampproject#24014)
  SwG release 0.1.22.63 (ampproject#23997)
  Resolve navTiming variable earlier if possible (ampproject#23580)
  🏗 Don't run all the runtime tests for validator-only changes (ampproject#24010)
  Collect document ready signal (ampproject#23981)
  Validator rollup (ampproject#24000)
  Remove flaky story branching test. (ampproject#23994)
  Include amp-base-carousel in amp-carousel's build. (ampproject#23984)
  Partial validator rollup (ampproject#23996)
  amp-bind: Rate-limit history operations (ampproject#23938)
  ...
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* Make DocInfo.pageViewId64 async.

* Fix types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Cannot read property 'obj' of undefined
4 participants