|
|
Created:
8 years ago by Justin Cohen (wrong one) Modified:
7 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, cbentzel+watch_chromium.org, erikwright+watch_chromium.org, sadrul, sail+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix iOS build for XCode 4.6.
The new clang has some bug fixes and is slightly pickier about
unused variables.
BUG=NONE
TBR=mark
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175587
Patch Set 1 #
Total comments: 1
Patch Set 2 : Split new define into multiple lines #
Total comments: 8
Patch Set 3 : nits #
Total comments: 2
Messages
Total messages: 26 (0 generated)
LGTM with nit. The proxy request ifdefs make me a bit sad, but I don't see a cleaner approach offhand; just adding a no-op reference to it doesn't seem any better. https://codereview.chromium.org/11635050/diff/1/chrome/common/mac/objc_zombie.mm File chrome/common/mac/objc_zombie.mm (right): https://codereview.chromium.org/11635050/diff/1/chrome/common/mac/objc_zombie... chrome/common/mac/objc_zombie.mm:39: #if !defined(OS_IOS) || (defined(OS_IOS) && (__clang_major__ > 4 || (__clang_major__ == 4 && __clang_minor__ >= 2))) Can you line-break this with \ ?
(I ignore emails that cc me on reviews without any comments. If you want me to look at anything here, please tell me at what.) On Wed, Jan 2, 2013 at 10:29 AM, <justincohen@google.com> wrote: > https://codereview.chromium.**org/11635050/<https://codereview.chromium.org/1... >
Mark: I need OWNERS approval for message pump mac and objc_zombie. wtc: I need OWNERS approval for connection_tester and proxy_service_factory.
Patch set 2 LGTM. https://codereview.chromium.org/11635050/diff/5001/base/message_pump_mac.mm File base/message_pump_mac.mm (right): https://codereview.chromium.org/11635050/diff/5001/base/message_pump_mac.mm#n... base/message_pump_mac.mm:30: bool not_using_crapp = false; Nit: this global variable should be named with a g_ prefix. Nit: I suggest changing "crapp" to "cr_app". Ideally the comment should point out what "CrApp" refers to, but I know that's probably more than what you planned to do in this CL. https://codereview.chromium.org/11635050/diff/5001/chrome/browser/net/connect... File chrome/browser/net/connection_tester.cc (right): https://codereview.chromium.org/11635050/diff/5001/chrome/browser/net/connect... chrome/browser/net/connection_tester.cc:55: proxy_request_context_(proxy_request_context), Does the compiler warn about an unused class member, but not an unused constructor argument? https://codereview.chromium.org/11635050/diff/5001/chrome/common/mac/objc_zom... File chrome/common/mac/objc_zombie.mm (right): https://codereview.chromium.org/11635050/diff/5001/chrome/common/mac/objc_zom... chrome/common/mac/objc_zombie.mm:38: // everyone is moved to XCode 4.6 b/7882496. Nit: the indentation of this TODO comment should match lines 34-36 to make it clearer that the TODO is referring to lines 34-36 rather than lines 28-32. https://codereview.chromium.org/11635050/diff/5001/chrome/common/mac/objc_zom... chrome/common/mac/objc_zombie.mm:40: (defined(OS_IOS) && \ Nit: defined(OS_IOS) is not necessary. If you prefer having it, that's fine by me.
https://codereview.chromium.org/11635050/diff/5001/base/message_pump_mac.mm File base/message_pump_mac.mm (right): https://codereview.chromium.org/11635050/diff/5001/base/message_pump_mac.mm#n... base/message_pump_mac.mm:30: bool not_using_crapp = false; On 2013/01/04 00:58:36, wtc wrote: > > Nit: this global variable should be named with a g_ prefix. > > Nit: I suggest changing "crapp" to "cr_app". Ideally the > comment should point out what "CrApp" refers to, but I know > that's probably more than what you planned to do in this CL. Done. https://codereview.chromium.org/11635050/diff/5001/chrome/browser/net/connect... File chrome/browser/net/connection_tester.cc (right): https://codereview.chromium.org/11635050/diff/5001/chrome/browser/net/connect... chrome/browser/net/connection_tester.cc:55: proxy_request_context_(proxy_request_context), Correct: chrome/browser/net/connection_tester.cc:289:33: error: private field 'proxy_request_context_' is not used [-Werror,-Wunused-private-field] net::URLRequestContext* const proxy_request_context_; On 2013/01/04 00:58:36, wtc wrote: > > Does the compiler warn about an unused class member, but not > an unused constructor argument? https://codereview.chromium.org/11635050/diff/5001/chrome/common/mac/objc_zom... File chrome/common/mac/objc_zombie.mm (right): https://codereview.chromium.org/11635050/diff/5001/chrome/common/mac/objc_zom... chrome/common/mac/objc_zombie.mm:38: // everyone is moved to XCode 4.6 b/7882496. On 2013/01/04 00:58:36, wtc wrote: > > Nit: the indentation of this TODO comment should match lines > 34-36 to make it clearer that the TODO is referring to lines > 34-36 rather than lines 28-32. Done. https://codereview.chromium.org/11635050/diff/5001/chrome/common/mac/objc_zom... chrome/common/mac/objc_zombie.mm:40: (defined(OS_IOS) && \ On 2013/01/04 00:58:36, wtc wrote: > > Nit: defined(OS_IOS) is not necessary. If you prefer having it, > that's fine by me. Done.
Patch set 3 LGTM. Thanks.
rsesek: I need owners approval for chrome/common/mac/objc_zombie.mm Thanks!
chrome/common/mac LGTM https://chromiumcodereview.appspot.com/11635050/diff/12001/base/message_pump_... File base/message_pump_mac.mm (left): https://chromiumcodereview.appspot.com/11635050/diff/12001/base/message_pump_... base/message_pump_mac.mm:29: bool not_using_crapp = false; I think we deliberately like this as crapp ;). In fact is your base/ change even necessary since this variable is only consulted for in methods that are already ifdefed?
https://chromiumcodereview.appspot.com/11635050/diff/12001/base/message_pump_... File base/message_pump_mac.mm (left): https://chromiumcodereview.appspot.com/11635050/diff/12001/base/message_pump_... base/message_pump_mac.mm:29: bool not_using_crapp = false; crapp: Heh, I do as I'm told! The newer clang complains about the unused variable, that's why this is needed. On 2013/01/08 18:24:55, rsesek wrote: > I think we deliberately like this as crapp ;). > > In fact is your base/ change even necessary since this variable is only > consulted for in methods that are already ifdefed?
Mark, I'm TBRing the change to message_pump_mac so we can get better coverage to look at XCode 4.6 issues and file radars before it's released.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@google.com/11635050/12001
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@google.com/11635050/12001
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@google.com/11635050/12001
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@google.com/11635050/12001
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@google.com/11635050/12001
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/justincohen@google.com/11635050/12001
Message was sent while issue was closed.
Change committed as 175587
Message was sent while issue was closed.
LGTM. It was intentionally crapp.
Message was sent while issue was closed.
On 2013/01/14 17:44:34, Mark Mentovai wrote: > LGTM. It was intentionally crapp. This shows my lack of programmer's humor. |