| Summary: | Add test coverage for sendBeacon() keepalive flag | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
| Component: | Tools / Tests | Assignee: | Chris Dumez <cdumez> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | beidson, commit-queue, ggaren, lforschler, ryanhaddad, sam, webkit-bug-importer, youennf | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=175388 | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 147885 | ||||||||
| Attachments: |
|
||||||||
|
Description
Chris Dumez
2017-08-04 14:43:58 PDT
Created attachment 317294 [details]
Patch
Comment on attachment 317294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317294&action=review > LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:25 > + }), 1000); 1000 may be too long in most cases and too short in others. Why not trying to fetch every 50 ms until you have the correct content type and fail after a longer time like 5 seconds? > LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:35 > + return pollResult(test, id) .then(result => { s/ .// Created attachment 317299 [details]
Patch
Forgot to say, why wpt and not imported? Are you planning to upstream these? Comment on attachment 317299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317299&action=review > LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15 > +function pollResult(test, id) { Maybe I am missing something but this is not really polling in the sense that you try until you either succeed or time out? If there is no need for polling and the fetch request will always happen after the beacon one, we just need to increase the timeout to 5000, right? This function is copied from other Beacon tests in WPT. (In reply to youenn fablet from comment #5) > Comment on attachment 317299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317299&action=review > > > LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15 > > +function pollResult(test, id) { > > Maybe I am missing something but this is not really polling in the sense > that you try until you either succeed or time out? We keep trying at regular intervals. I believe this is polling. > If there is no need for polling and the fetch request will always happen > after the beacon one, we just need to increase the timeout to 5000, right? I do not understand this part. A beacon may be sent after a while, so we need to keep polling until it comes. Comment on attachment 317299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317299&action=review >>> LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15 >>> +function pollResult(test, id) { >> >> Maybe I am missing something but this is not really polling in the sense that you try until you either succeed or time out? >> If there is no need for polling and the fetch request will always happen after the beacon one, we just need to increase the timeout to 5000, right? > > We keep trying at regular intervals. I believe this is polling. It seems there is only one fetch being ever called with this code. Is step_timeout expected to call fetch every 50 ms? (In reply to youenn fablet from comment #4) > Forgot to say, why wpt and not imported? > Are you planning to upstream these? I will try to upstream it, yes. (In reply to youenn fablet from comment #7) > Comment on attachment 317299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317299&action=review > > >>> LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15 > >>> +function pollResult(test, id) { > >> > >> Maybe I am missing something but this is not really polling in the sense that you try until you either succeed or time out? > >> If there is no need for polling and the fetch request will always happen after the beacon one, we just need to increase the timeout to 5000, right? > > > > We keep trying at regular intervals. I believe this is polling. > > It seems there is only one fetch being ever called with this code. > Is step_timeout expected to call fetch every 50 ms? Yes, we need to call fetch() at regular interval to ask the server if it received the beacon. Comment on attachment 317299 [details] Patch Clearing flags on attachment: 317299 Committed r220304: <http://trac.webkit.org/changeset/220304> All reviewed patches have been landed. Closing bug. > Yes, we need to call fetch() at regular interval to ask the server if it
> received the beacon.
Looking at step_timeout, it does not use setInterval and is calling setTimeout once.
It seems to me there will be only one fetch being called, fetch being done after waiting for 50 ms.
Also, even if fetch is called several times, it would seem better to wait for the completion of the first fetch before triggering a new fetch.
(In reply to youenn fablet from comment #13) > > Yes, we need to call fetch() at regular interval to ask the server if it > > received the beacon. > > Looking at step_timeout, it does not use setInterval and is calling > setTimeout once. > It seems to me there will be only one fetch being called, fetch being done > after waiting for 50 ms. > > Also, even if fetch is called several times, it would seem better to wait > for the completion of the first fetch before triggering a new fetch. oh, the switch to 50ms is likely going to cause flakiness then. I'll follow-up :( |