Disable FTP on iOS

Disable FTP on iOS

FTP is not supported on iOS.
This is not obvious, because net.gyp builds almost nothing on iOS currently, but we plan on keeping the ftp/ directory excluded from iOS builds.

BUG=145459
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154130

[cbentzel, 2012-08-27 19:56:16]

Would it make more sense to add a feature flag such as USE_FTP

On Mon, Aug 27, 2012 at 11:50 AM, <droger@chromium.org> wrote:

> Reviewers: willchan,
>
> Description:
> Disable FTP on iOS
>
> FTP is not supported on iOS.
> This is not obvious, because net.gyp builds almost nothing on iOS
> currently, but
> we plan on keeping the ftp/ directory excluded from iOS builds.
>
> BUG=b/6953028
>
> Please review this at
https://chromiumcodereview.**appspot.com/10880071/<https://chromiumcodereview...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>   M net/net.gyp
>   M net/url_request/url_request_**job_manager.cc
>   M net/url_request/url_request_**test_util.cc
>
>
> Index: net/net.gyp
> diff --git a/net/net.gyp b/net/net.gyp
> index 27632fbd29d29fd9e1d10c643f406a**7fe5008292..**
> 05ce6946a4fcfb2b26cae9794e19d7**f9eb677f75 100644
> --- a/net/net.gyp
> +++ b/net/net.gyp
> @@ -1075,6 +1075,7 @@
>                ['include', '^http/http_util\\.'],
>                ['include', '^http/http_util_icu\\.cc$'],
>                ['include', '^http/http_version\\.h$'],
> +              ['include', '^url_request/url_request_job_**manager\\.'],
>              ],
>            },
>          ],
> Index: net/url_request/url_request_**job_manager.cc
> diff --git a/net/url_request/url_request_**job_manager.cc
> b/net/url_request/url_request_**job_manager.cc
> index 2a68088688b171af9ddbc2a37360e8**127cf8716d..**
> b97a6716f9e3b73cac0cac37524fa8**9609baf43e 100644
> --- a/net/url_request/url_request_**job_manager.cc
> +++ b/net/url_request/url_request_**job_manager.cc
> @@ -37,7 +37,10 @@ static const SchemeToFactory kBuiltinFactories[] = {
>    { "http", URLRequestHttpJob::Factory },
>    { "https", URLRequestHttpJob::Factory },
>    { "file", URLRequestFileJob::Factory },
> +#if !defined(OS_IOS)
> +  // No FTP support on iOS.
>    { "ftp", URLRequestFtpJob::Factory },
> +#endif
>    { "about", URLRequestAboutJob::Factory },
>    { "data", URLRequestDataJob::Factory },
>  };
> Index: net/url_request/url_request_**test_util.cc
> diff --git a/net/url_request/url_request_**test_util.cc
> b/net/url_request/url_request_**test_util.cc
> index 53574f7277fb1ba9ee893e2ac5a614**b788e64771..**
> bb1444a95827f23726bdb0cab19dd1**25c93eb379 100644
> --- a/net/url_request/url_request_**test_util.cc
> +++ b/net/url_request/url_request_**test_util.cc
> @@ -67,8 +67,13 @@ void TestURLRequestContext::Init() {
>    if (!cert_verifier())
>      context_storage_.set_cert_**verifier(net::CertVerifier::**
> CreateDefault());
>    if (!ftp_transaction_factory()) {
> +#if defined(OS_IOS)
> +    // No FTP support on iOS.
> +    context_storage_.set_ftp_**transaction_factory(NULL);
> +#else
>      context_storage_.set_ftp_**transaction_factory(
>          new net::FtpNetworkLayer(host_**resolver()));
> +#endif  // defined(OS_IOS)
>    }
>    if (!ssl_config_service())
>      context_storage_.set_ssl_**config_service(new
> net::SSLConfigServiceDefaults)**;
>
>
>

[cbentzel, 2012-08-27 20:03:51]

See the similar USE_KERBEROS for an example.

On Mon, Aug 27, 2012 at 3:56 PM, Chris Bentzel <cbentzel@chromium.org>wrote:

> Would it make more sense to add a feature flag such as USE_FTP
>
> On Mon, Aug 27, 2012 at 11:50 AM, <droger@chromium.org> wrote:
>
>> Reviewers: willchan,
>>
>> Description:
>> Disable FTP on iOS
>>
>> FTP is not supported on iOS.
>> This is not obvious, because net.gyp builds almost nothing on iOS
>> currently, but
>> we plan on keeping the ftp/ directory excluded from iOS builds.
>>
>> BUG=b/6953028
>>
>> Please review this at
https://chromiumcodereview.**appspot.com/10880071/<https://chromiumcodereview...
>>
>> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>>
>> Affected files:
>>   M net/net.gyp
>>   M net/url_request/url_request_**job_manager.cc
>>   M net/url_request/url_request_**test_util.cc
>>
>>
>> Index: net/net.gyp
>> diff --git a/net/net.gyp b/net/net.gyp
>> index 27632fbd29d29fd9e1d10c643f406a**7fe5008292..**
>> 05ce6946a4fcfb2b26cae9794e19d7**f9eb677f75 100644
>> --- a/net/net.gyp
>> +++ b/net/net.gyp
>> @@ -1075,6 +1075,7 @@
>>                ['include', '^http/http_util\\.'],
>>                ['include', '^http/http_util_icu\\.cc$'],
>>                ['include', '^http/http_version\\.h$'],
>> +              ['include', '^url_request/url_request_job_**manager\\.'],
>>              ],
>>            },
>>          ],
>> Index: net/url_request/url_request_**job_manager.cc
>> diff --git a/net/url_request/url_request_**job_manager.cc
>> b/net/url_request/url_request_**job_manager.cc
>> index 2a68088688b171af9ddbc2a37360e8**127cf8716d..**
>> b97a6716f9e3b73cac0cac37524fa8**9609baf43e 100644
>> --- a/net/url_request/url_request_**job_manager.cc
>> +++ b/net/url_request/url_request_**job_manager.cc
>> @@ -37,7 +37,10 @@ static const SchemeToFactory kBuiltinFactories[] = {
>>    { "http", URLRequestHttpJob::Factory },
>>    { "https", URLRequestHttpJob::Factory },
>>    { "file", URLRequestFileJob::Factory },
>> +#if !defined(OS_IOS)
>> +  // No FTP support on iOS.
>>    { "ftp", URLRequestFtpJob::Factory },
>> +#endif
>>    { "about", URLRequestAboutJob::Factory },
>>    { "data", URLRequestDataJob::Factory },
>>  };
>> Index: net/url_request/url_request_**test_util.cc
>> diff --git a/net/url_request/url_request_**test_util.cc
>> b/net/url_request/url_request_**test_util.cc
>> index 53574f7277fb1ba9ee893e2ac5a614**b788e64771..**
>> bb1444a95827f23726bdb0cab19dd1**25c93eb379 100644
>> --- a/net/url_request/url_request_**test_util.cc
>> +++ b/net/url_request/url_request_**test_util.cc
>> @@ -67,8 +67,13 @@ void TestURLRequestContext::Init() {
>>    if (!cert_verifier())
>>      context_storage_.set_cert_**verifier(net::CertVerifier::**
>> CreateDefault());
>>    if (!ftp_transaction_factory()) {
>> +#if defined(OS_IOS)
>> +    // No FTP support on iOS.
>> +    context_storage_.set_ftp_**transaction_factory(NULL);
>> +#else
>>      context_storage_.set_ftp_**transaction_factory(
>>          new net::FtpNetworkLayer(host_**resolver()));
>> +#endif  // defined(OS_IOS)
>>    }
>>    if (!ssl_config_service())
>>      context_storage_.set_ssl_**config_service(new
>> net::SSLConfigServiceDefaults)**;
>>
>>
>>
>

[willchan no longer on Chromium, 2012-08-27 22:27:55]

+mmenke

I think that's a good suggestion. Also, is it appropriate to use an internal bug
report rather than a public one when upstreaming? I'm not sure the policy y'all
have settled on, but just wanted to point that out.

Also, I'm not entirely sure the URLRequestJobManager change you made will work
anymore. That code's been refactored recently, and lots of stuff has moved into
an FtpProtocolHandler. Let me add mmenke who should remember more clearly. I
suspect if you sync up to ToT, you'll find that the FooProtocolHandler
refactorings may have broken the iOS port since that wasn't upstreamed yet and I
forgot about it during the reviews. Should be trivial to fix though.

[mmenke, 2012-08-27 23:24:08]

On 2012/08/27 22:27:55, willchan wrote:
> +mmenke
> 
> I think that's a good suggestion. Also, is it appropriate to use an internal
bug
> report rather than a public one when upstreaming? I'm not sure the policy
y'all
> have settled on, but just wanted to point that out.
> 
> Also, I'm not entirely sure the URLRequestJobManager change you made will work
> anymore. That code's been refactored recently, and lots of stuff has moved
into
> an FtpProtocolHandler. Let me add mmenke who should remember more clearly. I
> suspect if you sync up to ToT, you'll find that the FooProtocolHandler
> refactorings may have broken the iOS port since that wasn't upstreamed yet and
I
> forgot about it during the reviews. Should be trivial to fix though.

ProfileIODataImpl and the incognito equivalent also has protocol factories that
also need to be if-deffed out.  (Currently, we have both the
URLRequestFtpJob::Factory and ProtocolHandlers, but the former are going away at
some point)

[droger, 2012-08-28 13:30:59]

Thanks for the comments.
I added a feature flag for FTP, and included the ProfileImplIOData bits.
Since the flag is used outside of net, I defined it in common.gypi.

[cbentzel, 2012-08-29 02:18:45]

Could you associate a bug with this? 

The general code looks fine, but Matt + Will should be final judge. I will
abstain from the vote.

[droger, 2012-08-29 10:06:30]

On 2012/08/29 02:18:45, cbentzel wrote:
> Could you associate a bug with this? 

Done

[mmenke, 2012-08-29 15:12:58]

LGTM.  We're still creating FtpAuthCaches when we construct the
URLRequestContext, though eventually that will probably be moved into the
FtpProtocolHandler.

[mmenke, 2012-08-29 15:14:14]

On 2012/08/29 15:12:58, Matt Menke wrote:
> LGTM.  We're still creating FtpAuthCaches when we construct the
> URLRequestContext, though eventually that will probably be moved into the
> FtpProtocolHandler.

Actually, you may want the flag to be disable_ftp_support, so ftp support is
enabled by default.

[droger, 2012-08-29 16:05:57]

> Actually, you may want the flag to be disable_ftp_support, so ftp support is
> enabled by default.

Done.

[mmenke, 2012-08-29 16:11:09]

On 2012/08/29 16:05:57, droger wrote:
> > Actually, you may want the flag to be disable_ftp_support, so ftp support is
> > enabled by default.
> 
> Done.

Oh, and I missed that you're no longer initializing the FTP auth cache,
obviously...Was looking at it in the profile files.  Anyways, still LGTM.

[commit-bot: I haz the power, 2012-08-29 16:22:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/10880071/15002

[commit-bot: I haz the power, 2012-08-29 18:16:08]

Try job failure for 10880071-15002 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-30 07:43:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/10880071/15002

[commit-bot: I haz the power, 2012-08-30 10:39:53]

Failed to apply patch for net/net.gyp:
While running patch -p1 --forward --force;
patching file net/net.gyp
Hunk #1 succeeded at 890 (offset 7 lines).
Hunk #2 succeeded at 1104 (offset 17 lines).
Hunk #3 FAILED at 1454.
1 out of 3 hunks FAILED -- saving rejects to file net/net.gyp.rej

[commit-bot: I haz the power, 2012-08-30 10:57:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/10880071/29001

[commit-bot: I haz the power, 2012-08-30 13:18:16]

Change committed as 154130