Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11490010:
[Media Galleries] Introduce a new type for Mac Image Capture. (Closed)
|
Created:
8 years ago by Greg Billock Modified:
8 years ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Media Galleries] Introduce a new type for Mac Image Capture. (Part 1)
R=thestig@chromium.org
BUG=151681
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172910
Patch Set 1 #
Total comments: 14
Patch Set 2 : Add tests, fix up. #
Total comments: 6
Patch Set 3 : Fix include #
Total comments: 12
Patch Set 4 : Improve test #Patch Set 5 : Add testing coverage todo #Patch Set 6 : Soothing strings for windows #Patch Set 7 : Just ifdef windows away from path construction. #
Total comments: 4
Patch Set 8 : Fix FILE_PATH_LITERAL #
Messages
Total messages: 21 (0 generated)
Hi Greg, it's not clear if this is just a partial CL or something that is actually meant to go in. If it's ready to go in then it needs tests. https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... File chrome/browser/system_monitor/disk_info_mac.h (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, where is this used? https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:143: // NOTE: instead of all this parsing, why not just pass the type itself? this comment isn't clear, is this supposed to be a TODO? maybe rephrase? https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:221: if (!CrackDeviceId(device_id, &type, NULL)) return false; return on separate line
This is split off from a larger group of changes. It isn't ready to check in yet, though. Adding some tests and fixing the split issues now... https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... File chrome/browser/system_monitor/disk_info_mac.h (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, Nowhere yet. This is part 1 of about 4. On 2012/12/10 19:38:48, sail wrote: > where is this used? https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:143: // NOTE: instead of all this parsing, why not just pass the type itself? Removing. This was a note-to-self. On 2012/12/10 19:38:48, sail wrote: > this comment isn't clear, is this supposed to be a TODO? > maybe rephrase? https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:221: if (!CrackDeviceId(device_id, &type, NULL)) return false; On 2012/12/10 19:38:48, sail wrote: > return on separate line Done.
https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... File chrome/browser/system_monitor/disk_info_mac.h (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, On 2012/12/10 19:47:37, Greg Billock wrote: > Nowhere yet. This is part 1 of about 4. > > On 2012/12/10 19:38:48, sail wrote: > > where is this used? > Ahh makes sense! Mayb add "Part 1 of 4" to the CL title?
https://codereview.chromium.org/11490010/diff/1/chrome/browser/media_gallery/... File chrome/browser/media_gallery/media_file_system_registry.cc (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/media_gallery/... chrome/browser/media_gallery/media_file_system_registry.cc:347: LOG(INFO) << "Getting MTP fs for " << path.value(); Remove these debugging statements? https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:76: bool IsMTPDeviceAttached(const std::string& id) { Isn't this function really "IsDeviceAttached()" ? https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:76: bool IsMTPDeviceAttached(const std::string& id) { Can this just call FindRemovableStorageLocationById() and see if the return value is empty? Are you special-casing this because the ImageCapture devices have no location? https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:226: return path.IsAbsolute() && path.ReferencesParent(); the second part is "not path.ReferencesParent()"
ok, added test and fixed up. https://codereview.chromium.org/11490010/diff/1/chrome/browser/media_gallery/... File chrome/browser/media_gallery/media_file_system_registry.cc (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/media_gallery/... chrome/browser/media_gallery/media_file_system_registry.cc:347: LOG(INFO) << "Getting MTP fs for " << path.value(); On 2012/12/10 21:53:41, Lei Zhang wrote: > Remove these debugging statements? Done. https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:76: bool IsMTPDeviceAttached(const std::string& id) { On 2012/12/10 21:53:41, Lei Zhang wrote: > Can this just call FindRemovableStorageLocationById() and see if the return > value is empty? Are you special-casing this because the ImageCapture devices > have no location? I think that's what I was doing. I didn't want to add a location (they really have none), but there ends up being too much reliance on that. I think in the end that's a really compelling indication that this (overloading location with bogus data) is a bad plan we should move away from. There's tons of baked-in assumptions about this location, virtually all of which are based on false premises for MTP devices. It leaves things very fragile -- anyone dealing with that location will be justified in doing something like checking for path existence or something, which will break the code. https://codereview.chromium.org/11490010/diff/1/chrome/browser/system_monitor... chrome/browser/system_monitor/media_storage_util.cc:226: return path.IsAbsolute() && path.ReferencesParent(); On 2012/12/10 21:53:41, Lei Zhang wrote: > the second part is "not path.ReferencesParent()" Yes, I'd noticed this up in my client. I don't remember why this was backwards, but now repaired.
https://codereview.chromium.org/11490010/diff/3002/chrome/browser/system_moni... File chrome/browser/system_monitor/media_storage_util_unittest.cc (right): https://codereview.chromium.org/11490010/diff/3002/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util_unittest.cc:55: EXPECT_TRUE(MediaStorageUtil::CanCreateFileSystem("ic:xyz", FilePath())); maybe also add a negative test for this? https://codereview.chromium.org/11490010/diff/3002/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util_unittest.cc:58: void SignalEvent(base::WaitableEvent* event) { move this to the anonymous namespace above? https://codereview.chromium.org/11490010/diff/3002/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util_unittest.cc:79: base::Bind(&SignalEvent, &event)); would base::Bind(&base::WaitableEvent::SignalEvent, &event) work? https://codereview.chromium.org/11490010/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/disk_info_mac.h (right): https://codereview.chromium.org/11490010/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, exercise this in unit tests?
https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:117: //static nit: space after slash https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/media_storage_util_unittest.cc (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:49: EXPECT_TRUE(MediaStorageUtil::CrackDeviceId("ic:xyz", &type, &id)); nit: replace "ic:xyz" with a constant. It's used all over the place in this test file. https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:85: "ic:xyz", ASCIIToUTF16("name"), "/location"); |location| needs to be FILE_PATH_LITERAL("/location") to build on Windows.
https://chromiumcodereview.appspot.com/11490010/diff/3002/chrome/browser/syst... File chrome/browser/system_monitor/media_storage_util_unittest.cc (right): https://chromiumcodereview.appspot.com/11490010/diff/3002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:55: EXPECT_TRUE(MediaStorageUtil::CanCreateFileSystem("ic:xyz", FilePath())); On 2012/12/10 23:06:36, sail wrote: > maybe also add a negative test for this? Done https://chromiumcodereview.appspot.com/11490010/diff/3002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:58: void SignalEvent(base::WaitableEvent* event) { On 2012/12/10 23:06:36, sail wrote: > move this to the anonymous namespace above? Done. https://chromiumcodereview.appspot.com/11490010/diff/3002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:79: base::Bind(&SignalEvent, &event)); On 2012/12/10 23:06:36, sail wrote: > would base::Bind(&base::WaitableEvent::SignalEvent, &event) work? Done. https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, On 2012/12/10 23:06:36, sail wrote: > exercise this in unit tests? I'll see about doing this later. I don't think a direct test of this will mean much. https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:117: //static On 2012/12/10 23:21:53, Lei Zhang wrote: > nit: space after slash Done. https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/media_storage_util_unittest.cc (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:49: EXPECT_TRUE(MediaStorageUtil::CrackDeviceId("ic:xyz", &type, &id)); On 2012/12/10 23:21:53, Lei Zhang wrote: > nit: replace "ic:xyz" with a constant. It's used all over the place in this test > file. Done. https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:49: EXPECT_TRUE(MediaStorageUtil::CrackDeviceId("ic:xyz", &type, &id)); On 2012/12/10 23:21:53, Lei Zhang wrote: > nit: replace "ic:xyz" with a constant. It's used all over the place in this test > file. Done. https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/media_storage_util_unittest.cc:85: "ic:xyz", ASCIIToUTF16("name"), "/location"); Good catch. Done. On 2012/12/10 23:21:53, Lei Zhang wrote: > |location| needs to be FILE_PATH_LITERAL("/location") to build on Windows.
https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, On 2012/12/11 00:11:22, Greg Billock wrote: > On 2012/12/10 23:06:36, sail wrote: > > exercise this in unit tests? > > I'll see about doing this later. I don't think a direct test of this will mean > much. Could you add a TODO? It would be good to have good test coverage even if all it does it call the function and not assert anything.
https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, OK. I do think this'll get tested, but I don't think a test file for this method alone is very valuable -- it'd essentially be a compiler test. On 2012/12/11 00:14:01, sail wrote: > On 2012/12/11 00:11:22, Greg Billock wrote: > > On 2012/12/10 23:06:36, sail wrote: > > > exercise this in unit tests? > > > > I'll see about doing this later. I don't think a direct test of this will mean > > much. > > Could you add a TODO? It would be good to have good test coverage even if all it > does it call the function and not assert anything.
https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac BuildDiskInfoFromICDevice(std::string device_id, On 2012/12/11 01:02:26, Greg Billock wrote: > OK. I do think this'll get tested, but I don't think a test file for this method > alone is very valuable -- it'd essentially be a compiler test. > > On 2012/12/11 00:14:01, sail wrote: > > On 2012/12/11 00:11:22, Greg Billock wrote: > > > On 2012/12/10 23:06:36, sail wrote: > > > > exercise this in unit tests? > > > > > > I'll see about doing this later. I don't think a direct test of this will > mean > > > much. > > > > Could you add a TODO? It would be good to have good test coverage even if all > it > > does it call the function and not assert anything. > The disk info stuff is exercised in removable_device_notifications_mac_unittest.mm. No new file needed.
Yes. This function will get exercised in the same way, I anticipate, when I add a test for the ImageCapture code in a subsequent test. In the meantime, I don't see it as a big code risk; the function is trivial, unlike the other factory method there. On Tue, Dec 11, 2012 at 10:40 AM, <sail@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... > File chrome/browser/system_monitor/disk_info_mac.h (right): > > https://chromiumcodereview.appspot.com/11490010/diff/5002/chrome/browser/syst... > chrome/browser/system_monitor/disk_info_mac.h:26: static DiskInfoMac > BuildDiskInfoFromICDevice(std::string device_id, > On 2012/12/11 01:02:26, Greg Billock wrote: >> >> OK. I do think this'll get tested, but I don't think a test file for > > this method >> >> alone is very valuable -- it'd essentially be a compiler test. > > >> On 2012/12/11 00:14:01, sail wrote: >> > On 2012/12/11 00:11:22, Greg Billock wrote: >> > > On 2012/12/10 23:06:36, sail wrote: >> > > > exercise this in unit tests? >> > > >> > > I'll see about doing this later. I don't think a direct test of > > this will >> >> mean >> > > much. >> > >> > Could you add a TODO? It would be good to have good test coverage > > even if all >> >> it >> > does it call the function and not assert anything. > > > > The disk info stuff is exercised in > removable_device_notifications_mac_unittest.mm. No new file needed. > > https://chromiumcodereview.appspot.com/11490010/
lgtm Getting FilePaths right is always a pain. https://codereview.chromium.org/11490010/diff/21002/chrome/browser/system_mon... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11490010/diff/21002/chrome/browser/system_mon... chrome/browser/system_monitor/media_storage_util.cc:350: return FilePath(kRootPath + device_id); I think if you make kRootPath a FilePath::StringType, then you can write: return FilePath(kRootPath).AppendASCII(device_id);
https://codereview.chromium.org/11490010/diff/21002/chrome/browser/system_mon... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11490010/diff/21002/chrome/browser/system_mon... chrome/browser/system_monitor/media_storage_util.cc:350: return FilePath(kRootPath + device_id); Yeah, I was looking at that, and I think the device ID will be ascii, but I didn't want to bake in that assumption. Ultimately, I think this'll be fixed if we can move to a metadata struct that can carry more information so we aren't faking a path, or when we are, it's one we know we're faking. On 2012/12/12 20:21:27, Lei Zhang wrote: > I think if you make kRootPath a FilePath::StringType, then you can write: > return FilePath(kRootPath).AppendASCII(device_id);
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11490010/21002
Sorry for I got bad news for ya. Compile failed with a clobber build on win_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... 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.
https://chromiumcodereview.appspot.com/11490010/diff/21002/chrome/browser/sys... File chrome/browser/system_monitor/media_storage_util_unittest.cc (right): https://chromiumcodereview.appspot.com/11490010/diff/21002/chrome/browser/sys... chrome/browser/system_monitor/media_storage_util_unittest.cc:60: FilePath("relative"))); needs more FILE_PATH_LITERAL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11490010/30001
https://chromiumcodereview.appspot.com/11490010/diff/21002/chrome/browser/sys... File chrome/browser/system_monitor/media_storage_util_unittest.cc (right): https://chromiumcodereview.appspot.com/11490010/diff/21002/chrome/browser/sys... chrome/browser/system_monitor/media_storage_util_unittest.cc:60: FilePath("relative"))); Oops. Yes. Forget about this spot... On 2012/12/13 01:40:16, Lei Zhang wrote: > needs more FILE_PATH_LITERAL.
Message was sent while issue was closed.
Change committed as 172910 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
