Bug 146710

Summary: Move PingLoaders to the NetworkingProcess
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cgarcia, commit-queue, dbates, japhet
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 146754    
Attachments:
Description Flags
Patch v1
none
Patch v2
thorton: review+
Patch for landing.
dbates: commit-queue-
Patch for landing
none
Patch for landing. none

Description Brady Eidson 2015-07-07 22:34:53 PDT
Move PingLoaders to the NetworkingProcess

<rdar://problem/18860263>
Comment 1 Brady Eidson 2015-07-08 12:29:00 PDT
Created attachment 256395 [details]
Patch v1
Comment 2 Brady Eidson 2015-07-08 13:59:14 PDT
Created attachment 256403 [details]
Patch v2
Comment 3 Tim Horton 2015-07-08 14:08:33 PDT
Comment on attachment 256403 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=256403&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:262
> +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : 0;

nullptr!
Comment 4 Brady Eidson 2015-07-08 14:47:09 PDT
Created attachment 256407 [details]
Patch for landing.
Comment 5 Daniel Bates 2015-07-08 14:52:46 PDT
Comment on attachment 256407 [details]
Patch for landing.

View in context: https://bugs.webkit.org/attachment.cgi?id=256407&action=review

> Source/WebCore/platform/network/PingHandle.h:65
> +    virtual bool usesAsyncCallbacks() { return m_usesAsyncCallbacks == UsesAsyncCallbacks::Yes; }

This needs to be marked override or it will cause a compiler error since we compile with -Winconsistent-missing-override.
Comment 6 Daniel Bates 2015-07-08 15:00:22 PDT
Created attachment 256408 [details]
Patch for landing

Added keyword override to the declaration of PingHandle::usesAsyncCallbacks.
Comment 7 Brady Eidson 2015-07-08 15:04:22 PDT
Created attachment 256409 [details]
Patch for landing.
Comment 8 Alexey Proskuryakov 2015-07-08 15:07:19 PDT
Comment on attachment 256408 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=256408&action=review

Finally! :)

> Source/WebCore/ChangeLog:9
> +        There's no current solution for communicating back to a page that a ping load has reached its target.

We could have the ping target change some global state (usually it's a file on disk), and then another document could poll the state.
Comment 9 Daniel Bates 2015-07-08 15:10:55 PDT
<rdar://problem/18860263>
Comment 10 Brady Eidson 2015-07-08 15:38:21 PDT
(In reply to comment #8)
> Comment on attachment 256408 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256408&action=review
> 
> Finally! :)
> 
> > Source/WebCore/ChangeLog:9
> > +        There's no current solution for communicating back to a page that a ping load has reached its target.
> 
> We could have the ping target change some global state (usually it's a file
> on disk), and then another document could poll the state.

That was my first idea, but then I realized that:
- Tests that rely on files on disk on the httpd side are often fragile
- Tests that rely on polling are... gross

I actually played around with running two php scripts and IPC'ing between them, and I think that will actually work, but there's a steep learning curve incompatible with getting this frick'in bug fix in!

I will revisit it soon - it's on my white board :)
Comment 11 Brady Eidson 2015-07-08 15:39:21 PDT
(In reply to comment #9)
> <rdar://problem/18860263>

That was in comment 1 ;)
Comment 12 Brady Eidson 2015-07-08 15:47:57 PDT
*** Bug 138688 has been marked as a duplicate of this bug. ***
Comment 13 WebKit Commit Bot 2015-07-08 15:54:47 PDT
Comment on attachment 256409 [details]
Patch for landing.

Clearing flags on attachment: 256409

Committed r186530: <http://trac.webkit.org/changeset/186530>
Comment 14 Alexey Proskuryakov 2015-07-09 08:05:20 PDT
> - Tests that rely on files on disk on the httpd side are often fragile

They actually aren't any more, as long as written carefully. We now make sure to have unique names (usually by using a sanitized test path name as temporary file name), and that works pretty reliably, even in most stress testing scenarios.

Some CGI scripts get the name implicitly (from Referer header field), others get it explicitly.
Comment 15 Brady Eidson 2015-07-09 08:20:54 PDT
(In reply to comment #14)
> > - Tests that rely on files on disk on the httpd side are often fragile
> 
> They actually aren't any more, as long as written carefully. We now make
> sure to have unique names (usually by using a sanitized test path name as
> temporary file name), and that works pretty reliably, even in most stress
> testing scenarios.
> 
> Some CGI scripts get the name implicitly (from Referer header field), others
> get it explicitly.

Good to know.

I could be able to use that plus a script that polls for the file before responding. That way the test itself won't have to poll the server - it can just wait for the response.

But as long as I'm polling for a file in php to do adhoc IPC between scripts, I could just use one of the "real" PHP IPC mechanisms.

Will revisit soon.