Implement media path filter

Implement media path filter

BUG=137670
TEST='NativeMediaFileUtilTest.*'


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149566

[tzik, 2012-07-26 18:44:55]

Could take a look to it?

I'll add some test soon.

[kinuko, 2012-07-26 20:21:56]

Can you fill the BUG= line and also (later) update the issue how
DeviceMediaFileUtil could implement the same/similar filtering?  Please also
consider if we should use layering or not.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
File webkit/fileapi/media/local_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:37: }
Why is this in a separate blocK?

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:38: return Next();
Can we make this non-recursive?

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:62: 
nit: please remove extra empty line.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:75: return
base::PLATFORM_FILE_ERROR_SECURITY;
Is this FileUtil also trying to explicitly disable write access??

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:160: *result =
base::PLATFORM_FILE_ERROR_SECURITY;
Why does this just return error?

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
File webkit/fileapi/media/local_media_file_util.h (right):

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.h:23: 
style-nit: please remove empty lines between virtual methods from the same base
class.

[kmadhusu, 2012-07-26 20:26:43]

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
File chrome/browser/intents/device_attached_intent_source.cc (right):

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
chrome/browser/intents/device_attached_intent_source.cc:57:
fileapi::kFileSystemTypeLocalMedia, device_path, &device_name);
This is a file system for attached device media. Can we call this as
kFileSystemTypeDeviceMedia?

[tzik, 2012-07-27 01:00:23]

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
File chrome/browser/intents/device_attached_intent_source.cc (right):

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
chrome/browser/intents/device_attached_intent_source.cc:57:
fileapi::kFileSystemTypeLocalMedia, device_path, &device_name);
On 2012/07/26 20:26:43, kmadhusu wrote:
> This is a file system for attached device media. Can we call this as
> kFileSystemTypeDeviceMedia?

Hmm. We might have two type of media device.
One is mounted device to which we can access like local filesystem (this case).
Another is the device that we need to use MTP or PTP.

How about using kFileSystemTypeMountedMedia?

[kmadhusu, 2012-07-27 01:22:42]

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
File chrome/browser/intents/device_attached_intent_source.cc (right):

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
chrome/browser/intents/device_attached_intent_source.cc:57:
fileapi::kFileSystemTypeLocalMedia, device_path, &device_name);
On 2012/07/27 01:00:23, tzik wrote:
> On 2012/07/26 20:26:43, kmadhusu wrote:
> > This is a file system for attached device media. Can we call this as
> > kFileSystemTypeDeviceMedia?
> 
> Hmm. We might have two type of media device.
> One is mounted device to which we can access like local filesystem (this
case).
> Another is the device that we need to use MTP or PTP.
> 
> How about using kFileSystemTypeMountedMedia?

I thought kFileSystemTypeLocalMedia corresponds to local media galleries such as
"C:\MyPictures\" etc.  I guess you are considering mass storage devices (such as
usb, external hard drive) and local media galleries("C:\MyPictures\") as
"kFileSystemTypeLocalMedia" and mtp/ptp device galleries as
"kFileSystemTypeDeviceMedia". Is it correct? If so, we need a better name for
file system types.

[tzik, 2012-07-27 17:18:55]

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
File webkit/fileapi/media/local_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:37: }
On 2012/07/26 20:21:56, kinuko wrote:
> Why is this in a separate blocK?

I think, this block makes Next() call tail recursion.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:38: return Next();
On 2012/07/26 20:21:56, kinuko wrote:
> Can we make this non-recursive?

Done.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:62: 
On 2012/07/26 20:21:56, kinuko wrote:
> nit: please remove extra empty line.

Done.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:75: return
base::PLATFORM_FILE_ERROR_SECURITY;
On 2012/07/26 20:21:56, kinuko wrote:
> Is this FileUtil also trying to explicitly disable write access??

Yes, for now. We need some special handling to support write access under
filtering.
Added comments.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:160: *result =
base::PLATFORM_FILE_ERROR_SECURITY;
On 2012/07/26 20:21:56, kinuko wrote:
> Why does this just return error?

