Created attachment 132692 [details] Patch

n order to make sure we get the right crash reports when running run-webkit-tests in parallel, we need WTR to log the pid of the WebProcess that crashes when it crashes (see bug 71380 for context). This patch is almost certainly not the right way to fix this, but I thought I'd post it to get feedback on what the right way is ... presumably I need to add a forwarding header for WKPagePrivateMac.h somewhere? Do we want to make this a generic function, since presumably we need the same functionality on the other ports?

Comment on attachment 132692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132692&action=review > This patch is almost certainly not the right way to fix this Is your concern only about the style of include? It's wrong indeed, suggestion below. I don't see anything else that would stand out as wrong here. > Do we want to make this a generic function, since presumably we need the same functionality on the other ports? It's best to factor out a universal solution when there is practical need for it. Otherwise, we risk creating a wrong design, or just over-engineering things. > Tools/WebKitTestRunner/TestController.cpp:43 > +#include "../../Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h" I think that this should work: #include <WebKit2/WKPagePrivateMac.h> > Tools/WebKitTestRunner/TestController.cpp:497 > - if (!resetStateToConsistentValues()) { > + if (!resetStateToConsistentValues()) { Wrong indentation. > Tools/WebKitTestRunner/TestController.cpp:499 > + int pid = WKPageGetProcessIdentifier(m_mainWebView->page()); This function returns pid_t, which is a platform dependent type. I'd use this type for the variable, and cast to long inside printf. > Tools/WebKitTestRunner/TestController.cpp:814 > + int pid = WKPageGetProcessIdentifier(m_mainWebView->page()); Same comment about return type.

(In reply to comment #3) > (From update of attachment 132692 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132692&action=review > > > This patch is almost certainly not the right way to fix this > > Is your concern only about the style of include? It's wrong indeed, suggestion below. I don't see anything else that would stand out as wrong here. > Mostly I was thinking that getting the subprocess pid is likely to be needed by every WTR implementation and so maybe it should in a generic header and not a platform-specific header. I'm happy to land this as-is and change it down the road as other WK2 ports want the same functionality, though. > > Do we want to make this a generic function, since presumably we need the same functionality on the other ports? > > It's best to factor out a universal solution when there is practical need for it. Otherwise, we risk creating a wrong design, or just over-engineering things. > Sure. > > Tools/WebKitTestRunner/TestController.cpp:43 > > +#include "../../Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h" > > I think that this should work: > > #include <WebKit2/WKPagePrivateMac.h> > Will give it a try. > > Tools/WebKitTestRunner/TestController.cpp:497 > > - if (!resetStateToConsistentValues()) { > > + if (!resetStateToConsistentValues()) { > > Wrong indentation. > Will fix. > > Tools/WebKitTestRunner/TestController.cpp:499 > > + int pid = WKPageGetProcessIdentifier(m_mainWebView->page()); > > This function returns pid_t, which is a platform dependent type. I'd use this type for the variable, and cast to long inside printf. > Will fix. Not sure how I missed that :(. Thanks for the review!

Created attachment 132926 [details] patch for landing

Comment on attachment 132926 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=132926&action=review > Tools/WebKitTestRunner/TestController.cpp:39 > +#include <signal.h> Why do you need signal.h?

Comment on attachment 132926 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=132926&action=review >> Tools/WebKitTestRunner/TestController.cpp:39 >> +#include <signal.h> > > Why do you need signal.h? Whoops. I don't; I thought I had removed that :(

Committed r111619: <http://trac.webkit.org/changeset/111619>