Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
468613002:
Readability review. (Closed)
|
Created:
6 years, 4 months ago by Wez Modified:
6 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chromoting-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionReadability review.
These CLs implement a simple frame recording and fetch "extension" for the Chromoting protocol, to allow sample sessions to be captured for performance testing.
The three main classes introduced are:
- HostExtensionSessionManager
This pulls logic for handling Chromoting extensions out of
ClientSession instances into a dedicated manager class.
It also introduces hooks through which extensions can
wrap or replace the Chromoting video encoder or capturer.
- VideoFrameRecorder
This allows a VideoEncoder to be wrapped to return a new
encoder that will optionally copy & deliver frames to the
VideoFrameRecorder before supplying them to the real
encoder.
- VideoFrameRecorderHostExtension
This extension uses a VideoFrameRecorder to allow a
connected client to start/stop recording, and to retrieve
the resulting frame data.
Original CLs:
crrev.com/402233003
crrev.com/339073002
crrev.com/372943002
crrev.com/462503002
BUG=260879
Committed: https://crrev.com/ea12516899b23b38bcd006b9b462eaead73f4cb5
Cr-Commit-Position: refs/heads/master@{#292541}
Patch Set 1 #
Total comments: 113
Patch Set 2 : Address review comments. #
Total comments: 21
Patch Set 3 : Address moar review comments #Patch Set 4 : Rebase #Messages
Total messages: 15 (0 generated)
Trimmed to just the new classes & tests.
Thanks for participating in the readability process! Hopefully the below comments are useful. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... File remoting/host/fake_host_extension.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.cc:49: if (extension_->steal_video_capturer_) { Nit: Completely up to you, but I think more Chromium code omits braces on single-line conditional bodies than includes them. (Same comment applies in other files) https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... File remoting/host/fake_host_extension.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:35: // Controls for testing. Nit: If all these are only for testing, and used only by one test fixture, consider whether making them private, with the test fixture as a friend, would prevent them being misused by other classes. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:36: void set_steal_video_capturer(bool steal_video_capturer); Nit: unix_hacker()-named methods should be inlined into the header. (Several places) https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:39: bool has_handled_message(); All of these can probably be const. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:51: bool steal_video_capturer_; Nit: At least this member might benefit from an explanatory comment. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:13: namespace remoting { Nit: At least one blank line above this https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:30: std::string capability = (*extension)->capability(); Nit: Could probably be a const std::string& (tiny efficiency boost in some cases) https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:43: HostExtensionSessionManager::OnCreateVideoCapturer( Nit: Elsewhere in this CL you don't indent the function name when you've wrapped after the return type. Be consistent. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:45: for(HostExtensionSessionList::const_iterator it = extension_sessions_.begin(); Nit: for ( (2 places) https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:36: HostExtensionSessionManager(const std::vector<HostExtension*>& extensions, Nit: Since you already have a typedef for this in the private section below, consider making it public and using it here, so callers can benefit from it. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:41: std::string GetCapabilities(); This can probably be const. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:44: // them an opportunity to wrap or replace video components. It seems like more comments could be useful. For example, what if multiple different extensions want to replace the relevant component -- what order do they go in? https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:45: scoped_ptr<webrtc::DesktopCapturer> OnCreateVideoCapturer( Nit: It might be clearer to have these functions look more like: void OnCreateVideoCapturer(scoped_ptr<webrtc::DesktopCapturer>* capturer); This makes it more obvious that the function is not so much taking ownership of the argument as potentially modifying it. (2 places) https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:51: // the set of |HostExtensionSession| to match the client's capabilities. Nit: |HostExtensionSession|s https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:56: // is handled, or none remain. Returns true if the message was handled. Again, what order do these go in? https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:60: typedef std::vector<HostExtension*> HostExtensionList; Nit: Don't use "List" in a typename, especially if it isn't actually a std::list<>. For example, "HostExtensions" would be a more generic/less misleading name here. (2 places) https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:68: // List of HostExtensions to attach to the session, if it reaches the Nit: Again, avoid "list" except perhaps where you're actually using a list. (2 places) https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager_unittest.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:20: extension2_("ext2", ""), Nit: Prefer std::string() over "" https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:28: protected: Nit: In general, the Google style guide prefers private data members to protected ones. While protected members are allowed when using gtest "for technical reasons", it's usually still preferable to make them private and provide accessors as necessary. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:38: }; Nit: DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:40: // Verifies that messages are passed to be handled by the correct extension. Nit: I'm not sure what this sentence means, but it seems like removing "passed to be" would make it clearer? https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:79: EXPECT_EQ(extension_manager.GetCapabilities(), "cap1 cap3"); Would it be equally valid to return "cap3 cap1"? Should the test account for this? https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:162: // Verifies that if an extension reports that they modify the video pipeline Nit: they modify -> it modifies https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.cc:31: : encoder_(encoder.Pass()), Nit: Avoid defining complex functions inline except in unittest files ( http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-What-ab... ). (Several places) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.cc:44: void SetEnableRecording(bool enable_recording) { Nit: This function could probably be left inlined, named unix_hacker()-style. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:8: #include <stdint.h> Nit: Prefer C++ headers to C ones (<cstdint>) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:31: // 1. Create the VideoFrameRecorder on the controlling thread. Nit: "on the controlling thread" unnecessary (you're in the "on the control thread" section) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:35: // 4. Hand the returned wrapper VideoEncoder of to the video encoding thread, Nit: of -> off ("Hand off the..." might be even better) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:53: // The caller must delete the previous recording VideoEncoder, or call Nit: Perhaps this second sentence would be clearer as (assuming I understand it correctly): This function must not be called when a functioning wrapper already exists; either delete the wrapper VideoEncoder entirely, or detach the wrapper from the underlying encoder by calling DetachVideoEncoderWrapper(), before calling this method to create a new wrapper. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:58: // Detaches the existing VideoEncoder wrapper, stopping it from recording. What about the lifetime of the wrapper? Will it exist but do nothing until WrapVideoEncoder() is called again? Say how/when it is cleaned up. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:65: // When this maximum is reached older frames will be discard to make space Nit: discarded https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder_host_extension.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:24: const char kVideoRecorderCapabilities[] = "videoRecorder"; Nit: Declare variables in the most local scope possible; it looks like these can mostly at least move into the specific functions below referencing them. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:39: virtual ~VideoFrameRecorderHostExtensionSession() {} Nit: Don't inline this https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:51: VideoEncoderVerbatim verbatim_encoder; Nit: Members should have a "_" suffix on their names. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:59: int64_t max_content_bytes) : first_frame_(false) { FYI nit: I personally read the Google style guide as saying that each initializer should get its own line when the whole function prototype + initializer list doesn't fit on one line. Not everyone agrees with me; therefore, you're welcome to format things this way, unless you prefer my rule :) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:88: if (value && value->GetAsDictionary(&client_message)) { Nit: Consider reversing this conditional and early-returning to avoid indenting the whole rest of the function https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:95: if (type == kStartType) { Nit: Could be something like this to avoid indenting the longest conditional arm: if (type != kNextFrameType) { video_frame_recorder.SetEnableRecording(type == kStartType); if (type == kStartType) first_frame_ = true; return true; } ... https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:123: data.resize(encoded_frame->ByteSize()); Nit: Can't the size be passed to the string as a constructor arg? https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:138: LOG(ERROR) << "Failed to create reply json"; Are you going to read these logs from client machines somehow? If not, try to avoid LOG(); instead, either handle the error if it can actually occur, or if this can truly never happen, don't handle it (or DCHECK() that it doesn't occur) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder_host_extension.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.h:18: VideoFrameRecorderHostExtension() {} Nit: In general, avoid defining constructors/destructors inline, even when they seem to be empty; this is a good way to cause a lot of code to be instantiated if the class currently, or in the future, ends up containing non-POD types, so http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... recommends avoiding this entirely (and the Chromium style guide specially calls out virtual methods like the destructor here). https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder_unittest.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:23: (a.dpi().equals(b.dpi()))) { Nit: Reversing this conditional takes no more LOC total but avoids one level of indenting for the remaining code. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:40: const int64_t kMaxContentBytes = 10 * 1024 * 1024; Nit: Again, define these in the most local scopes possible. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:52: void CreateAndWrapEncoder(); Comments? https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:60: protected: Nit: Again, try to avoid protected members if possible. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:66: std::list<webrtc::DesktopFrame*> test_frames_; Nit: Consider a typedef for this so you can use it in loops below https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:68: }; Nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:114: for (int i=0; i < kTestFrameCount; ++i) { Nit: Spaces around '=' Super-tiny nit: Since it's a count of objects in memory, the constant and loop index here should probably be size_t rather than int. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:120: std::list<webrtc::DesktopFrame*>::iterator i; Nit: Declare iterators in the loop declaration unless they're needed outside it https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:122: scoped_ptr<VideoPacket> packet = encoder_->Encode(*(*i)); Nit: "scoped_ptr<VideoPacket> packet = " unnecessary? (2 places) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:161: // VideoFrameRecorder don't end the world. Nit: don't end the world -> doesn't crash (or some other more precise, less colloquial phrase) https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:221: int64 frame_bytes = kWidth * kHeight * webrtc::DesktopFrame::kBytesPerPixel; Optional: const https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:242: // limit allows, the first encoded frames are dropped. This comment is the same as on the previous testcase.
Thanks Peter. Responses inline below. Note that this code is currently missing a unit-test for VideoFrameRecorderHostExtension; let me know if you'd like that adding in this review, otherwise I'll save it for a follow-up CL. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... File remoting/host/fake_host_extension.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.cc:49: if (extension_->steal_video_capturer_) { On 2014/08/22 06:38:27, Peter Kasting wrote: > Nit: Completely up to you, but I think more Chromium code omits braces on > single-line conditional bodies than includes them. (Same comment applies in > other files) Acknowledged. General preference on the Chromoting team is to include the braces, for safety, in new code. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... File remoting/host/fake_host_extension.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:35: // Controls for testing. On 2014/08/22 06:38:27, Peter Kasting wrote: > Nit: If all these are only for testing, and used only by one test fixture, > consider whether making them private, with the test fixture as a friend, would > prevent them being misused by other classes. Acknowledged. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:36: void set_steal_video_capturer(bool steal_video_capturer); On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: unix_hacker()-named methods should be inlined into the header. (Several > places) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:39: bool has_handled_message(); On 2014/08/22 06:38:28, Peter Kasting wrote: > All of these can probably be const. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:51: bool steal_video_capturer_; On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: At least this member might benefit from an explanatory comment. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:30: std::string capability = (*extension)->capability(); On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: Could probably be a const std::string& (tiny efficiency boost in some > cases) Making capability() return const std::string& risks broken code like: const char kCapability[] = "foo"; const std::string& FooExtension::capability() const { return kCapability; } so I'd prefer to have it return std::string instead. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:43: HostExtensionSessionManager::OnCreateVideoCapturer( On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: Elsewhere in this CL you don't indent the function name when you've wrapped > after the return type. Be consistent. Acknowledged; that was an emacs auto-indentationism that slipped through, I think. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:45: for(HostExtensionSessionList::const_iterator it = extension_sessions_.begin(); On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: for ( (2 places) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:36: HostExtensionSessionManager(const std::vector<HostExtension*>& extensions, On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: Since you already have a typedef for this in the private section below, > consider making it public and using it here, so callers can benefit from it. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:41: std::string GetCapabilities(); On 2014/08/22 06:38:28, Peter Kasting wrote: > This can probably be const. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:44: // them an opportunity to wrap or replace video components. On 2014/08/22 06:38:29, Peter Kasting wrote: > It seems like more comments could be useful. For example, what if multiple > different extensions want to replace the relevant component -- what order do > they go in? Clarified that the order is undefined. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:45: scoped_ptr<webrtc::DesktopCapturer> OnCreateVideoCapturer( On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: It might be clearer to have these functions look more like: > > void OnCreateVideoCapturer(scoped_ptr<webrtc::DesktopCapturer>* capturer); > > This makes it more obvious that the function is not so much taking ownership of > the argument as potentially modifying it. (2 places) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:51: // the set of |HostExtensionSession| to match the client's capabilities. On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: |HostExtensionSession|s Acknowledged. Removed the || since we don't seem to use those for type names, only variables & members. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:56: // is handled, or none remain. Returns true if the message was handled. On 2014/08/22 06:38:29, Peter Kasting wrote: > Again, what order do these go in? It's also not valid for two extensions to handle the same message - the function currently exits as soon as an extension handles the message. Added comment to clarify that. If the function instead called all extensions even after one has handled the message then it could DCHECK() that at most one handles it, plus every extension would always be called for every message, which allows things like adding an extension-message-logging extension, for example. WDYT? https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:60: typedef std::vector<HostExtension*> HostExtensionList; On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: Don't use "List" in a typename, especially if it isn't actually a > std::list<>. For example, "HostExtensions" would be a more generic/less > misleading name here. (2 places) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:68: // List of HostExtensions to attach to the session, if it reaches the On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: Again, avoid "list" except perhaps where you're actually using a list. (2 > places) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager_unittest.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:20: extension2_("ext2", ""), On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: Prefer std::string() over "" Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:28: protected: On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: In general, the Google style guide prefers private data members to > protected ones. While protected members are allowed when using gtest "for > technical reasons", it's usually still preferable to make them private and > provide accessors as necessary. For a general purpose class I'd agree, but this is a test fixture so it's only purpose is as a container for members that are common across several tests; making them private seems to add a lot of boilerplate for no clear benefit. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:38: }; On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:40: // Verifies that messages are passed to be handled by the correct extension. On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: I'm not sure what this sentence means, but it seems like removing "passed > to be" would make it clearer? Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:79: EXPECT_EQ(extension_manager.GetCapabilities(), "cap1 cap3"); On 2014/08/22 06:38:29, Peter Kasting wrote: > Would it be equally valid to return "cap3 cap1"? Should the test account for > this? Good point; have updated the test to account for that. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:162: // Verifies that if an extension reports that they modify the video pipeline On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: they modify -> it modifies Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.cc:31: : encoder_(encoder.Pass()), On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: Avoid defining complex functions inline except in unittest files ( > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-What-ab... > ). (Several places) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.cc:44: void SetEnableRecording(bool enable_recording) { On 2014/08/22 06:38:29, Peter Kasting wrote: > Nit: This function could probably be left inlined, named unix_hacker()-style. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:8: #include <stdint.h> On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Prefer C++ headers to C ones (<cstdint>) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:31: // 1. Create the VideoFrameRecorder on the controlling thread. On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: "on the controlling thread" unnecessary (you're in the "on the control > thread" section) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:35: // 4. Hand the returned wrapper VideoEncoder of to the video encoding thread, On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: of -> off ("Hand off the..." might be even better) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:53: // The caller must delete the previous recording VideoEncoder, or call On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Perhaps this second sentence would be clearer as (assuming I understand it > correctly): > > This function must not be called when a functioning wrapper already exists; > either delete the wrapper VideoEncoder entirely, or detach the wrapper from the > underlying encoder by calling DetachVideoEncoderWrapper(), before calling this > method to create a new wrapper. Acknowledged; I've gone with another rewording that I think is clearer. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:58: // Detaches the existing VideoEncoder wrapper, stopping it from recording. On 2014/08/22 06:38:30, Peter Kasting wrote: > What about the lifetime of the wrapper? Will it exist but do nothing until > WrapVideoEncoder() is called again? Say how/when it is cleaned up. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:65: // When this maximum is reached older frames will be discard to make space On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: discarded Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder_host_extension.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:24: const char kVideoRecorderCapabilities[] = "videoRecorder"; On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Declare variables in the most local scope possible; it looks like these can > mostly at least move into the specific functions below referencing them. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:39: virtual ~VideoFrameRecorderHostExtensionSession() {} On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Don't inline this Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:51: VideoEncoderVerbatim verbatim_encoder; On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Members should have a "_" suffix on their names. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:59: int64_t max_content_bytes) : first_frame_(false) { On 2014/08/22 06:38:30, Peter Kasting wrote: > FYI nit: I personally read the Google style guide as saying that each > initializer should get its own line when the whole function prototype + > initializer list doesn't fit on one line. Not everyone agrees with me; > therefore, you're welcome to format things this way, unless you prefer my rule > :) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:88: if (value && value->GetAsDictionary(&client_message)) { On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Consider reversing this conditional and early-returning to avoid indenting > the whole rest of the function Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:95: if (type == kStartType) { On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Could be something like this to avoid indenting the longest conditional > arm: > > if (type != kNextFrameType) { > video_frame_recorder.SetEnableRecording(type == kStartType); > if (type == kStartType) > first_frame_ = true; > return true; > } > > ... That ended up being less readable, IMO, so I broke out the individual message handlers into separate functions instead. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:123: data.resize(encoded_frame->ByteSize()); On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Can't the size be passed to the string as a constructor arg? Ideally we want to set the size without initializing the contents, since we're about to rewrite them, but it seems that resize() always initializes them to something anyway, so Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:138: LOG(ERROR) << "Failed to create reply json"; On 2014/08/22 06:38:30, Peter Kasting wrote: > Are you going to read these logs from client machines somehow? If not, try to > avoid LOG(); instead, either handle the error if it can actually occur, or if > this can truly never happen, don't handle it (or DCHECK() that it doesn't occur) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder_host_extension.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.h:18: VideoFrameRecorderHostExtension() {} On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: In general, avoid defining constructors/destructors inline, even when they > seem to be empty; this is a good way to cause a lot of code to be instantiated > if the class currently, or in the future, ends up containing non-POD types, so > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > recommends avoiding this entirely (and the Chromium style guide specially calls > out virtual methods like the destructor here). Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder_unittest.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:23: (a.dpi().equals(b.dpi()))) { On 2014/08/22 06:38:31, Peter Kasting wrote: > Nit: Reversing this conditional takes no more LOC total but avoids one level of > indenting for the remaining code. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:40: const int64_t kMaxContentBytes = 10 * 1024 * 1024; On 2014/08/22 06:38:31, Peter Kasting wrote: > Nit: Again, define these in the most local scopes possible. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:52: void CreateAndWrapEncoder(); On 2014/08/22 06:38:30, Peter Kasting wrote: > Comments? Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:60: protected: On 2014/08/22 06:38:31, Peter Kasting wrote: > Nit: Again, try to avoid protected members if possible. Again, since this is a test base, protected feels more appropriate, but let me know if you disagree. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:66: std::list<webrtc::DesktopFrame*> test_frames_; On 2014/08/22 06:38:31, Peter Kasting wrote: > Nit: Consider a typedef for this so you can use it in loops below Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:114: for (int i=0; i < kTestFrameCount; ++i) { On 2014/08/22 06:38:31, Peter Kasting wrote: > Nit: Spaces around '=' > > Super-tiny nit: Since it's a count of objects in memory, the constant and loop > index here should probably be size_t rather than int. Done, although I tend to prefer int over size_t on the basis of size_t potentially falling foul of unsigned int gotchas. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:120: std::list<webrtc::DesktopFrame*>::iterator i; On 2014/08/22 06:38:30, Peter Kasting wrote: > Nit: Declare iterators in the loop declaration unless they're needed outside it Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:122: scoped_ptr<VideoPacket> packet = encoder_->Encode(*(*i)); On 2014/08/22 06:38:31, Peter Kasting wrote: > Nit: "scoped_ptr<VideoPacket> packet = " unnecessary? (2 places) Done. In theory Encode() could return a NULL pointer, but never actually should, so I've added ASSERT_TRUE() around these two calls. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:161: // VideoFrameRecorder don't end the world. On 2014/08/22 06:38:31, Peter Kasting wrote: > Nit: don't end the world -> doesn't crash (or some other more precise, less > colloquial phrase) Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:221: int64 frame_bytes = kWidth * kHeight * webrtc::DesktopFrame::kBytesPerPixel; On 2014/08/22 06:38:30, Peter Kasting wrote: > Optional: const Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:242: // limit allows, the first encoded frames are dropped. On 2014/08/22 06:38:31, Peter Kasting wrote: > This comment is the same as on the previous testcase. Done.
(Have not yet re-reviewed) https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:30: std::string capability = (*extension)->capability(); On 2014/08/25 23:23:10, Wez wrote: > On 2014/08/22 06:38:28, Peter Kasting wrote: > > Nit: Could probably be a const std::string& (tiny efficiency boost in some > > cases) > > Making capability() return const std::string& risks broken code like: > > const char kCapability[] = "foo"; > > const std::string& FooExtension::capability() const { return kCapability; } > > so I'd prefer to have it return std::string instead. I actually meant that the local variable here could be a const std::string&; this is no worse than the current code if capability() returns by value, and slightly better if it returns by const ref. I didn't look at the prototype of capability(). https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.h:56: // is handled, or none remain. Returns true if the message was handled. On 2014/08/25 23:23:10, Wez wrote: > If the function instead called all extensions even after one has handled the > message then it could DCHECK() that at most one handles it, plus every extension > would always be called for every message, which allows things like adding an > extension-message-logging extension, for example. WDYT? I don't have an opinion -- that's less of a readability change and more of a functional one, so it'd be better posed to the relevant OWNERS here. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager_unittest.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:28: protected: On 2014/08/25 23:23:11, Wez wrote: > On 2014/08/22 06:38:29, Peter Kasting wrote: > > Nit: In general, the Google style guide prefers private data members to > > protected ones. While protected members are allowed when using gtest "for > > technical reasons", it's usually still preferable to make them private and > > provide accessors as necessary. > > For a general purpose class I'd agree, but this is a test fixture so it's only > purpose is as a container for members that are common across several tests; > making them private seems to add a lot of boilerplate for no clear benefit. The main benefits that normally accrue involve clarifying access patterns to the members. If lots of tests need to read and write each separate member it's not much of a win, but if for example the tests only ever read these (and the constructor does all the writing), having the tests only have access to getters can make it clearer to e.g. someone trying to add more functionality to the test fixture later that this is the case, and prevent someone from writing a test case that writes to these when it's not supposed to. While you can proceed with the code as you have it, I think the fact that the Google style guide allows this "for technical reasons" rather than giving a more readability-focused argument like the one you make suggests that the style guide authors didn't really allow this case because direct member access in tests allows avoiding ugly boilerplate, but rather because certain kinds of testing are hard to do at all without this exception. Up to you.
https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:13: namespace remoting { On 2014/08/22 06:38:28, Peter Kasting wrote: > Nit: At least one blank line above this Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:30: std::string capability = (*extension)->capability(); On 2014/08/25 23:39:06, Peter Kasting wrote: > On 2014/08/25 23:23:10, Wez wrote: > > On 2014/08/22 06:38:28, Peter Kasting wrote: > > > Nit: Could probably be a const std::string& (tiny efficiency boost in some > > > cases) > > > > Making capability() return const std::string& risks broken code like: > > > > const char kCapability[] = "foo"; > > > > const std::string& FooExtension::capability() const { return kCapability; } > > > > so I'd prefer to have it return std::string instead. > > I actually meant that the local variable here could be a const std::string&; > this is no worse than the current code if capability() returns by value, and > slightly better if it returns by const ref. I didn't look at the prototype of > capability(). The std::string returned by capability() will go out of scope following the assignment to the const std::string&, though, surely, leaving a danlging reference? https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager_unittest.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:28: protected: On 2014/08/25 23:39:06, Peter Kasting wrote: > On 2014/08/25 23:23:11, Wez wrote: > > On 2014/08/22 06:38:29, Peter Kasting wrote: > > > Nit: In general, the Google style guide prefers private data members to > > > protected ones. While protected members are allowed when using gtest "for > > > technical reasons", it's usually still preferable to make them private and > > > provide accessors as necessary. > > > > For a general purpose class I'd agree, but this is a test fixture so it's only > > purpose is as a container for members that are common across several tests; > > making them private seems to add a lot of boilerplate for no clear benefit. > > The main benefits that normally accrue involve clarifying access patterns to the > members. If lots of tests need to read and write each separate member it's not > much of a win, but if for example the tests only ever read these (and the > constructor does all the writing), having the tests only have access to getters > can make it clearer to e.g. someone trying to add more functionality to the test > fixture later that this is the case, and prevent someone from writing a test > case that writes to these when it's not supposed to. > > While you can proceed with the code as you have it, I think the fact that the > Google style guide allows this "for technical reasons" rather than giving a more > readability-focused argument like the one you make suggests that the style guide > authors didn't really allow this case because direct member access in tests > allows avoiding ugly boilerplate, but rather because certain kinds of testing > are hard to do at all without this exception. > > Up to you. OK, thanks for clarifying. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:8: #include <stdint.h> On 2014/08/25 23:23:12, Wez wrote: > On 2014/08/22 06:38:30, Peter Kasting wrote: > > Nit: Prefer C++ headers to C ones (<cstdint>) > > Done. Hmmm, stdint.h -> cstdint seems to break Mac Chromium Compile Dbg. :(
I think this is about ready to go. https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:30: std::string capability = (*extension)->capability(); On 2014/08/26 19:22:25, Wez wrote: > On 2014/08/25 23:39:06, Peter Kasting wrote: > > On 2014/08/25 23:23:10, Wez wrote: > > > On 2014/08/22 06:38:28, Peter Kasting wrote: > > > > Nit: Could probably be a const std::string& (tiny efficiency boost in some > > > > cases) > > > > > > Making capability() return const std::string& risks broken code like: > > > > > > const char kCapability[] = "foo"; > > > > > > const std::string& FooExtension::capability() const { return kCapability; } > > > > > > so I'd prefer to have it return std::string instead. > > > > I actually meant that the local variable here could be a const std::string&; > > this is no worse than the current code if capability() returns by value, and > > slightly better if it returns by const ref. I didn't look at the prototype of > > capability(). > > The std::string returned by capability() will go out of scope following the > assignment to the const std::string&, though, surely, leaving a danlging > reference? No; if you bind a returned temporary to a const reference, the life of the temporary is extended through the lifetime of the reference. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:8: #include <stdint.h> On 2014/08/26 19:22:25, Wez wrote: > On 2014/08/25 23:23:12, Wez wrote: > > On 2014/08/22 06:38:30, Peter Kasting wrote: > > > Nit: Prefer C++ headers to C ones (<cstdint>) > > > > Done. > > Hmmm, stdint.h -> cstdint seems to break Mac Chromium Compile Dbg. :( Huh! Apparently cstdint is formally C++11, and I guess Mac Clang is being strict about that. Better change back to stdint.h then... sorry! https://codereview.chromium.org/468613002/diff/40001/remoting/host/fake_host_... File remoting/host/fake_host_extension.h (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:69: // True if |CreateExtensionSession| was called on this extension. Nit: For function names, I think it's more typical to write Foo() than |Foo|. https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... File remoting/host/host_extension_session.h (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... remoting/host/host_extension_session.h:32: // modify the pipeline (see below). They may replace, wrap or take ownership Nit: For clarity, I think maybe "take ownership" should be removed, as it's redundant with the rest of your comment in the context of e.g. wrapping the provided object, and it doesn't make much sense in the context of an ownership transfer (since, as you note, the pipeline won't be constructed in that case). https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... remoting/host/host_extension_session.h:42: virtual bool ModifiesVideoPipeline() const; Tiny nit: I wonder if "WantsToModifyVideoPipeline" would be clearer? Not sure. https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... File remoting/host/host_extension_session_manager_unittest.cc (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:86: EXPECT_EQ(2U, reported_caps.size()); This should be ASSERT_EQ() to prevent possible memory errors below if e.g. |reported_caps| is empty. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... File remoting/host/video_frame_recorder.h (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:55: // DetachVideoEncoderWrapper(), before WrappVideoEncoder() is called again. Nit: Wrapp -> Wrap https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... File remoting/host/video_frame_recorder_host_extension.cc (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:57: ) { Nit: I have absolutely no idea how the Google style guide would suggest wrapping this -- I don't think there is a rule -- but I think if the function name were too long we'd normally break after the "::", so maybe doing that instead is better? Up to you. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:72: const char kVideoRecorderType[] = "video-recorder"; Nit: Since these two are used by multiple functions, it might be clearer to leave them atop the file where they used to be, or else maybe declare them in the class declaration. I don't think there are rules about this, though. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:109: VLOG(1) << "Unknown video-recorder message"; Nit: As noted before, it's usually better to avoid logging. As a reader, I don't know whether there actually are other message types, or this is just trying to be future-proof, and if control _does_ reach here, whether that's something bad that we ought to know about, or not. I think either of the following is probably clearer: just eliminate this "else" clause entirely (unknown messages are fine, drop them on the floor) or make the last handler more like: } else { DCHECK_EQ(kNextFrameType, type); OnNextFrame(client_stub); } (to indicate that we need to explicitly handle all messages and if someone adds one later without touching this it's a bug). https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... File remoting/host/video_frame_recorder_unittest.cc (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:79: const int kFrameWidth = 640; These should be static consts since they're not instantiation-dependent. Nit: You should probably be consistent between the two consts I pointed out in video_frame_recorder_host_extension.cc and here about whether the constants used in multiple member functions of a file-scoped class are declared within the class declaration or at file-scope (inside an anonymous namespace). https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:150: ASSERT_TRUE(encoder_->Encode(*(*i))); Nit: Could just write (**i)
https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... File remoting/host/host_extension_session_manager.cc (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/host_exten... remoting/host/host_extension_session_manager.cc:30: std::string capability = (*extension)->capability(); On 2014/08/26 20:58:14, Peter Kasting wrote: > On 2014/08/26 19:22:25, Wez wrote: > > On 2014/08/25 23:39:06, Peter Kasting wrote: > > > On 2014/08/25 23:23:10, Wez wrote: > > > > On 2014/08/22 06:38:28, Peter Kasting wrote: > > > > > Nit: Could probably be a const std::string& (tiny efficiency boost in > some > > > > > cases) > > > > > > > > Making capability() return const std::string& risks broken code like: > > > > > > > > const char kCapability[] = "foo"; > > > > > > > > const std::string& FooExtension::capability() const { return kCapability; > } > > > > > > > > so I'd prefer to have it return std::string instead. > > > > > > I actually meant that the local variable here could be a const std::string&; > > > this is no worse than the current code if capability() returns by value, and > > > slightly better if it returns by const ref. I didn't look at the prototype > of > > > capability(). > > > > The std::string returned by capability() will go out of scope following the > > assignment to the const std::string&, though, surely, leaving a danlging > > reference? > > No; if you bind a returned temporary to a const reference, the life of the > temporary is extended through the lifetime of the reference. Done. https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... File remoting/host/video_frame_recorder.h (right): https://codereview.chromium.org/468613002/diff/20001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:8: #include <stdint.h> On 2014/08/26 20:58:14, Peter Kasting wrote: > On 2014/08/26 19:22:25, Wez wrote: > > On 2014/08/25 23:23:12, Wez wrote: > > > On 2014/08/22 06:38:30, Peter Kasting wrote: > > > > Nit: Prefer C++ headers to C ones (<cstdint>) > > > > > > Done. > > > > Hmmm, stdint.h -> cstdint seems to break Mac Chromium Compile Dbg. :( > > Huh! Apparently cstdint is formally C++11, and I guess Mac Clang is being > strict about that. Better change back to stdint.h then... sorry! Done. https://codereview.chromium.org/468613002/diff/40001/remoting/host/fake_host_... File remoting/host/fake_host_extension.h (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/fake_host_... remoting/host/fake_host_extension.h:69: // True if |CreateExtensionSession| was called on this extension. On 2014/08/26 20:58:15, Peter Kasting wrote: > Nit: For function names, I think it's more typical to write Foo() than |Foo|. Done. https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... File remoting/host/host_extension_session.h (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... remoting/host/host_extension_session.h:32: // modify the pipeline (see below). They may replace, wrap or take ownership On 2014/08/26 20:58:15, Peter Kasting wrote: > Nit: For clarity, I think maybe "take ownership" should be removed, as it's > redundant with the rest of your comment in the context of e.g. wrapping the > provided object, and it doesn't make much sense in the context of an ownership > transfer (since, as you note, the pipeline won't be constructed in that case). Done. https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... remoting/host/host_extension_session.h:42: virtual bool ModifiesVideoPipeline() const; On 2014/08/26 20:58:15, Peter Kasting wrote: > Tiny nit: I wonder if "WantsToModifyVideoPipeline" would be clearer? Not sure. Acknowledged. I think Modifies* is sufficiently expressive, and more concise. :) https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... File remoting/host/host_extension_session_manager_unittest.cc (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/host_exten... remoting/host/host_extension_session_manager_unittest.cc:86: EXPECT_EQ(2U, reported_caps.size()); On 2014/08/26 20:58:15, Peter Kasting wrote: > This should be ASSERT_EQ() to prevent possible memory errors below if e.g. > |reported_caps| is empty. Done. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... File remoting/host/video_frame_recorder.h (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder.h:55: // DetachVideoEncoderWrapper(), before WrappVideoEncoder() is called again. On 2014/08/26 20:58:15, Peter Kasting wrote: > Nit: Wrapp -> Wrap Done. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... File remoting/host/video_frame_recorder_host_extension.cc (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:57: ) { On 2014/08/26 20:58:15, Peter Kasting wrote: > Nit: I have absolutely no idea how the Google style guide would suggest wrapping > this -- I don't think there is a rule -- but I think if the function name were > too long we'd normally break after the "::", so maybe doing that instead is > better? Up to you. Done. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:72: const char kVideoRecorderType[] = "video-recorder"; On 2014/08/26 20:58:15, Peter Kasting wrote: > Nit: Since these two are used by multiple functions, it might be clearer to > leave them atop the file where they used to be, or else maybe declare them in > the class declaration. I don't think there are rules about this, though. Declaring them in the class complicates things with "constexpr" subtlety, so I've moved them back above the session class declaration, to keep things simple. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_host_extension.cc:109: VLOG(1) << "Unknown video-recorder message"; On 2014/08/26 20:58:15, Peter Kasting wrote: > Nit: As noted before, it's usually better to avoid logging. As a reader, I > don't know whether there actually are other message types, or this is just > trying to be future-proof, and if control _does_ reach here, whether that's > something bad that we ought to know about, or not. > > I think either of the following is probably clearer: just eliminate this "else" > clause entirely (unknown messages are fine, drop them on the floor) or make the > last handler more like: > > } else { > DCHECK_EQ(kNextFrameType, type); > OnNextFrame(client_stub); > } > > (to indicate that we need to explicitly handle all messages and if someone adds > one later without touching this it's a bug). Removed the logging. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... File remoting/host/video_frame_recorder_unittest.cc (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:79: const int kFrameWidth = 640; On 2014/08/26 20:58:15, Peter Kasting wrote: > These should be static consts since they're not instantiation-dependent. > > Nit: You should probably be consistent between the two consts I pointed out in > video_frame_recorder_host_extension.cc and here about whether the constants used > in multiple member functions of a file-scoped class are declared within the > class declaration or at file-scope (inside an anonymous namespace). I was consistent until you asked me to move the constant definitions around. ;) The fact that the other constants are strings rather than integral types seems to complicate having them as in-class definitions, so I'm moving everything back to file-scope. Do file-scoped consts need an anonymous namespace? I thought they were implicitly static? https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:150: ASSERT_TRUE(encoder_->Encode(*(*i))); On 2014/08/26 20:58:15, Peter Kasting wrote: > Nit: Could just write (**i) Done.
LGTM Once this is landed, I'll mark the internal bug as fixed, which should grant readability. https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... File remoting/host/video_frame_recorder_unittest.cc (right): https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... remoting/host/video_frame_recorder_unittest.cc:79: const int kFrameWidth = 640; On 2014/08/28 00:13:21, Wez wrote: > On 2014/08/26 20:58:15, Peter Kasting wrote: > > These should be static consts since they're not instantiation-dependent. > > > > Nit: You should probably be consistent between the two consts I pointed out in > > video_frame_recorder_host_extension.cc and here about whether the constants > used > > in multiple member functions of a file-scoped class are declared within the > > class declaration or at file-scope (inside an anonymous namespace). > > I was consistent until you asked me to move the constant definitions around. ;) Sorry... > The fact that the other constants are strings rather than integral types seems > to complicate having them as in-class definitions, so I'm moving everything back > to file-scope. Yeah, pre-C++11 there's no constexpr and thus you'd need to declare and define non-integral constants of class scope separately. This is annoying enough to make it not worth it. > Do file-scoped consts need an anonymous namespace? I thought they were > implicitly static? They are indeed implicitly internal-linkage, so the anonymous namespace isn't necessary. (I had to look that up.) I personally might leave it anyway (a) in case someone makes one of these non-const or adds something else later, or (b) to assure readers these haven't been predeclared "extern" in the header or something. But neither of these is a very strong reason and you can do what you like.
On 2014/08/28 00:31:02, Peter Kasting wrote: > LGTM > > Once this is landed, I'll mark the internal bug as fixed, which should grant > readability. Thanks! :) > > https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... > File remoting/host/video_frame_recorder_unittest.cc (right): > > https://codereview.chromium.org/468613002/diff/40001/remoting/host/video_fram... > remoting/host/video_frame_recorder_unittest.cc:79: const int kFrameWidth = 640; > On 2014/08/28 00:13:21, Wez wrote: > > On 2014/08/26 20:58:15, Peter Kasting wrote: > > > These should be static consts since they're not instantiation-dependent. > > > > > > Nit: You should probably be consistent between the two consts I pointed out > in > > > video_frame_recorder_host_extension.cc and here about whether the constants > > used > > > in multiple member functions of a file-scoped class are declared within the > > > class declaration or at file-scope (inside an anonymous namespace). > > > > I was consistent until you asked me to move the constant definitions around. > ;) > > Sorry... > > > The fact that the other constants are strings rather than integral types seems > > to complicate having them as in-class definitions, so I'm moving everything > back > > to file-scope. > > Yeah, pre-C++11 there's no constexpr and thus you'd need to declare and define > non-integral constants of class scope separately. This is annoying enough to > make it not worth it. > > > Do file-scoped consts need an anonymous namespace? I thought they were > > implicitly static? > > They are indeed implicitly internal-linkage, so the anonymous namespace isn't > necessary. (I had to look that up.) I personally might leave it anyway (a) in > case someone makes one of these non-const or adds something else later, or (b) > to assure readers these haven't been predeclared "extern" in the header or > something. But neither of these is a very strong reason and you can do what you > like. If in doubt, leaving things in such a state that people don't have to look up corner cases seems preferable.
lgtm
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wez@chromium.org/468613002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as 74878db382df7cad65ffcf419898b4b1f1b6a616
Message was sent while issue was closed.
I have marked the internal bug as Fixed. If you don't automatically get readability granted within a day or two, ping me again.
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ea12516899b23b38bcd006b9b462eaead73f4cb5 Cr-Commit-Position: refs/heads/master@{#292541} |