Ah, it's an error. We shouldn't block this. Removed.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
File webkit/fileapi/media/local_media_file_util.h (right):

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.h:23: 
On 2012/07/26 20:21:56, kinuko wrote:
> style-nit: please remove empty lines between virtual methods from the same
base
> class.

Done.

[tzik, 2012-07-27 18:33:22]

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
File chrome/browser/intents/device_attached_intent_source.cc (right):

http://codereview.chromium.org/10825042/diff/1/chrome/browser/intents/device_...
chrome/browser/intents/device_attached_intent_source.cc:57:
fileapi::kFileSystemTypeLocalMedia, device_path, &device_name);
On 2012/07/27 01:22:42, kmadhusu wrote:
> On 2012/07/27 01:00:23, tzik wrote:
> > On 2012/07/26 20:26:43, kmadhusu wrote:
> > > This is a file system for attached device media. Can we call this as
> > > kFileSystemTypeDeviceMedia?
> > 
> > Hmm. We might have two type of media device.
> > One is mounted device to which we can access like local filesystem (this
> case).
> > Another is the device that we need to use MTP or PTP.
> > 
> > How about using kFileSystemTypeMountedMedia?
> 
> I thought kFileSystemTypeLocalMedia corresponds to local media galleries such
as
> "C:\MyPictures\" etc.  I guess you are considering mass storage devices (such
as
> usb, external hard drive) and local media galleries("C:\MyPictures\") as
> "kFileSystemTypeLocalMedia" and mtp/ptp device galleries as
> "kFileSystemTypeDeviceMedia". Is it correct? If so, we need a better name for
> file system types.

Yes, I mean that exactly. As we chatted, s/LocalMedia/NativeMedia/ looks good.

[tzik, 2012-07-28 01:17:54]

+Steve (vandebo@)
Could you take a look to it?

[kinuko, 2012-07-28 01:23:34]

looking good.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
File webkit/fileapi/media/local_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:37: }
On 2012/07/27 17:18:56, tzik wrote:
> On 2012/07/26 20:21:56, kinuko wrote:
> > Why is this in a separate blocK?
> 
> I think, this block makes Next() call tail recursion.

Ah oh ok :) clever.

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:57: }  // anonymous namespace
nit: please add empty line between line 56 and 57.
nit: 'anonymous namespace' -> just 'namespace' is fine

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util.h (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.h:13: class FILEAPI_EXPORT_PRIVATE
NativeMediaFileUtil : public IsolatedFileUtil {
Please add class-level comment!

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util_unittest.cc (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:7: #include
"base/bind.h"
not necessary?

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:17: #include
"webkit/fileapi/isolated_context.h"
please sort headers

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:58:
file_system_context_->GetFileUtil(kFileSystemTypeNativeMedia));
Does this need to be down-casted?

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:85: }
nit: this is not used

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:89: }
nit: this is not used

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:132: {
FPL("hoge/baz.txt"), false, false },
Can you add comments which line is testing what?  (E.g. "Regular file with media
extension" etc)

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:161:
MessageLoop::current()->RunAllPending();
I think we'll also need tests for ReadDirectory (probably more than FooExist
ones)

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:978: NOTREACHED();
This is not directly related to this cl but probably we should use if/else
instead of switch case here?  I don't think obfuscated_file_util needs to know
all other filesystem types.

[vandebo (ex-Chrome), 2012-07-30 17:01:43]

Looking good.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/media_...
File webkit/fileapi/media/media_path_filter.cc (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/media_...
webkit/fileapi/media/media_path_filter.cc:15:
net::GetImageExtensions(&media_file_extensions_);
This seems ok, but maybe we want to filter to only media types supported in
Chrome.  I don't see any helpers to do that generically.

[tzik, 2012-07-30 23:32:35]

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
File webkit/fileapi/media/local_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/1/webkit/fileapi/media/local_med...
webkit/fileapi/media/local_media_file_util.cc:57: }  // anonymous namespace
On 2012/07/28 01:23:35, kinuko wrote:
> nit: please add empty line between line 56 and 57.
> nit: 'anonymous namespace' -> just 'namespace' is fine

