Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10834051:
Bringing up the net target on iOS. (Closed)
|
Created:
8 years, 4 months ago by noyau (Ping after 24h) Modified:
8 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionBringing up the net target on iOS.
This builds a small subset of net containing only the dependencies needed to build the ui target.
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148954
Patch Set 1 #
Total comments: 16
Patch Set 2 : Feedback. #
Total comments: 21
Patch Set 3 : Sleevi's feedback. #Patch Set 4 : Rebase. #Patch Set 5 : Use the proper alphabetical order. #
Total comments: 21
Patch Set 6 : Nit fixing. #
Total comments: 10
Patch Set 7 : Applying wtc's feedback. #Messages
Total messages: 13 (0 generated)
Small changes, but otherwise LGTM as a toehold for getting net up and running for iOS. Once you make those changes, a real net reviewer can take a look :) https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode806 net/net.gyp:806: # as more is brought up for iOS. Move this in, and put v8 above it. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode807 net/net.gyp:807: ['OS!="ios"', { Spaces around != https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1062 net/net.gyp:1062: '$(SDKROOT)/System/Library/Frameworks/SystemConfiguration.framework', Order https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1069 net/net.gyp:1069: # target. Make this a TODO so it's clearer. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1071 net/net.gyp:1071: ['include', '^base/dns_util\\.cc$'], Change these all to \\.*' https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1125 net/net.gyp:1125: '../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations', Remove this too. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1486 net/net.gyp:1486: ['exclude', '.*'], Put a TODO to make it obvious what's going on here. If anything that we are building has unit tests that build, add them here. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1736 net/net.gyp:1736: # the code allwing them to run is not yet there. Typos (ont he, allwing) Put this back but remove the code.
+rsleevi for review. All requested changes done, including the bring up of the unit_tests. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode806 net/net.gyp:806: # as more is brought up for iOS. On 2012/07/27 15:34:00, stuartmorgan wrote: > Move this in, and put v8 above it. Done. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode807 net/net.gyp:807: ['OS!="ios"', { On 2012/07/27 15:34:00, stuartmorgan wrote: > Spaces around != Done. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1062 net/net.gyp:1062: '$(SDKROOT)/System/Library/Frameworks/SystemConfiguration.framework', On 2012/07/27 15:34:00, stuartmorgan wrote: > Order Done. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1069 net/net.gyp:1069: # target. On 2012/07/27 15:34:00, stuartmorgan wrote: > Make this a TODO so it's clearer. Done. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1071 net/net.gyp:1071: ['include', '^base/dns_util\\.cc$'], On 2012/07/27 15:34:00, stuartmorgan wrote: > Change these all to \\.*' Done. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1125 net/net.gyp:1125: '../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations', On 2012/07/27 15:34:00, stuartmorgan wrote: > Remove this too. Done. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1486 net/net.gyp:1486: ['exclude', '.*'], On 2012/07/27 15:34:00, stuartmorgan wrote: > Put a TODO to make it obvious what's going on here. > > If anything that we are building has unit tests that build, add them here. Added a TODO. In order for the tests to compile I had to remove net_test_support and add a main simpler than the one in net/base. https://chromiumcodereview.appspot.com/10834051/diff/1/net/net.gyp#newcode1736 net/net.gyp:1736: # the code allwing them to run is not yet there. On 2012/07/27 15:34:00, stuartmorgan wrote: > Typos (ont he, allwing) > > Put this back but remove the code. Done.
Note that I'll be a bit slow to respond as I'm on the road next week. I've added wtc@ who can stamp in a more timely manner. Overall I think it looks good, but I have some (perhaps nitty) stylistic concerns below from past experiences trying to make sense of GYP files. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/base/platform_m... File net/base/platform_mime_util_mac.mm (right): https://chromiumcodereview.appspot.com/10834051/diff/3002/net/base/platform_m... net/base/platform_mime_util_mac.mm:11: #endif // defined(OS_IOS) style nit: Platform-specific conditional includes go after the 'regular' include order, as per http://www.chromium.org/developers/coding-style#TOC-Platform-defines https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode805 net/net.gyp:805: 'dependencies': [ Any chance this can be OS == "ios" and dependencies!? While I suppose there is an open question as to whether it's more preferable to 'list all dependencies and subtract' or 'list common dependencies then add', I think following in the spirit of sources/sources!/sources/ (which lists all then subtracts, generally), it'd be good to use subtraction rather than addition here. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode808 net/net.gyp:808: # OS=ios. Move dependencies back to the main dependencies section above nit: 80 character wrapping https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1101: ['include', 'base/platform_mime_util_mac\\.mm$'], Any reason this is part of target_conditions and not part of the above sources inclusions (line 1071-1088) ? (FWIW, I have no idea why Android is doing what it's doing on 1096) https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1356: 'dependencies': [ same comments re: dependencies! https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1524: # We do not support PAC scripts on iOS. I seem to recall there being a conversation on IRC with willchan that decided proxies were supported? nit: I'll echo mark@'s normal comments and say "avoid 'we' in comments" (eg: "PAC scripts are not supported on iOS") https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1682: ['OS != "ios"', { If iOS is going to continue to be a very, very minimal set of net/ for the long-term (which I suspect it is), does it make more sense to do what Android did and that is to list individual, explicit targets within the iOS all.gyp, rather than move them wholesale to these sorts of conditionals? https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1684: # iOS doesn't have the concept of simple executables, those targets can't nit: line wrapping And is this an iOS limitation or a limitation with how iOS makes use of the XCode generator? I can't imagine that iOS cares one way or the other. Since it "should" be able to package helper executables the same as a normal app, I imagine that a target that produces a 'raw' executable isn't much different as far as the toolchain is concerned, it's only XCode that gets it wrong. Perhaps indicate that it's an XCode limitation, so that if (and a big if) iOS builds ever switched to say, ninja, we 'could' run such tests?
https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode805 net/net.gyp:805: 'dependencies': [ On 2012/07/28 00:33:13, Ryan Sleevi wrote: > Any chance this can be OS == "ios" and dependencies!? Unfortunately dependencies! doesn't work when the gyp file doesn't exist at all, and we don't pull a full tree on iOS, so there's no v8.gyp https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1101: ['include', 'base/platform_mime_util_mac\\.mm$'], On 2012/07/28 00:33:13, Ryan Sleevi wrote: > Any reason this is part of target_conditions and not part of the above sources > inclusions (line 1071-1088) ? > > (FWIW, I have no idea why Android is doing what it's doing on 1096) Same reason we're doing this probably; _mac. is excluded in a target_conditions section in filename_rules.gypi, and sources/ is order-sensitive (deliberately). So to re-include a file that's excluded by default due to platform rules, you have to put in in target_conditionals so that it will end up after those rules when everything is merged. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1682: ['OS != "ios"', { On 2012/07/28 00:33:13, Ryan Sleevi wrote: > If iOS is going to continue to be a very, very minimal set of net/ for the > long-term (which I suspect it is), does it make more sense to do what Android > did and that is to list individual, explicit targets within the iOS all.gyp, > rather than move them wholesale to these sorts of conditionals? Since an "All" target gets generated for each project, it's kind of annoying having targets exist that can't be compiled because it means you can't build "net (All)". What's the down-side to moving them? https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1684: # iOS doesn't have the concept of simple executables, those targets can't On 2012/07/28 00:33:13, Ryan Sleevi wrote: > And is this an iOS limitation or a limitation with how iOS makes use of the > XCode generator? I can't imagine that iOS cares one way or the other. Since it > "should" be able to package helper executables the same as a normal app, I > imagine that a target that produces a 'raw' executable isn't much different as > far as the toolchain is concerned, it's only XCode that gets it wrong. The fact that a command-line tool can't be build is an iOS limitation. It's possible to make bundles, but it requires a fair amount of hoop-jumping (see testing/gtest.gyp's iOS section). We could make a more generic version of that and have common.gypi inject it into all 'executable' type targets, I suppose, and maybe even build that into the generator. It's not at all clear how useful that is though, because you also have issues like, an app must service the event loop promptly, so any executable that doesn't run almost instantly needs potentially extensive code changes to ensure it doesn't get killed by the OS. We've done that for tests, but we can't do it generically for all possible programs.
Because the concerns are largely stylistic nits at this point, I'll stamp LGTM, but please see the feedback below. There is one request for a comment addition that I don't think should be too controversial. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode805 net/net.gyp:805: 'dependencies': [ On 2012/07/28 13:53:43, stuartmorgan wrote: > On 2012/07/28 00:33:13, Ryan Sleevi wrote: > > Any chance this can be OS == "ios" and dependencies!? > > Unfortunately dependencies! doesn't work when the gyp file doesn't exist at all, > and we don't pull a full tree on iOS, so there's no v8.gyp Is that the same for all of the targets below? It would seem that at least crypto/crypto.gyp should be checked out, since it's part of a 'main' Chromium check-out. My own take would be that it'd be more preferable to do the dependencies: { } just for any non-checked out paths, and dependencies! for the checked-out-but-not-used. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1101: ['include', 'base/platform_mime_util_mac\\.mm$'], On 2012/07/28 13:53:43, stuartmorgan wrote: > On 2012/07/28 00:33:13, Ryan Sleevi wrote: > > Any reason this is part of target_conditions and not part of the above sources > > inclusions (line 1071-1088) ? > > > > (FWIW, I have no idea why Android is doing what it's doing on 1096) > > Same reason we're doing this probably; _mac. is excluded in a target_conditions > section in filename_rules.gypi, and sources/ is order-sensitive (deliberately). > So to re-include a file that's excluded by default due to platform rules, you > have to put in in target_conditionals so that it will end up after those rules > when everything is merged. Ah, right. Would it be possible then to add a comment (on say, line 1093-1094) commenting on this fact? I know I've received several questions in the past re: GYP-in-Chromium that essentially boiled down to cargo-culting ("I don't understand why this is being done, but since it matches the existing code, I assume it's correct"). Since this part is subtle, it would be good to comment on it. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1682: ['OS != "ios"', { On 2012/07/28 13:53:43, stuartmorgan wrote: > On 2012/07/28 00:33:13, Ryan Sleevi wrote: > > If iOS is going to continue to be a very, very minimal set of net/ for the > > long-term (which I suspect it is), does it make more sense to do what Android > > did and that is to list individual, explicit targets within the iOS all.gyp, > > rather than move them wholesale to these sorts of conditionals? > > Since an "All" target gets generated for each project, it's kind of annoying > having targets exist that can't be compiled because it means you can't build > "net (All)". What's the down-side to moving them? My general concern is one of trying to be able to trace what a GYP file will do, particularly when tracking down build errors that arise inconsistently. Similar to the Chromium preference for conditionals that are structured as if (foo()) return; bar(); as opposed to if (!foo()) { bar(); } else { return; } I tend to prefer GYP files that are nested as little as possible, when possible. The use of "net (All)" seems to be exceptional and tied to iOS - the use of ninja or make (on Mac) have made such meta-targets unimportant, and the equivalent in Visual Studio (net.sln) has also been largely discarded as irrelevant in the face of chrome.sln / all.sln / some.sln. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1684: # iOS doesn't have the concept of simple executables, those targets can't On 2012/07/28 13:53:43, stuartmorgan wrote: > On 2012/07/28 00:33:13, Ryan Sleevi wrote: > > And is this an iOS limitation or a limitation with how iOS makes use of the > > XCode generator? I can't imagine that iOS cares one way or the other. Since it > > "should" be able to package helper executables the same as a normal app, I > > imagine that a target that produces a 'raw' executable isn't much different as > > far as the toolchain is concerned, it's only XCode that gets it wrong. > > The fact that a command-line tool can't be build is an iOS limitation. It's > possible to make bundles, but it requires a fair amount of hoop-jumping (see > testing/gtest.gyp's iOS section). > > We could make a more generic version of that and have common.gypi inject it into > all 'executable' type targets, I suppose, and maybe even build that into the > generator. > > It's not at all clear how useful that is though, because you also have issues > like, an app must service the event loop promptly, so any executable that > doesn't run almost instantly needs potentially extensive code changes to ensure > it doesn't get killed by the OS. We've done that for tests, but we can't do it > generically for all possible programs. The android port has addressed this via the <(gtest_target_type) [or something like that], which alternates between 'shared_library' (when building a JNI-based test executable) and 'executable' (when building as an NDK executable) I thought XCode supported bundled executables (eg: under Contents/Resources or w/e) regardless of targeting for OS X or iOS, which is why they didn't need to exclude these targets, even though they never use them (and ergo, never build them). If it does support those, and I'm not mistaken, then it should be "as simple as" marking these as executables, but not actually building/including them in any bundle. That said, this is admittedly largely a stylistic concern with the GYP file, but it seems like this concern will be one with quite a bit of the iOS upstreaming, so like with the Android team, I'm just trying to find a solution early if there is one.
Added comments, fixed some of the dependencies as requested. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode805 net/net.gyp:805: 'dependencies': [ On 2012/07/29 09:01:25, Ryan Sleevi wrote: > On 2012/07/28 13:53:43, stuartmorgan wrote: > > On 2012/07/28 00:33:13, Ryan Sleevi wrote: > > > Any chance this can be OS == "ios" and dependencies!? > > > > Unfortunately dependencies! doesn't work when the gyp file doesn't exist at > all, > > and we don't pull a full tree on iOS, so there's no v8.gyp > > Is that the same for all of the targets below? It would seem that at least > crypto/crypto.gyp should be checked out, since it's part of a 'main' Chromium > check-out. > > My own take would be that it'd be more preferable to do the dependencies: { } > just for any non-checked out paths, and dependencies! for the > checked-out-but-not-used. I've reduced the list as much as I could. However including crypto and removing it via a conditional makes gyp_chromium fail so I left it there for now. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode808 net/net.gyp:808: # OS=ios. Move dependencies back to the main dependencies section above On 2012/07/28 00:33:13, Ryan Sleevi wrote: > nit: 80 character wrapping Done. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1101: ['include', 'base/platform_mime_util_mac\\.mm$'], On 2012/07/29 09:01:25, Ryan Sleevi wrote: > On 2012/07/28 13:53:43, stuartmorgan wrote: > > On 2012/07/28 00:33:13, Ryan Sleevi wrote: > > > Any reason this is part of target_conditions and not part of the above > sources > > > inclusions (line 1071-1088) ? > > > > > > (FWIW, I have no idea why Android is doing what it's doing on 1096) > > > > Same reason we're doing this probably; _mac. is excluded in a > target_conditions > > section in filename_rules.gypi, and sources/ is order-sensitive > (deliberately). > > So to re-include a file that's excluded by default due to platform rules, you > > have to put in in target_conditionals so that it will end up after those rules > > when everything is merged. > > Ah, right. > > Would it be possible then to add a comment (on say, line 1093-1094) commenting > on this fact? I know I've received several questions in the past re: > GYP-in-Chromium that essentially boiled down to cargo-culting ("I don't > understand why this is being done, but since it matches the existing code, I > assume it's correct"). Since this part is subtle, it would be good to comment on > it. Done. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1356: 'dependencies': [ On 2012/07/28 00:33:13, Ryan Sleevi wrote: > same comments re: dependencies! Same answer. Moved one I could up, but I could not move crypto. https://chromiumcodereview.appspot.com/10834051/diff/3002/net/net.gyp#newcode... net/net.gyp:1524: # We do not support PAC scripts on iOS. On 2012/07/28 00:33:13, Ryan Sleevi wrote: > I seem to recall there being a conversation on IRC with willchan that decided > proxies were supported? > > nit: I'll echo mark@'s normal comments and say "avoid 'we' in comments" (eg: > "PAC scripts are not supported on iOS") This comment is copied from the comment in the ios tree where this test has been explicitely disabled. Fixed the comment.
LGTM with nits. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/base/platform_m... File net/base/platform_mime_util_mac.mm (right): https://chromiumcodereview.appspot.com/10834051/diff/1006/net/base/platform_m... net/base/platform_mime_util_mac.mm:11: #endif // defined(OS_IOS) Move this whole block to after all other headers. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode812 net/net.gyp:812: # above as more is brought up for iOS. Since there's just one thing, change everything after the ";" to "move this back to the main dependencies section once crypto builds for iOS" https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1074: ['include', '^base/dns_util\\..*$'], .*$ is silly; just omit it. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1098: # are needed in specific cases on other platforms. To explicitely s/To explicitely re-include those files/Re-including them/ https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1099: # re-include those files can only be done in target_conditions as it re-including https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1100: # evaluate after the platform rules. s/evaluate/is evaluated/ https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1365: # TODO(ios): This is temporary; Move dependencies back to the main s/dependencies // https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1366: # dependencies section as more is brought up for iOS. s/more/crypto/ https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1482: # TODO: For now this only test the subset of code that was enabled in s/TODO/TODO(ios)/ s/test/tests/ s/was/is/ https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1535: # TODO:(ios): Enable those tests once the code to exercise is s/those/these/ https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1693: # iOS doesn't have the concept of simple executables, those targets s/those/so these/
On 2012/07/29 09:01:25, Ryan Sleevi wrote: > My general concern is one of trying to be able to trace what a GYP file will do, > particularly when tracking down build errors that arise inconsistently. I fundamentally think that having a target that exists but a) doesn't build, and b) doesn't do anything meaningful if built, is highly confusing. I don't think it serves the conceptual idea of making it clear what a gyp file does, since having the build system make targets that are unbuildable is very unintuitive. There's significant precedent for platform-specific targets in Chromium, so I don't think we're breaking new ground here. > The use of "net (All)" seems to be exceptional and tied to iOS - the use of > ninja or make (on Mac) have made such meta-targets unimportant 'All' was just an example; anything that makes it appear to someone looking at gyp or an IDE that a target exists (meaningfully) for iOS when it doesn't hurts understandability of the build system. > I thought XCode supported bundled executables (eg: under Contents/Resources or > w/e) regardless of targeting for OS X or iOS See my previous reply; the issue is iOS itself, not Xcode. > If it does support those, and I'm not mistaken, then it should be "as > simple as" marking these as executables, but not actually building/including > them in any bundle. I'm not following. Are you suggesting that we make the targets build as bundles that can't possibly be run? (See my previous reply for the reasons they won't run) I find that as ugly as having unbuildable targets. > I'm just trying to find a solution early if there is one. Sure, but I think solution when a target either doesn't build or can't actually be run on a platform *is* to exclude it on the platform. Anything else seems like it favors abstract purity of a gyp file itself vs. having gyp actually expressing what we are--and should be--building, which seems like a bad tradeoff to me. If you still disagree, I'd suggest raising this on chromium-dev and we can continue the discussion there.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noyau@chromium.org/10834051/10005
Change committed as 148954
Review comments on patch set 6: I have some questions about net/net.gyp. Everything else looks good. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1059: [ 'OS == "ios"', { Nit: it would be nice to list this ios section right below the mac section (lines 1017-1032) because they are closely related. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1480: ['OS == "ios"', { Nit: same here: I suggest listing the ios section right below the mac section (lines 1442-1449). https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1612: ['inside_chromium_build==1 and OS != "ios"', { This change is probably worth a comment. I can't figure out why iOS is excluded here. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1695: 'target_name': 'dnssec_chain_verify', Nit: I can't figure out the order in which you listed these executables. They aren't listed in the original order, and they aren't listed in alphabetical order, either. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1840: ['inside_chromium_build==1', { Where did this section come from? I found a section for net_unittests_run at the end of this file (lines 2081-2116). Should that be removed?
All requested changes. As this CL is already closed, I've uploaded the result as a separate CL: https://chromiumcodereview.appspot.com/10832082 https://chromiumcodereview.appspot.com/10834051/diff/1006/net/base/platform_m... File net/base/platform_mime_util_mac.mm (right): https://chromiumcodereview.appspot.com/10834051/diff/1006/net/base/platform_m... net/base/platform_mime_util_mac.mm:11: #endif // defined(OS_IOS) On 2012/07/30 10:52:12, stuartmorgan wrote: > Move this whole block to after all other headers. Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode812 net/net.gyp:812: # above as more is brought up for iOS. On 2012/07/30 10:52:12, stuartmorgan wrote: > Since there's just one thing, change everything after the ";" to "move this back > to the main dependencies section once crypto builds for iOS" Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1074: ['include', '^base/dns_util\\..*$'], On 2012/07/30 10:52:12, stuartmorgan wrote: > .*$ is silly; just omit it. Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1098: # are needed in specific cases on other platforms. To explicitely On 2012/07/30 10:52:12, stuartmorgan wrote: > s/To explicitely re-include those files/Re-including them/ Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1100: # evaluate after the platform rules. On 2012/07/30 10:52:12, stuartmorgan wrote: > s/evaluate/is evaluated/ Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1365: # TODO(ios): This is temporary; Move dependencies back to the main On 2012/07/30 10:52:12, stuartmorgan wrote: > s/dependencies // Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1366: # dependencies section as more is brought up for iOS. On 2012/07/30 10:52:12, stuartmorgan wrote: > s/more/crypto/ Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1482: # TODO: For now this only test the subset of code that was enabled in On 2012/07/30 10:52:12, stuartmorgan wrote: > s/TODO/TODO(ios)/ > s/test/tests/ > s/was/is/ Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1535: # TODO:(ios): Enable those tests once the code to exercise is On 2012/07/30 10:52:12, stuartmorgan wrote: > s/those/these/ Done. https://chromiumcodereview.appspot.com/10834051/diff/1006/net/net.gyp#newcode... net/net.gyp:1693: # iOS doesn't have the concept of simple executables, those targets On 2012/07/30 10:52:12, stuartmorgan wrote: > s/those/so these/ Done. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp File net/net.gyp (right): https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1059: [ 'OS == "ios"', { On 2012/07/30 19:33:15, wtc wrote: > > Nit: it would be nice to list this ios section right below > the mac section (lines 1017-1032) because they are closely > related. Done. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1480: ['OS == "ios"', { On 2012/07/30 19:33:15, wtc wrote: > > Nit: same here: I suggest listing the ios section right below > the mac section (lines 1442-1449). Done. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1612: ['inside_chromium_build==1 and OS != "ios"', { On 2012/07/30 19:33:15, wtc wrote: > > This change is probably worth a comment. I can't figure out > why iOS is excluded here. To be honest, I don't know. This was added in the ios tree and I parroted it here. I'm guessing that as we don't compile nor use a test server on ios there is no point for those dependencies anyway? But that's just an educated guess. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1695: 'target_name': 'dnssec_chain_verify', On 2012/07/30 19:33:15, wtc wrote: > > Nit: I can't figure out the order in which you listed these > executables. They aren't listed in the original order, > and they aren't listed in alphabetical order, either. They are listed in the order they failed while running the build over and over. Reordering alphabetically. https://chromiumcodereview.appspot.com/10834051/diff/10005/net/net.gyp#newcod... net/net.gyp:1840: ['inside_chromium_build==1', { On 2012/07/30 19:33:15, wtc wrote: > > Where did this section come from? > > I found a section for net_unittests_run at the end of this > file (lines 2081-2116). Should that be removed? Good catch. This was already removed by https://chromiumcodereview.appspot.com/10829089 |
