Fix issue #238, use pagehide not unload listener

[rhelmer]

This seems to work fine in Firefox and (mostly) in Chrome, but it intermittently fails to fire in Chrome.

Per MDN, the visibilitychange event is preferred, but it does note the back/forward cache behavior. Using pagehide when that isn't available is suggested as well.

I think the root cause might be that the back/forward cache doesn't get canceled in Chrome the way it does in Firefox, but I'm having a lot of trouble reproducing it in a test environment. Using pagehide instead seems to work when I test sites in the wild, and it still passes the test suite.

[rhelmer]

Hm, on further testing I don't Chrome supports document.visibilityState === 'unloaded' and document.visibilityState === 'hidden' isn't quite what we want, but I'll see what I can determine.

See issue #238 for more.

[aaga]

confirmed test passes locally for me as well.

@rhelmer just want to check my understanding from reading the linked issue: the plan is to eventually use tabs.onChange, but we're doing it this way for now for MVP?

[rhelmer]

confirmed test passes locally for me as well.

@rhelmer just want to check my understanding from reading the linked issue: the plan is to eventually use tabs.onChange, but we're doing it this way for now for MVP?

Yes, I think ultimately depending on the content script environment is going to mean depending on implementation details and we won't get predictable cross-browser behavior there anytime soon. The tabs.* APIs on the other hand are more amenable to standardizing.