Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
285433004:
Have FindContainerScanResults() find broader media container directories (Closed)
|
Created:
6 years, 7 months ago by Kevin Bailey Modified:
6 years, 6 months ago Reviewers:
vandebo (ex-Chrome) CC:
chromium-reviews, Lei Zhang, tommycli, Greg Billock Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionHave FindContainerScanResults() attempt to find media "container" directories as parents of found media folders with sufficient density (which gives us confidence that it is a media container.) This version looks beyond the immediate parents, further up the directory tree.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275264
Patch Set 1 : General idea compiles #Patch Set 2 : Much much better version #Patch Set 3 : Removed unused template function #Patch Set 4 : Minor clean up #Patch Set 5 : Missed a removal #Patch Set 6 : Remove unneeded local variable #
Total comments: 14
Patch Set 7 : Include and forward the original media counts #Patch Set 8 : Keeping original scan results #
Total comments: 13
Patch Set 9 : Keep better state of candidates #
Total comments: 24
Patch Set 10 : Stop removing children #Patch Set 11 : Simplified some variables #Patch Set 12 : Unit tests (and fixes they found) #
Total comments: 16
Patch Set 13 : New test and comment changes #Patch Set 14 : Forgot/added helper function #
Total comments: 6
Patch Set 15 : More liberal inclusion. Changed tests. #
Total comments: 13
Patch Set 16 : Moved anonymous function. Added comments. #
Total comments: 2
Patch Set 17 : More clear comments #Patch Set 18 : Exclude candidates with sole child, add unit test for special case #Patch Set 19 : Depth first traversal #
Total comments: 11
Patch Set 20 : Simplification and recursion #Patch Set 21 : Forgot comment #Patch Set 22 : Removed recursive fix-up, other responses #
Total comments: 3
Patch Set 23 : Even cleaner early out. Better named tests. #Patch Set 24 : Properly formatted #Patch Set 25 : Fixed other unit tests #
Messages
Total messages: 32 (0 generated)
https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:216: // immediate container directories. nit: remove "immediate" https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:227: // Count the number of scan results with the same parent directory. This comment seems wrong now https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:230: ContainerCandidates candidates; Having result, candidates, and interesting_candidates makes things hard to follow, can you come up with some better names to explain the difference? I wasn't able to work through this logic easily, so I stopped. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:237: // TODO: We could even keep the original audio_count, ... Container results should be reported with zero counts - It turns out that totalling up counts is profile specific, so can't be done at this level. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:241: candidates[it->first] = it->second.audio_count + it->second.image_count + Why do we need the counts of media files? https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:296: // We're keeping this parent. Remove all children. Which children to remove is Profile specific, so you can't do it here. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:524: AddScanResultsForProfile(preferences, container_folders); We need to add all the scan results - this is the only place results are added - not just container folders. If this is all the results (including the containers), then the variable should be renamed.
Restored original behavior of keeping scan results, but still recursively considering parent directories (slightly more memory efficiently than previous patch.) https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:216: // immediate container directories. On 2014/05/14 22:33:08, vandebo wrote: > nit: remove "immediate" Done. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:227: // Count the number of scan results with the same parent directory. On 2014/05/14 22:33:08, vandebo wrote: > This comment seems wrong now Better? https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:230: ContainerCandidates candidates; On 2014/05/14 22:33:08, vandebo wrote: > Having result, candidates, and interesting_candidates makes things hard to > follow, can you come up with some better names to explain the difference? I > wasn't able to work through this logic easily, so I stopped. I renamed to |candidates_to_check|, and commented each one. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:237: // TODO: We could even keep the original audio_count, ... On 2014/05/14 22:33:08, vandebo wrote: > Container results should be reported with zero counts - It turns out that > totalling up counts is profile specific, so can't be done at this level. Done. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:241: candidates[it->first] = it->second.audio_count + it->second.image_count + On 2014/05/14 22:33:08, vandebo wrote: > Why do we need the counts of media files? Gone. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:296: // We're keeping this parent. Remove all children. On 2014/05/14 22:33:08, vandebo wrote: > Which children to remove is Profile specific, so you can't do it here. Gone, but we do remove children that we've added. https://codereview.chromium.org/285433004/diff/100001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:524: AddScanResultsForProfile(preferences, container_folders); On 2014/05/14 22:33:08, vandebo wrote: > We need to add all the scan results - this is the only place results are added - > not just container folders. If this is all the results (including the > containers), then the variable should be renamed. Restored original code.
https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:227: // Find parent directories with majority of media directories, I think this change needs some unit testing. One layout that I think is important/likely that this doesn't handle the way we'd like is: Library/ Artist1/ Album1/ [bunch of songs] Artist2/ Album2/ [bunch of songs] Album3/ [bunch of songs] Album4/ [bunch of songs] Artist3/ Album5/ [bunch of songs] Album6/ [bunch of songs] Album7/ [bunch of songs] Artist4/ Album8/ [bunch of songs] Album9/ [bunch of songs] https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:252: if (it == candidates.end()) Confused... candidates starts emtpy, so we won't find anything in it, and always continue, so the code below here is dead. https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:261: result.erase(result.find(candidate)); Why erase it from result? Just don't add it to begin with. https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:273: result.erase(result.find(candidate)); ditto https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:291: for (base::FilePath name = dir_counter.Next(); This does a directory traversal on every increment above 1, which seems unnecessary. Maybe cache the value. https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:298: // We're keeping this parent. Remove any children. Why remove the children? They'll get taken care of in the following phase.
I'll add those unit tests when I've got the intended behavior correct. (Next patch?) https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:227: // Find parent directories with majority of media directories, On 2014/05/20 20:51:45, vandebo wrote: > I think this change needs some unit testing. One layout that I think is > important/likely that this doesn't handle the way we'd like is: > > Library/ > Artist1/ > Album1/ [bunch of songs] > Artist2/ > Album2/ [bunch of songs] > Album3/ [bunch of songs] > Album4/ [bunch of songs] > Artist3/ > Album5/ [bunch of songs] > Album6/ [bunch of songs] > Album7/ [bunch of songs] > Artist4/ > Album8/ [bunch of songs] > Album9/ [bunch of songs] Will do. Here's another one: A/ B/ D/ E/media F/media G/ H/media I/media C/media It should include A but exclude B, D and G. D and G are the tricky ones. However, perhaps I'm being too exclusive. More below. https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:252: if (it == candidates.end()) On 2014/05/20 20:51:45, vandebo wrote: > Confused... candidates starts emtpy, so we won't find anything in it, and always > continue, so the code below here is dead. arg, leftover cruft. Missed it in the diffs. https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:261: result.erase(result.find(candidate)); On 2014/05/20 20:51:45, vandebo wrote: > Why erase it from result? Just don't add it to begin with. In fact, these lines could have simply been removed from this patch, since it's not adding all the incoming ones any more. But beyond this, to avoid re-scanning at the end, the strategy was to add/remove as something became un/worthy. Now that it's caching results, it's trivial to do it at the end again. https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:273: result.erase(result.find(candidate)); On 2014/05/20 20:51:45, vandebo wrote: > ditto ack https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:291: for (base::FilePath name = dir_counter.Next(); On 2014/05/20 20:51:45, vandebo wrote: > This does a directory traversal on every increment above 1, which seems > unnecessary. Maybe cache the value. Done. https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:298: // We're keeping this parent. Remove any children. On 2014/05/20 20:51:45, vandebo wrote: > Why remove the children? They'll get taken care of in the following phase. Not positive which phase that you're referring to but I figured that this function should consolidate a media directory tree up to its root, no matter who will look at it. Again, perhaps it is being too exclusive. It's trivial to remove the |has_parent| stuff, if you like.
https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/140001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:298: // We're keeping this parent. Remove any children. On 2014/05/21 14:41:45, Kevin Bailey wrote: > On 2014/05/20 20:51:45, vandebo wrote: > > Why remove the children? They'll get taken care of in the following phase. > > Not positive which phase that you're referring to but I figured that this See PartitionChildScanResults() > function should consolidate a media directory tree up to its root, no matter who > will look at it. Again, perhaps it is being too exclusive. It's trivial to > remove the |has_parent| stuff, if you like. Yea, lets take that stuff out which will leave this aspect of operation unchanged. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:218: :seen_count(0), entries_count(-1), has_parent(false) {} nit: previous line, and ": " https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:242: // threshold to be returned, and whose parents should likewise be checked. nit: "...and whose parents still need to be checked." https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:264: candidates.erase(candidates.find(candidate)); Not needed - you only add to candidates after this check, so a sensitive folder will never get added. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:275: candidates.erase(candidates.find(candidate)); ditto https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:286: parent_it->second.seen_count++; If you like, use ContainerCount* parent_counts = &parent_it->second in place of parent_it->second https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:290: base::FileEnumerator dir_counter(parent_it->first, false /*recursive*/, parent_it->first => parent_directory https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:293: int count = 0; nit: you could use parent_it->second.entries_count directly. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:308: child_it->first.DirName() == parent_it->first) { parent_it->first => parent_directory https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:313: candidates_to_check.insert(parent_it->first); parent_it->first => parent_directory https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:320: if (it->second.seen_count > 1 && it->second.seen_count * 100 / Instead of duplicating the criteria here would it be better to add another boolean to ContainerCount ?
More tweaks and comments... https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:218: :seen_count(0), entries_count(-1), has_parent(false) {} On 2014/05/21 15:55:09, vandebo wrote: > nit: previous line, and ": " Done. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:242: // threshold to be returned, and whose parents should likewise be checked. On 2014/05/21 15:55:09, vandebo wrote: > nit: "...and whose parents still need to be checked." I was really trying to say that we would recur on the parents, and their relevant parents, but this is fine. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:264: candidates.erase(candidates.find(candidate)); On 2014/05/21 15:55:09, vandebo wrote: > Not needed - you only add to candidates after this check, so a sensitive folder > will never get added. I think this one was left-over but the next one is necessary. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:275: candidates.erase(candidates.find(candidate)); On 2014/05/21 15:55:09, vandebo wrote: > ditto I don't think so. Consider: A/ sensitive/ B/ C/media D/media E/ F/media G/media So we add B because it has media subdirs, then we add E likewise. Now, because A has 2 entries (pretend it reached 80%), we consider it and discover that it is sensitive. We should therefore remove it. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:286: parent_it->second.seen_count++; On 2014/05/21 15:55:09, vandebo wrote: > If you like, use ContainerCount* parent_counts = &parent_it->second in place of > parent_it->second Done. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:290: base::FileEnumerator dir_counter(parent_it->first, false /*recursive*/, On 2014/05/21 15:55:09, vandebo wrote: > parent_it->first => parent_directory Done. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:293: int count = 0; On 2014/05/21 15:55:09, vandebo wrote: > nit: you could use parent_it->second.entries_count directly. I generally prefer to write in an exception safe style, even when not needed, in order to better remember during those times when it is, in fact, necessary. Simplicity is a noble goal too, though. :) https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:308: child_it->first.DirName() == parent_it->first) { On 2014/05/21 15:55:09, vandebo wrote: > parent_it->first => parent_directory Done. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:313: candidates_to_check.insert(parent_it->first); On 2014/05/21 15:55:09, vandebo wrote: > parent_it->first => parent_directory Done. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:320: if (it->second.seen_count > 1 && it->second.seen_count * 100 / On 2014/05/21 15:55:09, vandebo wrote: > Instead of duplicating the criteria here would it be better to add another > boolean to ContainerCount ? Sure, swapping it for |has_parent| is free.
Looks reasonable. I have two concerns: I'm not sure this works for the case I suggested last round because a directory doesn't go into candidates_to_check until it has two children that are media directories process it. The unit test should be able to figure this out. It feels like candidates and candidates_to_check should/could be a single list, but I can't think of a way to make it work easily, so maybe it can't be. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:275: candidates.erase(candidates.find(candidate)); On 2014/05/21 17:29:36, Kevin Bailey wrote: > On 2014/05/21 15:55:09, vandebo wrote: > > ditto > > I don't think so. Consider: > > A/ > sensitive/ > B/ > C/media > D/media > E/ > F/media > G/media > > So we add B because it has media subdirs, then we add E likewise. Now, because A > has 2 entries (pretend it reached 80%), we consider it and discover that it is > sensitive. We should therefore remove it. Please walk be through it if I'm missing something... on patch set 9, line 283 is where we add a directory, this check is on line 272, so if a directory is sensitive, it'll never get added.
The message box keeps disappearing when I paste text so I'm just going to type it: regarding 1st concern, correct, the first child will add the parent to |candidates| with a count of 1, the second will increment it and count it's subdirs. IIRC, your example will then have 100% and *it's* parent will get bumped. regarding the 2nd concern, yes, I too believe that it can be one structure but it would require either an iterative search for candidates that we haven't checked yet, or (if we made it a list) iteratively searching for paths. boost multi-index containers would solve this neatly. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:275: candidates.erase(candidates.find(candidate)); On 2014/05/21 19:21:00, vandebo wrote: > > Please walk be through it if I'm missing something... on patch set 9, line 283 > is where we add a directory, this check is on line 272, so if a directory is > sensitive, it'll never get added. Recall that a directory itself need not be sensitive but if one of it's children are, then it gets excluded: line 269. In my example, A has a sensitive child.
On 2014/05/21 20:17:37, Kevin Bailey wrote: > The message box keeps disappearing when I paste text so I'm just going to type > it: > > regarding 1st concern, correct, the first child will add the parent to > |candidates| with a count of 1, the second will increment it and count it's > subdirs. IIRC, your example will then have 100% and *it's* parent will get > bumped. Artist1 only has one child. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:275: candidates.erase(candidates.find(candidate)); On 2014/05/21 20:17:38, Kevin Bailey wrote: > On 2014/05/21 19:21:00, vandebo wrote: > > > > Please walk be through it if I'm missing something... on patch set 9, line 283 > > is where we add a directory, this check is on line 272, so if a directory is > > sensitive, it'll never get added. > > Recall that a directory itself need not be sensitive but if one of it's children > are, then it gets excluded: line 269. In my example, A has a sensitive child. Won't that check on 269 prevent A from ever being added though? At B, we'll consider A, A is a parent of sensitive, is_sensitive gets set. Same at E.
'think I addressed all the comments. 2 new tests: one for general recursion, one for sensitive directories. https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/160001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:275: candidates.erase(candidates.find(candidate)); On 2014/05/22 17:35:51, vandebo wrote: > On 2014/05/21 20:17:38, Kevin Bailey wrote: > > On 2014/05/21 19:21:00, vandebo wrote: > > > > > > Please walk be through it if I'm missing something... on patch set 9, line > 283 > > > is where we add a directory, this check is on line 272, so if a directory is > > > sensitive, it'll never get added. > > > > Recall that a directory itself need not be sensitive but if one of it's > children > > are, then it gets excluded: line 269. In my example, A has a sensitive child. > > Won't that check on 269 prevent A from ever being added though? At B, we'll > consider A, A is a parent of sensitive, is_sensitive gets set. Same at E. Done.
https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:404: bool is_sensitive = false; nit: move this to line 409 https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:418: // Don't bother with ones we already have. nit: add a blank line above comment. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:421: ContainerCandidates::iterator parent_it = candidates.find(parent_directory); nit: add a blank line, otherwise at first glance this appears to be part of the previous block which ignores dirs. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:428: // If a parent directory has more than one scan result, consider it. nit: this comment should probably go up on line 421? https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:429: // If we haven't scanned it yet, do so. nit: scanned is a fairly generic term.. => If needed, count the number of directory entries. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:431: parent_counts->entries_count = 0; nit: Consider putting this into a helper function in order to shorten up this function. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:318: TEST_F(MediaScanManagerTest, MergeRedundant) { This is a fine test, but doesn't test the new functionality - only one level of parenting is done. The test case that I'd like to see, which I suspect still doesn't work is: A/B/files A/C/files A/D/files A/E/files A/F/files B-F only get a count of because they actually only have one child, but I would like A to be added as a container (because it really is).
https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:404: bool is_sensitive = false; On 2014/05/27 19:07:16, vandebo wrote: > nit: move this to line 409 Done. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:418: // Don't bother with ones we already have. On 2014/05/27 19:07:16, vandebo wrote: > nit: add a blank line above comment. Done. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:421: ContainerCandidates::iterator parent_it = candidates.find(parent_directory); On 2014/05/27 19:07:16, vandebo wrote: > nit: add a blank line, otherwise at first glance this appears to be part of the > previous block which ignores dirs. Done. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:428: // If a parent directory has more than one scan result, consider it. On 2014/05/27 19:07:16, vandebo wrote: > nit: this comment should probably go up on line 421? I was attempting to say, "Since at this point it has more than 1 occurrence, let's consider it." (The code above it doesn't represent this, so I put the comment below it.) Let me know if the new comment is any better. Happy to change it. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:429: // If we haven't scanned it yet, do so. On 2014/05/27 19:07:16, vandebo wrote: > nit: scanned is a fairly generic term.. => If needed, count the number of > directory entries. Done with respect to the previous comment. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:431: parent_counts->entries_count = 0; On 2014/05/27 19:07:16, vandebo wrote: > nit: Consider putting this into a helper function in order to shorten up this > function. Done. Didn't spot anything in base/files already doing it. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:318: TEST_F(MediaScanManagerTest, MergeRedundant) { On 2014/05/27 19:07:16, vandebo wrote: > This is a fine test, but doesn't test the new functionality - only one level of > parenting is done. > > The test case that I'd like to see, which I suspect still doesn't work is: > > A/B/files > A/C/files > A/D/files > A/E/files > A/F/files > > B-F only get a count of because they actually only have one child, but I would > like A to be added as a container (because it really is). Added test that duplicates this. Passes. Did you mean this? A/B1/B2/files A/C1/C2/files A/D1/D2/files That is easy for humans to spot, but harder for computers to (and reject false ones.)
https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:318: TEST_F(MediaScanManagerTest, MergeRedundant) { On 2014/05/27 21:04:36, Kevin Bailey wrote: > On 2014/05/27 19:07:16, vandebo wrote: > > This is a fine test, but doesn't test the new functionality - only one level > of > > parenting is done. > > > > The test case that I'd like to see, which I suspect still doesn't work is: > > > > A/B/files > > A/C/files > > A/D/files > > A/E/files > > A/F/files > > > > B-F only get a count of because they actually only have one child, but I would > > like A to be added as a container (because it really is). > > Added test that duplicates this. Passes. > > Did you mean this? > > A/B1/B2/files > A/C1/C2/files > A/D1/D2/files > > That is easy for humans to spot, but harder for computers to (and reject false > ones.) Oops, yes, I meant what you typed. This is the motivation for this change... Library/Artist/Album/files. Maybe the more than one directory requirement isn't needed, and the key is high percentage of media locations (80% right now). https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:364: int MediaScanManager::CountDirectoryEntries(const base::FilePath& path) { This can live in the anonymous namespace. https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:445: // If we haven't counted its entries yet, do so. With the helper function, this comment should probably be removed.
All addressed. https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/200001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:318: TEST_F(MediaScanManagerTest, MergeRedundant) { On 2014/05/28 00:36:59, vandebo wrote: > > Oops, yes, I meant what you typed. This is the motivation for this change... > Library/Artist/Album/files. > > Maybe the more than one directory requirement isn't needed, and the key is high > percentage of media locations (80% right now). I lowered the requirement to a single reference but, as predicted, it includes things your intuition might not. See line 378 where I had to manually exclude what would be the home directory(~). https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:364: int MediaScanManager::CountDirectoryEntries(const base::FilePath& path) { On 2014/05/28 00:37:00, vandebo wrote: > This can live in the anonymous namespace. Done, but if I could editorialize a bit... This is the cost of letting the Java style leak into C++. Some people think it's "6 of one, half dozen of the other", but the truth is that it prevented us from just doing: count.entries_count = std::count_if(dir_counter.begin(), dir_counter.end(), std::not1(std::ptr_fun(base::IsLink))); Using boost would be even more readable. Another advantage, besides less code, is that you can see the logic right there without paging up. https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:445: // If we haven't counted its entries yet, do so. On 2014/05/28 00:37:00, vandebo wrote: > With the helper function, this comment should probably be removed. Done.
https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:364: int MediaScanManager::CountDirectoryEntries(const base::FilePath& path) { On 2014/05/28 15:05:36, Kevin Bailey wrote: > On 2014/05/28 00:37:00, vandebo wrote: > > This can live in the anonymous namespace. > > Done, but if I could editorialize a bit... This is the cost of letting the Java > style leak into C++. Some people think it's "6 of one, half dozen of the other", We should always aim for what's most readable. > but the truth is that it prevented us from just doing: > > count.entries_count = std::count_if(dir_counter.begin(), dir_counter.end(), > std::not1(std::ptr_fun(base::IsLink))); > > Using boost would be even more readable. Another advantage, besides less code, > is that you can see the logic right there without paging up. I have been trying to use more of the standard library, but in almost all cases, it'd require thunks or some other wart. Part of the problem is that Chrome has it's own callback system which isn't compatible with the primordial one that's in C++03. Once we have C++11, things should get better. In this case, at least one of the obstacles is that FielEnumerator is not a real iterator. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:364: namespace { All the anonymous functions should go into the block at the top of the file. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:446: parent_it = candidates.insert(std::make_pair(parent_directory, With this, does that mean we walk to the root from each found_folder? If so, the code could be simplified to do exactly that. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:318: TEST_F(MediaScanManagerTest, MergeRedundant) { nit: for all of these tests, a comment laying out the structure and anticipated result would make things easier to peruse. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:378: MakeTestFolder("D", &path); So I can see both sides of the story on this one, what's your opinion? If we have the example A/B/C/D/E/files - where each level has just the one subdirectory, then it's probably some kind of organization scheme that either is brand new (only one thing in it), or a failed scheme (everything ends up in E), or a bunch of extra directories (an unzipped set of files?). It seems that the first option is least likely. I wonder if intermediate directories should be allowed to have just one child, but the top level ones should be required to have more? i.e. A/B/C/files, A/D/E/files should find A, but F/G/H/files should only have H (which would have been passed in).
Addressed all except the last comment. https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/240001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:364: int MediaScanManager::CountDirectoryEntries(const base::FilePath& path) { On 2014/05/28 21:01:06, vandebo wrote: > > In this case, at least one of the obstacles is that FileEnumerator is not a real > iterator. Yes, the non-iterator is exactly what I'm referring to. It tangibly curbs better coding opportunities. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:364: namespace { On 2014/05/28 21:01:06, vandebo wrote: > All the anonymous functions should go into the block at the top of the file. Ah, THEE anonymous namespace, not THUH anonymous namespace. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:446: parent_it = candidates.insert(std::make_pair(parent_directory, On 2014/05/28 21:01:06, vandebo wrote: > With this, does that mean we walk to the root from each found_folder? If so, > the code could be simplified to do exactly that. If I understand the question, not exactly. We would stop as soon as a directory contained at least one uninteresting item (the <80% rule). https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:318: TEST_F(MediaScanManagerTest, MergeRedundant) { On 2014/05/28 21:01:06, vandebo wrote: > nit: for all of these tests, a comment laying out the structure and anticipated > result would make things easier to peruse. Done. Should look better. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:378: MakeTestFolder("D", &path); On 2014/05/28 21:01:06, vandebo wrote: > So I can see both sides of the story on this one, what's your opinion? > > If we have the example A/B/C/D/E/files - where each level has just the one > subdirectory, then it's probably some kind of organization scheme that either is > brand new (only one thing in it), or a failed scheme (everything ends up in E), > or a bunch of extra directories (an unzipped set of files?). It seems that the > first option is least likely. I wonder if intermediate directories should be > allowed to have just one child, but the top level ones should be required to > have more? > > i.e. A/B/C/files, A/D/E/files should find A, but F/G/H/files should only have H > (which would have been passed in). I could change the behavior of the final loop - copying into |result| - such that something had to be |qualified| AND have at least 2 entries. This would cause us to investigate "Artist" directories but not include them, but would include the parent "Media" directory. Media/ * Artist1/ Album1/files Artist2/ Album1/files I looked back through the emails and it wasn't clear how important it was to include intermediates.
Sorry for the delay. Your message got buried in my inbox. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:446: parent_it = candidates.insert(std::make_pair(parent_directory, On 2014/05/29 00:36:41, Kevin Bailey wrote: > On 2014/05/28 21:01:06, vandebo wrote: > > With this, does that mean we walk to the root from each found_folder? If so, > > the code could be simplified to do exactly that. > > If I understand the question, not exactly. We would stop as soon as a directory > contained at least one uninteresting item (the <80% rule). But if it was a container, we'd eventually hit it again when the 80% rule was true and would potentially walk to it's parent. The code for this kind of approach might be easier to understand. I think it amounts to depth first instead of breadth first approach. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:378: MakeTestFolder("D", &path); On 2014/05/29 00:36:41, Kevin Bailey wrote: > On 2014/05/28 21:01:06, vandebo wrote: > > So I can see both sides of the story on this one, what's your opinion? > > > > If we have the example A/B/C/D/E/files - where each level has just the one > > subdirectory, then it's probably some kind of organization scheme that either > is > > brand new (only one thing in it), or a failed scheme (everything ends up in > E), > > or a bunch of extra directories (an unzipped set of files?). It seems that > the > > first option is least likely. I wonder if intermediate directories should be > > allowed to have just one child, but the top level ones should be required to > > have more? > > > > i.e. A/B/C/files, A/D/E/files should find A, but F/G/H/files should only have > H > > (which would have been passed in). > > I could change the behavior of the final loop - copying into |result| - such > that something had to be |qualified| AND have at least 2 entries. This would > cause us to investigate "Artist" directories but not include them, but would > include the parent "Media" directory. > > Media/ * > Artist1/ > Album1/files > Artist2/ > Album1/files > > I looked back through the emails and it wasn't clear how important it was to > include intermediates. In general, I think intermediates should be passed up to the caller for consideration. I don't think it's required though, so for this particular case, it seems reasonable to do that. https://codereview.chromium.org/285433004/diff/270001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/270001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:322: // A/B/ * * = container result? Please note.
https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:446: parent_it = candidates.insert(std::make_pair(parent_directory, On 2014/05/30 20:30:24, vandebo wrote: > > But if it was a container, we'd eventually hit it again when the 80% rule was > true and would potentially walk to it's parent. > > The code for this kind of approach might be easier to understand. I think it > amounts to depth first instead of breadth first approach. Traversing depth first lets us get rid of |candidates_to_check|, but I'd hesitate to call it simpler. Despite the diff, very little changed. Nevertheless, better? Note change at 439. I added a unit test which exposes the bug without the extra check. https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:378: MakeTestFolder("D", &path); On 2014/05/30 20:30:24, vandebo wrote: > > In general, I think intermediates should be passed up to the caller for > consideration. I don't think it's required though, so for this particular case, > it seems reasonable to do that. I *could* add another loop to *include* parents with sole children who have an "interesting" ancestor. So Home/Media/Artist would be included, but Home/ wouldn't. A small recursive function would be easiest. https://codereview.chromium.org/285433004/diff/270001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager_unittest.cc (right): https://codereview.chromium.org/285433004/diff/270001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager_unittest.cc:322: // A/B/ * On 2014/05/30 20:30:24, vandebo wrote: > * = container result? Please note. Done.
https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/260001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:446: parent_it = candidates.insert(std::make_pair(parent_directory, On 2014/06/01 21:12:40, Kevin Bailey wrote: > On 2014/05/30 20:30:24, vandebo wrote: > > > > But if it was a container, we'd eventually hit it again when the 80% rule was > > true and would potentially walk to it's parent. > > > > The code for this kind of approach might be easier to understand. I think it > > amounts to depth first instead of breadth first approach. > > Traversing depth first lets us get rid of |candidates_to_check|, but I'd > hesitate to call it simpler. Despite the diff, very little changed. > Nevertheless, better? There's one less mechanism going on, so I find it conceptually simpler, and better, yes. > Note change at 439. I added a unit test which exposes the bug without the extra > check. Noted. https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:393: // Find parent directories with majority of media directories, Recursively find... ? https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:401: base::FilePath candidate = it->first; nit: it->first is already in, it's the parent that's the candidate. Maybe s/candidate/media_dir/ s/parent_directory/candidate/ https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:439: if ((parent_it->second.seen_count - 1) * 100 / Instead of doing the extra math, just check that it wasn't already qualified; better yet, do an early exit if it was. if (parent_it->second.is_qualified) break; ... https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:455: if (it->second.is_qualified && it->second.seen_count >= 2) Are you sure this is sufficient? It seems that you'd need to check the qualified flag all the way back down to the media directory. (I think you can compute it on the to the root). i.e. R/ R/X/P/A/media R/X/P/B/media R/X/P/C/media R/X/M R/X/N R/Y/Q/D/media R/Y/Q/E/media R/Y/Q/F/media R/Y/I R/Y/J P and Q are clearly containers, but I think this code will find R as a container, when it's not.
I saw the "recursively" comment and assumed you were wondering what my "use recursion to include intermediates" proposal would look like. So it's included, but git will easily let me take it out if you don't like it. https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:393: // Find parent directories with majority of media directories, On 2014/06/02 16:29:25, vandebo wrote: > Recursively find... ? Done. https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:401: base::FilePath candidate = it->first; On 2014/06/02 16:29:25, vandebo wrote: > nit: it->first is already in, it's the parent that's the candidate. > > Maybe s/candidate/media_dir/ s/parent_directory/candidate/ Indeed, I reused the name. But since it follows |parent_directory| up the stack, maybe |child_directory| is better? https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:439: if ((parent_it->second.seen_count - 1) * 100 / On 2014/06/02 16:29:25, vandebo wrote: > Instead of doing the extra math, just check that it wasn't already qualified; > better yet, do an early exit if it was. > if (parent_it->second.is_qualified) > break; > ... Done. For some reason, I thought it wouldn't work for a newly added entry. https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:455: if (it->second.is_qualified && it->second.seen_count >= 2) On 2014/06/02 16:29:25, vandebo wrote: > Are you sure this is sufficient? It seems that you'd need to check the qualified > flag all the way back down to the media directory. (I think you can compute it > on the to the root). > > i.e. > > R/ > R/X/P/A/media > R/X/P/B/media > R/X/P/C/media > R/X/M > R/X/N > R/Y/Q/D/media > R/Y/Q/E/media > R/Y/Q/F/media > R/Y/I > R/Y/J > > P and Q are clearly containers, but I think this code will find R as a > container, when it's not. Neither X nor Y will be qualified, due to 80% rule, so we'll never even visit R. If you thought the rule was 60%, I would argue that R would be a container under those rules.
On 2014/06/02 21:03:42, Kevin Bailey wrote: > I saw the "recursively" comment and assumed you were wondering what my "use > recursion to include intermediates" proposal would look like. So it's included, > but git will easily let me take it out if you don't like it. No, I just wanted the comment updated - the old version only found direct parent containers. I don't think the new code is needed, please remove it. https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:439: if ((parent_it->second.seen_count - 1) * 100 / On 2014/06/02 21:03:43, Kevin Bailey wrote: > On 2014/06/02 16:29:25, vandebo wrote: > > Instead of doing the extra math, just check that it wasn't already qualified; > > better yet, do an early exit if it was. > > if (parent_it->second.is_qualified) > > break; > > ... > > Done. For some reason, I thought it wouldn't work for a newly added entry. You didn't do the early exit, which makes things easier to scan. https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:455: if (it->second.is_qualified && it->second.seen_count >= 2) On 2014/06/02 21:03:43, Kevin Bailey wrote: > On 2014/06/02 16:29:25, vandebo wrote: > > Are you sure this is sufficient? It seems that you'd need to check the > qualified > > flag all the way back down to the media directory. (I think you can compute > it > > on the to the root). > > > > i.e. > > > > R/ > > R/X/P/A/media > > R/X/P/B/media > > R/X/P/C/media > > R/X/M > > R/X/N > > R/Y/Q/D/media > > R/Y/Q/E/media > > R/Y/Q/F/media > > R/Y/I > > R/Y/J > > > > P and Q are clearly containers, but I think this code will find R as a > > container, when it's not. > > Neither X nor Y will be qualified, due to 80% rule, so we'll never even visit R. Hmm, indeed. SG then. > > If you thought the rule was 60%, I would argue that R would be a container under > those rules.
Also, please update the description.
Removed recursive fix-up. Updated description. https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/330001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:439: if ((parent_it->second.seen_count - 1) * 100 / On 2014/06/03 16:27:21, vandebo wrote: > On 2014/06/02 21:03:43, Kevin Bailey wrote: > > On 2014/06/02 16:29:25, vandebo wrote: > > > Instead of doing the extra math, just check that it wasn't already > qualified; > > > better yet, do an early exit if it was. > > > if (parent_it->second.is_qualified) > > > break; > > > ... > > > > Done. For some reason, I thought it wouldn't work for a newly added entry. > > You didn't do the early exit, which makes things easier to scan. Done.
I meant to also ask if you were ok with the unit test names.
LGTM after nit https://codereview.chromium.org/285433004/diff/390001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/390001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:447: break; nit: This else break construct is kind of hard to read / ugly. I was hoping you'd take care of it with the early exit. Can you make the early exit: if (parent_it->second.is_qualified || parent_it->second.seen_count * 100 / parent_it->second.entries_count < kContainerDirectoryMinimumPercent) { break; } Then line 445 joins up with line 449,450.
LGTM after nit
On 2014/06/03 18:04:30, Kevin Bailey wrote: > I meant to also ask if you were ok with the unit test names. 2 & 3 could have better names...
And improved test names. https://codereview.chromium.org/285433004/diff/390001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/390001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:447: break; On 2014/06/03 18:08:31, vandebo wrote: > nit: This else break construct is kind of hard to read / ugly. I was hoping > you'd take care of it with the early exit. Can you make the early exit: > > if (parent_it->second.is_qualified || parent_it->second.seen_count * 100 / > parent_it->second.entries_count < kContainerDirectoryMinimumPercent) { > break; > } > > Then line 445 joins up with line 449,450. Couldn't make it *exactly* the same due to line length. Hope it's ok.
BUG=NONE https://codereview.chromium.org/285433004/diff/390001/chrome/browser/media_ga... File chrome/browser/media_galleries/media_scan_manager.cc (right): https://codereview.chromium.org/285433004/diff/390001/chrome/browser/media_ga... chrome/browser/media_galleries/media_scan_manager.cc:447: break; On 2014/06/03 18:31:25, Kevin Bailey wrote: > On 2014/06/03 18:08:31, vandebo wrote: > > nit: This else break construct is kind of hard to read / ugly. I was hoping > > you'd take care of it with the early exit. Can you make the early exit: > > > > if (parent_it->second.is_qualified || parent_it->second.seen_count * 100 / > > parent_it->second.entries_count < kContainerDirectoryMinimumPercent) { > > break; > > } > > > > Then line 445 joins up with line 449,450. > > Couldn't make it *exactly* the same due to line length. Hope it's ok. Assumed... as is, it violated style guide. I'd suggest: if (parent_it->second.is_qualified || parent_it->second.seen_count * 100 / parent_it->second.entries_count < kContainerDirectoryMinimumPercent) { You can also see what git cl format suggests.
The CQ bit was checked by krb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krb@chromium.org/285433004/450001
Message was sent while issue was closed.
Change committed as 275264 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
