Extract external file systems handling from isolated context.

Extract external file systems handling from isolated context.

Move mount point info bookeeping from CrosMountPointProvider to ExternalMountPoints.

Add some tests for CrosMountPointProvider.

BUG=158837
TEST=content_unittests:CrosMountPointProvider*

TBR=benwells@chromium.org

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

[kinuko, 2013-01-08 12:22:43]

While I'm not fully clear about the final design of this refactoring this cl
looks nicer now.  I'm a bit worried about how we should handle Media file
systems in CrosMPP though... their code is based on IsolatedContext and if we
don't change the part CrosMPP will need to handle Mount/Unmount for isolated
filesystem paths. (That's part of the reason I tried to have a common interface
for Isolated/External mount points initially) Do you have an idea how we could
achieve that?

https://codereview.chromium.org/11648027/diff/13044/chrome/browser/ui/views/s...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

https://codereview.chromium.org/11648027/diff/13044/chrome/browser/ui/views/s...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:125:
"Downloads");
nit: This hard-coded string may look less nicer. Worth a comment?

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:52:
fileapi::ExternalMountPoints* system_mount_points)
Once we feel more comfortable about the basic architecture can we drop changes
related to separating the external mount points code into system and per-context
to make the final cl clearer?  (Having them is helpful for now as we can see how
the final code should look)

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:183:
system_mount_points_->AppendMountPoints(&files);
This method name AppendMountPoints looks like it's adding mount points to the
target object. Hmm... GetMountPointInfoAndAppendTo() maybe?

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:204: &id, &type, &path)) {
Does it make sense if we also return |type| in GetVirtualPath (both CrosMPP and
ExternalMP ones) and make this method take |type| so that we can remove this
extra Crack?

(I'm asking this w/o deep understanding of the cros/drive code, so please just
let me know if I'm wrong)

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:1: 
License comment?

It's great to have this test file, thanks for adding this!

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:60: return
g_external_mount_points.Pointer()->get();
Hmm, this is not thread safe and we seem to be losing the point of using
LazyInstance.  I think we should split the class for SystemMountPoints.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:142:
ExternalMountPoints::ExternalMountPoints(bool is_system_instance) {
Can we avoid using boolean here? (It's discouraged in general)

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.h (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.h:51: // Overlapping mount points are not
allowed. Adding mount point whose path
nit: "Overlapping mount points in a single MountPoints instance is not allowed."

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:73:
external_mount_points_(fileapi::ExternalMountPoints::CreateRefCounted()) {
Can we drop this change from this CL? It doesn't seem to be used yet.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:86:
fileapi::ExternalMountPoints::GetSystemInstance()));
ditto.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.cc:159: if (is_valid_ &&
IsolatedContext::IsIsolatedType(type_)) {
Maybe we should do like:

if (!is_valid_)
  return;

if (ExternalMountPoints::GetInstance()->CanCrack(type_)) {
  ...
  return;
}
if (IsolatedContext::GetInstance()->CanCrack(type_)) {
  ...
  return;
}

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:349: base::AutoLock locker(lock_);
nit:indent

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:353: 
nit: extra empty line

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.h (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.h:66: static bool IsIsolatedType(FileSystemType
type);
Can we deprecate this method?

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.h (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:21: class WEBKIT_STORAGE_EXPORT MountPoints {
For now this common interface class is not necessary?

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:46: WEBKIT_STORAGE_EXPORT static bool
HasMountPointsImplementation(
Can we remove this method?

[tbarzic, 2013-01-09 01:26:34]

https://codereview.chromium.org/11648027/diff/13044/chrome/browser/ui/views/s...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

https://codereview.chromium.org/11648027/diff/13044/chrome/browser/ui/views/s...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:125:
"Downloads");
On 2013/01/08 12:22:43, kinuko wrote:
> nit: This hard-coded string may look less nicer. Worth a comment?

Done.

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:52:
fileapi::ExternalMountPoints* system_mount_points)
On 2013/01/08 12:22:43, kinuko wrote:
> Once we feel more comfortable about the basic architecture can we drop changes
> related to separating the external mount points code into system and
per-context
> to make the final cl clearer?  (Having them is helpful for now as we can see
how
> the final code should look)

I think we'll still have to do this somewhere, but yeah, I think it would be
nicer not to do it here (maybe ExternalMountPoints?).

I'm keeping the logic here for now because I feel things could get little
subtler..

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:183:
system_mount_points_->AppendMountPoints(&files);
On 2013/01/08 12:22:43, kinuko wrote:
> This method name AppendMountPoints looks like it's adding mount points to the
> target object. Hmm... GetMountPointInfoAndAppendTo() maybe?

Done.

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:204: &id, &type, &path)) {
On 2013/01/08 12:22:43, kinuko wrote:
> Does it make sense if we also return |type| in GetVirtualPath (both CrosMPP
and
> ExternalMP ones) and make this method take |type| so that we can remove this
> extra Crack?
> 
> (I'm asking this w/o deep understanding of the cros/drive code, so please just
> let me know if I'm wrong)

I guess we could, but I'd still prefer to crack the path here, to verify that
virtual_path is valid for current context.
An alternative would be to pass FileSystemURL here, but that would make file
browser code a bit more awkward.

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:1: 
On 2013/01/08 12:22:43, kinuko wrote:
> License comment?
> 
> It's great to have this test file, thanks for adding this!

yeah, test files are always good, I even caught a couple of bugs (mainly in
GetVirtualPath method) :)

Done.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:60: return
g_external_mount_points.Pointer()->get();
On 2013/01/08 12:22:43, kinuko wrote:
> Hmm, this is not thread safe and we seem to be losing the point of using
> LazyInstance.  I think we should split the class for SystemMountPoints.

Yeah, I was concerned about that too. I think the new implementation should be
fine.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:142:
ExternalMountPoints::ExternalMountPoints(bool is_system_instance) {
On 2013/01/08 12:22:43, kinuko wrote:
> Can we avoid using boolean here? (It's discouraged in general)

Done.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.h (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.h:51: // Overlapping mount points are not
allowed. Adding mount point whose path
On 2013/01/08 12:22:43, kinuko wrote:
> nit: "Overlapping mount points in a single MountPoints instance is not
allowed."

Done.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:73:
external_mount_points_(fileapi::ExternalMountPoints::CreateRefCounted()) {
On 2013/01/08 12:22:43, kinuko wrote:
> Can we drop this change from this CL? It doesn't seem to be used yet.

Done.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
webkit/fileapi/file_system_context.cc:86:
fileapi::ExternalMountPoints::GetSystemInstance()));
On 2013/01/08 12:22:43, kinuko wrote:
> ditto.

I'd rather leave this (this cl does separate system and per-context external
mount points in cros_mount_point_provider, it just does not utilize it outside
the test code).

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_url.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/file_syste...
webkit/fileapi/file_system_url.cc:159: if (is_valid_ &&
IsolatedContext::IsIsolatedType(type_)) {
On 2013/01/08 12:22:43, kinuko wrote:
> Maybe we should do like:
> 
> if (!is_valid_)
>   return;
> 
> if (ExternalMountPoints::GetInstance()->CanCrack(type_)) {
>   ...
>   return;
> }
> if (IsolatedContext::GetInstance()->CanCrack(type_)) {
>   ...
>   return;
> }

I plan to do something like this in the next cl
(https://codereview.chromium.org/11787028/ ; note that this one is WIP :) )

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.cc (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:349: base::AutoLock locker(lock_);
On 2013/01/08 12:22:43, kinuko wrote:
> nit:indent

I think indent is fine..

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.cc:353: 
On 2013/01/08 12:22:43, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
File webkit/fileapi/isolated_context.h (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/isolated_c...
webkit/fileapi/isolated_context.h:66: static bool IsIsolatedType(FileSystemType
type);
On 2013/01/08 12:22:43, kinuko wrote:
> Can we deprecate this method?

I think we'll be able to do that, but I'd leave it for the subsequent change.

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.h (right):

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:21: class WEBKIT_STORAGE_EXPORT MountPoints {
On 2013/01/08 12:22:43, kinuko wrote:
> For now this common interface class is not necessary?

It's used both in IsolatedContext and ExternalMountPoints..

https://codereview.chromium.org/11648027/diff/13044/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:46: WEBKIT_STORAGE_EXPORT static bool
HasMountPointsImplementation(
On 2013/01/08 12:22:43, kinuko wrote:
> Can we remove this method?

Done.

[tbarzic, 2013-01-09 01:47:25]

As for handling Media file systems..
first thing that comes to mind (without lot of digging) is to handle those as
external filesystems instead of isolated. It would probably be a bit weird to
handle Isolated file system types using external mount points (I don't know
maybe we could change their URLs to use "external" instead of "isolated").

Or maybe we could add/remove media file systems both to isolated and external
mount points.. and use isolated mount points to crack filesystem url while using
external mount points to do CrosMPP specific stuff.
Still, feels hacky.. and I could also be underestimating the problem :)

[kinuko, 2013-01-09 16:37:12]

(I haven't looked through all files yet but sending out some comments now)

https://codereview.chromium.org/11648027/diff/36035/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/11648027/diff/36035/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:222: bool
include_system_mount_points) const {
Can we remove this boolean parameter? (In general boolean parameters should be
discouraged)
Currently this method is only called with 'true' except for the unittests.

When we add more code where we only want to get local root directories can we
simply add a new method like GetLocalRootDirectories or something like that? (Or
using enum is another common way to avoid boolean)

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:70: // |remote_proxy| is allowed only
for drive file system.
nit: allowed only for -> used only by?

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:249: bool
ExternalMountPoints::CrackExternalPath(const FilePath& virtual_path,
Maybe this should be named CrackVirtualPath (to be consistent with
GetVirtualPath)?

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_types.h (right):

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/file_syste...
webkit/fileapi/file_system_types.h:62: kFileSystemTypeIsolatedNativeLocal,
Why do we need this separate type?

[tonibarzic, 2013-01-09 17:09:46]

https://codereview.chromium.org/11648027/diff/36035/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/11648027/diff/36035/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:222: bool
include_system_mount_points) const {
On 2013/01/09 16:37:12, kinuko wrote:
> Can we remove this boolean parameter? (In general boolean parameters should be
> discouraged)
> Currently this method is only called with 'true' except for the unittests.
> 
> When we add more code where we only want to get local root directories can we
> simply add a new method like GetLocalRootDirectories or something like that?
(Or
> using enum is another common way to avoid boolean)

yeah, makes sense, will do

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_types.h (right):

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/file_syste...
webkit/fileapi/file_system_types.h:62: kFileSystemTypeIsolatedNativeLocal,
On 2013/01/09 16:37:12, kinuko wrote:
> Why do we need this separate type?

I don't think we need.. I wanted to check something and forgot to undo this
change  before upload

[tbarzic, 2013-01-10 00:39:56]

https://codereview.chromium.org/11648027/diff/36035/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/11648027/diff/36035/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:222: bool
include_system_mount_points) const {
On 2013/01/09 17:09:46, tonibarzic wrote:
> On 2013/01/09 16:37:12, kinuko wrote:
> > Can we remove this boolean parameter? (In general boolean parameters should
be
> > discouraged)
> > Currently this method is only called with 'true' except for the unittests.
> > 
> > When we add more code where we only want to get local root directories can
we
> > simply add a new method like GetLocalRootDirectories or something like that?
> (Or
> > using enum is another common way to avoid boolean)
> 
> yeah, makes sense, will do

Done.

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:70: // |remote_proxy| is allowed only
for drive file system.
On 2013/01/09 16:37:12, kinuko wrote:
> nit: allowed only for -> used only by?

Done.

https://codereview.chromium.org/11648027/diff/36035/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:249: bool
ExternalMountPoints::CrackExternalPath(const FilePath& virtual_path,
On 2013/01/09 16:37:12, kinuko wrote:
> Maybe this should be named CrackVirtualPath (to be consistent with
> GetVirtualPath)?

Done.

Also added to MountPoints interface (and changed name of
IsolatedContext::CrackIsolatedPath)

[kinuko, 2013-01-10 05:47:31]

Ok, I think it's getting nicer now! One high-level comment I had is if we can
have external_mount_points_unittest and move some of the tests from
cros_mount_point_provider... that'd be desirable to add a new class.

https://codereview.chromium.org/11648027/diff/33007/chrome/browser/intents/de...
File chrome/browser/intents/device_attached_intent_source.cc (left):

https://codereview.chromium.org/11648027/diff/33007/chrome/browser/intents/de...
chrome/browser/intents/device_attached_intent_source.cc:131: 
nit: let's not make nop changes in the files you're not touching

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:249: return NULL;
Please fill *error_code
(Or change this to DCHECK)

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:254: return NULL;
ditto.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
nit: 2012 -> 2013

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:40: }
style-nit: no { } for one line block

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:60: // Fourth
mount point is Downloads, but it's local path is device specific.
nit: it's local path -> its local path ?

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:148: // Can't
remove system nount point.
nount -> mount

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:153: // The same
path
nit: ends comments with period (or ':' in this case maybe)

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:184:
TEST(CrosMountPointProviderTest, AddMountPoint) {
Shouldn't this test be rather added as ExternalMountPointsTest?

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:339:
GetFilePath("removable/"));
nit: indent

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:344: //
TODO(tbarzic): This could be moved to external_mount_points tests.
Yep, this would fit better in external_mount_points_test.

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:245: // TODO(tbarzic): Reorder methods
to match .h before committing!!!!!!
yes please ;)

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.h (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.h:64: // Same as
|RegisterExternalFileSystem|, but registers a remote file system
nit: but registers -> but also registers

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.h (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:70: // TODO(tbarzic): Add CrackURL methods.
Let's not add this TODO in this cl.

[tonibarzic, 2013-01-11 01:50:04]

https://codereview.chromium.org/11648027/diff/33007/chrome/browser/intents/de...
File chrome/browser/intents/device_attached_intent_source.cc (left):

https://codereview.chromium.org/11648027/diff/33007/chrome/browser/intents/de...
chrome/browser/intents/device_attached_intent_source.cc:131: 
On 2013/01/10 05:47:31, kinuko wrote:
> nit: let's not make nop changes in the files you're not touching

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:249: return NULL;
On 2013/01/10 05:47:31, kinuko wrote:
> Please fill *error_code
> (Or change this to DCHECK)

good catch
Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:254: return NULL;
On 2013/01/10 05:47:31, kinuko wrote:
> ditto.

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/01/10 05:47:31, kinuko wrote:
> nit: 2012 -> 2013

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:40: }
On 2013/01/10 05:47:31, kinuko wrote:
> style-nit: no { } for one line block

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:60: // Fourth
mount point is Downloads, but it's local path is device specific.
On 2013/01/10 05:47:31, kinuko wrote:
> nit: it's local path -> its local path ?

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:148: // Can't
remove system nount point.
On 2013/01/10 05:47:31, kinuko wrote:
> nount -> mount

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:153: // The same
path
On 2013/01/10 05:47:31, kinuko wrote:
> nit: ends comments with period (or ':' in this case maybe)

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:184:
TEST(CrosMountPointProviderTest, AddMountPoint) {
On 2013/01/10 05:47:31, kinuko wrote:
> Shouldn't this test be rather added as ExternalMountPointsTest?

yes, it should (fwiw that's what I planned to do yesterday, but got
distracted).. also, I added some test cases for windows separators

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:339:
GetFilePath("removable/"));
On 2013/01/10 05:47:31, kinuko wrote:
> nit: indent

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:344: //
TODO(tbarzic): This could be moved to external_mount_points tests.
On 2013/01/10 05:47:31, kinuko wrote:
> Yep, this would fit better in external_mount_points_test.

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:245: // TODO(tbarzic): Reorder methods
to match .h before committing!!!!!!
On 2013/01/10 05:47:31, kinuko wrote:
> yes please ;)

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.h (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.h:64: // Same as
|RegisterExternalFileSystem|, but registers a remote file system
On 2013/01/10 05:47:31, kinuko wrote:
> nit: but registers -> but also registers

Done.

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.h (right):

https://codereview.chromium.org/11648027/diff/33007/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.h:70: // TODO(tbarzic): Add CrackURL methods.
On 2013/01/10 05:47:31, kinuko wrote:
> Let's not add this TODO in this cl.

Done.

[kinuko, 2013-01-11 08:20:52]

fileapi lgtm with some nits

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:17: #define FPL(x)
FILE_PATH_LITERAL(x)
tiny nit: this is totally fine but having #define in namespace {} may feel a bit
misleading as cpp defines do nothing to do with namespaces. Maybe it should be
placed just outside the namespace?

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:23:
FilePath(path));
Alternatively you can just take const char* (so you need no FPL()) and call
FilePath::FromUTF8Unsafe(path)

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:49:
EXPECT_TRUE(VectorHasPath(root_dirs, FilePath(FPL("/usr/share/oem"))));
Well, maybe... using std::set like following might be less error prone and more
common.

#include "base/stl_util.h"

std::set<FilePath> root_dir_set(root_dirs.begin(), root_dirs.end());
EXPECT_TRUE(ContainsKey(root_dir_set, FilePath(...));
EXPECT_TRUE(ContainsKey(root_dir_set, FilePath(...));

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:244:
ExternalMountPoints::ExternalMountPoints() { }
nit: no space necessary in empty {}

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:252: const FilePath& path) {
Can you call lock_.AssertAcquired() here to make sure?

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.h (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.h:113: explicit ExternalMountPoints();
nit: no need for explicit?

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points_unittest.cc:125: 
nit: extra empty line

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points_unittest.cc:150: }
If you don't refer kTestMountPoints again isn't it simpler to just write down
like:

mount_points->RegisterFileSystem("c", type, FilePath(...));
mount_points->RegisterFileSystem("x", type, FilePath(...));

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.cc:7: #include "webkit/fileapi/file_system_types.h"
This is not necessary?

[tbarzic, 2013-01-11 09:13:16]

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:17: #define FPL(x)
FILE_PATH_LITERAL(x)
On 2013/01/11 08:20:52, kinuko wrote:
> tiny nit: this is totally fine but having #define in namespace {} may feel a
bit
> misleading as cpp defines do nothing to do with namespaces. Maybe it should be
> placed just outside the namespace?

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:23:
FilePath(path));
On 2013/01/11 08:20:52, kinuko wrote:
> Alternatively you can just take const char* (so you need no FPL()) and call
> FilePath::FromUTF8Unsafe(path)

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:49:
EXPECT_TRUE(VectorHasPath(root_dirs, FilePath(FPL("/usr/share/oem"))));
On 2013/01/11 08:20:52, kinuko wrote:
> Well, maybe... using std::set like following might be less error prone and
more
> common.
> 
> #include "base/stl_util.h"
> 
> std::set<FilePath> root_dir_set(root_dirs.begin(), root_dirs.end());
> EXPECT_TRUE(ContainsKey(root_dir_set, FilePath(...));
> EXPECT_TRUE(ContainsKey(root_dir_set, FilePath(...));

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:244:
ExternalMountPoints::ExternalMountPoints() { }
On 2013/01/11 08:20:52, kinuko wrote:
> nit: no space necessary in empty {}

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.cc:252: const FilePath& path) {
On 2013/01/11 08:20:52, kinuko wrote:
> Can you call lock_.AssertAcquired() here to make sure?

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points.h (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points.h:113: explicit ExternalMountPoints();
On 2013/01/11 08:20:52, kinuko wrote:
> nit: no need for explicit?

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
File webkit/fileapi/external_mount_points_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points_unittest.cc:125: 
On 2013/01/11 08:20:52, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/external_m...
webkit/fileapi/external_mount_points_unittest.cc:150: }
On 2013/01/11 08:20:52, kinuko wrote:
> If you don't refer kTestMountPoints again isn't it simpler to just write down
> like:
> 
> mount_points->RegisterFileSystem("c", type, FilePath(...));
> mount_points->RegisterFileSystem("x", type, FilePath(...));

Done.

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/mount_poin...
File webkit/fileapi/mount_points.cc (right):

https://codereview.chromium.org/11648027/diff/56002/webkit/fileapi/mount_poin...
webkit/fileapi/mount_points.cc:7: #include "webkit/fileapi/file_system_types.h"
On 2013/01/11 08:20:52, kinuko wrote:
> This is not necessary?

Done.

[tbarzic, 2013-01-12 03:20:14]

Kinuko, would you mind taking another look (at diff to the PatchSet 23)

[tbarzic, 2013-01-14 19:18:27]

+jam, for content/content_tests.gypi

[jam, 2013-01-14 21:05:46]

https://codereview.chromium.org/11648027/diff/79001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11648027/diff/79001/content/content_tests.gyp...
content/content_tests.gypi:591:
'../webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc'
you should list this file path above. by convention targets should include files
for all platforms, then exclude them when they need to be excluded. this keeps
all the files visible in each platform's IDE. also in this case, you wouldn't
need to exclude it since the path includes chromeos, which means that the
filename filters should automatically do this

[tbarzic, 2013-01-14 21:36:28]

https://chromiumcodereview.appspot.com/11648027/diff/79001/content/content_te...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/11648027/diff/79001/content/content_te...
content/content_tests.gypi:591:
'../webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc'
On 2013/01/14 21:05:46, John Abd-El-Malek wrote:
> you should list this file path above. by convention targets should include
files
> for all platforms, then exclude them when they need to be excluded. this keeps
> all the files visible in each platform's IDE. also in this case, you wouldn't
> need to exclude it since the path includes chromeos, which means that the
> filename filters should automatically do this

Done.

[jam, 2013-01-15 01:37:10]

https://codereview.chromium.org/11648027/diff/78003/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11648027/diff/78003/content/content_tests.gyp...
content/content_tests.gypi:596: ['exclude', '^../webkit/chromeos/'],
are you sure you need this? the file filters should already be doing this...

[tbarzic, 2013-01-15 02:11:27]

https://codereview.chromium.org/11648027/diff/78003/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11648027/diff/78003/content/content_tests.gyp...
content/content_tests.gypi:596: ['exclude', '^../webkit/chromeos/'],
On 2013/01/15 01:37:10, John Abd-El-Malek wrote:
> are you sure you need this? the file filters should already be doing this...

it didn't compile on Linux without this..

[kinuko, 2013-01-15 16:19:16]

still lgtm with one nit

https://codereview.chromium.org/11648027/diff/66001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/66001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:52:
EXPECT_TRUE(root_dirs_set.count(FilePath(FPL("/usr/share/oem"))));
Can we do the same in other tests too so that we can remove VectorHasPath?

[tbarzic, 2013-01-15 16:37:48]

btw.  Patch 23 doesn't have the changes I pinged you to take another look at,
what I meant was "Can you take another look at patch 27, difference to Patch 23
" :)

https://codereview.chromium.org/11648027/diff/66001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/66001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:52:
EXPECT_TRUE(root_dirs_set.count(FilePath(FPL("/usr/share/oem"))));
On 2013/01/15 16:19:16, kinuko wrote:
> Can we do the same in other tests too so that we can remove VectorHasPath?

I already did that..

[jam, 2013-01-15 20:52:45]

https://codereview.chromium.org/11648027/diff/78003/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11648027/diff/78003/content/content_tests.gyp...
content/content_tests.gypi:596: ['exclude', '^../webkit/chromeos/'],
On 2013/01/15 02:11:27, tbarzic wrote:
> On 2013/01/15 01:37:10, John Abd-El-Malek wrote:
> > are you sure you need this? the file filters should already be doing this...
> 
> it didn't compile on Linux without this..

ah, looks like filename_rules.gypi is missing an entry for directories named
"chromeos". can you please fix it there instead of here? with that, lgtm

[kinuko, 2013-01-16 02:57:12]

https://codereview.chromium.org/11648027/diff/66001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/11648027/diff/66001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider_unittest.cc:52:
EXPECT_TRUE(root_dirs_set.count(FilePath(FPL("/usr/share/oem"))));
On 2013/01/15 16:37:48, tbarzic wrote:
> On 2013/01/15 16:19:16, kinuko wrote:
> > Can we do the same in other tests too so that we can remove VectorHasPath?
> 
> I already did that..

My bad, thanks for fixing them!

[commit-bot: I haz the power, 2013-01-18 00:13:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/11648027/57002

[commit-bot: I haz the power, 2013-01-18 00:14:01]

Failed to apply patch for
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
  Hunk #1 FAILED at 28.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc.rej

Patch:       chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
Index: chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
diff --git a/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
b/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
index
8054334d07b916a1589ab95d4719f088bb4da34f..cd08963d72fa9457bba9176b1f889ead1a1c0240
100644
--- a/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
+++ b/chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
@@ -28,6 +28,7 @@
 #include "content/public/test/test_utils.h"
 #include "ui/base/dialogs/select_file_dialog.h"
 #include "ui/base/dialogs/selected_file_info.h"
+#include "webkit/fileapi/external_mount_points.h"
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_mount_point_provider.h"
 
@@ -119,7 +120,12 @@ class SelectFileDialogExtensionBrowserTest : public
ExtensionBrowserTest {
     fileapi::ExternalFileSystemMountPointProvider* provider =
         BrowserContext::GetDefaultStoragePartition(browser()->profile())->
             GetFileSystemContext()->external_provider();
-    provider->AddLocalMountPoint(path);
+
+    // The Downloads mount point already exists so it must be removed before
+    // adding the test mount point (which will also be mapped as Downloads).
+    fileapi::ExternalMountPoints::GetSystemInstance()->RevokeFileSystem(
+        path.BaseName().AsUTF8Unsafe());
+    EXPECT_TRUE(provider->AddLocalMountPoint(path));
   }
 
   void CheckJavascriptErrors() {

[commit-bot: I haz the power, 2013-01-18 00:24:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/11648027/112002

[commit-bot: I haz the power, 2013-01-18 02:47:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/11648027/112002

[commit-bot: I haz the power, 2013-01-18 03:13:03]

Change committed as 177578