Bug 172033

<rdar://problem/32157616>

Created attachment 309907 [details] Patch

Comment on attachment 309907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309907&action=review > Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:239 > + def crash_log_path(self): > + return '/Users/mock/Library/Logs/DiagnosticReports' It seems sufficient to define a class constant, maybe DARWIN_MOCK_CRASH_DIRECTORY, to be "/Users/mock/Library/Logs/DiagnosticReports" instead of defining a function. Notice that I explicitly mentioned "DARWIN" in the name of the constant since the path it represents is only meaningful on Darwin-based OSes. > Tools/Scripts/webkitpy/port/base.py:1351 > + return self.results_directory() I take it we consider the location of the crash log on Windows typical for most ports. That is, the crash log/core dump will be next to the executable. > Tools/Scripts/webkitpy/port/darwin.py:93 > + if self.host.filesystem.exists(self.host.filesystem.join(log_directory, 'DiagnosticReports')): > + return self.host.filesystem.join(log_directory, 'DiagnosticReports') We may compute the path to the DiagnosticReports directory twice - once to check its existence and once more if it exists. I suggest that we compute it once and cache it in a local variable. > Tools/Scripts/webkitpy/port/darwin.py:95 > + else: > + return self.host.filesystem.join(log_directory, 'CrashReporter') We tend to follow the WebKit Code Style Guidelines such that this return statement should be outside of the else scope since the preceding if concludes with a return statement [1]. [1] <https://webkit.org/code-style-guidelines/#linebreaking-else-if> > Tools/Scripts/webkitpy/port/ios_device_unittest.py:42 > + def test_crashlog_path(self): > + pass What is the purpose of this test method?

Comment on attachment 309907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309907&action=review >> Tools/Scripts/webkitpy/port/base.py:1351 >> + return self.results_directory() > > I take it we consider the location of the crash log on Windows typical for most ports. That is, the crash log/core dump will be next to the executable. Correct. Additionally, even if this assumption is incorrect, it makes generalizing crash logic easier because this directory will definitely exist. >> Tools/Scripts/webkitpy/port/ios_device_unittest.py:42 >> + pass > > What is the purpose of this test method? The IOSDeviceTest inherits from DarwinTest (through IOSTest). Until we have implemented the crash log path for iOS, we need this to keep tests passing.

Created attachment 309924 [details] Patch

(In reply to Jonathan Bedard from comment #4) > Comment on attachment 309907 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309907&action=review > > >> Tools/Scripts/webkitpy/port/base.py:1351 > >> + return self.results_directory() > > > > I take it we consider the location of the crash log on Windows typical for most ports. That is, the crash log/core dump will be next to the executable. > > Correct. OK. > Additionally, even if this assumption is incorrect, it makes > generalizing crash logic easier because this directory will definitely exist. > I'm unclear how the existence of a directory "makes generalizing crash logic easier". It seems that if crash logs/core dumps are not placed in self.results_directory() for some port/OS P and CrashLog was taught to work for P then it would not find any crash logs. This would likely surprise the implementer that taught CrashLog to work for P and though discoverable when testing ones code would be more easily discoverable (and less surprising) if the default implementation of Port.path_to_crash_logs raised a NotImplementedError. The exception is more discoverable because it is not subtle and it would immediately force the implementer to implement this method for P. > >> Tools/Scripts/webkitpy/port/ios_device_unittest.py:42 > >> + pass > > > > What is the purpose of this test method? > > The IOSDeviceTest inherits from DarwinTest (through IOSTest). Until we have > implemented the crash log path for iOS, we need this to keep tests passing. :(

Comment on attachment 309924 [details] Patch Clearing flags on attachment: 309924 Committed r216776: <http://trac.webkit.org/changeset/216776>

All reviewed patches have been landed. Closing bug.

(In reply to Daniel Bates from comment #6) > > ... > > I'm unclear how the existence of a directory "makes generalizing crash logic > easier". It seems that if crash logs/core dumps are not placed in > self.results_directory() for some port/OS P and CrashLog was taught to work > for P then it would not find any crash logs. This would likely surprise the > implementer that taught CrashLog to work for P and though discoverable when > testing ones code would be more easily discoverable (and less surprising) if > the default implementation of Port.path_to_crash_logs raised a > NotImplementedError. The exception is more discoverable because it is not > subtle and it would immediately force the implementer to implement this > method for P. > > ... > That's a good point. <https://trac.webkit.org/r216788> changes it so that it will throw an exception by default.