Created attachment 247810 [details]
Patch v1

Comment on attachment 247810 [details]
Patch v1

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

> Tools/WebKitTestRunner/TestController.cpp:698
> +#if PLATFORM(COCOA)
>      return "com.apple.WebKit.WebContent.Development";

Do we care about this being correct on the device? I think that there, it wouldn't have the Development suffix.

(In reply to comment #2)
> Comment on attachment 247810 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247810&action=review
> 
> > Tools/WebKitTestRunner/TestController.cpp:698
> > +#if PLATFORM(COCOA)
> >      return "com.apple.WebKit.WebContent.Development";
> 
> Do we care about this being correct on the device? I think that there, it
> wouldn't have the Development suffix.

We care when we start running tests on the device.  Why wouldn't the roots we install on the device have the .Development prefix as well?

(In reply to comment #0)
> Crash logs can't be found when running tests on the ios-simulator platform
> because WebKitTestRunner returns the wrong process name.

BTW, this only fixes WK2 tests.  I think I know why WK1 tests fail (we're searching for crashes using 'DumpRenderTree.app' as the process name instead of just 'DumpRenderTree'), but that's a separate fix in Python code.

Also, while testing this patch, I noticed that sometimes the wrong test would be marked as crashing, so there still may be situations where we're not getting crash logs, but that's a different bug.

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 247810 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=247810&action=review
> > 
> > > Tools/WebKitTestRunner/TestController.cpp:698
> > > +#if PLATFORM(COCOA)
> > >      return "com.apple.WebKit.WebContent.Development";
> > 
> > Do we care about this being correct on the device? I think that there, it
> > wouldn't have the Development suffix.
> 
> We care when we start running tests on the device.  Why wouldn't the roots
> we install on the device have the .Development prefix as well?

Because they get installed into the system location, which how we determine which variant to run (as far as I remember).

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 247810 [details]
> > > Patch v1
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=247810&action=review
> > > 
> > > > Tools/WebKitTestRunner/TestController.cpp:698
> > > > +#if PLATFORM(COCOA)
> > > >      return "com.apple.WebKit.WebContent.Development";
> > > 
> > > Do we care about this being correct on the device? I think that there, it
> > > wouldn't have the Development suffix.
> > 
> > We care when we start running tests on the device.  Why wouldn't the roots
> > we install on the device have the .Development prefix as well?
> 
> Because they get installed into the system location, which how we determine
> which variant to run (as far as I remember).

See:

bool isWebKitDevelopmentBuild = ![[[[NSBundle bundleWithIdentifier:@"com.apple.WebKit"] bundlePath] stringByDeletingLastPathComponent] hasPrefix:systemDirectoryPath()];

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Comment on attachment 247810 [details]
> > > > Patch v1
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=247810&action=review
> > > > 
> > > > > Tools/WebKitTestRunner/TestController.cpp:698
> > > > > +#if PLATFORM(COCOA)
> > > > >      return "com.apple.WebKit.WebContent.Development";
> > > > 
> > > > Do we care about this being correct on the device? I think that there, it
> > > > wouldn't have the Development suffix.
> > > 
> > > We care when we start running tests on the device.  Why wouldn't the roots
> > > we install on the device have the .Development prefix as well?
> > 
> > Because they get installed into the system location, which how we determine
> > which variant to run (as far as I remember).
> 
> See:
> 
> bool isWebKitDevelopmentBuild = ![[[[NSBundle bundleWithIdentifier:@"com.apple.WebKit"] bundlePath] stringByDeletingLastPathComponent] hasPrefix:systemDirectoryPath()];

Huh.  That seems weirdly arbitrary.  I figured we would have done something like bake .Development into all engineering builds, and only omit the .Development suffix on Production builds.

Since we're going to have to make lots of changes to support device testing anyway (like looking for crash logs on device or in a different directory), I don't think that's a big concern at this time.

Comment on attachment 247810 [details]
Patch v1

Clearing flags on attachment: 247810

Committed r180983: <http://trac.webkit.org/changeset/180983>

All reviewed patches have been landed.  Closing bug.