Media Galleries: Access MTP devices by file ids rather than file paths.

Media Galleries: Access MTP devices by file ids rather than file paths.

File paths can be very slow to access because finding a file by path requires
O(N) metadata access to figure out which file id corresponds to a given name.

BUG=385307
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283732

[tommycli, 2014-07-09 21:48:31]

thestig: Seems legit.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:296:
uint32 cached_file_id;
This block could be abstracted into the EnsureCacheFilledAndGetId .

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:641:
fileapi::AsyncFileUtil::EntryList normalized_file_list;
My initial reaction was: Why not just clear the list of children, then re-add.
But now I understand you want to keep the subtrees.

Maybe we should change the names?
AddChild's real intent is "EnsureChildExists"
and 
ClearChildren's real intent is "ClearNonexistentChildren"

I'm not sure how to make this more clear, since it's a pretty complicated
operation.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:720:
base::FilePath MTPDeviceDelegateImplLinux::NextUncachedPathComponent(
I really like how this method is structured.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h
(right):

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:55: class
MTPFileNode;
Either remove fwd decl or move inner class decl to cc file.

I think I prefer it to be in the cc file.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:61:
typedef base::ScopedPtrHashMap<std::string, MTPFileNode> ChildNodes;
Since this is only used in the inner class, it should be declared in inner
class.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:234: bool
CachedPathToId(const base::FilePath& path, uint32* id) const;
Since every caller really just wants a "EnsureCacheFilledThenGetIdForPath"
method, is it possible to just have that one member function?

Maybe these can be in the anonymous namespace?

[Lei Zhang, 2014-07-14 23:46:33]

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
(right):

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:296:
uint32 cached_file_id;
On 2014/07/09 21:48:31, tommycli wrote:
> This block could be abstracted into the EnsureCacheFilledAndGetId .

|readdir_success_callback| is different for each one of these calls. So we're
only going to save a few lines at the cost of having to create the Closure every
time.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc:641:
fileapi::AsyncFileUtil::EntryList normalized_file_list;
On 2014/07/09 21:48:31, tommycli wrote:
> My initial reaction was: Why not just clear the list of children, then re-add.
> But now I understand you want to keep the subtrees.
> 
> Maybe we should change the names?
> AddChild's real intent is "EnsureChildExists"
> and 
> ClearChildren's real intent is "ClearNonexistentChildren"
> 
> I'm not sure how to make this more clear, since it's a pretty complicated
> operation.

Done.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h
(right):

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:55: class
MTPFileNode;
On 2014/07/09 21:48:31, tommycli wrote:
> Either remove fwd decl or move inner class decl to cc file.
> 
> I think I prefer it to be in the cc file.

Done.

https://codereview.chromium.org/377383002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.h:61:
typedef base::ScopedPtrHashMap<std::string, MTPFileNode> ChildNodes;
On 2014/07/09 21:48:31, tommycli wrote:
> Since this is only used in the inner class, it should be declared in inner
> class.

Done.

[Lei Zhang, 2014-07-16 03:37:35]

See patch set 4.

[tommycli, 2014-07-16 16:29:52]

lgtm

I feel bad because my comments have caused the SLOC to increase rather than
decrease.

Feel free to commit whichever version you personally think is better.

[Lei Zhang, 2014-07-16 17:55:52]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-07-16 17:59:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/377383002/120001

[commit-bot: I haz the power, 2014-07-17 00:04:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-17 02:45:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 02:45:43]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[Lei Zhang, 2014-07-17 03:53:21]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-07-17 03:55:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/377383002/140001

[commit-bot: I haz the power, 2014-07-17 04:45:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29427)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/33836)

[commit-bot: I haz the power, 2014-07-17 04:48:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 04:48:59]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29467)

[Lei Zhang, 2014-07-17 05:16:04]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-07-17 05:19:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/377383002/160001

[commit-bot: I haz the power, 2014-07-17 09:55:39]

Change committed as 283732