Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10916112:
Forwarder 2 implementation (Closed)
|
Created:
8 years, 3 months ago by felipeg Modified:
8 years, 3 months ago CC:
chromium-reviews, bulach+watch_chromium.org, tfarina Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionForwarder 2 implementation
This is the first CL for implementing the Forwarder2, necessary to android testing.
The big picture CL can be seem here: https://chromiumcodereview.appspot.com/10918057/
BUG=146502
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155446
Patch Set 1 #
Total comments: 29
Patch Set 2 : #
Total comments: 25
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 20 (0 generated)
I've made a first pass through it but not a domain expert. You should probably add someone from the net/ OWNERs list to review it in detail https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/common/a... File tools/android/common/adb_connection.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/common/a... tools/android/common/adb_connection.cc:9: #include <stdio.h> why is this required? https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:21: #define PRESERVE_ERRNO_HANDLE_EINTR(Func) \ is preserving the errno important? i.e. are we writing code that will check the errno value upon return instead of what the functions here return? https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:111: if (abstract + path.size() + 1 /* '\0' */ > kPathMax) { I don't understand the reason to add the boolean with length of path with null terminator. Did you mean to write 'if (abstract && ....)'? If not, can you explain the reasoning? https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:150: << "Error initializing socket."; the message should be about the failure, so could change to 'Invalid address format family' or something similar https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:154: addr_len_ = sizeof(sockaddr_in); would be better to use sizeof(addr_.addr4) since thats the pointer you are taking in the previous line. Similarly for the case in the else block https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:13: #include <string> move up to line 10 https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:21: class Socket { add a comment explaining what this class does https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:31: void Shutdown(); please add comments explaining what these do, and what the difference between close and shutdown is https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:44: // Same as Read(), just a wrapper around write(). leave an empty line above full length comments like these for better readability https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:73: void unset_notifier_fd() { exit_notifier_fd_ = -1; } unset -> reset https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:117: int exit_notifier_fd_; comment please
https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:111: if (abstract + path.size() + 1 /* '\0' */ > kPathMax) { On 2012/09/06 15:53:12, Satish wrote: > I don't understand the reason to add the boolean with length of path with null > terminator. Did you mean to write 'if (abstract && ....)'? If not, can you > explain the reasoning? It's not obvious indeed. When using the abstract namespace Linux extension, the path written to the |sun_path| field below must be prefixed by a zero. See the comment below starting from line 121 for more details.
https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/common/a... File tools/android/common/adb_connection.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/common/a... tools/android/common/adb_connection.cc:9: #include <stdio.h> On 2012/09/06 15:53:12, Satish wrote: > why is this required? Compilation complains about the usage of snprintf. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:21: #define PRESERVE_ERRNO_HANDLE_EINTR(Func) \ On 2012/09/06 15:53:12, Satish wrote: > is preserving the errno important? i.e. are we writing code that will check the > errno value upon return instead of what the functions here return? I currently don't, but Digit told me that it would be a good idea to preserve the errno in the case of the Close method since when I am closing the socket it may mean that some previously important error has occurred, and the call to close could change errno and mask the real cause of problem. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:111: if (abstract + path.size() + 1 /* '\0' */ > kPathMax) { On 2012/09/06 15:53:12, Satish wrote: > I don't understand the reason to add the boolean with length of path with null > terminator. Did you mean to write 'if (abstract && ....)'? If not, can you > explain the reasoning? FYI, this check also came from net/base/unix_domain_socket_posix.cc Changed to be easier to read. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:150: << "Error initializing socket."; On 2012/09/06 15:53:12, Satish wrote: > the message should be about the failure, so could change to 'Invalid address > format family' or something similar Done. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:154: addr_len_ = sizeof(sockaddr_in); On 2012/09/06 15:53:12, Satish wrote: > would be better to use sizeof(addr_.addr4) since thats the pointer you are > taking in the previous line. Similarly for the case in the else block Done. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:13: #include <string> On 2012/09/06 15:53:12, Satish wrote: > move up to line 10 But linter tool says C++ headers should come in a separate section. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:21: class Socket { On 2012/09/06 15:53:12, Satish wrote: > add a comment explaining what this class does Done. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:31: void Shutdown(); On 2012/09/06 15:53:12, Satish wrote: > please add comments explaining what these do, and what the difference between > close and shutdown is This is a wrapper around unix sockets. The explanation and the difference are in the manpages. I guess we can just let the reader to look at the manpages for explanations if needed. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:44: // Same as Read(), just a wrapper around write(). On 2012/09/06 15:53:12, Satish wrote: > leave an empty line above full length comments like these for better readability Done. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:73: void unset_notifier_fd() { exit_notifier_fd_ = -1; } On 2012/09/06 15:53:12, Satish wrote: > unset -> reset Done. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.h:117: int exit_notifier_fd_; On 2012/09/06 15:53:12, Satish wrote: > comment please Done.
just some tiny nits below... if digit is happy, I'm happy too! :) http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... File tools/android/forwarder2/socket.h (right): http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:17: using std::string; nit: this is uncommon, we normally just have "std::string" everywhere.. http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:19: namespace forwarder { nit: I guess it'd be clearer as forwarder2 http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:66: return WriteNumBytes(buffer.c_str(), buffer.size()); nit: shouldn't be inlined, move to the .cc file.. http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:69: static int GetHighestFileDescriptor(const Socket& s1, const Socket& s2); nit: move this one to 80 http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:71: bool HasError() const { return socket_error_; } nit: "has_error", otherwise it can't be inlined..
https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:21: #define PRESERVE_ERRNO_HANDLE_EINTR(Func) \ On 2012/09/06 16:19:06, felipeg1 wrote: > On 2012/09/06 15:53:12, Satish wrote: > > is preserving the errno important? i.e. are we writing code that will check > the > > errno value upon return instead of what the functions here return? > > I currently don't, but Digit told me that it would be a good idea to preserve > the errno in the case of the Close method since when I am closing the socket it > may mean that some previously important error has occurred, and the call to > close could change errno and mask the real cause of problem. I'd prefer we don't add code expecting in future some code may need errno to be preserved. Lets cross that bridge when we come to it and for now just add the stuff necessary for forwarder2. If this functionality isn't used in forwarder2 I'd suggest removing it.
http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... File tools/android/forwarder2/socket.h (right): http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:17: using std::string; On 2012/09/07 08:30:28, bulach wrote: > nit: this is uncommon, we normally just have "std::string" everywhere.. specially in a HEADER file! ;) http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:107: typedef union { Also move it to the beginning of private section. before the function declarations. http://codereview.chromium.org/10916112/diff/3001/tools/android/forwarder2/so... tools/android/forwarder2/socket.h:107: typedef union { does this needs to be typedefed? this is c++ land, not C.
https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:21: #define PRESERVE_ERRNO_HANDLE_EINTR(Func) \ On 2012/09/07 09:11:38, Satish wrote: > On 2012/09/06 16:19:06, felipeg1 wrote: > > On 2012/09/06 15:53:12, Satish wrote: > > > is preserving the errno important? i.e. are we writing code that will check > > the > > > errno value upon return instead of what the functions here return? > > > > I currently don't, but Digit told me that it would be a good idea to preserve > > the errno in the case of the Close method since when I am closing the socket > it > > may mean that some previously important error has occurred, and the call to > > close could change errno and mask the real cause of problem. > > I'd prefer we don't add code expecting in future some code may need errno to be > preserved. Lets cross that bridge when we come to it and for now just add the > stuff necessary for forwarder2. If this functionality isn't used in forwarder2 > I'd suggest removing it. Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:17: using std::string; On 2012/09/07 08:30:28, bulach wrote: > nit: this is uncommon, we normally just have "std::string" everywhere.. Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:17: using std::string; On 2012/09/07 11:39:39, tfarina wrote: > On 2012/09/07 08:30:28, bulach wrote: > > nit: this is uncommon, we normally just have "std::string" everywhere.. > specially in a HEADER file! ;) Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:19: namespace forwarder { On 2012/09/07 08:30:28, bulach wrote: > nit: I guess it'd be clearer as forwarder2 Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:66: return WriteNumBytes(buffer.c_str(), buffer.size()); On 2012/09/07 08:30:28, bulach wrote: > nit: shouldn't be inlined, move to the .cc file.. Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:69: static int GetHighestFileDescriptor(const Socket& s1, const Socket& s2); On 2012/09/07 08:30:28, bulach wrote: > nit: move this one to 80 Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:71: bool HasError() const { return socket_error_; } On 2012/09/07 08:30:28, bulach wrote: > nit: "has_error", otherwise it can't be inlined.. Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:107: typedef union { On 2012/09/07 11:39:39, tfarina wrote: > Also move it to the beginning of private section. before the function > declarations. Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:107: typedef union { On 2012/09/07 11:39:39, tfarina wrote: > does this needs to be typedefed? this is c++ land, not C. Done.
https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:21: #define PRESERVE_ERRNO_HANDLE_EINTR(Func) \ Preserving errno for Close() is important because the function is very often used in cleanup code, after an error occurred, and it is very easy to pass an invalid file descriptor to close() in this context, or more rarely, a spurious signal might make close() return -1 + setting errno to EINTR, masking the real reason for the original error. This leads to very unpleasant debugging sessions :-) I believe that all subtle things in the code should be well documented, so maybe add a comment on top of this declaration to explain that. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:111: if (abstract + path.size() + 1 /* '\0' */ > kPathMax) { I'd suggest adding a comment here explaining this subtle detail here. Doesn't have to be long, e.g. "For abstract sockets we need one extra byte for the leading zero" https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.cc:205: hints.ai_family = PF_UNSPEC; nit: should be AF_UNSPEC since this is an address family, not a protocol family (yeah, both macros resolve to 0 otherwise). https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:71: bool HasError() const { return socket_error_; } On 2012/09/07 08:30:28, bulach wrote: > nit: "has_error", otherwise it can't be inlined.. The style guide says that lower_case() method should match the internal member field, which isn't the case here. Also, why can't a simple function be inlined even if it uses CamelCase()? I'm not aware of such rule. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:76: void set_notifier_fd(int notifier_fd) { exit_notifier_fd_ = notifier_fd; } nit: this doesn't match the internal field name. https://chromiumcodereview.appspot.com/10916112/diff/3009/tools/android/forwa... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/3009/tools/android/forwa... tools/android/forwarder2/socket.h:106: // Family of the socket (IF_INET, IF_INET6 or PF_UNIX). Nit: IF_INET/IF_INET6 do not exist, you probably mean PF_INET/PF_INET6 which are the values expected by the socket() call, even though most code uses the equivalent AF_INET/AF_INET6 values. In theory, PF_XX is for socket/protocols, and AF_XX is for address families, it's all confusing isn't it :-)
forgot to say lgtm :)
https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:71: bool HasError() const { return socket_error_; } On 2012/09/07 08:30:28, bulach wrote: > nit: "has_error", otherwise it can't be inlined.. Like digit pointed out. I think it's fine as is. This is a trivial getter accessor. I don't think renaming to socket_error() will help much. HasError() seems to be the best option here.
https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:21: #define PRESERVE_ERRNO_HANDLE_EINTR(Func) \ On 2012/09/07 13:47:12, digit wrote: > Preserving errno for Close() is important because the function is very often > used in cleanup code, after an error occurred, and it is very easy to pass an > invalid file descriptor to close() in this context, or more rarely, a spurious > signal might make close() return -1 + setting errno to EINTR, masking the real > reason for the original error. This leads to very unpleasant debugging sessions > :-) > > I believe that all subtle things in the code should be well documented, so maybe > add a comment on top of this declaration to explain that. Done. https://chromiumcodereview.appspot.com/10916112/diff/1/tools/android/forwarde... tools/android/forwarder2/socket.cc:111: if (abstract + path.size() + 1 /* '\0' */ > kPathMax) { On 2012/09/07 13:47:12, digit wrote: > I'd suggest adding a comment here explaining this subtle detail here. Doesn't > have to be long, e.g. "For abstract sockets we need one extra byte for the > leading zero" Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... File tools/android/forwarder2/socket.cc (right): https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.cc:205: hints.ai_family = PF_UNSPEC; On 2012/09/07 13:47:12, digit wrote: > nit: should be AF_UNSPEC since this is an address family, not a protocol family > (yeah, both macros resolve to 0 otherwise). Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:71: bool HasError() const { return socket_error_; } On 2012/09/07 13:50:46, tfarina wrote: > On 2012/09/07 08:30:28, bulach wrote: > > nit: "has_error", otherwise it can't be inlined.. > Like digit pointed out. I think it's fine as is. This is a trivial getter > accessor. I don't think renaming to socket_error() will help much. HasError() > seems to be the best option here. Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:71: bool HasError() const { return socket_error_; } On 2012/09/07 13:47:12, digit wrote: > On 2012/09/07 08:30:28, bulach wrote: > > nit: "has_error", otherwise it can't be inlined.. > > The style guide says that lower_case() method should match the internal member > field, which isn't the case here. Also, why can't a simple function be inlined > even if it uses CamelCase()? I'm not aware of such rule. Done. https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:76: void set_notifier_fd(int notifier_fd) { exit_notifier_fd_ = notifier_fd; } On 2012/09/07 13:47:12, digit wrote: > nit: this doesn't match the internal field name. Done. https://chromiumcodereview.appspot.com/10916112/diff/3009/tools/android/forwa... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/3009/tools/android/forwa... tools/android/forwarder2/socket.h:106: // Family of the socket (IF_INET, IF_INET6 or PF_UNIX). On 2012/09/07 13:47:12, digit wrote: > Nit: IF_INET/IF_INET6 do not exist, you probably mean PF_INET/PF_INET6 which are > the values expected by the socket() call, even though most code uses the > equivalent AF_INET/AF_INET6 values. > > In theory, PF_XX is for socket/protocols, and AF_XX is for address families, > it's all confusing isn't it :-) Done.
just clarifying my earlier comment :) https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... File tools/android/forwarder2/socket.h (right): https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwa... tools/android/forwarder2/socket.h:71: bool HasError() const { return socket_error_; } On 2012/09/07 13:56:57, felipeg1 wrote: > On 2012/09/07 13:47:12, digit wrote: > > On 2012/09/07 08:30:28, bulach wrote: > > > nit: "has_error", otherwise it can't be inlined.. > > > > The style guide says that lower_case() method should match the internal member > > field, which isn't the case here. Also, why can't a simple function be inlined > > even if it uses CamelCase()? I'm not aware of such rule. > > Done. it's a recommendation from: http://dev.chromium.org/developers/coding-style Google style strongly encourages (but does not mandate) unix_hacker style on member functions that do so little work they're effectively free, such as simple, non-virtual getters and setters. This is also the team position. Some code uses CamelCase for everything; avoid writing patches just to convert these functions to unix_hacker style, but try to use it in new code you write unless there are encapsulation reasons why CamelCase would be much more appropriate.
On Fri, Sep 7, 2012 at 4:03 PM, <bulach@chromium.org> wrote: > just clarifying my earlier comment :) > > > > https://chromiumcodereview.**appspot.com/10916112/diff/** > 3001/tools/android/forwarder2/**socket.h<https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwarder2/socket.h> > File tools/android/forwarder2/**socket.h (right): > > https://chromiumcodereview.**appspot.com/10916112/diff/** > 3001/tools/android/forwarder2/**socket.h#newcode71<https://chromiumcodereview.appspot.com/10916112/diff/3001/tools/android/forwarder2/socket.h#newcode71> > tools/android/forwarder2/**socket.h:71: bool HasError() const { return > socket_error_; } > On 2012/09/07 13:56:57, felipeg1 wrote: > >> On 2012/09/07 13:47:12, digit wrote: >> > On 2012/09/07 08:30:28, bulach wrote: >> > > nit: "has_error", otherwise it can't be inlined.. >> > >> > The style guide says that lower_case() method should match the >> > internal member > >> > field, which isn't the case here. Also, why can't a simple function >> > be inlined > >> > even if it uses CamelCase()? I'm not aware of such rule. >> > > Done. >> > > it's a recommendation from: > http://dev.chromium.org/**developers/coding-style<http://dev.chromium.org/dev... > Google style strongly encourages (but does not mandate) unix_hacker > style on member functions that do so little work they're effectively > free, such as simple, non-virtual getters and setters. This is also the > team position. Some code uses CamelCase for everything; avoid writing > patches just to convert these functions to unix_hacker style, but try to > use it in new code you write unless there are encapsulation reasons why > CamelCase would be much more appropriate. > Done again > > https://chromiumcodereview.**appspot.com/10916112/<https://chromiumcodereview... >
lgtm, thanks a lot everyone!! one suggestion below: https://chromiumcodereview.appspot.com/10916112/diff/7002/tools/android/forwa... File tools/android/forwarder2/forwarder.gyp (right): https://chromiumcodereview.appspot.com/10916112/diff/7002/tools/android/forwa... tools/android/forwarder2/forwarder.gyp:16: 'target_name': 'device_forwarder', nit: would this work, similar to 46? 'toolsets': ['target'],
https://chromiumcodereview.appspot.com/10916112/diff/7002/tools/android/forwa... File tools/android/forwarder2/forwarder.gyp (right): https://chromiumcodereview.appspot.com/10916112/diff/7002/tools/android/forwa... tools/android/forwarder2/forwarder.gyp:16: 'target_name': 'device_forwarder', On 2012/09/07 14:31:32, bulach wrote: > nit: would this work, similar to 46? > > 'toolsets': ['target'], Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/10916112/8010
Try job failure for 10916112-8010 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... 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/felipeg@chromium.org/10916112/8010
Change committed as 155446 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
