Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10828070:
Don't directly link to the private Xcode framework, instead find it at runtime (Closed)
|
Created:
8 years, 4 months ago by TVL Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
Description- Don't directly link to the private Xcode framework, instead find it at runtime
and lookup the classes by name. This allows the binary to work with multiple
versions of Xcode and doesn't tile the binary to an Xcode install location.
- Fix problem where the tool could log the wrong thing since basename()
returns an internal buffer.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149067
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 29
Patch Set 7 : #
Total comments: 8
Patch Set 8 : #Patch Set 9 : #Messages
Total messages: 12 (0 generated)
LGTM - a few formatting nits https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:282: [[[NSString alloc] initWithData:output indent by 4 https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:296: NSString* devToolsFoundationPath = indent by 4 https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:299: [NSBundle bundleWithPath:devToolsFoundationPath]; indent by 4 https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:303: [developerDir stringByAppendingPathComponent:kSimulatorFrameworkPathLeaf]; indent by 4 https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:324: FindClassByName(@"DTiPhoneSimulatorApplicationSpecifier"); indent by 4
https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:282: [[[NSString alloc] initWithData:output On 2012/07/30 17:18:51, Lane LiaBraaten wrote: > indent by 4 Done. https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:296: NSString* devToolsFoundationPath = On 2012/07/30 17:18:51, Lane LiaBraaten wrote: > indent by 4 Done. https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:299: [NSBundle bundleWithPath:devToolsFoundationPath]; On 2012/07/30 17:18:51, Lane LiaBraaten wrote: > indent by 4 Done. https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:303: [developerDir stringByAppendingPathComponent:kSimulatorFrameworkPathLeaf]; On 2012/07/30 17:18:51, Lane LiaBraaten wrote: > indent by 4 Done. https://chromiumcodereview.appspot.com/10828070/diff/3001/testing/iossim/ioss... testing/iossim/iossim.mm:324: FindClassByName(@"DTiPhoneSimulatorApplicationSpecifier"); On 2012/07/30 17:18:51, Lane LiaBraaten wrote: > indent by 4 Done.
LGTM with a whole bunch of nits. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:47: #define kSimulatorFrameworkPathLeaf \ s/PathLeaf/RelativePath/ ? PathLeaf sounds like it should be only the last component to me. Also, change both #defines to NSString* const https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:262: NSString* devDir = [env objectForKey:@"DEVELOPER_DIR"]; developerDir https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:265: } No {} for one-line statements, per Chromium style. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:268: NSTask* task; xcodeSelectTask https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:273: NSPipe* pipe = [NSPipe pipe]; outputPipe https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:275: NSFileHandle* file = [pipe fileHandleForReading]; outputFile https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:281: NSString* outputStr = Seems like outputData and then output would make more sense that output and outputStr, since the string is the output in the format we really want? https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:284: NSCharacterSet* wsNLSet = [NSCharacterSet whitespaceAndNewlineCharacterSet]; Ugh. Inlining the char set would be much less ugly than this name, even with having to line-break after the : https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:288: } No {} https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:295: // frameworks, manually load them first so everything can be linked up. s/,/;/ https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:311: Class FindClassByName(NSString* clazzName) { Is className as an identifier really protected? I thought it was just a method name. If we have to use something else, how about nameOfClass; it's not great, but clazzName burns my eyes :) https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:468: // internal buffer. Give it a copy to modify, and copy what it returns. You say "give it a copy", but you don't give it the copy. Did you mean basename(worker)? https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:473: if (toolName != NULL) Do we really need to handle OOM gracefully here? Seems like the chances that the program is going to run successfully if we OOM on practically the first allocation is roughly 0, so we may as well just ignore the failure case. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:476: if (worker != NULL) Same here. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:539: // Figure out the developer dir. Seems like these two comments don't add much, since they are followed by self-explanatory method names.
https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:47: #define kSimulatorFrameworkPathLeaf \ On 2012/07/30 19:34:21, stuartmorgan wrote: > s/PathLeaf/RelativePath/ ? PathLeaf sounds like it should be only the last > component to me. > > Also, change both #defines to NSString* const Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:262: NSString* devDir = [env objectForKey:@"DEVELOPER_DIR"]; On 2012/07/30 19:34:21, stuartmorgan wrote: > developerDir Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:265: } On 2012/07/30 19:34:21, stuartmorgan wrote: > No {} for one-line statements, per Chromium style. Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:268: NSTask* task; On 2012/07/30 19:34:21, stuartmorgan wrote: > xcodeSelectTask Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:273: NSPipe* pipe = [NSPipe pipe]; On 2012/07/30 19:34:21, stuartmorgan wrote: > outputPipe Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:275: NSFileHandle* file = [pipe fileHandleForReading]; On 2012/07/30 19:34:21, stuartmorgan wrote: > outputFile Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:281: NSString* outputStr = On 2012/07/30 19:34:21, stuartmorgan wrote: > Seems like outputData and then output would make more sense that output and > outputStr, since the string is the output in the format we really want? Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:284: NSCharacterSet* wsNLSet = [NSCharacterSet whitespaceAndNewlineCharacterSet]; On 2012/07/30 19:34:21, stuartmorgan wrote: > Ugh. Inlining the char set would be much less ugly than this name, even with > having to line-break after the : Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:288: } On 2012/07/30 19:34:21, stuartmorgan wrote: > No {} Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:295: // frameworks, manually load them first so everything can be linked up. On 2012/07/30 19:34:21, stuartmorgan wrote: > s/,/;/ Done. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:311: Class FindClassByName(NSString* clazzName) { On 2012/07/30 19:34:21, stuartmorgan wrote: > Is className as an identifier really protected? I thought it was just a method > name. > > If we have to use something else, how about nameOfClass; it's not great, but > clazzName burns my eyes :) I thought i'd hit it before. renamed. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:468: // internal buffer. Give it a copy to modify, and copy what it returns. On 2012/07/30 19:34:21, stuartmorgan wrote: > You say "give it a copy", but you don't give it the copy. Did you mean > basename(worker)? doh. fixed. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:473: if (toolName != NULL) On 2012/07/30 19:34:21, stuartmorgan wrote: > Do we really need to handle OOM gracefully here? Seems like the chances that the > program is going to run successfully if we OOM on practically the first > allocation is roughly 0, so we may as well just ignore the failure case. I'll remove them if you want, was just trying to be "correct" incase this gets copied to other places, etc. https://chromiumcodereview.appspot.com/10828070/diff/10002/testing/iossim/ios... testing/iossim/iossim.mm:539: // Figure out the developer dir. On 2012/07/30 19:34:21, stuartmorgan wrote: > Seems like these two comments don't add much, since they are followed by > self-explanatory method names. removed. Was just following the style of the next two ifs.
Still LGTM; just tiny nits. https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:74: NSString * const kSimulatorFrameworkRelativePath = NSString* to match the rest of the file. https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:283: [output stringByTrimmingCharactersInSet: Pull this up to the previous line (and then just indent the next line 4) https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:295: [developerDir Same https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:302: [developerDir Same
https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... File testing/iossim/iossim.mm (right): https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:74: NSString * const kSimulatorFrameworkRelativePath = On 2012/07/30 21:12:25, stuartmorgan wrote: > NSString* to match the rest of the file. Done. https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:283: [output stringByTrimmingCharactersInSet: On 2012/07/30 21:12:25, stuartmorgan wrote: > Pull this up to the previous line (and then just indent the next line 4) Done. https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:295: [developerDir On 2012/07/30 21:12:25, stuartmorgan wrote: > Same Done. https://chromiumcodereview.appspot.com/10828070/diff/5003/testing/iossim/ioss... testing/iossim/iossim.mm:302: [developerDir On 2012/07/30 21:12:25, stuartmorgan wrote: > Same Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thomasvl@chromium.org/10828070/3004
Presubmit check for 10828070-3004 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). testing/iossim/iossim.mm, line 75, 116 chars
I wrapped the string
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thomasvl@chromium.org/10828070/6007
Change committed as 149067 |
