Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
113403002:
Move iossim to DVTiPhoneSimulatorRemoteClient. (Closed)
|
Created:
7 years ago by justincohen Modified:
6 years, 11 months ago CC:
chromium-reviews, TVL Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMove iossim to DVTiPhoneSimulatorRemoteClient.
Future versions of Xcode might not work with iPhoneSimulatorRemoteClient.framework. Moving to
DVTiPhoneSimulatorRemoteClient.framework seems to be better supported.
DVTiPhoneSimulatorRemoteClient's API for launching the simulator is mostly
the same, and allows for setting the deviceName string directly instead of
via a preference.
BUG=328874
NOTREECHECKS=true
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247245
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address TVL and lliabraa comments #
Total comments: 4
Patch Set 3 : Check snprintf return and more class-dump param comments #
Total comments: 2
Patch Set 4 : Space nit #Patch Set 5 : Require 64bit too #
Messages
Total messages: 23 (0 generated)
This needs more testing, and I'm not happy with how the sed needed to go into redirect-stdout.sh. But it's a start. WDYT?
Also tested with Xcode 5.0.
Awesome. It failed to compile on the bots - maybe because they're using 6.1 SDK to compile. https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:321: [[NSString stringWithFormat:@"%d", [session simulatedApplicationPID]] why change from stringValue to stringWithFormat? https://codereview.chromium.org/113403002/diff/1/testing/iossim/redirect-stdo... File testing/iossim/redirect-stdout.sh (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/redirect-stdo... testing/iossim/redirect-stdout.sh:20: $1 | sed /cxx_destruct/d > $2 I'm fine with this but we should update the comments in this file if we can't find a way to ignore the cxx_destruct files.
https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:321: [[NSString stringWithFormat:@"%d", [session simulatedApplicationPID]] Because it's not a NSNumber anymore, it's an int. On 2013/12/12 15:01:15, lliabraa wrote: > why change from stringValue to stringWithFormat?
How far back does this work? Specifically will it work with Xcode 4.5 & 4.6? https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.gyp File testing/iossim/iossim.gyp (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.gyp#ne... testing/iossim/iossim.gyp:44: # the output to the file specified as the second argument. Document the args in the comment? (what's the -I do?) Did you also need to update class-dump? https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:321: [[NSString stringWithFormat:@"%d", [session simulatedApplicationPID]] This whole thing would be less scary with a char[20] on the stack and snprintf. Right now it needs things to live in the autorelease pool while the query runs. https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:414: // Helper to find a class by name and die if it isn't found. any reason this got moved? https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:444: [DVTPlatformClass loadAllPlatformsReturningError:nil]; You should check the bool return result, and wouldn't hurt to collect the error so you can log if it does fail to start up.
PTAL! https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.gyp File testing/iossim/iossim.gyp (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.gyp#ne... testing/iossim/iossim.gyp:44: # the output to the file specified as the second argument. On 2013/12/12 16:08:27, TVL wrote: > Document the args in the comment? (what's the -I do?) > > Did you also need to update class-dump? Done. https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:321: [[NSString stringWithFormat:@"%d", [session simulatedApplicationPID]] Done, converted to snprintf. On 2013/12/12 16:08:27, TVL wrote: > This whole thing would be less scary with a char[20] on the stack and snprintf. > Right now it needs things to live in the autorelease pool while the query runs. https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:414: // Helper to find a class by name and die if it isn't found. Forward declaration. On 2013/12/12 16:08:27, TVL wrote: > any reason this got moved? https://codereview.chromium.org/113403002/diff/1/testing/iossim/iossim.mm#new... testing/iossim/iossim.mm:444: [DVTPlatformClass loadAllPlatformsReturningError:nil]; On 2013/12/12 16:08:27, TVL wrote: > You should check the bool return result, and wouldn't hurt to collect the error > so you can log if it does fail to start up. Done. https://codereview.chromium.org/113403002/diff/1/testing/iossim/redirect-stdo... File testing/iossim/redirect-stdout.sh (right): https://codereview.chromium.org/113403002/diff/1/testing/iossim/redirect-stdo... testing/iossim/redirect-stdout.sh:20: $1 | sed /cxx_destruct/d > $2 On 2013/12/12 15:01:15, lliabraa wrote: > I'm fine with this but we should update the comments in this file if we can't > find a way to ignore the cxx_destruct files. Done.
Is the classdump update in another cl? https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.gyp File testing/iossim/iossim.gyp (right): https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.gy... testing/iossim/iossim.gyp:45: # -I sorts classes, categories, and protocols by inheritance. add the -C ? https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.mm... testing/iossim/iossim.mm:319: snprintf(session_id, 20, "%d", [session simulatedApplicationPID]); check the result & error out?
Thanks! PTAL! https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.gyp File testing/iossim/iossim.gyp (right): https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.gy... testing/iossim/iossim.gyp:45: # -I sorts classes, categories, and protocols by inheritance. On 2013/12/16 18:50:28, TVL wrote: > add the -C ? Done. https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/20001/testing/iossim/iossim.mm... testing/iossim/iossim.mm:319: snprintf(session_id, 20, "%d", [session simulatedApplicationPID]); On 2013/12/16 18:50:28, TVL wrote: > check the result & error out? Done.
I didn't need to update class-dump at all for this to work. There's an older bug to update class-dump here: crbug.com/318160 Updating class-dump dind't fix the cxx_destruct problem...
Sorry I haven't responded on this CL. I don't know much about Objective-C (and I assume I'm only a reviewer on this CL as an FYI). Thanks for doing the work for this, Justin. We definitely need this. Will it be pulled into the Chrome for iOS repository as well? LGTM for the concept, even though the implementation looks like it is still being worked on.
This should be the only change needed, although it's blocked on the bots moving to Xcode 5.0. Earlier versions don't seem to support this, although I don't know exactly how far back this is supported.
lgtm https://codereview.chromium.org/113403002/diff/40001/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/40001/testing/iossim/iossim.mm... testing/iossim/iossim.mm:34: + (BOOL)loadAllPlatformsReturningError:(id *)arg1; no space before *
Over to TVL. Then need to wait for Xcode 5 on bots. https://codereview.chromium.org/113403002/diff/40001/testing/iossim/iossim.mm File testing/iossim/iossim.mm (right): https://codereview.chromium.org/113403002/diff/40001/testing/iossim/iossim.mm... testing/iossim/iossim.mm:34: + (BOOL)loadAllPlatformsReturningError:(id *)arg1; On 2013/12/17 12:36:52, lliabraa wrote: > no space before * Done.
Hey Justin, We've finally got the upstream waterfalls on Xcode 5.0.2, compiling with the 7.0 SDKs. I've sent your CL back to the iOS trybots.
Looks like it's compiling with 5.0, but not running. http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui... I wonder if this requires 5.1? On Fri, Jan 24, 2014 at 5:26 AM, <smut@google.com> wrote: > Hey Justin, > > We've finally got the upstream waterfalls on Xcode 5.0.2, compiling with > the 7.0 > SDKs. I've sent your CL back to the iOS trybots. > > https://codereview.chromium.org/113403002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Needed to force it to 64bit when built with Xcode. Bots are going green. PTAL!
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@chromium.org/113403002/190001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@chromium.org/113403002/190001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@chromium.org/113403002/190001
Message was sent while issue was closed.
Change committed as 247245 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
