Skip to content

Make DocInfo.pageViewId64 async#23998

Merged
dreamofabear merged 2 commits intoampproject:masterfrom
dreamofabear:pageViewId64-async
Aug 16, 2019
Merged

Make DocInfo.pageViewId64 async#23998
dreamofabear merged 2 commits intoampproject:masterfrom
dreamofabear:pageViewId64-async

Conversation

@dreamofabear
Copy link
Copy Markdown

@dreamofabear dreamofabear commented Aug 15, 2019 •
edited
Loading

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

@dreamofabear dreamofabear mentioned this pull request Aug 15, 2019
Merged
zhouyx
zhouyx approved these changes Aug 15, 2019
View reviewed changes
Copy link
Copy Markdown
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!

src/service/document-info-impl.js
canonicalUrl,
pageViewId,
pageViewId64,
get pageViewId64() {
Copy link
Copy Markdown
Contributor

@zhouyx zhouyx Aug 15, 2019

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
Copy Markdown
Author

@dreamofabear dreamofabear Aug 16, 2019

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
Copy Markdown
Contributor

jridgewell commented Aug 15, 2019

Does this even need to be on document info?

@zhouyx
Copy link
Copy Markdown
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
Copy Markdown
Author

dreamofabear commented Aug 16, 2019

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.

amphtml/extensions/amp-a4a/0.1/a4a-variable-source.js

Lines 26 to 71 in dcaf2ce

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
Copy Markdown
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
Copy Markdown
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 dreamofabear mentioned this pull request Aug 16, 2019
Closed
dreamofabear pushed a commit that referenced this pull request Aug 16, 2019
Make DocInfo.pageViewId64 async (#23998)
afe7a4c 
* Make DocInfo.pageViewId64 async.

* Fix types.
westonruter added a commit to westonruter/amphtml that referenced this pull request Aug 17, 2019
@westonruter
Merge branch 'master' of github.com:ampproject/amphtml into allow/nos…
54ebf90 
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zhouyx zhouyx zhouyx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error: Cannot read property 'obj' of undefined

4 participants

@dreamofabear @jridgewell @zhouyx @googlebot