Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2882393002:
Refactored adopting a socket for devtools into cleaner, more idiomatic code. (Closed)
DescriptionRefactored adopting a socket for devtools into cleaner, more idiomatic code.
McGreevy because he suggested it. Sami for Owners.
BUG=624837
Review-Url: https://codereview.chromium.org/2882393002
Cr-Commit-Position: refs/heads/master@{#474157}
Committed: https://chromium.googlesource.com/chromium/src/+/43017673005af77aba9dc160a4c1ee4ee47076ff
Patch Set 1 #Patch Set 2 : Fixed compile on Windows #Patch Set 3 : Fixed another win-clang compile error. #
Total comments: 1
Patch Set 4 : Added comment per reviewer request #
Total comments: 15
Patch Set 5 : Updated per reviewer comments #
Total comments: 2
Patch Set 6 : Formatting per reviewer comment #Messages
Total messages: 32 (23 generated)
Description was changed from ========== Refactored adopting a socket for devtools into cleaner, more idiomatic code. BUG= ========== to ========== Refactored adopting a socket for devtools into cleaner, more idiomatic code. McGreevy because he suggested it. Sami for Owners. BUG=624837 ==========
rvera@chromium.org changed reviewers: + mcgreevy@chromium.org, skyostil@chromium.org
rvera@chromium.org changed required reviewers: + skyostil@chromium.org
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This is a minor refactoring along lines suggested by mcgreevy@. There is no functional change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks for the cleanup! https://codereview.chromium.org/2882393002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_devtools.cc (right): https://codereview.chromium.org/2882393002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:82: #endif nit: Please add // defined(OS_POSIX) to make it easier to see which block this belongs to.
On 2017/05/16 at 12:46:52, skyostil wrote: > lgtm, thanks for the cleanup! > > https://codereview.chromium.org/2882393002/diff/40001/headless/lib/browser/he... > File headless/lib/browser/headless_devtools.cc (right): > > https://codereview.chromium.org/2882393002/diff/40001/headless/lib/browser/he... > headless/lib/browser/headless_devtools.cc:82: #endif > nit: Please add // defined(OS_POSIX) to make it easier to see which block this belongs to. Done. Thanks!
https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... File headless/lib/browser/headless_devtools.cc (right): https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:30: std::unique_ptr<net::ServerSocket> CreateForTethering( I'm not a fan of implementation inheritance, and I don't think there's really a need for it here (having said that, I'm not familiar with the codebase conventions). If you just duplicate this method in TCPEndpointServerSocketFactory and TCPAdoptServerSocketFactory, then you can remove this whole type. You would end up with a pure interface (DevToolsSocketFactory) and two implementations (in this file, at least) of that interface, rather than an inheritance hierarchy with implementation sprinkled through it. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:40: DCHECK(endpoint_.address().IsValid()); It's odd that CreateForHttpServer and CreateForTethering were (and still are) private when they are declared as public in DevToolsSocketFactory. I suppose this *works* because once one of these factories is created, it's passed around as a DevToolsSocketFactory (where these members are public), but it feels a bit odd that you wouldn't be able to call these methods on the derived types. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:44: // content::DevToolsSocketFactory implementation: These comments seem unnecessary. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:58: #if defined(OS_POSIX) Does this really need to be conditionally compiled? I don't know what the typical style is in this codebase, but AFAICT this will slightly reduce the binary size at the cost of readability. I don't think it's needed for correctness, as the conditional compilation on line 91 should take care of that. And I don't see anything here that would prevent this code from actually compiling on other platforms. But maybe I'm missing something. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:73: // Note that we assume that the socket is already listening, so we don't You should probably document that assumption on the TCPAdoptServerSocketFactory constructor. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:95: LOG(ERROR) << "Can't inherit an open socket on non-Posix systems"; In this case, socket_factory will be a null pointer, which you then pass into StartRemoteDebuggingServer. I don't think you want to do this. I'd suggest creating another class which derives from content::DevToolsSocketFactory and just returns nullptr from its CreateForHttpServer method, and instantiate one of those here.
PTAL. In the end I accepted most of your comments. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... File headless/lib/browser/headless_devtools.cc (right): https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:30: std::unique_ptr<net::ServerSocket> CreateForTethering( On 2017/05/23 at 04:31:07, mcgreevy wrote: > I'm not a fan of implementation inheritance, and I don't think there's really a need for it here (having said that, I'm not familiar with the codebase conventions). If you just duplicate this method in TCPEndpointServerSocketFactory and TCPAdoptServerSocketFactory, then you can remove this whole type. You would end up with a pure interface (DevToolsSocketFactory) and two implementations (in this file, at least) of that interface, rather than an inheritance hierarchy with implementation sprinkled through it. What you suggest is what I did first. I added the inheritance to avoid duplication. What's wrong with implementation inheritance if all the subclasses do the exact same thing for that method? Particularly when the whole hierarchy is right here in the one file. You would really prefer duplication? Admittedly the method here is small, so I'll do it, but still. Done. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:40: DCHECK(endpoint_.address().IsValid()); On 2017/05/23 at 04:31:07, mcgreevy wrote: > It's odd that CreateForHttpServer and CreateForTethering were (and still are) private when they are declared as public in DevToolsSocketFactory. I suppose this *works* because once one of these factories is created, it's passed around as a DevToolsSocketFactory (where these members are public), but it feels a bit odd that you wouldn't be able to call these methods on the derived types. There was a long thread about this on chromium-dev a few weeks ago, becasue I gather it's pretty common in the codebase. It's an odd pattern, but it makes some sense when the subclasses should never be called directly. Note that they are also declared in an unnamed namespace so they can't be accessed outside this file. Defence in depth I guess. All of this was already here, and I don't want to change the relationship of this code to the rest of devtools if I don't need to. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:44: // content::DevToolsSocketFactory implementation: On 2017/05/23 at 04:31:07, mcgreevy wrote: > These comments seem unnecessary. They were there before. I'll remove them. Done. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:58: #if defined(OS_POSIX) On 2017/05/23 at 04:31:07, mcgreevy wrote: > Does this really need to be conditionally compiled? I don't know what the typical style is in this codebase, but AFAICT this will slightly reduce the binary size at the cost of readability. I don't think it's needed for correctness, as the conditional compilation on line 91 should take care of that. And I don't see anything here that would prevent this code from actually compiling on other platforms. But maybe I'm missing something. It won't compile as is on Windows, where a socket is a SOCKET instead of a size_t, and I can't pass one in on the command line nor call AdoptSocket with a size_t. In /net SocketSescriptor is defined conditionally to make the network code cross platform (compile-time polymorphism ftw :-), so I would have to include that and rewrite this stuff to match, even though it won't ever get called. I'm not sure it would be any more readable, either. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:73: // Note that we assume that the socket is already listening, so we don't On 2017/05/23 at 04:31:07, mcgreevy wrote: > You should probably document that assumption on the TCPAdoptServerSocketFactory constructor. Done. And clarified this comment, too. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:95: LOG(ERROR) << "Can't inherit an open socket on non-Posix systems"; On 2017/05/23 at 04:31:07, mcgreevy wrote: > In this case, socket_factory will be a null pointer, which you then pass into StartRemoteDebuggingServer. I don't think you want to do this. I'd suggest creating another class which derives from content::DevToolsSocketFactory and just returns nullptr from its CreateForHttpServer method, and instantiate one of those here. Good point. Done.
The CQ bit was checked by rvera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, with one formatting nit. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... File headless/lib/browser/headless_devtools.cc (right): https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:30: std::unique_ptr<net::ServerSocket> CreateForTethering( > What's wrong with implementation inheritance if all the subclasses > do the exact same thing for that method? In general I find it harder to reason about code where logic is spread through the inheritance hierarchy (this is especially the case where one method calls another method from a different level in the hierarchy, sometimes even bouncing up and down the hierarchy. This is not the case here, obviously). I think that the extra boilerplate (whether it's duplication of straightforward logic, such as here, or a small method which makes a call to some code that you've factored out into another class or function) is worth it for the simpler structure. Concretely, if you have a DevToolsSocketFactory that is of dynamic type TCPEndpointServerSocketFactory, you can find all of its implementation in the TCPEndpointServerSocketFactory class. If CreateForTethering were to require real logic, you could factor that out and call it explicitly from each of the leaf classes. A future reader could then still start understanding the implementation of TCPEndpointServerSocketFactory by looking at its code, and then following the explicit call to the factored out implementation, rather than needing to figure out which override in the hierarchy provides the implementation. Obviously the code in this CL is a simple case, where it wouldn't really hurt to go either way, but it think it's better to start with a flatter structure, because code tends to have a tendency to grow over time and no longer be so simple. > Particularly when the whole hierarchy is right here in the one file. The fact that they are all in the same file does help mitigate my concern, and the total amount of code here is small, so it relatively easy to understand. But if this code grew, the extra inheritance would IMO be a drag on readability and simplicity. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:40: DCHECK(endpoint_.address().IsValid()); On 2017/05/24 00:22:30, Raul Vera wrote: > On 2017/05/23 at 04:31:07, mcgreevy wrote: > > It's odd that CreateForHttpServer and CreateForTethering were (and still are) > private when they are declared as public in DevToolsSocketFactory. I suppose > this *works* because once one of these factories is created, it's passed around > as a DevToolsSocketFactory (where these members are public), but it feels a bit > odd that you wouldn't be able to call these methods on the derived types. > > There was a long thread about this on chromium-dev a few weeks ago, becasue I > gather it's pretty common in the codebase. It's an odd pattern, but it makes > some sense when the subclasses should never be called directly. Note that they > are also declared in an unnamed namespace so they can't be accessed outside this > file. Defence in depth I guess. All of this was already here, and I don't want > to change the relationship of this code to the rest of devtools if I don't need > to. Fair enough. https://codereview.chromium.org/2882393002/diff/60001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:58: #if defined(OS_POSIX) On 2017/05/24 00:22:30, Raul Vera wrote: > On 2017/05/23 at 04:31:07, mcgreevy wrote: > > Does this really need to be conditionally compiled? I don't know what the > typical style is in this codebase, but AFAICT this will slightly reduce the > binary size at the cost of readability. I don't think it's needed for > correctness, as the conditional compilation on line 91 should take care of that. > And I don't see anything here that would prevent this code from actually > compiling on other platforms. But maybe I'm missing something. > > It won't compile as is on Windows, where a socket is a SOCKET instead of a > size_t, and I can't pass one in on the command line nor call AdoptSocket with a > size_t. In /net SocketSescriptor is defined conditionally to make the network > code cross platform (compile-time polymorphism ftw :-), so I would have to > include that and rewrite this stuff to match, even though it won't ever get > called. I'm not sure it would be any more readable, either. OK. https://codereview.chromium.org/2882393002/diff/80001/headless/lib/browser/he... File headless/lib/browser/headless_devtools.cc (right): https://codereview.chromium.org/2882393002/diff/80001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:84: // Placeholder class to use when a socket_fd is passed in on non-Posix. Can you prepend a blank line here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rvera@chromium.org
Thanks https://codereview.chromium.org/2882393002/diff/80001/headless/lib/browser/he... File headless/lib/browser/headless_devtools.cc (right): https://codereview.chromium.org/2882393002/diff/80001/headless/lib/browser/he... headless/lib/browser/headless_devtools.cc:84: // Placeholder class to use when a socket_fd is passed in on non-Posix. On 2017/05/24 at 01:23:19, mcgreevy wrote: > Can you prepend a blank line here? Done.
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, mcgreevy@chromium.org Link to the patchset: https://codereview.chromium.org/2882393002/#ps100001 (title: "Formatting per reviewer comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495594805945770,
"parent_rev": "965b137cf430b48de05d64418cadc40e9ff42c91", "commit_rev":
"43017673005af77aba9dc160a4c1ee4ee47076ff"}
Message was sent while issue was closed.
Description was changed from ========== Refactored adopting a socket for devtools into cleaner, more idiomatic code. McGreevy because he suggested it. Sami for Owners. BUG=624837 ========== to ========== Refactored adopting a socket for devtools into cleaner, more idiomatic code. McGreevy because he suggested it. Sami for Owners. BUG=624837 Review-Url: https://codereview.chromium.org/2882393002 Cr-Commit-Position: refs/heads/master@{#474157} Committed: https://chromium.googlesource.com/chromium/src/+/43017673005af77aba9dc160a4c1... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/43017673005af77aba9dc160a4c1... |
