Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
385163007:
Fix the build when disable_ftp_support is set to 1 (Closed)
|
Created:
6 years, 5 months ago by zhaoze.zhou Modified:
6 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Paweł Hajdan Jr. Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix the build when disable_ftp_support is set to 1
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283340
Patch Set 1 : Fix the build when disable_ftp_support is set to 1. #
Total comments: 1
Patch Set 2 : New version of fix of disable_ftp_support #
Total comments: 1
Patch Set 3 : fix the code style of the patch #
Total comments: 1
Patch Set 4 : Fix the mistake ifndef #Messages
Total messages: 15 (0 generated)
The CQ bit was checked by zhaoze.zhou@partner.samsung.com
The CQ bit was unchecked by zhaoze.zhou@partner.samsung.com
Please take a look
https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... File content/child/ftp_directory_listing_response_delegate.cc (right): https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... content/child/ftp_directory_listing_response_delegate.cc:88: int rv = net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); Would the following would work instead (I have not tried, but it seems saner/"dry"-er (and no #else case). int rv = -1; #ifndef DISABLE_FTP_SUPPORT rv = net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); #endif if (rv != net::OK) { ....
On 2014/07/12 01:51:54, lgombos wrote: > https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... > File content/child/ftp_directory_listing_response_delegate.cc (right): > > https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... > content/child/ftp_directory_listing_response_delegate.cc:88: int rv = > net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); > Would the following would work instead (I have not tried, but it seems > saner/"dry"-er (and no #else case). > > int rv = -1; > > #ifndef DISABLE_FTP_SUPPORT > rv = net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); > #endif > > if (rv != net::OK) { > .... yes, this could work.
On 2014/07/12 01:51:54, lgombos wrote: > https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... > File content/child/ftp_directory_listing_response_delegate.cc (right): > > https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... > content/child/ftp_directory_listing_response_delegate.cc:88: int rv = > net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); > Would the following would work instead (I have not tried, but it seems > saner/"dry"-er (and no #else case). > > int rv = -1; > > #ifndef DISABLE_FTP_SUPPORT > rv = net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); > #endif > > if (rv != net::OK) { > .... yes, this could work.
On 2014/07/12 01:51:54, lgombos wrote: > https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... > File content/child/ftp_directory_listing_response_delegate.cc (right): > > https://codereview.chromium.org/385163007/diff/20001/content/child/ftp_direct... > content/child/ftp_directory_listing_response_delegate.cc:88: int rv = > net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); > Would the following would work instead (I have not tried, but it seems > saner/"dry"-er (and no #else case). > > int rv = -1; > > #ifndef DISABLE_FTP_SUPPORT > rv = net::ParseFtpDirectoryListing(buffer_, base::Time::Now(), &entries); > #endif > > if (rv != net::OK) { > .... yes, this could work.
https://codereview.chromium.org/385163007/diff/40001/content/child/ftp_direct... File content/child/ftp_directory_listing_response_delegate.cc (left): https://codereview.chromium.org/385163007/diff/40001/content/child/ftp_direct... content/child/ftp_directory_listing_response_delegate.cc:106: You probably should not remove this line.
On 2014/07/14 15:00:57, lgombos wrote: > https://codereview.chromium.org/385163007/diff/40001/content/child/ftp_direct... > File content/child/ftp_directory_listing_response_delegate.cc (left): > > https://codereview.chromium.org/385163007/diff/40001/content/child/ftp_direct... > content/child/ftp_directory_listing_response_delegate.cc:106: > You probably should not remove this line. fix.
https://codereview.chromium.org/385163007/diff/60001/content/child/ftp_direct... File content/child/ftp_directory_listing_response_delegate.cc (right): https://codereview.chromium.org/385163007/diff/60001/content/child/ftp_direct... content/child/ftp_directory_listing_response_delegate.cc:86: #ifdef DISABLE_FTP_SUPPORT Should be #ifndef instead ?
lgtm
The CQ bit was checked by zhaoze.zhou@partner.samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/385163...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 283340 |
