Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15624003:
Validate image files before writing them to media galleries. (Closed)
|
Created:
7 years, 7 months ago by vandebo (ex-Chrome) Modified:
7 years, 6 months ago CC:
chromium-reviews, tzik+watch_chromium.org, jam, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionValidate image files before writing them to media galleries.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203038
Patch Set 1 #
Total comments: 46
Patch Set 2 : Address comments and rebase #
Total comments: 22
Patch Set 3 : Rebase, address comments, unit test. #
Total comments: 26
Patch Set 4 : Address comments #Patch Set 5 : Override #Messages
Total messages: 17 (0 generated)
Still working on the unit test, but we can start the review.
https://codereview.chromium.org/15624003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:189: policy->GrantCreateFileForFileSystem(child_id, filesystems[i].fsid); whitespace. But does this need to get gated by "if (has_write_permission)" which would get set up by a parallel to kReadPermission? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc:138: case fileapi::kFileSystemTypeItunes: Bad merge? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc:151: MediaFileSystemMountPointProvider::InitializeCopyOrMoveFileValidatorFactory( How about just removing this and call sites? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:23: virtual void StartValidation( The signature I'd expect here is StartValidation($FILE, callback). https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:46: if (SupportedImageTypeValidator::SupportsFileType(src_path)) I'd move all this logic to the SupportedImageTypeValidator. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:48: // TODO(vandebo) Support other file types. ":" https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.h (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.h:17: class comment https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.h:25: const fileapi::FileSystemURL& src, Why does the factory method take these args? They seem unexpected for something that will validate either a particular file, or files in general. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:25: const int kMaxImageFileSize = 50*1014*1024; Do we have this limit available somewhere lower in the analysis code to use? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:38: scoped_ptr<std::string> ReadOnFileThread(const base::FilePath& path) { Would it be better to pass the ScopedPlatformFile back and have the validator read it? I could see us only needing to read a very small portion of the file sometimes, but sometimes needing larger portions. And being able to consume less memory seems nice. nm, I see ImageDecoder needs the whole image. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:49: file_info.size > kMaxImageFileSize) { Do we just want to read the first kMaxImageFileSize bytes? I think this supports returning the file handle. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:83: base::Bind(&ImageDecoderDelegateAdapter::OnDecodeDoneOnIOThread, Can we just run the callback_ directly from here? base::Bind(callback_, base::PLATFORM_FILE_???). Then you could just call delete(this) or scoped_ptr(this) instead of using base::Owned, as well, which is a bit more clear. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:117: return extension == FILE_PATH_LITERAL(".gif") || Is this list going to grow big enough that we ought to put the extentions into a list and iterate? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:121: extension == FILE_PATH_LITERAL(".jpg") || How about sorting the list? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:147: void SupportedImageTypeValidator::OnValidateData(scoped_ptr<std::string> data) { I think this should be "OnOpenFile" or "OnReadFile". Given the name I'm expecting the validation to be run above, not here. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:156: ImageDecoderDelegateAdapter* adapter = Is there a way we can hook this up earlier, so that the filesystem api runs the validator while it is streaming the bytes out, instead of after they're already received? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.h (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:20: class SupportedImageTypeValidator : public fileapi::CopyOrMoveFileValidator { Class comment. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:26: virtual void StartValidation( Even if these are per-file, I'd still expect the API signature to be SupportedImageTypeValidator() and StartValidation($FILE, callback). path_ isn't really used outside StartValidation, so we can avoid that object member (and hopefully the callback_ member too...) https://codereview.chromium.org/15624003/diff/1/webkit/fileapi/mock_file_syst... File webkit/fileapi/mock_file_system_context.cc (right): https://codereview.chromium.org/15624003/diff/1/webkit/fileapi/mock_file_syst... webkit/fileapi/mock_file_system_context.cc:30: ScopedVector<FileSystemMountPointProvider> additional_providers, Does this need to add the TestMountPointProvider? If not, perhaps note that in the .h file. Is that class public and can be added by callers?
Still working on the unit test, but forgot to add the file in the last patch set. https://codereview.chromium.org/15624003/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:189: policy->GrantCreateFileForFileSystem(child_id, filesystems[i].fsid); On 2013/05/22 16:21:59, Greg Billock wrote: > whitespace. But does this need to get gated by "if (has_write_permission)" which > would get set up by a parallel to kReadPermission? Oops, this is only for debugging. There are another set of changes need for write permissions that are not in scope for this CL. Removed. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc:138: case fileapi::kFileSystemTypeItunes: On 2013/05/22 16:21:59, Greg Billock wrote: > Bad merge? No, we'll need this to support writing to iTunes's "import" directory. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc:151: MediaFileSystemMountPointProvider::InitializeCopyOrMoveFileValidatorFactory( On 2013/05/22 16:21:59, Greg Billock wrote: > How about just removing this and call sites? CopyorMoveValidator was added as a generic mechanism and this method is needed for the test for the generic mechanism. However, it is no longer needed for our use. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:23: virtual void StartValidation( On 2013/05/22 16:21:59, Greg Billock wrote: > The signature I'd expect here is StartValidation($FILE, callback). $FILE is passed into the factory so you can create an appropriate validator. The caller doesn't want to pass it twice, and it should be able to pass in a different file than it passed to the factory. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:46: if (SupportedImageTypeValidator::SupportsFileType(src_path)) On 2013/05/22 16:21:59, Greg Billock wrote: > I'd move all this logic to the SupportedImageTypeValidator. Not sure what you mean, SupportsFileType is a member of SupportedImageTypeValidator. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:48: // TODO(vandebo) Support other file types. On 2013/05/22 16:21:59, Greg Billock wrote: > ":" Done. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.h (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.h:17: On 2013/05/22 16:21:59, Greg Billock wrote: > class comment Done. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.h:25: const fileapi::FileSystemURL& src, On 2013/05/22 16:21:59, Greg Billock wrote: > Why does the factory method take these args? They seem unexpected for something > that will validate either a particular file, or files in general. There is a comment explaining the args in the parent class. Added a comment to indicate that this is an override. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:25: const int kMaxImageFileSize = 50*1014*1024; On 2013/05/22 16:21:59, Greg Billock wrote: > Do we have this limit available somewhere lower in the analysis code to use? Not sure what you mean. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:49: file_info.size > kMaxImageFileSize) { On 2013/05/22 16:21:59, Greg Billock wrote: > Do we just want to read the first kMaxImageFileSize bytes? I think this supports > returning the file handle. ImageDecoder needs the whole file. GetPlatformFileInfo is effectively stat(). https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:83: base::Bind(&ImageDecoderDelegateAdapter::OnDecodeDoneOnIOThread, On 2013/05/22 16:21:59, Greg Billock wrote: > Can we just run the callback_ directly from here? > base::Bind(callback_, base::PLATFORM_FILE_???). I thought I tried that before and it didn't work. Seems to work now. Done. > Then you could just call delete(this) or scoped_ptr(this) instead of using > base::Owned, as well, which is a bit more clear. Done. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:117: return extension == FILE_PATH_LITERAL(".gif") || On 2013/05/22 16:21:59, Greg Billock wrote: > Is this list going to grow big enough that we ought to put the extentions into a > list and iterate? I do not expect this list to grow. Since this is a static method, I'd have to use a lazy instance or something similar to construct the list, so I don't think it's worthwhile. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:121: extension == FILE_PATH_LITERAL(".jpg") || On 2013/05/22 16:21:59, Greg Billock wrote: > How about sorting the list? Done. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:147: void SupportedImageTypeValidator::OnValidateData(scoped_ptr<std::string> data) { On 2013/05/22 16:21:59, Greg Billock wrote: > I think this should be "OnOpenFile" or "OnReadFile". Given the name I'm > expecting the validation to be run above, not here. Done. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:156: ImageDecoderDelegateAdapter* adapter = On 2013/05/22 16:21:59, Greg Billock wrote: > Is there a way we can hook this up earlier, so that the filesystem api runs the > validator while it is streaming the bytes out, instead of after they're already > received? I'm not sure what you mean. This is validating a file before it is copied or moved. It is already on disk and we're going to copy or move it to a different location. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.h (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:20: class SupportedImageTypeValidator : public fileapi::CopyOrMoveFileValidator { On 2013/05/22 16:21:59, Greg Billock wrote: > Class comment. Done. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.h:26: virtual void StartValidation( On 2013/05/22 16:21:59, Greg Billock wrote: > Even if these are per-file, I'd still expect the API signature to be > SupportedImageTypeValidator() and StartValidation($FILE, callback). path_ isn't > really used outside StartValidation, so we can avoid that object member (and > hopefully the callback_ member too...) See previous reply. https://codereview.chromium.org/15624003/diff/1/webkit/fileapi/mock_file_syst... File webkit/fileapi/mock_file_system_context.cc (right): https://codereview.chromium.org/15624003/diff/1/webkit/fileapi/mock_file_syst... webkit/fileapi/mock_file_system_context.cc:30: ScopedVector<FileSystemMountPointProvider> additional_providers, On 2013/05/22 16:21:59, Greg Billock wrote: > Does this need to add the TestMountPointProvider? If not, perhaps note that in > the .h file. Is that class public and can be added by callers? Added a comment about it being up to the caller.
https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:23: virtual void StartValidation( On 2013/05/22 18:41:17, vandebo wrote: > On 2013/05/22 16:21:59, Greg Billock wrote: > > The signature I'd expect here is StartValidation($FILE, callback). > > $FILE is passed into the factory so you can create an appropriate validator. > The caller doesn't want to pass it twice, and it should be able to pass in a > different file than it passed to the factory. The same validator always gets created, right? https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:46: if (SupportedImageTypeValidator::SupportsFileType(src_path)) On 2013/05/22 18:41:17, vandebo wrote: > On 2013/05/22 16:21:59, Greg Billock wrote: > > I'd move all this logic to the SupportedImageTypeValidator. > > Not sure what you mean, SupportsFileType is a member of > SupportedImageTypeValidator. Meaning I'd have this method just return new SupportedImageTypeValidator(path). Then have the StartValidation routine make this call to SupportsFileType, and return the sec error if not. Then you don't need the whole InvalidFileValidator class at all, and the whole validation operation resides in one spot, instead of split between here and the actual validator. It'd also mean you can ditch the constructor argument, which is odd, and perhaps even ditch the factory entirely, which would be even better. Is there a plan to make all different sorts of validators? That would motivate this, but it seems overkill for one. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:25: const int kMaxImageFileSize = 50*1014*1024; On 2013/05/22 18:41:17, vandebo wrote: > On 2013/05/22 16:21:59, Greg Billock wrote: > > Do we have this limit available somewhere lower in the analysis code to use? > > Not sure what you mean. Meaning is there an ImageDecoder::MAX_FILE_SIZE or something. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:49: file_info.size > kMaxImageFileSize) { On 2013/05/22 18:41:17, vandebo wrote: > On 2013/05/22 16:21:59, Greg Billock wrote: > > Do we just want to read the first kMaxImageFileSize bytes? I think this > supports > > returning the file handle. > > ImageDecoder needs the whole file. GetPlatformFileInfo is effectively stat(). Yeah. I should have put this under ReadPlatformFile I guess. Anyway, if the decoder needs the whole thing, then... https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:156: ImageDecoderDelegateAdapter* adapter = On 2013/05/22 18:41:17, vandebo wrote: > On 2013/05/22 16:21:59, Greg Billock wrote: > > Is there a way we can hook this up earlier, so that the filesystem api runs > the > > validator while it is streaming the bytes out, instead of after they're > already > > received? > > I'm not sure what you mean. This is validating a file before it is copied or > moved. It is already on disk and we're going to copy or move it to a different > location. Right. If there were a hook we could insert this as it's getting written, that'd be nice -- we already have it in memory at that point, so we wouldn't have to work through the disk again.
https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:23: virtual void StartValidation( On 2013/05/23 18:18:29, Greg Billock wrote: > On 2013/05/22 18:41:17, vandebo wrote: > > On 2013/05/22 16:21:59, Greg Billock wrote: > > > The signature I'd expect here is StartValidation($FILE, callback). > > > > $FILE is passed into the factory so you can create an appropriate validator. > > The caller doesn't want to pass it twice, and it should be able to pass in a > > different file than it passed to the factory. > > The same validator always gets created, right? Right now we only have one validator because that's all I've done. But we'll need more to support audio, video, other image formats, etc. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:46: if (SupportedImageTypeValidator::SupportsFileType(src_path)) On 2013/05/23 18:18:29, Greg Billock wrote: > On 2013/05/22 18:41:17, vandebo wrote: > > On 2013/05/22 16:21:59, Greg Billock wrote: > > > I'd move all this logic to the SupportedImageTypeValidator. > > > > Not sure what you mean, SupportsFileType is a member of > > SupportedImageTypeValidator. > > Meaning I'd have this method just return new SupportedImageTypeValidator(path). > Then have the StartValidation routine make this call to SupportsFileType, and > return the sec error if not. Then you don't need the whole InvalidFileValidator > class at all, and the whole validation operation resides in one spot, instead of > split between here and the actual validator. It'd also mean you can ditch the > constructor argument, which is odd, and perhaps even ditch the factory entirely, > which would be even better. > > Is there a plan to make all different sorts of validators? That would motivate > this, but it seems overkill for one. Yes, there will be multiple validators and I will need to figure out which one to use. The factory may be a bit overkill - it was designed when all the code still lived in webkit/fileapi. Now I could just have the code from this function live in MediaFileSystemMountPointProvider, but that means that the mount point provider will have to be able to depend on the validator in the generic mechanism. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:25: const int kMaxImageFileSize = 50*1014*1024; On 2013/05/23 18:18:29, Greg Billock wrote: > On 2013/05/22 18:41:17, vandebo wrote: > > On 2013/05/22 16:21:59, Greg Billock wrote: > > > Do we have this limit available somewhere lower in the analysis code to use? > > > > Not sure what you mean. > > Meaning is there an ImageDecoder::MAX_FILE_SIZE or something. This isn't a limit of ImageDecoder, it's an arbitrary limit imposed her to ensure that we don't spend too much time/memory trying to decode a potentially malicious image. https://codereview.chromium.org/15624003/diff/1/chrome/browser/media_gallerie... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:156: ImageDecoderDelegateAdapter* adapter = On 2013/05/23 18:18:29, Greg Billock wrote: > On 2013/05/22 18:41:17, vandebo wrote: > > On 2013/05/22 16:21:59, Greg Billock wrote: > > > Is there a way we can hook this up earlier, so that the filesystem api runs > > the > > > validator while it is streaming the bytes out, instead of after they're > > already > > > received? > > > > I'm not sure what you mean. This is validating a file before it is copied or > > moved. It is already on disk and we're going to copy or move it to a > different > > location. > > Right. If there were a hook we could insert this as it's getting written, that'd > be nice -- we already have it in memory at that point, so we wouldn't have to > work through the disk again. No, there isn't really a hook to do it while copying, but that would kind of defeat the point, since we'd have to delete it if the validation failed. However, the OS buffer cache will save us in this case, the disk will probably only get hit once.
+thestig, Greg is out on vacation for a day or two, can you take a look?
Still looking... https://codereview.chromium.org/15624003/diff/11001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/15624003/diff/11001/content/content_tests.gyp... content/content_tests.gypi:473: '../webkit/fileapi/copy_or_move_file_validator_unittest.cc', file doesn't exist? https://codereview.chromium.org/15624003/diff/11001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/15624003/diff/11001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator.h:16: namespace base { merge error? ^^ https://codereview.chromium.org/15624003/diff/11001/webkit/fileapi/mock_file_... File webkit/fileapi/mock_file_system_context.h (right): https://codereview.chromium.org/15624003/diff/11001/webkit/fileapi/mock_file_... webkit/fileapi/mock_file_system_context.h:26: // |additiona_providers| if needed. typo
https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:10: #include "content/public/browser/browser_thread.h" not used? ditto with line 14. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.h (right): https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.h:9: #include "base/files/file_path.h" forward decl https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:35: typedef ScopedGenericObj<base::PlatformFile, PlatformFileCloser> I think you can replace this with a scoped_ptr that has a custom deleter. ScopedGenericObj seems to be on its way out. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:49: file_info.size > kMaxImageFileSize) { Is this to prevent the utility process from going OOM? How does the rest of the ImageDecoder users handle large images? https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:53: scoped_ptr<char[]> data(new char[file_info.size]); You can probably get rid of |data| and the need to construct a new std::string if you turn |empty_result| into |result|, and here do: result->resize(file_info.size); ... ReadPlatformFile(..., string_as_array(result), ...) ... string_as_array() is in base/stl_util.h. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:78: const SkBitmap& /*decoded_image*/) { OVERRIDE, same with failed. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:119: callback_ = result_callback; It seems the current usage is to create a new validator per file, so this method is not re-entrant. At least DCHECK(!callback_) as a pre-condition? https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:138: if (!data.get()) { What about when the file is 0 bytes?
+kinuko for webkit/ changes. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc (right): https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.cc:10: #include "content/public/browser/browser_thread.h" On 2013/05/25 00:09:01, Lei Zhang wrote: > not used? ditto with line 14. Done. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_factory.h (right): https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_factory.h:9: #include "base/files/file_path.h" On 2013/05/25 00:09:01, Lei Zhang wrote: > forward decl Done. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:35: typedef ScopedGenericObj<base::PlatformFile, PlatformFileCloser> On 2013/05/25 00:09:01, Lei Zhang wrote: > I think you can replace this with a scoped_ptr that has a custom deleter. > ScopedGenericObj seems to be on its way out. Done. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:49: file_info.size > kMaxImageFileSize) { On 2013/05/25 00:09:01, Lei Zhang wrote: > Is this to prevent the utility process from going OOM? How does the rest of the > ImageDecoder users handle large images? It's a sanity check. An image file (in a supported format) that is > 50MB is unreasonable. I'm happy to adjust the limit if you like. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:53: scoped_ptr<char[]> data(new char[file_info.size]); On 2013/05/25 00:09:01, Lei Zhang wrote: > You can probably get rid of |data| and the need to construct a new std::string > if you turn |empty_result| into |result|, and here do: > > result->resize(file_info.size); > ... ReadPlatformFile(..., string_as_array(result), ...) ... > > string_as_array() is in base/stl_util.h. Done. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:78: const SkBitmap& /*decoded_image*/) { On 2013/05/25 00:09:01, Lei Zhang wrote: > OVERRIDE, same with failed. Done. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:119: callback_ = result_callback; On 2013/05/25 00:09:01, Lei Zhang wrote: > It seems the current usage is to create a new validator per file, so this method > is not re-entrant. At least DCHECK(!callback_) as a pre-condition? Done. https://codereview.chromium.org/15624003/diff/11001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:138: if (!data.get()) { On 2013/05/25 00:09:01, Lei Zhang wrote: > What about when the file is 0 bytes? It can't happen because a zero byte file will not pass the mime checker. But by the same reason, it can't be a valid image file at zero bytes. https://codereview.chromium.org/15624003/diff/11001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/15624003/diff/11001/content/content_tests.gyp... content/content_tests.gypi:473: '../webkit/fileapi/copy_or_move_file_validator_unittest.cc', On 2013/05/24 22:50:44, Lei Zhang wrote: > file doesn't exist? Done. https://codereview.chromium.org/15624003/diff/11001/webkit/browser/fileapi/co... File webkit/browser/fileapi/copy_or_move_file_validator.h (right): https://codereview.chromium.org/15624003/diff/11001/webkit/browser/fileapi/co... webkit/browser/fileapi/copy_or_move_file_validator.h:16: namespace base { On 2013/05/24 22:50:44, Lei Zhang wrote: > merge error? ^^ Done. https://codereview.chromium.org/15624003/diff/11001/webkit/fileapi/mock_file_... File webkit/fileapi/mock_file_system_context.h (right): https://codereview.chromium.org/15624003/diff/11001/webkit/fileapi/mock_file_... webkit/fileapi/mock_file_system_context.h:26: // |additiona_providers| if needed. On 2013/05/24 22:50:44, Lei Zhang wrote: > typo Done.
webkit/ lgtm
https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc (right): https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc:140: NOTREACHED(); This method is gone now. (Just landed) https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc (right): https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:69: // it into a media file system. The result is compared to |exepcted_result|. typo https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:87: base::FilePath src_path = base_dir.Append(FILE_PATH_LITERAL("src_fs")); Just AppendASCII() instead? Ditto below. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:88: file_util::CreateDirectory(src_path); ASSERT_TRUE? Ditto below. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:101: base::FilePath::FromUTF8Unsafe(filename.c_str())); omit the .c_str() https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:104: base::FilePath test_file = src_path.AppendASCII(filename.c_str()); Ditto? I think you can omit it. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:114: size_t extension_index = filename.find_last_of("."); Maybe |filename| should just be a FilePath so you don't have to do this. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:200: MessageLoop::current()->Quit(); needs base::, the crutch in message_loop.h will go away soon. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:203: fileapi::FileSystemOperation* CreateFSOp(fileapi::FileSystemURL url) { |url| can be const ref. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:207: base::ScopedTempDir base_; how about base_dir_? https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:11: #include "base/memory/scoped_generic_obj.h" This file no longer exists. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:26: const int kMaxImageFileSize = 50*1014*1024; You mentioned images > 50 MB is unreasonable. Please add a comment here. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:29: void operator()(base::PlatformFile* file) const { no const to be consistent with the other scoped_ptr custom deleters.
Address comments
https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc (right): https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.cc:140: NOTREACHED(); On 2013/05/29 09:40:58, Lei Zhang wrote: > This method is gone now. (Just landed) Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc (right): https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:69: // it into a media file system. The result is compared to |exepcted_result|. On 2013/05/29 09:40:58, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:87: base::FilePath src_path = base_dir.Append(FILE_PATH_LITERAL("src_fs")); On 2013/05/29 09:40:58, Lei Zhang wrote: > Just AppendASCII() instead? Ditto below. Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:88: file_util::CreateDirectory(src_path); On 2013/05/29 09:40:58, Lei Zhang wrote: > ASSERT_TRUE? Ditto below. Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:101: base::FilePath::FromUTF8Unsafe(filename.c_str())); On 2013/05/29 09:40:58, Lei Zhang wrote: > omit the .c_str() Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:104: base::FilePath test_file = src_path.AppendASCII(filename.c_str()); On 2013/05/29 09:40:58, Lei Zhang wrote: > Ditto? I think you can omit it. Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:114: size_t extension_index = filename.find_last_of("."); On 2013/05/29 09:40:58, Lei Zhang wrote: > Maybe |filename| should just be a FilePath so you don't have to do this. GURL takes std::string/16, not FilePath::StringType. I could do an ifdef for windows, but for a unit test I think this is ok. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:200: MessageLoop::current()->Quit(); On 2013/05/29 09:40:58, Lei Zhang wrote: > needs base::, the crutch in message_loop.h will go away soon. Fixed up in a cleaner way. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:203: fileapi::FileSystemOperation* CreateFSOp(fileapi::FileSystemURL url) { On 2013/05/29 09:40:58, Lei Zhang wrote: > |url| can be const ref. Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/media_file_validator_unittest.cc:207: base::ScopedTempDir base_; On 2013/05/29 09:40:58, Lei Zhang wrote: > how about base_dir_? Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc (right): https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:11: #include "base/memory/scoped_generic_obj.h" On 2013/05/29 09:40:58, Lei Zhang wrote: > This file no longer exists. Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:26: const int kMaxImageFileSize = 50*1014*1024; On 2013/05/29 09:40:58, Lei Zhang wrote: > You mentioned images > 50 MB is unreasonable. Please add a comment here. Done. https://codereview.chromium.org/15624003/diff/29001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:29: void operator()(base::PlatformFile* file) const { On 2013/05/29 09:40:58, Lei Zhang wrote: > no const to be consistent with the other scoped_ptr custom deleters. DefaultDeleter and FreeDeleter have const?
Override
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/15624003/40002
Message was sent while issue was closed.
Change committed as 203038 |
