Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified AppCache template. IMHO the timer is unnecessary. Also, do… #241

Closed
wants to merge 1 commit into from

Conversation

jampy
Copy link

@jampy jampy commented Apr 25, 2017

potential fix for #210

  • removed 5000ms timeout, so that events work all the time
  • removed interval-polling. AFAIK there is a race condition until the event handlers have been registered, but afterwards there is no need to continue polling. effectively I got all events twice in Chrome
  • remove "cleanup" code - don't see any need for that (not even for reducing memory footprint), reducing complexity

I might have overlooked something and thus have removed important code. Please bear with me, the original code was not completely clear to me.

Tested successfully with Chrome 58 on Windows (SW disabled) and Safari on OSX.

Please accept and publish if you are okay with this patch; this is much needed, thank you.

Reference: http://labnote.beedesk.com/the-pitfalls-of-html5-applicationcache

…n't unregister events after 5000ms

potential fix for NekR#210
@NekR
Copy link
Owner

NekR commented Apr 25, 2017

Thanks for a PR!

First of all, I want you understand that every line in that AppCache code in on purpose. I have done extensive testing on all browsers and that's a nightmare. So even if we are going to remove something, we have to this very carefully and i won't rush with this by any means.

I for example don't understand why you delete those timer checks if your read the Pitfalls from linked article. All browsers are buggy with AppCache and some of them don't call some of the events (like UDPATEREADY) at all, especially in iframe.

Also, when reading those articles please take a note that in our case everything is in iframe, and that makes things even more complicated. Some browser for example call lifecycle events individually on top frame, other start calling them and updating status in top frame and then calling some of them in the iframe once that is loaded.

Also, some browsers may not set UDPATEREADY/CACHED/OBSOLETE in the iframe at all, but set in only in top frame, that's why 5000 seconds timeout needed -- to not run it forever.

Those are just from the top of my head, there are more quirks which I cannot remember right now. I'll think what we can do here, but don't expect it be fully fixed or 100% properly implemented. I don't want to spent weeks on testing all the things again.

@jampy
Copy link
Author

jampy commented Apr 26, 2017

I don't see where in that article there is a hint that timer checks are needed. There seems to be only this race condition, which is solved by two checks, not a timer:

You might poll the status and if it is not ready, listen to the CACHEREADY event. But, in between the poll and registering of the event you might miss the CACHEREADY event.

Anyway, you obviously did a lot of testing and have reasons for the timers (documented somewhere?).

The main problem with the current design is the 5000ms timeout. In my situation that is never enough to update the application (real world situation). So, all those workarounds are useless because offline-cache is essentially too impatient ;-)

Just removing the cleanup timer would leave a 50ms timer running, which obviously is not a good idea (and the reason you implemented the cleanup in the first place, I guess).

Any idea how to solve this dilemma?

Is the timer also needed for the (most important) "updateReady" event?

I'll try with a new PR soon.

@jampy jampy closed this Apr 26, 2017
@NekR
Copy link
Owner

NekR commented Apr 26, 2017

The main problem with the current design is the 5000ms timeout. In my situation that is never enough to update the application (real world situation). So, all those workarounds are useless because offline-cache is essentially too impatient ;-)

Yes, I see the problem, but we should be very careful touching anything in that code.

@NekR
Copy link
Owner

NekR commented Apr 26, 2017

Any idea how to solve this dilemma?

Now yet, thinking.

Is the timer also needed for the (most important) "updateReady" event?

Yes, I'm pretty sure there are problems with that event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants