Compatibility improvements and new onFinish event

[juanparati]

Hi John,

I added some compatibility improvements regarding to:

• IE 9 CORS request
• Compatibility with new Google Chrome 46+ (See details in https://code.google.com/p/chromium/issues/detail?id=549103)

I added the "onFinish" event, that allow to know when the VAST requests was finished and also I improved the CORS request in order to accept cookies (VAST providers usually inject cookies for metrics purposes).

[jonhoo]

Hi @juanparati,

Thanks for the PR. A couple of comments;

• First, please split this into multiple PRs, one for each change you want to merge. For example, 326154f contains at least three independent changes.
• Please rebase your changes on master to avoid merge conflicts.
• I will not merge the XDomainRequest stuff, as this feature has been dropped by IE (>=10), and is not on any standards track. Instead, XMLHttpRequest should be used, which already checks for CORS headers.
• I don't see why you feel the need for onFinish? Why is onFetched not sufficient?
• The Chrome compatibility I'm happy to merge. @Fire-Brand is also working on this in VASTAds Constructor fix for new Chrome #20, but I'm still waiting for him to clean up the PR, so if you do it first, I'll merge yours.

[Fire-Brand]

Hi @jonhoo,

I believe my PR resolved that issue with namespaceURI on #20, didn't it?

[jonhoo]

@Fire-Brand: yes, it resolves the issue, but you haven't cleaned up the PR yet (it is still two changes in one).

[Fire-Brand]

Haven't you merged both my pr1 and pr2 ?

[jonhoo]

You haven't submitted them as PRs as far as I can tell?

[shlomitc]

I think that adding the request.withCredentials = true; by default is not a good idea.
It's enough that a one VAST provider will return the Access-Control-Allow-Origin header wild-carded, and the request will fail. You can read about a request with credentials here

It will be better to set it as with some flag, maybe by adding an options object in the queryVAST() function.

[juanparati]

For some Ads providers is quite important to inject cookies to users. They use the cookies for metrics and retargeting purposes. The only issue using "withCredentials" is that XMLHttpRequest will not work with IE9, this is the reason why I submitted a workaround for IE9.

I also observed that big Ad providers Live Rail, Almedia and Tremor Video uses "Access-Control-Allow-Origin". The Ad providers and Ad propagators usually require to specify the domains where the Advertisement is published, and this is reflected in the "Access-Controll-Allow-Origin".

I think that we should keep "withCredentials" with value equal to "true" by default and as you proposed we can also add a parameter in order to change the "withCredentials" value.

NOTE: If we use "withCredentials" the XMLHttpRequest will miserably fail in IE9, however we can use XDomainRequest for this old web browser.

[juanparati]

@jonhoo : Regarding to the "onFinish" event, I think that is quite important, The reason is that sometimes the Ad provider send empty VAST documents (without linear and non-linear ads), so the "onFetched" and "onError" event is never raise. If you implement VAST in a video player that should start to play when the Ad is returned, the video player can wait until the infinity waiting for "onFetched" call.

In order to avoid the "infinity waiting" problem I had two solutions:

1. Implement a timer mechanism, that wait 3 or 4 second for the onFetched event
2. Just to wait by "onError", "onFetched" or "onFinished"

The first solution is quite dirty and it will not work on mobile devices, because mobile devices require human intervention in order to play videos (You cannot autoplay videos in mobile devices, it is an intentional limitation), in the moment that you attach a timer to the "Play" button click event, the video play will become broken. This is the reason why is so quite important to have an "onFinished" event.

[shlomitc]

@juanparati I agree that cookies are important and should be added to this library. Nevertheless, I think that this breaks the default behavior currently used, and could lead to dangerous behavior if you already use providers that do not support it.

[jonhoo]

Re CORS: I'm happy to add withCredentials: true as the default, and add an option to not include cookies. However, I will not also merge the IE9 workaround. It is estimated that IE<10 has an estimated market share of ~2%, and it's dropping quickly. People who absolutely rely on supporting older versions should maintain this workaround themselves.

Re onFinish: Is there a good reason not to simply replace onFetched with onFinish, and have it also fire in the case where there are no ads?

[juanparati]

I think that to use onFetched also as onFinish is also a fair solution, so for example if Ad is not retrieved the onFetched callback can return "null".

[jonhoo]

Yeah, something like that. This will break the promise in the existing documentation for onAdsAvailable though, so that will have to change too.

[jonhoo]

#34 should help with parts of this.