Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11442047:
Media Galleries: Add more tests for media gallery names. (Closed)
|
Created:
8 years ago by Lei Zhang Modified:
8 years ago CC:
chromium-reviews Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionMedia Galleries: Add more tests for media gallery names.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174126
Patch Set 1 : #
Total comments: 7
Patch Set 2 : fix nits #Patch Set 3 : #Patch Set 4 : #
Total comments: 14
Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Messages
Total messages: 19 (0 generated)
kmadhusu: This is what all the yak shaving was for. khorimoto: This is what you really want, right?
https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... File chrome/browser/media_gallery/media_file_system_registry.cc (right): https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... chrome/browser/media_gallery/media_file_system_registry.cc:337: "isRemovable", MediaStorageUtil::IsRemovableDevice(device_id)); I am not sure why we need a new key value pair to identify whether the gallery is removable or not. Can't we easily find that information with the existing key-values from the json string? If (json string has "deviceId" value) Current gallery belongs to removable device https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... File chrome/browser/media_gallery/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:159: for (size_t i = 0; i < file_systems.size(); i++) { nit: ++i https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:713: "mtp_fake_id", style nit: Fix indentation. https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:742: for (size_t i = 0; i < intersection_names.size(); i++) { nit: ++i
https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... File chrome/browser/media_gallery/media_file_system_registry.cc (right): https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... chrome/browser/media_gallery/media_file_system_registry.cc:337: "isRemovable", MediaStorageUtil::IsRemovableDevice(device_id)); On 2012/12/12 17:45:02, kmadhusu wrote: > I am not sure why we need a new key value pair to identify whether the gallery > is removable or not. > > Can't we easily find that information with the existing key-values from the json > string? > > If (json string has "deviceId" value) > Current gallery belongs to removable device That's a good point. We can discuss this on the bug. https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... File chrome/browser/media_gallery/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:159: for (size_t i = 0; i < file_systems.size(); i++) { On 2012/12/12 17:45:02, kmadhusu wrote: > nit: ++i The rest of the file had i++, so I was being consistent, but I might as well fix it all. https://codereview.chromium.org/11442047/diff/3001/chrome/browser/media_galle... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:713: "mtp_fake_id", On 2012/12/12 17:45:02, kmadhusu wrote: > style nit: Fix indentation. Done.
Patch set 4 tests all the cases in https://code.google.com/p/chromium/issues/detail?id=163494#c7
https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:166: nit: remove a blank line. https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:180: &device_id)) << name; style nit: Add one more space before &device_id https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:185: EXPECT_TRUE(dict->HasKey(MediaFileSystemRegistry::kGalleryIdKey)) << name; Can we also make sure !gallery_id.empty()? https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:191: &gallery_name)) << name; style nit: Add one more space before &gallery_name https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:198: &actual_user_added)) << name; style nit: Add one more space before &gallery_name https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:319: const std::set<std::string>& gallery_names, nit: std::set<std::string> is used at several places. For better readability, can we add typedef? https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:521: base::Bind(&GetGalleryNamesCB, base::Unretained(&results))); Just curious about the "GetGalleryNamesCB", Are we allowed to use abbreviations in the function name (Callback => CB)?
Kyle@: Can you explain the use case for this bug? Thanks.
https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:166: On 2012/12/14 03:29:39, kmadhusu wrote: > nit: remove a blank line. Done. https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:180: &device_id)) << name; On 2012/12/14 03:29:39, kmadhusu wrote: > style nit: Add one more space before &device_id Done. https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:185: EXPECT_TRUE(dict->HasKey(MediaFileSystemRegistry::kGalleryIdKey)) << name; On 2012/12/14 03:29:39, kmadhusu wrote: > Can we also make sure !gallery_id.empty()? galleryId is a number. https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:191: &gallery_name)) << name; On 2012/12/14 03:29:39, kmadhusu wrote: > style nit: Add one more space before &gallery_name Done. https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:198: &actual_user_added)) << name; On 2012/12/14 03:29:39, kmadhusu wrote: > style nit: Add one more space before &gallery_name Done. https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:319: const std::set<std::string>& gallery_names, On 2012/12/14 03:29:39, kmadhusu wrote: > nit: std::set<std::string> is used at several places. For better readability, > can we add typedef? I don't see what's so hard to understand about a set of strings. https://codereview.chromium.org/11442047/diff/13001/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:521: base::Bind(&GetGalleryNamesCB, base::Unretained(&results))); On 2012/12/14 03:29:39, kmadhusu wrote: > Just curious about the "GetGalleryNamesCB", Are we allowed to use abbreviations > in the function name (Callback => CB)? Renamed.
On 2012/12/14 03:32:06, kmadhusu wrote: > Kyle@: Can you explain the use case for this bug? > > Thanks. Apps need the ability to distinguish between four types of galleries: local folders users have added, local folders that come as default galleries, devices that that come as default galleries (e.g., cameras, SD cards), and galleries that users have added on devices. This CL allows these 4 cases to be distinguished.
On 2012/12/14 18:21:51, Kyle Horimoto wrote: > On 2012/12/14 03:32:06, kmadhusu wrote: > > Kyle@: Can you explain the use case for this bug? > > > > Thanks. > > Apps need the ability to distinguish between four types of galleries: local > folders users have added, local folders that come as default galleries, devices > that that come as default galleries (e.g., cameras, SD cards), and galleries > that users have added on devices. This CL allows these 4 cases to be > distinguished. I understand why you want to know which galleries are on removable media - so that an app can copy that stuff as quickly as possible, but why would an app need to know the difference between default and user added galleries? One way of thinking of it is that as soon as the user has viewed the permission dialog they've explicitly allowed all the currently enabled galleries, i.e. they are all user added.
On 2012/12/14 18:30:05, vandebo wrote: > On 2012/12/14 18:21:51, Kyle Horimoto wrote: > > On 2012/12/14 03:32:06, kmadhusu wrote: > > > Kyle@: Can you explain the use case for this bug? > > > > > > Thanks. > > > > Apps need the ability to distinguish between four types of galleries: local > > folders users have added, local folders that come as default galleries, > devices > > that that come as default galleries (e.g., cameras, SD cards), and galleries > > that users have added on devices. This CL allows these 4 cases to be > > distinguished. > > I understand why you want to know which galleries are on removable media - so > that an app can copy that stuff as quickly as possible, but why would an app > need to know the difference between default and user added galleries? One way > of thinking of it is that as soon as the user has viewed the permission dialog > they've explicitly allowed all the currently enabled galleries, i.e. they are > all user added. Some apps will need to know the difference between a known media device (i.e., a camera or SD card) and a removable device that simply has media on it (e.g., a USB flash drive that has an image on it). Lei has told me that the simplest way to provide such a distinction would be to supply a "userAdded" flag.
It sounds like we still don't know what we really really want. I'm just going to use this CL to add more tests, sans the userAdded bits. PTAL.
lgtm https://codereview.chromium.org/11442047/diff/13003/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/11442047/diff/13003/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:805: AttachDevice(MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM, nit: indent 4 spaces. https://codereview.chromium.org/11442047/diff/13003/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:809: AddUserGallery(MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM, nit: As per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi..., "AddUserGallery(" will be in previous line.
https://codereview.chromium.org/11442047/diff/13003/chrome/browser/media_gall... File chrome/browser/media_gallery/media_file_system_registry_unittest.cc (right): https://codereview.chromium.org/11442047/diff/13003/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:805: AttachDevice(MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM, On 2012/12/15 02:46:06, kmadhusu wrote: > nit: indent 4 spaces. Done. https://codereview.chromium.org/11442047/diff/13003/chrome/browser/media_gall... chrome/browser/media_gallery/media_file_system_registry_unittest.cc:809: AddUserGallery(MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM, On 2012/12/15 02:46:06, kmadhusu wrote: > nit: As per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi..., > "AddUserGallery(" will be in previous line. This is acceptable style and is consistent with the rest of the file. i.e. line 432, 434, 729.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/11442047/8004
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
PTAL at patch set 8. After I fixed the ScopedMTPDeviceMapEntry race, and fixed the cross platform issues, these tests should work reliably on all platforms now. https://chromiumcodereview.appspot.com/11442047/diff/47001/chrome/browser/med... File chrome/browser/media_gallery/scoped_mtp_device_map_entry.cc (right): https://chromiumcodereview.appspot.com/11442047/diff/47001/chrome/browser/med... chrome/browser/media_gallery/scoped_mtp_device_map_entry.cc:28: scoped_refptr<base::SequencedTaskRunner> GetSequencedTaskRunner() { If I don't do this, I get a crash in debug mode: FATAL:ref_counted.cc(74)] Check failed: !in_dtor_. #0 base::debug::BreakDebugger() ../../base/debug/debugger_posix.cc:256 #1 logging::LogMessage::~LogMessage (this=0x7fffffffc5d0, __in_chrg=<optimized out>) at ../../base/logging.cc:649 #2 base::subtle::RefCountedThreadSafeBase::AddRef (this=0x7fffe8ee0578) at ../../base/memory/ref_counted.cc:74 #3 base::RefCountedThreadSafe<base::TaskRunner, base::TaskRunnerTraits>::AddRef (this=0x7fffe8ee0578) at ../../base/memory/ref_counted.h:137 #4 scoped_refptr<base::SequencedTaskRunner>::scoped_refptr (this=0x7fffffffc830, p=0x7fffe8ee0570) at ../../base/memory/ref_counted.h:234 #5 chrome::ScopedMTPDeviceMapEntry::ScopedMTPDeviceMapEntry (this=0x7fffe8ee0630, device_location="/mtp_bogus", on_destruction_callback=...) at ../../chrome/browser/media_gallery/scoped_mtp_device_map_entry.cc:47
lgtm. Thanks for fixing it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/11442047/47001
Message was sent while issue was closed.
Change committed as 174126 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
