Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9622020:
C++ Readability Review for thestig. (Closed)
|
Created:
8 years, 9 months ago by Lei Zhang Modified:
8 years, 8 months ago CC:
joi+watch-content_chromium.org, darin-cc_chromium.org, vandebo (ex-Chrome) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionC++ Readability Review for thestig.
Original review: https://chromiumcodereview.appspot.com/9560008/
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131917
Patch Set 1 #
Total comments: 62
Patch Set 2 : rebase and address comments #
Total comments: 19
Patch Set 3 : address comments #
Total comments: 4
Patch Set 4 : address another round of comments #
Total comments: 2
Patch Set 5 : fix minor comment issue #Patch Set 6 : blank line #
Total comments: 2
Messages
Total messages: 17 (0 generated)
Hi Lei, First of all, thanks for submitting your code for readability review. As we continue to grow it becomes more and more important to maintain a uniform style in our codebase, and readability reviews are an important part of this effort. Please remember that the purpose of this review is not to be a commentary on your ability to write code, but to ensure that you understand Google's style guidelines, which aren't necessarily better or worse than what you may be used to, but are almost certainly different and can take some getting used to. Overall this code looks good. You've clearly read through the style guide and made a good effort to make your code consistent with google conventions. I'm particularly glad to see the good namespace use and exhaustive unit test. My first round of comments are below. I know there are a number of comments here, but don't let that disturb you - it's our job as readability reviewers to point out every style concern we can find, no matter what. In these comments, I usually only point out the first instance of each readability issue I find. You should, however, try to correct all instances of each issue that you can find in the code you submitted. Also note that this isn't a code review: I didn't thoroughly inspect your code for correctness, that's the job of the engineers who work in the area it applies to. There are probably a number of style guide rules which can't be strictly adhered to in Chrome code, and I'm willing to make some reasonable exceptions for such cases. But my assumption is that in applying for readability review you want to follow the google style guide as closely as possible, and I will be mostly reviewing this according to the same standards as normal google code. Andrew https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:16: using base::SystemMonitor; If you don't use this outside your project namespace (content) it would be kind of nice to move this inside the namespace. It would reduce its scope a little bit, and otherwise be completely harmless. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:20: const char* const kDCIMDirName = "DCIM"; There is a preference in google code to declare C string constants as char[] rather than char*. It is slightly safer and more efficient, since the array can't possibly be reassigned to point to something else and the offset is completely known at compile time. Same elsewhere. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:49: known_file_systems_.insert(kKnownFileSystems[i]); For this particular case, I think it would help readability to put braces around the for statement body even though it is a one liner, since you are putting the body on a separate line from the start, and there is a closing brace after it for an outer scope. Having braces around the inner scope makes it absolutely clear which braces go with which statement. Same in similar cases elsewhere (applies to if, while, anonymous nested scopes, etc. also). https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:63: NOTREACHED(); How about a comment explaining what this means? https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:75: watcher_delegate_ = new WatcherDelegate(this); Is there a method call (like scoped_ptr::reset()) you could use for this, instead of the overloaded assignment operator? That would make it much more obvious in the local context that watcher_delegate_ is not a raw pointer (and hence the reader does not need to worry about when this is deleted). https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:100: continue; Do I understand correctly that the only thing this actually skips is the ++it on line 103? So it would be equivalent just to remove ++ on the previous line and remove this? If so, it seems good to do that, since it would be simpler. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:110: const MountPoint& mount_point(newiter->first); It is true that the style guide allows using either constructor or assignment-style notation for assigning pointers, references, and primitive values, but there is a pretty overwhelming preference in google code for using the assignment style instead. In addition to following convention, another good reason for this style is that it clearly distinguishes in notation between simple primitive assignment and instantiating class-type objects by "calling" a constructor. So, I would recommend changing to using assignment notation consistently for initializing references in this code. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:112: const MountDevice& mount_device(newiter->second.first); Why not use the temporary you created on the previous line here? Same below. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:119: mtab_[mount_point] = mount_device_and_id; Can you use insert() here rather than the [] operator? There is a preference in google code against using [] for maps when possible (see comment near the top of util/gtl/map-util.h for an explanation). If you don't have access to map-util.h in chrome, there are some cases where using [] might be ok where it would not be in google3 code. It seems like since this is just inserting, and not relying on default initialization behavior or overwriting existing values, that insert() should work pretty easily here, so I think you should change it. Same in similar cases elsewhere. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:3: // found in the LICENSE file. The style guide requires a descriptive file-level comment after the copyright notice: http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co... Clearly for Chrome code, because of the different licensing, you can't follow the standard google3 file comment format exactly, but it seems like you ought to still be able to include a file description. Same in all other files. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:20: #include "content/common/content_export.h" Are all of these #includes actually necessary in this immediate file? The style guide prefers avoiding unnecessary includes: http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co... And encourages the use of forward declarations instead of includes where possible. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:37: typedef std::string MountDevice; The use of a typedef here strikes me as superfluous and actually somewhat harmful for readability. These typedefs are useful when the full type name is awkwardly long or complicated, or when the typedef name gives some useful information about the use of the type that the full name doesn't include. On the other hand, though, by hiding the actual type, they create some additional burden on readers since readers must consult the typedef definition to understand the actual type and how it can be used, while if the real time is a well understood standard type if there was no typedef they might not need to consult any external resources. In this case, I don't think the typedef makes the name usefully shorter (the full name is fairly short), and I don't think it adds any useful information either (a good variable name should make clear that string value contains a mount device). It does, however, hide some important details that are useful to readers - namely the actual type of the object (string). Based on that, the typedef here seems like a net negative and I would prefer if you removed it. Same with MountPoint also, and in similar cases. I think MountDeviceAndId shortens the name enough that it seems good to keep, however, and while MountMap is kind of borderline I am ok with keeping it also. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:45: class WatcherDelegate : public base::files::FilePathWatcher::Delegate { Do you actually need the full definition of this class in the header file? Since it is private it can't be used by external users and the only reference to it in this file is in the declaration of a scoped_refptr. If it is like the standard google3 smart pointer classes, you really only need the type declaration for the scoped_refptr. In that case, you could move the definition of the class to your .cc file and only have a forward declaration in this header file. That would improve readability of the header by making it shorter and reducing the amount of extraneous (to external users) information, and also follow the general principle of minimizing the extent and scope of all definitions. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:56: MediaDeviceNotificationsLinux* notifier_; Is this owned or not? If owned, when is it deleted? If not owned, who does own it? There should be a comment answering those questions here. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:61: virtual ~MediaDeviceNotificationsLinux(); For me, it would have been useful to have a comment with these explaining why the destructor is private (so it can only be deleted by the reference-counting smart pointer I guess?). Maybe this is completely obvious to everyone in chrome, but if there is any possibility of confusion a comment seems good. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:10: The style guide now prefers *not* to have an empty line separating C and C++ standard library includes, see code snippets here: http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In... Note that they do still need to be grouped separately, there should just not be any empty line between the groups. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:21: #include "content/browser/media_device_notifications_linux.h" This actually needs to be the very first file included here (note the reference to testing in the linked style guide section): http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In... https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:44: class MediaDeviceNotificationsLinuxTest : public testing::Test { Can you put this test code inside an anonymous namespace? It is preferred if possible to minimize the scope of the test code, since it shouldn't be referenced externally anyway. Usually the only reason you can't do this is if some test class needs to be a friend of the class it is testing. I don't think you are doing that. Note that you can keep your tests inside your project namespace to have unqualified access to project names - just use an anonymous namespace *inside* your project namespace. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:47: MtabTestData(const char* mount_device, It seems like it could make this code simpler and possibly safer if you passed and stored these as C++ strings rather than const char* (the usual way would be to make the arguments references and copy them to non-reference data members in the constructor). This would eliminate various tricky details around const char* (NULL, ownership of the data, length, lack of built in methods for common operations) and also better follow google convention. The down side is that it would involve more copying and you would have to manually convert to const char* when passing as arguments to code that requires that - I don't think those are very serious problems compared to the advantages, especially in a test. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:63: system_monitor_.reset(new base::SystemMonitor()); Why not do this in the initializer list, instead of resetting in in the constructor body? (although this will be moot if you follow my suggestion and change to a default data member) https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:78: struct MtabTestData initial_test_data[] = { C++ does not require prepending 'struct' to the name of struct types, and since this is consistent with how class names are referenced, not including them is the preference and the overwhelming convention in google code. So please change this to just 'MtabTestData initial...'. Same in similar cases elsewhere. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:123: return *mock_devices_changed_observer_.get(); Is get() needed here? Most smart pointer classes have * overriden to dereference the wrapped pointer and using that (without get()) is quite conventional in google code. (it is more standard to use get(), even when not strictly necessary, when accessing the pointer value directly) https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:131: void WriteToMtab(struct MtabTestData* data, Can data be const? The style guide prefers variables and parameters to be declared const whenever possible: http://www.corp.google.com/eng/doc/cppguide.xml?showone=Use_of_const#Use_of_c... https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:142: entry.mnt_fsname = strdup(data[i].mount_device); Why does the string need to be copied here? Is there any reason you can't just copy the pointer from the original location, given it seems to be in the right format (null terminated string) already and will live longer than this copy? It would be nice to get rid of this strdup() if so, especially since it would eliminate the need for C-style free() calls below (very unusual in google code). https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:158: ASSERT_GE(fd, 0); Just FYI: the gunit assertion macros (I'm assuming whatever the Chrome codebase has is similar) treat the first argument as the 'expected' value and the second as the 'actual'. This doesn't really affect the correctness of the test, since equality is a symmetric operator, but it does affect the error messages that are printed when assertions fail: If you put the arguments in the opposite order, the test will say 'expected result B, actual was A' when it actually should be the opposite. So, to make the test output nicer, you might want to switch around the order of arguments here and elsewhere. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:160: int fsync_result = fsync(fd); Are you sure this temporary is necessary? Sometimes having a variable like this can, through its name, give some additional useful information, but I think here it would be easy enough to understand if you eliminated it and just moved the fsync call instead the ASSERT. Same with close_result below, and other similar cases. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:172: scoped_ptr<base::SystemMonitor> system_monitor_; Does this really need to be allocated on the heap, or could you change this to a direct data member, instead of a scoped_ptr? If so that is simpler and could be a little more efficient. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:185: TEST_F(MediaDeviceNotificationsLinuxTest, BasicAttachDetach) { It would be helpful to readers if you added a short comment before each test method explaining what it tests in a little more detail than is offered by just the name. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:187: FilePath test_path = CreateMountPoint(kMountPointA, true); When you pass literals like true as function arguments, you need to have a short inline comment next to them indicating what they mean. See here (under "NULL, true/false, 1, 2, 3...") for the recommended style: http://www.corp.google.com/eng/doc/cppguide.xml?showone=Implementation_Commen... Same elsewhere https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:193: EXPECT_CALL(observer(), OnMediaDeviceAttached(0, kDevice2, test_path)) Please add a comment explaining what this actually does. I can't tell to be honest. In general gmock syntax is incredibly opaque to readers, so some kind of comment is necessary. Same in similar cases elsewhere.
Thanks for the review. I certainly learned a few things I had missed from the style guide. The diff between patch set 1 and 2 is a bit distorted, because I had to fix some flakiness in the meantime: http://src.chromium.org/viewvc/chrome?view=rev&revision=127282 https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:16: using base::SystemMonitor; On 2012/03/23 22:47:39, ajl wrote: > If you don't use this outside your project namespace (content) it would be kind > of nice to move this inside the namespace. > > It would reduce its scope a little bit, and otherwise be completely harmless. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:20: const char* const kDCIMDirName = "DCIM"; On 2012/03/23 22:47:39, ajl wrote: > There is a preference in google code to declare C string constants as char[] > rather than char*. It is slightly safer and more efficient, since the array > can't possibly be reassigned to point to something else and the offset is > completely known at compile time. > > Same elsewhere. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:49: known_file_systems_.insert(kKnownFileSystems[i]); On 2012/03/23 22:47:39, ajl wrote: > For this particular case, I think it would help readability to put braces around > the for statement body even though it is a one liner, since you are putting the > body on a separate line from the start, and there is a closing brace after it > for an outer scope. Having braces around the inner scope makes it absolutely > clear which braces go with which statement. > > Same in similar cases elsewhere (applies to if, while, anonymous nested scopes, > etc. also). Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:63: NOTREACHED(); On 2012/03/23 22:47:39, ajl wrote: > How about a comment explaining what this means? Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:75: watcher_delegate_ = new WatcherDelegate(this); On 2012/03/23 22:47:39, ajl wrote: > Is there a method call (like scoped_ptr::reset()) you could use for this, > instead of the overloaded assignment operator? > > That would make it much more obvious in the local context that watcher_delegate_ > is not a raw pointer (and hence the reader does not need to worry about when > this is deleted). Unfortunately no. See http://src.chromium.org/viewvc/chrome/trunk/src/base/memory/ref_counted.h?rev..., but I understand your concern. I've seen it elsewhere and went "err, where is Foo getting deleted?" https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:100: continue; On 2012/03/23 22:47:39, ajl wrote: > Do I understand correctly that the only thing this actually skips is the ++it on > line 103? So it would be equivalent just to remove ++ on the previous line and > remove this? > > If so, it seems good to do that, since it would be simpler. The code is a bit subtle. When I call map::erase(), the current iterator is no longer valid. i.e. it = map.begin(); map.erase(it); ++it; does not work. What I'm actually doing is: it = map.begin(); it2 = it; ++it2; map.erase(it); it = it2; but just not as verbose. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:110: const MountPoint& mount_point(newiter->first); On 2012/03/23 22:47:39, ajl wrote: > It is true that the style guide allows using either constructor or > assignment-style notation for assigning pointers, references, and primitive > values, but there is a pretty overwhelming preference in google code for using > the assignment style instead. > > In addition to following convention, another good reason for this style is that > it clearly distinguishes in notation between simple primitive assignment and > instantiating class-type objects by "calling" a constructor. > > So, I would recommend changing to using assignment notation consistently for > initializing references in this code. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:112: const MountDevice& mount_device(newiter->second.first); On 2012/03/23 22:47:39, ajl wrote: > Why not use the temporary you created on the previous line here? Same below. Sure, but not for the next one. It's not const and I actually want to write to it. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:119: mtab_[mount_point] = mount_device_and_id; On 2012/03/23 22:47:39, ajl wrote: > Can you use insert() here rather than the [] operator? There is a preference in > google code against using [] for maps when possible (see comment near the top of > util/gtl/map-util.h for an explanation). > > If you don't have access to map-util.h in chrome, there are some cases where > using [] might be ok where it would not be in google3 code. It seems like since > this is just inserting, and not relying on default initialization behavior or > overwriting existing values, that insert() should work pretty easily here, so I > think you should change it. > > Same in similar cases elsewhere. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:3: // found in the LICENSE file. On 2012/03/23 22:47:39, ajl wrote: > The style guide requires a descriptive file-level comment after the copyright > notice: > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co... > > Clearly for Chrome code, because of the different licensing, you can't follow > the standard google3 file comment format exactly, but it seems like you ought to > still be able to include a file description. > > Same in all other files. We don't identify the original author in Chromium, but I will add a description here. Though Chromium code almost never do this for .cc files, so I won't either. I'm not sure what I'd put there any way. "MediaDeviceNotificationsLinux implementation" and "MediaDeviceNotificationsLinux unit test" ? https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:20: #include "content/common/content_export.h" On 2012/03/23 22:47:39, ajl wrote: > Are all of these #includes actually necessary in this immediate file? The style > guide prefers avoiding unnecessary includes: > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co... > > And encourages the use of forward declarations instead of includes where > possible. Yes, except for FilePath, which I forward declared. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:37: typedef std::string MountDevice; On 2012/03/23 22:47:39, ajl wrote: > The use of a typedef here strikes me as superfluous and actually somewhat > harmful for readability. > > These typedefs are useful when the full type name is awkwardly long or > complicated, or when the typedef name gives some useful information about the > use of the type that the full name doesn't include. On the other hand, though, > by hiding the actual type, they create some additional burden on readers since > readers must consult the typedef definition to understand the actual type and > how it can be used, while if the real time is a well understood standard type if > there was no typedef they might not need to consult any external resources. > > In this case, I don't think the typedef makes the name usefully shorter (the > full name is fairly short), and I don't think it adds any useful information > either (a good variable name should make clear that string value contains a > mount device). It does, however, hide some important details that are useful to > readers - namely the actual type of the object (string). > > Based on that, the typedef here seems like a net negative and I would prefer if > you removed it. Same with MountPoint also, and in similar cases. > > I think MountDeviceAndId shortens the name enough that it seems good to keep, > however, and while MountMap is kind of borderline I am ok with keeping it also. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:45: class WatcherDelegate : public base::files::FilePathWatcher::Delegate { On 2012/03/23 22:47:39, ajl wrote: > Do you actually need the full definition of this class in the header file? > Since it is private it can't be used by external users and the only reference to > it in this file is in the declaration of a scoped_refptr. > > If it is like the standard google3 smart pointer classes, you really only need > the type declaration for the scoped_refptr. > > In that case, you could move the definition of the class to your .cc file and > only have a forward declaration in this header file. That would improve > readability of the header by making it shorter and reducing the amount of > extraneous (to external users) information, and also follow the general > principle of minimizing the extent and scope of all definitions. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:56: MediaDeviceNotificationsLinux* notifier_; On 2012/03/23 22:47:39, ajl wrote: > Is this owned or not? If owned, when is it deleted? If not owned, who does own > it? > > There should be a comment answering those questions here. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.h:61: virtual ~MediaDeviceNotificationsLinux(); On 2012/03/23 22:47:39, ajl wrote: > For me, it would have been useful to have a comment with these explaining why > the destructor is private (so it can only be deleted by the reference-counting > smart pointer I guess?). > > Maybe this is completely obvious to everyone in chrome, but if there is any > possibility of confusion a comment seems good. Done. Ya, it seems everyone just does this without a comment. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:10: On 2012/03/23 22:47:39, ajl wrote: > The style guide now prefers *not* to have an empty line separating C and C++ > standard library includes, see code snippets here: > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In... > > Note that they do still need to be grouped separately, there should just not be > any empty line between the groups. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:21: #include "content/browser/media_device_notifications_linux.h" On 2012/03/23 22:47:39, ajl wrote: > This actually needs to be the very first file included here (note the reference > to testing in the linked style guide section): > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In... Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:44: class MediaDeviceNotificationsLinuxTest : public testing::Test { On 2012/03/23 22:47:39, ajl wrote: > Can you put this test code inside an anonymous namespace? It is preferred if > possible to minimize the scope of the test code, since it shouldn't be > referenced externally anyway. > > Usually the only reason you can't do this is if some test class needs to be a > friend of the class it is testing. I don't think you are doing that. > > Note that you can keep your tests inside your project namespace to have > unqualified access to project names - just use an anonymous namespace *inside* > your project namespace. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:47: MtabTestData(const char* mount_device, On 2012/03/23 22:47:39, ajl wrote: > It seems like it could make this code simpler and possibly safer if you passed > and stored these as C++ strings rather than const char* (the usual way would be > to make the arguments references and copy them to non-reference data members in > the constructor). > > This would eliminate various tricky details around const char* (NULL, ownership > of the data, length, lack of built in methods for common operations) and also > better follow google convention. > > The down side is that it would involve more copying and you would have to > manually convert to const char* when passing as arguments to code that requires > that - I don't think those are very serious problems compared to the advantages, > especially in a test. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:63: system_monitor_.reset(new base::SystemMonitor()); On 2012/03/23 22:47:39, ajl wrote: > Why not do this in the initializer list, instead of resetting in in the > constructor body? > > (although this will be moot if you follow my suggestion and change to a default > data member) It is indeed moot. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:78: struct MtabTestData initial_test_data[] = { On 2012/03/23 22:47:39, ajl wrote: > C++ does not require prepending 'struct' to the name of struct types, and since > this is consistent with how class names are referenced, not including them is > the preference and the overwhelming convention in google code. > > So please change this to just 'MtabTestData initial...'. > > Same in similar cases elsewhere. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:123: return *mock_devices_changed_observer_.get(); On 2012/03/23 22:47:39, ajl wrote: > Is get() needed here? Most smart pointer classes have * overriden to > dereference the wrapped pointer and using that (without get()) is quite > conventional in google code. > > (it is more standard to use get(), even when not strictly necessary, when > accessing the pointer value directly) No, it's not, and I've already removed it in the CL that fixes the test flakiness. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:131: void WriteToMtab(struct MtabTestData* data, On 2012/03/23 22:47:39, ajl wrote: > Can data be const? The style guide prefers variables and parameters to be > declared const whenever possible: > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Use_of_const#Use_of_c... Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:142: entry.mnt_fsname = strdup(data[i].mount_device); On 2012/03/23 22:47:39, ajl wrote: > Why does the string need to be copied here? Is there any reason you can't just > copy the pointer from the original location, given it seems to be in the right > format (null terminated string) already and will live longer than this copy? > > It would be nice to get rid of this strdup() if so, especially since it would > eliminate the need for C-style free() calls below (very unusual in google code). The mntent struct has char* for these fields, whereas the MtabTestData struct members, converted to std::string, will give a const char* with .c_str(). https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:158: ASSERT_GE(fd, 0); On 2012/03/23 22:47:39, ajl wrote: > Just FYI: the gunit assertion macros (I'm assuming whatever the Chrome codebase > has is similar) treat the first argument as the 'expected' value and the second > as the 'actual'. This doesn't really affect the correctness of the test, since > equality is a symmetric operator, but it does affect the error messages that are > printed when assertions fail: If you put the arguments in the opposite order, > the test will say 'expected result B, actual was A' when it actually should be > the opposite. > > So, to make the test output nicer, you might want to switch around the order of > arguments here and elsewhere. I've been pretty good about the _EQ macros, but I tend to forget with the others, since I think of it as |fd| >= 0, rather than 0 <= |fd|. However, this particular case got removed in the flakiness test fix CL. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:160: int fsync_result = fsync(fd); On 2012/03/23 22:47:39, ajl wrote: > Are you sure this temporary is necessary? Sometimes having a variable like this > can, through its name, give some additional useful information, but I think here > it would be easy enough to understand if you eliminated it and just moved the > fsync call instead the ASSERT. > > Same with close_result below, and other similar cases. This particular instance got removed in the follow up CL that fixed test flakiness. I fixed other similar cases. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:172: scoped_ptr<base::SystemMonitor> system_monitor_; On 2012/03/23 22:47:39, ajl wrote: > Does this really need to be allocated on the heap, or could you change this to a > direct data member, instead of a scoped_ptr? > > If so that is simpler and could be a little more efficient. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:185: TEST_F(MediaDeviceNotificationsLinuxTest, BasicAttachDetach) { On 2012/03/23 22:47:39, ajl wrote: > It would be helpful to readers if you added a short comment before each test > method explaining what it tests in a little more detail than is offered by just > the name. Done. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:187: FilePath test_path = CreateMountPoint(kMountPointA, true); On 2012/03/23 22:47:39, ajl wrote: > When you pass literals like true as function arguments, you need to have a short > inline comment next to them indicating what they mean. See here (under "NULL, > true/false, 1, 2, 3...") for the recommended style: > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Implementation_Commen... > > Same elsewhere It's a bit tedious to repeat the same comment for all instances. Instead, I just wrote methods with better names to wrap the method that takes true/false. https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:193: EXPECT_CALL(observer(), OnMediaDeviceAttached(0, kDevice2, test_path)) On 2012/03/23 22:47:39, ajl wrote: > Please add a comment explaining what this actually does. I can't tell to be > honest. In general gmock syntax is incredibly opaque to readers, so some kind > of comment is necessary. > > Same in similar cases elsewhere. Done. The more complicated cases are already commented.
Great job with your revisions, this code is coming along very well. I have a few more comments below from looking over the code again, in addition to my replies in the other message. Sorry for the long wait. Andrew https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... File content/browser/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:58: // No need to reference it, as that would create a circular reference. What do you mean by "reference" here - are you referring to reference counts? If so, that should be stated explicitly. In purely literal terms this pointer *does* reference the MediaDeviceNotificationsLinux instance (it points or refers to it), which makes the comment confusing. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:101: // This cannot happen unless FileWatcher is buggy. Thanks for adding a comment here. I think it would be useful to indicate what exactly NOTREACHED does as well as why you call it. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:196: device_map[entry.mnt_fsname] = Here is another place where it looks like you could use insert() instead of []. Can you place check through and update all of these if possible? https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... File content/browser/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.h:39: // Avoids code deleting the object while there are references to it. I think it would be better in this to explicitly mention how this prevents deletion except by the reference counted pointer class, i.e. explain *how* making the destructor protected avoids deleting the objects while references exist. Otherwise the connection isn't clear. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:134: 0); // NO data length. Does 'no' really need to be all caps? https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:140: FilePath CreateMountPointWithDCIMDir(const char* dir) { Similar to a previous comment, can 'dir' be passed as a C++ string to this method (and all the other similar methods)? https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:250: FilePath test_pathB = CreateMountPointWithoutDCIMDir(kMountPointB); Based on the standard variable naming rules, which require variable names to be all lowercase with underscores separating words: http://www.corp.google.com/eng/doc/cppguide.xml?showone=Variable_Names#Variab... This should be 'test_path_b'. Same elsewhere. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:331: // More complicated test case with multiple devices on one mount points. ...one mount point.
The comments from the email did not show up here. I've included the ones I replied to below. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... File content/browser/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:4: > The style guide does require this for all files. Its common to omit them for .cc files in google3 code also, but that does not mean it is allowed by the style guide. Your examples would be acceptable if there is no more implementation-specific detail that you think is needed. Done. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:58: // No need to reference it, as that would create a circular reference. On 2012/04/03 23:35:57, ajl wrote: > What do you mean by "reference" here - are you referring to reference counts? > If so, that should be stated explicitly. > > In purely literal terms this pointer *does* reference the > MediaDeviceNotificationsLinux instance (it points or refers to it), which makes > the comment confusing. Done. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:101: // This cannot happen unless FileWatcher is buggy. On 2012/04/03 23:35:57, ajl wrote: > Thanks for adding a comment here. I think it would be useful to indicate what > exactly NOTREACHED does as well as why you call it. Done. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:138: mtab_.erase(it++); > Yikes. I think it would be worthwhile actually to do this the more verbose way, and include a comment that erase() invalidates the iterator. I might even favor something more pedantic and obvious (like moving all modification of mtab_ out of this loop - so you would build up a list of ids/iterators/etc. to delete inside the loop, then actually delete them in a separate pass afterward. Ok, I'll put all the erase() calls in a separate loop below. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.cc:196: device_map[entry.mnt_fsname] = On 2012/04/03 23:35:57, ajl wrote: > Here is another place where it looks like you could use insert() instead of []. > > Can you place check through and update all of these if possible? Done. I believe this is the only other one. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... File content/browser/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux.h:39: // Avoids code deleting the object while there are references to it. On 2012/04/03 23:35:57, ajl wrote: > I think it would be better in this to explicitly mention how this prevents > deletion except by the reference counted pointer class, i.e. explain *how* > making the destructor protected avoids deleting the objects while references > exist. Otherwise the connection isn't clear. Done. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:134: 0); // NO data length. On 2012/04/03 23:35:57, ajl wrote: > Does 'no' really need to be all caps? Nope. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:140: FilePath CreateMountPointWithDCIMDir(const char* dir) { On 2012/04/03 23:35:57, ajl wrote: > Similar to a previous comment, can 'dir' be passed as a C++ string to this > method (and all the other similar methods)? Done. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:183: entry.mnt_opts = strdup("rw"); > Eek! > > So I still think it would still be good to try to use a safe copy function with bounds checking, and also use C++ allocation. There is a google library function strndup_with_new (from strings/util.h) that gives you both - if you have that in chrome it would be good to use it. Even better would be combining with a scoped_array (like scoped_ptr for arrays, also in scoped_ptr.h) to eliminate the need for explicit deleting, if you have that in chrome also. In other words something like: > > scoped_array<char> chars(strndump_with_new(string.c_str(), string.size())); > > If you don't have strndup_with_new it would still be better (IMHO) to manually allocate a char[] using new, then copy (with bounds checking) using strncpy, although I know this would be longer - maybe you could wrap this in a helper function? We don't have strndump_with_new(). I added a similar function along with scoped_array. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:250: FilePath test_pathB = CreateMountPointWithoutDCIMDir(kMountPointB); On 2012/04/03 23:35:57, ajl wrote: > Based on the standard variable naming rules, which require variable names to be > all lowercase with underscores separating words: > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Variable_Names#Variab... > > This should be 'test_path_b'. Same elsewhere. Done. https://chromiumcodereview.appspot.com/9622020/diff/3002/content/browser/medi... content/browser/media_device_notifications_linux_unittest.cc:331: // More complicated test case with multiple devices on one mount points. On 2012/04/03 23:35:57, ajl wrote: > ...one mount point. Done.
https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux_unittest.cc:10: On 2012/03/23 22:47:39, ajl wrote: > The style guide now prefers *not* to have an empty line separating C and C++ > standard library includes, see code snippets here: > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Names_and_Order_of_In... > > Note that they do still need to be grouped separately, there should just not be > any empty line between the groups. Ah, I asked mmentovai@ about this. This is a recent style change in google3, but Chromium started with an older edition of the google3 style guide and never picked up this rule. So Chromium code still prefers a blank line between the C and C++ includes. I'll keep this in mind when/if I work on google3 code. See https://chromiumcodereview.appspot.com/9838033/diff/33001/base/android/build_... for example.
Once again, excellent work with your revisions. I only have a couple more very minor comments below, and once these are addressed (and the #include blank line issue is settled) I think you will be finished. Thanks for your patience so far. Regarding the blank line issue, just to summarize my position: I think you should try to follow the current style guide rule even though if it is inconsistent with existing code, but if your team is very adamant about leaving as is it is ok with me to do so. Andrew https://chromiumcodereview.appspot.com/9622020/diff/13011/content/browser/med... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/13011/content/browser/med... content/browser/media_device_notifications_linux_unittest.cc:134: // Append to the mtab with |data| of |data_size| and run the message loop. Grammar for this comment is kind of awkward. Something like 'Append mtab entries from |data| to mtab, where |data| points to an array of size |data_size|, and...' seems better to me. Same in similar cases below. https://chromiumcodereview.appspot.com/9622020/diff/13011/content/browser/med... content/browser/media_device_notifications_linux_unittest.cc:289: MtabTestData(kDevice1, test_path_a.value().c_str(), kValidFS), This c_str() here is unnecessary now, right? Same in similar cases below.
I put in the blank line because my team is prefers that style in Chromium code. The important point is knowing the difference between the google3 and Chromium style guide. Just like knowing the distinction between a check and a cheque. https://chromiumcodereview.appspot.com/9622020/diff/13011/content/browser/med... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/13011/content/browser/med... content/browser/media_device_notifications_linux_unittest.cc:134: // Append to the mtab with |data| of |data_size| and run the message loop. On 2012/04/10 01:28:27, ajl wrote: > Grammar for this comment is kind of awkward. Something like 'Append mtab > entries from |data| to mtab, where |data| points to an array of size > |data_size|, and...' seems better to me. > > Same in similar cases below. Done. https://chromiumcodereview.appspot.com/9622020/diff/13011/content/browser/med... content/browser/media_device_notifications_linux_unittest.cc:289: MtabTestData(kDevice1, test_path_a.value().c_str(), kValidFS), On 2012/04/10 01:28:27, ajl wrote: > This c_str() here is unnecessary now, right? Same in similar cases below. Done.
There is still one more issue that is not quite addressed here. Once that is done, the review will be finished. Andrew https://chromiumcodereview.appspot.com/9622020/diff/18002/content/browser/med... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/18002/content/browser/med... content/browser/media_device_notifications_linux_unittest.cc:134: // Append mtab entries from the |data| array of |data_size| to the mtab file, This still isn't exactly right. You need to specify what property of |data| |data_size| specifies. If you phrase it this way you should change the wording to "Append mtab entries from the |data| array of size |data_size|...", or alternately use exactly the language I suggested originally. Same below.
https://chromiumcodereview.appspot.com/9622020/diff/18002/content/browser/med... File content/browser/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/18002/content/browser/med... content/browser/media_device_notifications_linux_unittest.cc:134: // Append mtab entries from the |data| array of |data_size| to the mtab file, On 2012/04/11 02:10:30, ajl wrote: > This still isn't exactly right. You need to specify what property of |data| > |data_size| specifies. > > If you phrase it this way you should change the wording to "Append mtab entries > from the |data| array of size |data_size|...", or alternately use exactly the > language I suggested originally. > > Same below. Done.
All right, everything looks good to me now. You're done. Congratulations, and thanks for your hard work!! I'll close the bug as fixed once you submit this changelist. Your readability review will be registered in perforce soon after that. Please don't submit this changelist immediately, though - you should also make sure to have a member of your team do a normal code review on this CL before submitting it. As I said to begin with, I've just reviewed this for readability and I don't have the domain knowledge of this project to sign off on this for functional correctness. I encourage you to take another look at any code you've submitted previously for issues similar to those addressed in this review, and correct any such issues you find. I don't know how or if I'll be notified when a Chromium change has been submitted, so please ping me once it has been submitted so I can close the bug. Congratulations again, Andrew
vandebo: Please review this CL. jam: content OWNERS review.
On 2012/04/11 20:26:02, Lei Zhang wrote: > vandebo: Please review this CL. > jam: content OWNERS review. lgtm
LGTM https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... File content/browser/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/9622020/diff/1/content/browser/media_d... content/browser/media_device_notifications_linux.cc:20: const char* const kDCIMDirName = "DCIM"; On 2012/03/23 22:47:39, ajl wrote: > There is a preference in google code to declare C string constants as char[] > rather than char*. It is slightly safer and more efficient, since the array > can't possibly be reassigned to point to something else and the offset is > completely known at compile time. > > Same elsewhere. ajl: For my benefit can you explain the decision here more? Various places on the internet claim that char foo[] = "constant" copies the string to the stack. That seems less efficient. https://chromiumcodereview.appspot.com/9622020/diff/28002/content/browser/med... File content/browser/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/9622020/diff/28002/content/browser/med... content/browser/media_device_notifications_linux.h:38: // Aside from the base::RefCountedThreadSafe friend class, and derived nit: I don't think derived classes can safely call the destructor, only RefCountedThreadSafe can.
https://chromiumcodereview.appspot.com/9622020/diff/28002/content/browser/med... File content/browser/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/9622020/diff/28002/content/browser/med... content/browser/media_device_notifications_linux.h:38: // Aside from the base::RefCountedThreadSafe friend class, and derived On 2012/04/11 21:45:16, vandebo wrote: > nit: I don't think derived classes can safely call the destructor, only > RefCountedThreadSafe can. MediaDeviceNotificationsLinuxTestWrapper needs to be able to call it as well. If you make this dtor private, you get: ./content/browser/media_device_notifications_linux.h: In constructor ‘content::{anonymous}::MediaDeviceNotificationsLinuxTestWrapper::MediaDeviceNotificationsLinuxTestWrapper(const FilePath&, MessageLoop*)’: ./content/browser/media_device_notifications_linux.h:46:11: error: ‘content::MediaDeviceNotificationsLinux::~MediaDeviceNotificationsLinux()’ is private content/browser/media_device_notifications_linux_unittest.cc:61:35: error: within this context
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/9622020/28002
Change committed as 131917 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
