Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(140)

Issue 8598010: replace platform conditions with os_posix and os_bsd where applicable (Closed)

Created:
9 years, 1 month ago by Robert Nagy
Modified:
9 years ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, mmenke
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

replace platform conditions with os_posix and os_bsd where applicable this patch also adds a comment that describes why EAI_NODATA is disabled on FreeBSD BUG= TEST= TBR=wtc Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111253

Patch Set 1 #

Patch Set 2 : comment #

Patch Set 3 : EAI_ADDRFAMILY is obsolete on FreeBSD so comment about it #

Patch Set 4 : rebase #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -17 lines) Patch
M base/base.gyp View 1 chunk +1 line, -1 line 0 comments Download
M content/content_browser.gypi View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/content_plugin.gypi View 1 chunk +1 line, -1 line 0 comments Download
M crypto/crypto.gyp View 1 2 3 1 chunk +1 line, -1 line 1 comment Download
M net/base/host_resolver_impl.cc View 1 2 3 1 chunk +3 lines, -1 line 1 comment Download
M net/net.gyp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M ppapi/ppapi_internal.gyp View 1 chunk +1 line, -1 line 0 comments Download
M ppapi/ppapi_tests.gypi View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M printing/printing.gyp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M sdch/sdch.gyp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/expat/expat.gyp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M third_party/iccjpeg/README.chromium View 1 chunk +1 line, -1 line 0 comments Download
M third_party/iccjpeg/iccjpeg.gyp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/libevent/libevent.gyp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/libjingle/libjingle.gyp View 2 chunks +2 lines, -2 lines 0 comments Download
M ui/ui.gyp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 30 (0 generated)
Robert Nagy
review please
9 years, 1 month ago (2011-11-18 11:25:26 UTC) #1
Mark Mentovai
LGTM
9 years, 1 month ago (2011-11-18 14:26:41 UTC) #2
willchan no longer on Chromium
LGTM
9 years, 1 month ago (2011-11-18 14:54:17 UTC) #3
sky
Forgive my simple mind. Doesn't this make things inconsistent in that we'll have OS=="openbsd" and ...
9 years, 1 month ago (2011-11-18 18:34:50 UTC) #4
Mark Mentovai
This is the same sort of arrangement as having the os_posix GYP variable.
9 years, 1 month ago (2011-11-18 19:05:39 UTC) #5
sky
On 2011/11/18 19:05:39, Mark Mentovai wrote: > This is the same sort of arrangement as ...
9 years, 1 month ago (2011-11-18 19:06:56 UTC) #6
Mark Mentovai
sky@chromium.org wrote: > On 2011/11/18 19:05:39, Mark Mentovai wrote: >> >> This is the same ...
9 years, 1 month ago (2011-11-18 19:08:12 UTC) #7
sky
Of course. LGTM
9 years, 1 month ago (2011-11-18 20:11:37 UTC) #8
ruben
Why does this patch remove the FreeBSD ifdef in net/base/host_resolver_impl.cc? Those symbols were obsoleted in ...
9 years, 1 month ago (2011-11-19 00:49:25 UTC) #9
Robert Nagy
On 2011/11/19 00:49:25, ruben wrote: > Why does this patch remove the FreeBSD ifdef in ...
9 years, 1 month ago (2011-11-19 16:03:37 UTC) #10
ruben
On 2011/11/19 16:03:37, Robert Nagy wrote: > yeah you are right, i've fixed that and ...
9 years, 1 month ago (2011-11-19 22:08:36 UTC) #11
Robert Nagy
On 2011/11/19 22:08:36, ruben wrote: > On 2011/11/19 16:03:37, Robert Nagy wrote: > > yeah ...
9 years, 1 month ago (2011-11-19 22:18:47 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8598010/7003
9 years, 1 month ago (2011-11-21 19:06:13 UTC) #13
commit-bot: I haz the power
Presubmit check for 8598010-7003 failed and returned exit status 1. Running presubmit commit checks ...
9 years, 1 month ago (2011-11-21 19:06:22 UTC) #14
Lei Zhang
LGTM for printing/
9 years, 1 month ago (2011-11-21 19:09:22 UTC) #15
Robert Nagy
Can you guys please review the missing files? Thank you
9 years, 1 month ago (2011-11-22 18:51:39 UTC) #16
Mark Mentovai
Who? Please say what you expect from each reviewer. I’ve already given LG.
9 years, 1 month ago (2011-11-22 18:59:47 UTC) #17
Robert Nagy
On 2011/11/22 18:59:47, Mark Mentovai wrote: > Who? > > Please say what you expect ...
9 years, 1 month ago (2011-11-22 19:03:49 UTC) #18
Mark Mentovai
Brett’s not currently doing reviews. You can choose another OWNER for the ppapi portion.
9 years, 1 month ago (2011-11-22 19:08:04 UTC) #19
Robert Nagy
Added piman to request review for ppapi.
9 years, 1 month ago (2011-11-22 19:11:22 UTC) #20
piman
ppapi LGTM
9 years, 1 month ago (2011-11-22 20:06:33 UTC) #21
willchan no longer on Chromium
wtc is on vacation until next week On Tue, Nov 22, 2011 at 12:06 PM, ...
9 years, 1 month ago (2011-11-22 20:14:24 UTC) #22
Robert Nagy
On 2011/11/22 20:14:24, willchan wrote: > wtc is on vacation until next week > > ...
9 years, 1 month ago (2011-11-22 21:04:16 UTC) #23
willchan no longer on Chromium
Check the OWNERS. rsleevi is probably good. On Tue, Nov 22, 2011 at 1:04 PM, ...
9 years, 1 month ago (2011-11-22 21:25:01 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8598010/7003
9 years, 1 month ago (2011-11-22 21:28:36 UTC) #25
commit-bot: I haz the power
Can't apply patch for file third_party/expat/expat.gyp. While running patch -p1 --forward --force; patching file third_party/expat/expat.gyp ...
9 years, 1 month ago (2011-11-22 21:28:46 UTC) #26
Mark Mentovai
Needs an update.
9 years, 1 month ago (2011-11-22 21:29:11 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8598010/20001
9 years, 1 month ago (2011-11-22 21:45:01 UTC) #28
commit-bot: I haz the power
Change committed as 111253
9 years, 1 month ago (2011-11-22 23:41:47 UTC) #29
wtc
9 years ago (2011-11-30 01:08:15 UTC) #30
Patch Set 4 LGTM.  I only reviewed base, crypto, and net.

http://codereview.chromium.org/8598010/diff/20001/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/8598010/diff/20001/crypto/crypto.gyp#newcode55
crypto/crypto.gyp:55: [ 'os_bsd==1', {

Nit: the prevalent style in this file has spaces before and
after ==.  (It's not worth fixing this.  Just for future
reference.)

http://codereview.chromium.org/8598010/diff/20001/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/8598010/diff/20001/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:93: #endif

It seems better to use independent ifdefs rather than nesting
them:

#if !defined(OS_ANDROID) && !defined(OS_FREEBSD)
    // ...
    EAI_ADDRFAMILY,
#endif
#if !defined(OS_FREEBSD)
    // ...
    EAI_NODATA,
#endif

Powered by Google App Engine
This is Rietveld 408576698