Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10805004:
Add iossim testing tool for running iOS unit tests. (Closed)
|
Created:
8 years, 5 months ago by lliabraa Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd iossim testing tool for running iOS unit tests.
iossim is a command line tool used to run an iOS app in the iOS Simulator.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148753
Patch Set 1 #
Total comments: 2
Patch Set 2 : Set correct revision for class-dump in DEPS #
Total comments: 72
Patch Set 3 : addressed review comments #
Total comments: 8
Patch Set 4 : "addressed second round of comments #
Messages
Total messages: 9 (0 generated)
https://chromiumcodereview.appspot.com/10805004/diff/1/DEPS File DEPS (right): https://chromiumcodereview.appspot.com/10805004/diff/1/DEPS#newcode413 DEPS:413: "/trunk/deps/third_party/class-dump@99016", I'll need to update this rev once https://chromiumcodereview.appspot.com/10790046/ is committed.
Can you add an OWNERS in the new directory? Since you aren't a committer, put me and Rohit. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... File testing/iossim/RedirectStdout.sh (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... testing/iossim/RedirectStdout.sh:7: # the command's stdout to the file given as the second argument. Can you expand this comment to explain why having a script for this is necessary in the first place? From reading just this file it would seem really random. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... testing/iossim/RedirectStdout.sh:9: # Usage: RedirectStdout.sh <command> <output file> Can you change this from a comment to a real usage check that looks for two arguments and prints usage info if there aren't? I know people aren't likely to just try running it, but there's no reason not to. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... testing/iossim/RedirectStdout.sh:14: exec $1 > $2 We should quote $1 and $2 https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... File testing/iossim/iossim.gyp (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.gyp:1: { Add a license block. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:24: // file, so it must be defined here before we import the generated s/we import/importing/ https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:30: @class DTiPhoneSimulatorApplicationSpecifier; Move this up to the top so it's alphabetical. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:31: @protocol DTiPhoneSimulatorSessionDelegate Add a blank line before this. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:44: #define kHomeEnvVariable "HOME" Let's change these to const char* const, and the ones below to const int. Also, I know nothing else would actually link this file in, but just for consistency with Chromium code let's put all of this in the anonymous namespace. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:59: #define kSessionStartTimeout 30 // seconds This one would be const NSTimeInterval I guess (since it says double in the generated header, we could use double, but it's really likely that's what it really is). https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:63: #define ASL_KEY_REF_PID "RefPID" This one we can leave as-is for consistency with the things in parallels in asl.h. Move it up to before all the stuff that will go in the namespace though. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:67: static const char* gToolName = "iossim"; static is redundant. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:73: NSString* str = s/str/message/ in these functions https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:101: NSString* stdioPath_; // weak Two spaces before // rather than one. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:109: // from the Simulator; anything from io buffering to output interleaved s/;/,/ https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:110: // between the two mean they will show up out of onder here. Putting them s/onder/order/ https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:121: stdioPath_ = stdioPath; The only reason this doesn't eventually crash is that the autorelease pool lives for the lifetime of the app; we should make it less fragile and do a copy here, and a release in dealloc. (We could bring in scoped_nsobject, but it seems silly to make this code depend on Chromium just for that.) https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:122: } No {} for the one-line condition https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:130: // window). As a workaround, iossim creates temp file to hold output, which s/temp file/a temp file/ https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:141: [NSThread sleepForTimeInterval:0.1]; // seconds Remove the // seconds since TimeInterval is always seconds. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:146: // the last sleep cycle. Could we just swap the sleep and the print in the loop and use a do {} while () instead? Seems like a 0.1 second delay at the start wouldn't hurt anything. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:195: [NSThread sleepForTimeInterval:0.1]; // seconds Remove the comment. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:259: } No {} https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:283: // [NSDictionary dictionary]; If this doesn't work, let's not leave the code here. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:307: stringByAppendingPathComponent:dirNameTemplate]; Just indent 4 from the start of the previous line. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:311: } No {} https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:349: } No {} https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:357: LogError(@"Unable to set %s environment variables."); This will crash if it's ever reached; there's no argument for the format string. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:368: fprintf(stderr, " where <appPath> is the path to the .app directory and " Why all the separate fprintf statements instead of just string continuations? https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:380: } // namespace One more space before // https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:408: simHomePath = [NSString stringWithUTF8String:optarg]; This should be using NSFileManager's stringWithFileSystemRepresentation:length: https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:422: appPath = [NSString stringWithUTF8String:argv[optind++]]; As should this. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:471: dirNameTemplate); Indent 1 more https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:477: simHomePath); Indent 1 more https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:504: } Remove {}s on both
Thanks for the review. https://chromiumcodereview.appspot.com/10805004/diff/1/DEPS File DEPS (right): https://chromiumcodereview.appspot.com/10805004/diff/1/DEPS#newcode413 DEPS:413: "/trunk/deps/third_party/class-dump@99016", On 2012/07/18 08:47:13, Lane LiaBraaten wrote: > I'll need to update this rev once > https://chromiumcodereview.appspot.com/10790046/ is committed. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... File testing/iossim/RedirectStdout.sh (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... testing/iossim/RedirectStdout.sh:7: # the command's stdout to the file given as the second argument. On 2012/07/20 07:45:44, stuartmorgan wrote: > Can you expand this comment to explain why having a script for this is necessary > in the first place? From reading just this file it would seem really random. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... testing/iossim/RedirectStdout.sh:9: # Usage: RedirectStdout.sh <command> <output file> On 2012/07/20 07:45:44, stuartmorgan wrote: > Can you change this from a comment to a real usage check that looks for two > arguments and prints usage info if there aren't? I know people aren't likely to > just try running it, but there's no reason not to. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/Redi... testing/iossim/RedirectStdout.sh:14: exec $1 > $2 On 2012/07/20 07:45:44, stuartmorgan wrote: > We should quote $1 and $2 Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... File testing/iossim/iossim.gyp (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.gyp:1: { On 2012/07/20 07:45:44, stuartmorgan wrote: > Add a license block. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:24: // file, so it must be defined here before we import the generated On 2012/07/20 07:45:44, stuartmorgan wrote: > s/we import/importing/ Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:30: @class DTiPhoneSimulatorApplicationSpecifier; On 2012/07/20 07:45:44, stuartmorgan wrote: > Move this up to the top so it's alphabetical. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:31: @protocol DTiPhoneSimulatorSessionDelegate On 2012/07/20 07:45:44, stuartmorgan wrote: > Add a blank line before this. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:44: #define kHomeEnvVariable "HOME" On 2012/07/20 07:45:44, stuartmorgan wrote: > Let's change these to const char* const, and the ones below to const int. Done. > > Also, I know nothing else would actually link this file in, but just for > consistency with Chromium code let's put all of this in the anonymous namespace. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:59: #define kSessionStartTimeout 30 // seconds On 2012/07/20 07:45:44, stuartmorgan wrote: > This one would be const NSTimeInterval I guess (since it says double in the > generated header, we could use double, but it's really likely that's what it > really is). Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:63: #define ASL_KEY_REF_PID "RefPID" On 2012/07/20 07:45:44, stuartmorgan wrote: > This one we can leave as-is for consistency with the things in parallels in > asl.h. Move it up to before all the stuff that will go in the namespace though. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:67: static const char* gToolName = "iossim"; On 2012/07/20 07:45:44, stuartmorgan wrote: > static is redundant. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:73: NSString* str = On 2012/07/20 07:45:44, stuartmorgan wrote: > s/str/message/ in these functions Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:101: NSString* stdioPath_; // weak On 2012/07/20 07:45:44, stuartmorgan wrote: > Two spaces before // rather than one. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:109: // from the Simulator; anything from io buffering to output interleaved On 2012/07/20 07:45:44, stuartmorgan wrote: > s/;/,/ Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:110: // between the two mean they will show up out of onder here. Putting them On 2012/07/20 07:45:44, stuartmorgan wrote: > s/onder/order/ Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:121: stdioPath_ = stdioPath; On 2012/07/20 07:45:44, stuartmorgan wrote: > The only reason this doesn't eventually crash is that the autorelease pool lives > for the lifetime of the app; we should make it less fragile and do a copy here, > and a release in dealloc. > > (We could bring in scoped_nsobject, but it seems silly to make this code depend > on Chromium just for that.) Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:122: } On 2012/07/20 07:45:44, stuartmorgan wrote: > No {} for the one-line condition Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:130: // window). As a workaround, iossim creates temp file to hold output, which On 2012/07/20 07:45:44, stuartmorgan wrote: > s/temp file/a temp file/ Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:141: [NSThread sleepForTimeInterval:0.1]; // seconds On 2012/07/20 07:45:44, stuartmorgan wrote: > Remove the // seconds since TimeInterval is always seconds. Is this a hard convention? I don't see how it hurts to have the comment - it can only help folks that may be reading this code but aren't familiar with the API https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:146: // the last sleep cycle. On 2012/07/20 07:45:44, stuartmorgan wrote: > Could we just swap the sleep and the print in the loop and use a do {} while () > instead? Seems like a 0.1 second delay at the start wouldn't hurt anything. I think it is better to leave this final read/write in place. Suppose this thread reads some data from the file then while it is writing the data to stdout, the app writes a little more to the file and quits. That last bit would not be read from the file and written to stdout. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:195: [NSThread sleepForTimeInterval:0.1]; // seconds On 2012/07/20 07:45:44, stuartmorgan wrote: > Remove the comment. See above. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:259: } On 2012/07/20 07:45:44, stuartmorgan wrote: > No {} Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:283: // [NSDictionary dictionary]; On 2012/07/20 07:45:44, stuartmorgan wrote: > If this doesn't work, let's not leave the code here. I changed this to a more generic TODO for supporting environment variables to send to the app. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:307: stringByAppendingPathComponent:dirNameTemplate]; On 2012/07/20 07:45:44, stuartmorgan wrote: > Just indent 4 from the start of the previous line. I've changed the formatting to put the entire call on one line. Is there a preference between that and what you suggested? What I did: NSString* fullPathTemplate = [NSTemporaryDirectory() stringByAppendingPathComponent:dirNameTemplate]; What you suggested: NSString* fullPathTemplate = [NSTemporaryDirectory() stringByAppendingPathComponent:dirNameTemplate]; https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:311: } On 2012/07/20 07:45:44, stuartmorgan wrote: > No {} Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:349: } On 2012/07/20 07:45:44, stuartmorgan wrote: > No {} Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:357: LogError(@"Unable to set %s environment variables."); On 2012/07/20 07:45:44, stuartmorgan wrote: > This will crash if it's ever reached; there's no argument for the format string. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:368: fprintf(stderr, " where <appPath> is the path to the .app directory and " On 2012/07/20 07:45:44, stuartmorgan wrote: > Why all the separate fprintf statements instead of just string continuations? No idea. Changed to string continuations https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:380: } // namespace On 2012/07/20 07:45:44, stuartmorgan wrote: > One more space before // Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:408: simHomePath = [NSString stringWithUTF8String:optarg]; On 2012/07/20 07:45:44, stuartmorgan wrote: > This should be using NSFileManager's stringWithFileSystemRepresentation:length: Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:422: appPath = [NSString stringWithUTF8String:argv[optind++]]; On 2012/07/20 07:45:44, stuartmorgan wrote: > As should this. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:471: dirNameTemplate); On 2012/07/20 07:45:44, stuartmorgan wrote: > Indent 1 more Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:477: simHomePath); On 2012/07/20 07:45:44, stuartmorgan wrote: > Indent 1 more Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:504: } On 2012/07/20 07:45:44, stuartmorgan wrote: > Remove {}s on both Done.
FYI - The chrome-infra folks have added the git mirror for class-dump: http://code.google.com/p/chromium/issues/detail?id=139155
LGTM with nits. However, you may want to wait until qsr upstreams the Mac target build support, since that changes this gyp file. But if you'd rather land it this way and then follow up with a change after, that's fine. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:141: [NSThread sleepForTimeInterval:0.1]; // seconds On 2012/07/26 19:14:11, Lane LiaBraaten wrote: > On 2012/07/20 07:45:44, stuartmorgan wrote: > > Remove the // seconds since TimeInterval is always seconds. > > Is this a hard convention? Yes, see https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Founda... > I don't see how it hurts to have the comment - it can > only help folks that may be reading this code but aren't familiar with the API That argument works for anything though (e.g., "[pool drain] // releases everything autoreleased in the pool's scope), so it seems weird to single this out. NSTimeInterval is a pretty common type, so the fact that it's seconds can be assumed to be understood by most readers. If you want to keep it spelled out, then let's make it self-documenting: create a file-level const NSTimeInterval kOutPollIntervalSeconds = 0.1 and use it here and below. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:307: stringByAppendingPathComponent:dirNameTemplate]; On 2012/07/26 19:14:11, Lane LiaBraaten wrote: > I've changed the formatting to put the entire call on one line. Is there a > preference between that and what you suggested? Nope, both are fine. I tend toward what you did as well when it fits. https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/OWNERS File testing/iossim/OWNERS (right): https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/OWNE... testing/iossim/OWNERS:2: rohitrao@chromium.org Alphabetize these https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... File testing/iossim/iossim.gyp (right): https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... testing/iossim/iossim.gyp:32: 'LD_RUNPATH_SEARCH_PATHS': '<(iphone_sim_path) <(other_frameworks_path)', This should be an array (I think qsr just changed this in the original file) https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... testing/iossim/iossim.gyp:48: '<(DEPTH)/testing/iossim/RedirectStdout.sh', I noticed while upstreaming another script today that the chromium naming convention is generally of the form redirect-stdout.sh; could you rename this to match that? https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... testing/iossim/iossim.mm:174: // (i.e. /.../iPhoneSimulator4.3.sdk/usr/libexec/installd -t 30) that will s/i.e./e.g.,/
thanks! https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:141: [NSThread sleepForTimeInterval:0.1]; // seconds On 2012/07/26 19:47:05, stuartmorgan wrote: > On 2012/07/26 19:14:11, Lane LiaBraaten wrote: > > On 2012/07/20 07:45:44, stuartmorgan wrote: > > > Remove the // seconds since TimeInterval is always seconds. > > > > Is this a hard convention? > > Yes, see > https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Founda... I meant is it a hard convention not to document units of time like this. I only see inline comments used like this occasionally in the chrome codebase. > > > I don't see how it hurts to have the comment - it can > > only help folks that may be reading this code but aren't familiar with the API > > That argument works for anything though (e.g., "[pool drain] // releases > everything autoreleased in the pool's scope), so it seems weird to single this > out. NSTimeInterval is a pretty common type, so the fact that it's seconds can > be assumed to be understood by most readers. That's an overgeneralization. This kind of comment is useful when a hard-coded value is passed into a function (e.g. doFoo(FALSE /* isBar */)) so the reader doesn't have to look up the API. Though maybe it's just a red flag that using a variable would be more readable anyway (as you've suggested). > > If you want to keep it spelled out, then let's make it self-documenting: create > a file-level const NSTimeInterval kOutPollIntervalSeconds = 0.1 and use it here > and below. Done. https://chromiumcodereview.appspot.com/10805004/diff/1005/testing/iossim/ioss... testing/iossim/iossim.mm:307: stringByAppendingPathComponent:dirNameTemplate]; On 2012/07/26 19:47:05, stuartmorgan wrote: > On 2012/07/26 19:14:11, Lane LiaBraaten wrote: > > I've changed the formatting to put the entire call on one line. Is there a > > preference between that and what you suggested? > > Nope, both are fine. I tend toward what you did as well when it fits. Ack. https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/OWNERS File testing/iossim/OWNERS (right): https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/OWNE... testing/iossim/OWNERS:2: rohitrao@chromium.org On 2012/07/26 19:47:06, stuartmorgan wrote: > Alphabetize these Done. https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... File testing/iossim/iossim.gyp (right): https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... testing/iossim/iossim.gyp:32: 'LD_RUNPATH_SEARCH_PATHS': '<(iphone_sim_path) <(other_frameworks_path)', On 2012/07/26 19:47:06, stuartmorgan wrote: > This should be an array (I think qsr just changed this in the original file) Done. https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... testing/iossim/iossim.gyp:48: '<(DEPTH)/testing/iossim/RedirectStdout.sh', On 2012/07/26 19:47:06, stuartmorgan wrote: > I noticed while upstreaming another script today that the chromium naming > convention is generally of the form redirect-stdout.sh; could you rename this to > match that? Done. https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10805004/diff/9001/testing/iossim/ioss... testing/iossim/iossim.mm:174: // (i.e. /.../iPhoneSimulator4.3.sdk/usr/libexec/installd -t 30) that will On 2012/07/26 19:47:06, stuartmorgan wrote: > s/i.e./e.g.,/ Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lliabraa@chromium.org/10805004/10008
Change committed as 148753 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
