Skip to content

More visibility spec implementation.#3110

Merged
avimehta merged 4 commits intoampproject:masterfrom
avimehta:ads2
May 6, 2016
Merged

More visibility spec implementation.#3110
avimehta merged 4 commits intoampproject:masterfrom
avimehta:ads2

Conversation

@avimehta
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

jridgewell commented May 5, 2016

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

Likely not, though we could easily add support.

@avimehta
Copy link
Copy Markdown
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
Copy Markdown
Contributor

jridgewell commented May 5, 2016

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

avimehta commented May 6, 2016

ptal.

/cc @cramforce

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

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

@cramforce cramforce May 6, 2016

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

@avimehta avimehta May 6, 2016

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

cramforce commented May 6, 2016

Looks good. Just a few comments.

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