Done.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/media_...
File webkit/fileapi/media/media_path_filter.cc (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/media_...
webkit/fileapi/media/media_path_filter.cc:15:
net::GetImageExtensions(&media_file_extensions_);
On 2012/07/30 17:01:44, vandebo wrote:
> This seems ok, but maybe we want to filter to only media types supported in
> Chrome.  I don't see any helpers to do that generically.

Done.

I restrict them to supported type.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util.h (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.h:13: class FILEAPI_EXPORT_PRIVATE
NativeMediaFileUtil : public IsolatedFileUtil {
On 2012/07/28 01:23:35, kinuko wrote:
> Please add class-level comment!

Done.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util_unittest.cc (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:7: #include
"base/bind.h"
On 2012/07/28 01:23:35, kinuko wrote:
> not necessary?

It's needed for l158.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:17: #include
"webkit/fileapi/isolated_context.h"
On 2012/07/28 01:23:35, kinuko wrote:
> please sort headers

Done. I forgot it.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:58:
file_system_context_->GetFileUtil(kFileSystemTypeNativeMedia));
On 2012/07/28 01:23:35, kinuko wrote:
> Does this need to be down-casted?

Done.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:85: }
On 2012/07/28 01:23:35, kinuko wrote:
> nit: this is not used

Done.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:89: }
On 2012/07/28 01:23:35, kinuko wrote:
> nit: this is not used

Done.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:132: {
FPL("hoge/baz.txt"), false, false },
On 2012/07/28 01:23:35, kinuko wrote:
> Can you add comments which line is testing what?  (E.g. "Regular file with
media
> extension" etc)

Done.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:161:
MessageLoop::current()->RunAllPending();
On 2012/07/28 01:23:35, kinuko wrote:
> I think we'll also need tests for ReadDirectory (probably more than FooExist
> ones)

Done.

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/8006/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:978: NOTREACHED();
On 2012/07/28 01:23:35, kinuko wrote:
> This is not directly related to this cl but probably we should use if/else
> instead of switch case here?  I don't think obfuscated_file_util needs to know
> all other filesystem types.

Done.

[kinuko, 2012-07-31 01:26:35]

lgtm mod nits.

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_types.h (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/file_system_...
webkit/fileapi/file_system_types.h:41: kFileSystemTypeDeviceMedia,
nit: Can you add a brief comment to each type to reflect what you/Kausalya
agreed?

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.cc:56: }  // anonymous namespace
style-nit: "anonymous" could be removed

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.cc:125:
DCHECK(context->media_path_filter());
nit: why do we DCHECK these values only in this method?

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util.h (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.h:14: // manner to native
filesystem.
How about: "This class handles native file system operations with media type
filtering which is passed to each method via FileSystemOperationContext"

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util_unittest.cc (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:132:
TEST_F(NativeMediaFileUtilTest, FilteringTest) {
For easier diagnosis could we split this test into two or three (e.g. one for
FooExists and the other for ReadDirectory)?  The setup part could be in a
separate method or in SetUp().

[tzik, 2012-07-31 22:31:23]

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_types.h (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/file_system_...
webkit/fileapi/file_system_types.h:41: kFileSystemTypeDeviceMedia,
On 2012/07/31 01:26:35, kinuko wrote:
> nit: Can you add a brief comment to each type to reflect what you/Kausalya
> agreed?

Done.

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.cc:56: }  // anonymous namespace
On 2012/07/31 01:26:35, kinuko wrote:
> style-nit: "anonymous" could be removed

Done.

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.cc:125:
DCHECK(context->media_path_filter());
On 2012/07/31 01:26:35, kinuko wrote:
> nit: why do we DCHECK these values only in this method?

No special reason. I just forgot them.

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util.h (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util.h:14: // manner to native
filesystem.
On 2012/07/31 01:26:35, kinuko wrote:
> How about: "This class handles native file system operations with media type
> filtering which is passed to each method via FileSystemOperationContext"

Done.

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
File webkit/fileapi/media/native_media_file_util_unittest.cc (right):

http://codereview.chromium.org/10825042/diff/9008/webkit/fileapi/media/native...
webkit/fileapi/media/native_media_file_util_unittest.cc:132:
TEST_F(NativeMediaFileUtilTest, FilteringTest) {
On 2012/07/31 01:26:35, kinuko wrote:
> For easier diagnosis could we split this test into two or three (e.g. one for
> FooExists and the other for ReadDirectory)?  The setup part could be in a
> separate method or in SetUp().

Done.

[kinuko, 2012-07-31 23:36:50]

lgtm

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/media...
File webkit/fileapi/media/media_path_filter.h (right):

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/media...
webkit/fileapi/media/media_path_filter.h:21: typedef
std::vector<FilePath::StringType> MediaFileExtensionList;
nit: this could be probably private?

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/nativ...
File webkit/fileapi/media/native_media_file_util_unittest.cc (right):

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/nativ...
webkit/fileapi/media/native_media_file_util_unittest.cc:112:
isolated_context()->CreateVirtualRootPath(filesystem_id_);
this is not used?

[tzik, 2012-08-01 00:54:05]

kinuko: Thanks for reviewing.

kmadhusu, vandebo: Could you take a look to it? Does it work well for media
stuff?

avi: PTAL to content_tests.gypi. Can I add a test file to content_unittests?

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/media...
File webkit/fileapi/media/media_path_filter.h (right):

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/media...
webkit/fileapi/media/media_path_filter.h:21: typedef
std::vector<FilePath::StringType> MediaFileExtensionList;
On 2012/07/31 23:36:50, kinuko wrote:
> nit: this could be probably private?

Done.

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/nativ...
File webkit/fileapi/media/native_media_file_util_unittest.cc (right):

http://codereview.chromium.org/10825042/diff/13020/webkit/fileapi/media/nativ...
webkit/fileapi/media/native_media_file_util_unittest.cc:112:
isolated_context()->CreateVirtualRootPath(filesystem_id_);
On 2012/07/31 23:36:50, kinuko wrote:
> this is not used?

Done.

[vandebo (ex-Chrome), 2012-08-01 01:11:04]

LGTM

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/isolated_mount...
File webkit/fileapi/isolated_mount_point_provider.h (right):

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/isolated_mount...
webkit/fileapi/isolated_mount_point_provider.h:61: scoped_ptr<MediaPathFilter> 
media_path_filter_;
nit: two spaces

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/media_pa...
File webkit/fileapi/media/media_path_filter.cc (right):

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/media_pa...
webkit/fileapi/media/media_path_filter.cc:39:
std::sort(media_file_extensions_.begin(), media_file_extensions_.end());
For me information, what is the list of extensions we currently end up with
here?

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/native_m...
File webkit/fileapi/media/native_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/native_m...
webkit/fileapi/media/native_media_file_util.cc:5: #include
"webkit/fileapi/media/native_media_file_util.h"
Maybe I'm wrong, but it seems that in the end, this file is going to be a copy
of native_file_util plus code to apply the media filter?  Would it be easier to
maintain the two classes by having just one copy that knows how to use the
filter but has no filter in the non-media case?

[Avi (use Gerrit), 2012-08-01 01:17:51]

adding test to gypi++

LGTM

[kmadhusu, 2012-08-01 01:54:37]

lgtm

[tzik, 2012-08-01 21:46:05]

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/isolated_mount...
File webkit/fileapi/isolated_mount_point_provider.h (right):

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/isolated_mount...
webkit/fileapi/isolated_mount_point_provider.h:61: scoped_ptr<MediaPathFilter> 
media_path_filter_;
On 2012/08/01 01:11:05, vandebo wrote:
> nit: two spaces

Done.

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/media_pa...
File webkit/fileapi/media/media_path_filter.cc (right):

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/media_pa...
webkit/fileapi/media/media_path_filter.cc:39:
std::sort(media_file_extensions_.begin(), media_file_extensions_.end());
On 2012/08/01 01:11:05, vandebo wrote:
> For me information, what is the list of extensions we currently end up with
> here?

It contains:
.bmp .gif .jfif .jpeg .jpg .oga .ogg .ogm .ogv .pjp .pjpeg .png .svg .wav .webm
.webm .webp .xbm

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/native_m...
File webkit/fileapi/media/native_media_file_util.cc (right):

http://codereview.chromium.org/10825042/diff/36/webkit/fileapi/media/native_m...
webkit/fileapi/media/native_media_file_util.cc:5: #include
"webkit/fileapi/media/native_media_file_util.h"
On 2012/08/01 01:11:05, vandebo wrote:
> Maybe I'm wrong, but it seems that in the end, this file is going to be a copy
> of native_file_util plus code to apply the media filter?  Would it be easier
to
> maintain the two classes by having just one copy that knows how to use the
> filter but has no filter in the non-media case?

NativeFileUtil is no longer a subclass of FileSystemFileUtil, it is just a set
of helper function.
And FileUtil's are directly call file_util:: functions instead of
NativeFileUtil.
So, I'd like not to merge it to NativeFileUtil.

Should we use layering? Hmm....

[commit-bot: I haz the power, 2012-08-01 21:46:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10825042/9012

[commit-bot: I haz the power, 2012-08-01 21:46:36]

Presubmit check for 10825042-9012 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/intents

Presubmit checks took 2.6s to calculate.

[Lei Zhang, 2012-08-01 22:00:27]

chrome/ lgtm

[tzik, 2012-08-01 22:04:50]

On 2012/08/01 22:00:27, Lei Zhang wrote:
> chrome/ lgtm

Ah, I forgot ask you to review, Lei.
Thanks!

[commit-bot: I haz the power, 2012-08-01 22:05:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10825042/9012

[commit-bot: I haz the power, 2012-08-01 22:33:29]

Try job failure for 10825042-9012 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-01 22:35:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10825042/9015

[commit-bot: I haz the power, 2012-08-01 23:49:19]

Try job failure for 10825042-9015 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-02 00:57:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10825042/16009

[commit-bot: I haz the power, 2012-08-02 02:55:30]

Change committed as 149566

[Mihai Parparita -not on Chrome, 2012-08-03 00:45:05]

This causes the following assert when loading an unpacked extension in a debug
build on Linux:

[7122:7122:8144315839934:FATAL:thread_restrictions.cc(38)] Function marked as
IO-only was called from a thread that disallows IO!  If this thread really
should be allowed to make IO calls, adjust the call to
base::ThreadRestrictions::SetIOAllowed() in this thread's startup.
Backtrace:
	base::debug::StackTrace::StackTrace() [0x7fa8d7489156]
	logging::LogMessage::~LogMessage() [0x7fa8d74bccdf]
	base::ThreadRestrictions::AssertIOAllowed() [0x7fa8d7536eb1]
	base::nix::GetFileMimeType() [0x7fa8d755569a]
	net::PlatformMimeUtil::GetPlatformMimeTypeFromExtension() [0x7fa8d6d67933]
	net::MimeUtil::GetMimeTypeFromExtensionHelper() [0x7fa8d6d3615e]
	net::MimeUtil::GetMimeTypeFromExtension() [0x7fa8d6d35fd2]
	net::GetMimeTypeFromExtension() [0x7fa8d6d37699]
	fileapi::(anonymous namespace)::IsUnsupportedExtension() [0x7fa8d6611a28]
	std::__find_if<>() [0x7fa8d6612541]
	std::find_if<>() [0x7fa8d661215d]
	std::remove_if<>() [0x7fa8d6611e20]
	fileapi::MediaPathFilter::MediaPathFilter() [0x7fa8d6611c28]
	fileapi::IsolatedMountPointProvider::IsolatedMountPointProvider()
[0x7fa8d66031dd]
	fileapi::FileSystemContext::FileSystemContext() [0x7fa8d65c941a]
	CreateFileSystemContext() [0x7fa8ce09bf95]
	content::StoragePartition::Create() [0x7fa8ce2a203b]
	content::StoragePartitionMap::Get() [0x7fa8ce2a2d3e]
	content::(anonymous namespace)::GetStoragePartition() [0x7fa8cdfc28db]
	content::BrowserContext::GetDOMStorageContext() [0x7fa8cdfc2e80]
	content::RenderProcessHostImpl::CreateMessageFilters() [0x7fa8ce1fb4c8]
	content::RenderProcessHostImpl::Init() [0x7fa8ce1fae10]
	content::RenderViewHostImpl::CreateRenderView() [0x7fa8ce20ed8c]
	WebContentsImpl::CreateRenderViewForRenderManager() [0x7fa8ce2ca2c2]
	RenderViewHostManager::InitRenderView() [0x7fa8ce2b79d3]
	RenderViewHostManager::CreateRenderView() [0x7fa8ce2b78ac]
	RenderViewHostManager::UpdateRendererStateForNavigate() [0x7fa8ce2b84aa]
	RenderViewHostManager::Navigate() [0x7fa8ce2b6092]
	WebContentsImpl::NavigateToEntry() [0x7fa8ce2c3ac3]
	WebContentsImpl::NavigateToPendingEntry() [0x7fa8ce2c39a6]
	NavigationControllerImpl::NavigateToPendingEntry() [0x7fa8ce2b1aaa]
	NavigationControllerImpl::LoadEntry() [0x7fa8ce2ad662]
	NavigationControllerImpl::LoadURLWithUserAgentOverride() [0x7fa8ce2ae59d]
	NavigationControllerImpl::LoadURL() [0x7fa8ce2ae473]
	extensions::ExtensionHost::LoadInitialURL() [0x7fa8dacf0eef]
	extensions::ExtensionHost::CreateRenderViewNow() [0x7fa8dacf0c2c]
	extensions::ExtensionHost::ProcessCreationQueue::ProcessOneHost()
[0x7fa8dacf2f19]
	base::internal::RunnableAdapter<>::Run() [0x7fa8dacf3fed]
	base::internal::InvokeHelper<>::MakeItSo() [0x7fa8dacf3ed0]
	base::internal::Invoker<>::Run() [0x7fa8dacf3d99]
	base::Callback<>::Run() [0x7fa8d74812f9]
	MessageLoop::RunTask() [0x7fa8d74c2df6]
	MessageLoop::DeferOrRunPendingTask() [0x7fa8d74c2f0e]
	MessageLoop::DoWork() [0x7fa8d74c3729]
	base::MessagePumpGlib::RunWithDispatcher() [0x7fa8d746721e]
	base::MessagePumpGlib::Run() [0x7fa8d74675fe]
	MessageLoop::RunInternal() [0x7fa8d74c2abb]
	MessageLoop::RunHandler() [0x7fa8d74c2972]
	base::RunLoop::Run() [0x7fa8d74f6246]
	ChromeBrowserMainParts::MainMessageLoopRun() [0x7fa8da6a53ae]
	content::BrowserMainLoop::RunMainMessageLoopParts() [0x7fa8cdfc6d91]
	(anonymous namespace)::BrowserMainRunnerImpl::Run() [0x7fa8cdfc92b2]
	BrowserMain() [0x7fa8cdfc4934]
	content::RunNamedProcessTypeMain() [0x7fa8cdfa0322]
	content::ContentMainRunnerImpl::Run() [0x7fa8cdfa1168]
	content::ContentMain() [0x7fa8cdf9f989]
	ChromeMain [0x7fa8d9b8978d]
	main [0x7fa8d9b8974c]
	0x7fa8c6831c4d
	0x7fa8d9b89659

[kinuko, 2012-08-03 01:23:38]

http://crbug.com/140401