Build failed for reasons outside of scope of this PR I think.

Executing tests in test suite JavascriptTests...
Test failed in module externals: 'JSLint' 
Error: JSLint 
Source:     at http://localhost/tests/javascript/assets/qunit.js:534
    at http://localhost/tests/javascript/:173
    at http://localhost/tests/javascript/assets/qunit.js:203
    at http://localhost/tests/javascript/assets/qunit.js:361
    at process (http://localhost/tests/javascript/assets/qunit.js:1453)
    at http://localhost/tests/javascript/assets/qunit.js:479

Hi @Sija
Can you please provide a use case for this?

Yeah, sure.

It's useful mostly in the case when you want to track outlinks, while being sure the connection to piwik server will succeed before redirecting user to foreign website.

I understand that beforeUnloadHandler method is meant to handle that case, but based on the timeout idea. Proposed change guarantees recording event, regardless of default 500ms timeout.

Build failed for reasons outside of scope of this PR I think.

it means the JSLint failed, please go to: http://jslint.com/ copy paste the piwik.js code and you can see the error message there. JSLint is a bit tricky but once you're used to it may make sense.

I'm not sure about this PR, maybe there could be a setter to set the callback rather than pass it around all methods?

Or maybe you could reuse the piwik.js plugin mechanism introduced in https://github.com/piwik/piwik/pull/336/files ?

@mattab Getter/Setters aren't really a thing in JS. Using the last parameter as callback is the norm and the correct thing to do (if we want to do it).

@mattab Done!
@halfdan that's right, we could use options object instead but right now there's no such approach used anywhere else in the code.

-1 If you don't want the delay, set the delay to 0. A callback that does window.location = url; is just as likely to cancel the outlink tracker.

No, because callback is going to be invoked only after xhr request succeeded or failed and not before.

On 28 Jul 2014, at 04:24, Anthon Pang notifications@github.com wrote:

-1 If you don't want the delay, set the delay to 0. A callback that does window.location = url; is just as likely to cancel the outlink tracker.

—
Reply to this email directly or view it on GitHub.

callback is going to be invoked only after xhr request succeeded or failed and not before.

So maybe it could be called something else, eg. callbackOnError or better?

@mattab It's not a callback that is called on error - it is always called after xhr. I like the current implementation (actually meant to implement that myself for quite some time).

sorry, I misread while glancing thru the PR. the callback idea is fine.

I would consider something like this https://github.com/robocoder/piwik/commit/e041d365bf500aff23846e4eb6d6128e5c9b83bf instead of

sendRequest(request, (callback ? 0 : configTrackerPause), callback);

but that's just me.

@Sija since @halfdan thinks the callback is a good idea, I think we can merge it.

Maybe you would be kind to add some tests for this new functionnality? the piwik.js is tested in this file: https://github.com/piwik/piwik/tree/master/tests/javascript#javascript-tests

@mattab Piwik's test files are tad confusing I must say—at the beginning I wasn't sure which file to edit, but it seems that index.php is the one, right? I'll do my best ;)

@Sija yes sorry, the piwik.js file is index.php and yes it's quite confusing. We should split it across several files to make it more readable. Sorry about that and thanks for trying 👍

@mattab Callback test added.

@Sija test looks good!

Also would you mind check that the JSLint test passes? http://jslint.com/

@mattab I don't now how should I lint test file since it's a mix of everything, but the code is simple enough to see through its complexity without the need of external tools (I hope) :) piwik.js is already lint-ed though.

@Sija I meant to check the piwik.js is JSLinted (not the test file).

can you please rebase the PR and then I'll merge it. Thanks!

@mattab Done!

@Sija Thanks for the pull request!

@mattab Sure! Thanks for Piwik! :)

Also want to thank you for this, I have been using this change for quite some time because in my experience it is the only reliable way to track outlinks and downloads as long as some browsers cancel pending requests on window.location changes. I never liked the 500ms workaround, 500ms is not enough time if you have visitors from halfway around the globe with bad internet connections. Increasing it just to be safe only adds a long delay for everyone and it is still just guess work.

@fredli74 you need this: #6641

Adding a callback to other "track" functions could prove useful as well. Since it is a common asynchronous paradigm and we won't have to guess using setTimeout. What are your thoughts?

I'd like to see it on trackEvent especially since there are a couple events that i'd like to track which which can possibly navigate the page.

@pbojinov FYI there is a PR at #7946 adding callback mechanism to trackEvent