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.

Sign up for GitHub

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

Jump to bottom

More visibility spec implementation. #3110

Merged
merged 4 commits into from
May 6, 2016
Merged

More visibility spec implementation. #3110

merged 4 commits into from
May 6, 2016

Conversation

avimehta
Copy link
Contributor

@avimehta avimehta commented May 5, 2016

This PR adds support for:
Conditions:
"continuousTimeMin": ,
"continuousTimeMax": ,
"totalTimeMin": ,
"totalTimeMax": ,

And Macros:
minVisiblePercentage
maxVisiblePercentage
totalVisibleTime
continuousVisibleTime
firstSeenTime
firstVisibleTime
lastSeenTime
lastVisibleTime

Intent to implement in #1297

@avimehta
More visibility spec implementation.
d2f7390 
This PR adds support for:
Conditions:
    "continuousTimeMin": <milliseconds>,
    "continuousTimeMax": <milliseconds>,
    "totalTimeMin": <milliseconds>,
    "totalTimeMax": <milliseconds>,

And Macros:
    `minVisiblePercentage`
    `maxVisiblePercentage`
    `totalVisibleTime`
    `continuousVisibleTime`
    `firstSeenTime`
    `firstVisibleTime`
    `lastSeenTime`
    `lastVisibleTime`
@avimehta
Copy link
Contributor Author

avimehta commented May 5, 2016 •
edited by jridgewell
Loading

@jridgewell The tests are failing with following error:

> Chrome 50.0.2661 (Linux 0.0.0) Visibility (tag: amp-analytics) fires for trivial config FAILED
>         TypeError: babelHelpers.defineProperty is not a function
>             at Visibility.listenOnce (/tmp/60a144f13bb7ba1df80a3d72d415a67d.browserify:56729:29 <- /usr/local/google/home/avimehta/amp/amphtml/src/service/visibility-impl.js:250:11)
>             at listen (/tmp/60a144f13bb7ba1df80a3d72d415a67d.browserify:81747:16 <- /usr/local/google/home/avimehta/amp/amphtml/test/functional/test-visibility.js:75:15)
>             at Context.<anonymous> (/tmp/60a144f13bb7ba1df80a3d72d415a67d.browserify:81759:5 <- /usr/local/google/home/avimehta/amp/amphtml/test/functional/test-visibility.js:87:4)
> Chrome 50.0.2661 (Linux 0.0.0) Visibility (tag: amp-analytics) fires for non-trivial config FAILED
>         TypeError: babelHelpers.defineProperty is not a function
>             at Visibility.listenOnce (/tmp/60a144f13bb7ba1df80a3d72d415a67d.browserify:56729:29 <- /usr/local/google/home/avimehta/amp/amphtml/src/service/visibility-impl.js:250:11)
>             at listen (/tmp/60a144f13bb7ba1df80a3d72d415a67d.browserify:81747:16 <- /usr/local/google/home/avimehta/amp/amphtml/test/functional/test-visibility.js:75:15)
>             at Context.<anonymous> (/tmp/60a144f13bb7ba1df80a3d72d415a67d.browserify:81764:5 <- /usr/local/google/home/avimehta/amp/amphtml/test/functional/test-visibility.js:92:4)
> Chrome 50.0.2661 (Linux 0.0.0) Visibility (tag: amp-analytics) fires only once FAILED
>         TypeError: babelHelpers.defineProperty is not a function
>             at Visibility.listenOnce (/tmp/60a144f13bb7ba1df80a3d72d415a67d.browserify:56729:29 <- /usr/local/google/home/avimehta/amp/amphtml/src/service/visibility-impl.js:250:11)

Are (http://es6-features.org/#ComputedPropertyNames)[computer property names] not supported in tests?

@jridgewell
Copy link
Contributor

Are (http://es6-features.org/#ComputedPropertyNames)[computer property names] not supported in tests?

Likely not, though we could easily add support.

@avimehta
Copy link
Contributor Author

avimehta commented May 5, 2016

Can we do that? How do I go about doing it?

On Thu, May 5, 2016, 11:28 AM Justin Ridgewell notifications@github.com
wrote:

Are (http://es6-features.org/#ComputedPropertyNames)[computer property
names] not supported in tests?

Likely not, though we could easily add support.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#3110 (comment)

@jridgewell
Copy link
Contributor

We define the babel helpers in third_party/babel/custom-babel-helpers.js. Looks like this should work, but you'll need to confirm:

babelHelpers.defineProperty = function(obj, key, value) {
  obj[key] = value;
};

@michaelkleber michaelkleber mentioned this pull request May 6, 2016
Closed
@avimehta
Copy link
Contributor Author

avimehta commented May 6, 2016

ptal.

/cc @cramforce

cramforce
cramforce reviewed May 6, 2016
View reviewed changes
src/service/visibility-impl.js
/** @const {string} */
const TOTAL_TIME_MIN = 'totalTimeMin';

/** @const {string} */
Copy link
Member

Choose a reason for hiding this comment

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

No need for these annotations. Closure compiler infers this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed these

@cramforce
Copy link
Member

Looks good. Just a few comments.

@avimehta
Copy link
Contributor Author

avimehta commented May 6, 2016

Thanks for the review. Merging the PR.

@avimehta avimehta merged commit bf1623d into ampproject:master May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
WG: analytics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@avimehta @jridgewell @cramforce @dvoytenko