@flamisz opened this Pull Request on April 7th 2021 Contributor

Description:

fixes #17259

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on April 7th 2021 Member

@flamisz I think for that issue, based on a slack discussion, we were going to use the old API by default and switch to the new one only if the old one wasn't available to support older browsers that may not support the new API (eg, on mobile devices).

@tsteur commented on April 7th 2021 Member

@flamisz as suggested in https://github.com/matomo-org/matomo/issues/17259#issuecomment-809684976 wondering if it makes sense to prefer reading data from the older API and only fallback to the newer API if the older doesn't exist anymore for reasons mentioned in the comment and also in case eg some browsers now have not responseEnd but domLoading etc. It seems like a safer way.

Additionally would need to check if performanceAlias.timing API has responseEnd or if a different property needs to be used there (domLoading).

Also to fully fix the issue need to add the rounding to integer values (see eg https://github.com/matomo-org/matomo/issues/17259#issuecomment-785559464 and https://github.com/matomo-org/matomo/issues/17259#issuecomment-809684976 as the newer API returns floats.).

And we'd also need to check it eg with iPad running iOS9.3 with Safari 9 if any possible to see if the newer API behaves faulty there maybe as we say in our analytics some funny numbers there (we have browserstack account for this)

@flamisz commented on April 7th 2021 Contributor

@tsteur and @diosmosis thanks for the review.
The performance.timing API contains responseEnd, we already use it for another data.

I can make the old one default and round them integer (interestingly Firefox uses integer, but Chrome does not).

I thought now we know what is the main issue, we'd like to use the new API because we talked about using the old one by default before @sgiehl found the problem, that's why I didn't change that part, but I'll do it now.

@flamisz commented on April 7th 2021 Contributor

The rounding was already solved in this PR #17372

@flamisz commented on April 7th 2021 Contributor

build js

@flamisz commented on April 7th 2021 Contributor

build js

@flamisz commented on April 7th 2021 Contributor

build js

@diosmosis commented on April 12th 2021 Member

@flamisz looks like the JS tests aren't passing (JSLint failing), can you take a look? Would be ready to merge afterwards.

@flamisz commented on April 12th 2021 Contributor

jslint error:

if ('domLoading' in performanceData)
Unexpected 'in'. Compare with undefined, or use the hasOwnProperty method instead.
@diosmosis commented on April 12th 2021 Member

@flamisz is it possible to disable jslint for that line?

@flamisz commented on April 12th 2021 Contributor

@flamisz is it possible to disable jslint for that line?

@diosmosis I can use undefined (newest commit) or try to disable for that line, but this is from the jslint doc:

JSLint was designed to reject code that some would consider to be perfectly fine...
...JSLint will hurt your feelings. Side effects may include headache, irritability, ...

😄

@flamisz commented on April 12th 2021 Contributor

build js

@diosmosis commented on April 13th 2021 Member

@flamisz just had a thought, I'm not sure what valid values are acceptable for this code, but are null values accepted here or should we also check for them? (I know the previous code would have let them pass, just thought about it now.)

@tsteur commented on April 13th 2021 Member

btw could maybe just use hasOwnProperty or so instead of ignoring jslint there maybe?

@flamisz commented on April 13th 2021 Contributor

@flamisz just had a thought, I'm not sure what valid values are acceptable for this code, but are null values accepted here or should we also check for them? (I know the previous code would have let them pass, just thought about it now.)

@diosmosis it returns a unsigned long long, it can be 0 but not null.

btw could maybe just use hasOwnProperty or so instead of ignoring jslint there maybe?

@tsteur I used the undefined check instead, because the hasOwnProperty tricky sometimes, if it's an inherited property. The undefined should work the same way as the in, because the value itself can't be undefined, only unsigned long long.

@diosmosis commented on April 13th 2021 Member
@tsteur commented on April 13th 2021 Member

@diosmosis Might be good to test this in IE10 or so (seems pretty much the oldest browser that supports this API see https://caniuse.com/?search=performance ). Not sure if it could potentially result in an error if it wasn't defined etc. I think usually we'd maybe do typeof aaa.bbb !== 'undefined' (the result of typeof would need to be assigned to a variable first). Seems in past we usually used isDefined(aaa.bbb)? No big preference though. For consistency might be better to use isDefined().

@diosmosis commented on April 13th 2021 Member

Hi @flamisz, when you're able to can you take a look at https://github.com/matomo-org/matomo/pull/17427#issuecomment-818411155? (making this comment since you weren't pinged in thomas' comment)

Powered by GitHub Issue Mirror