Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2914433002:
arc: Use the MIME type returned by the container to handle content URLs (Closed)
|
Created:
3 years, 6 months ago by hashimoto Modified:
3 years, 6 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, darin (slow to review), oshima+watch_chromium.org, yusukes+watch_chromium.org, tzik, hidehiko+watch_chromium.org, oka+watch_chromium.org, viettrungluu+watch_chromium.org, nhiroki, abarth-chromium, Aaron Boodman, rginda+watch_chromium.org, lhchavez+watch_chromium.org, yzshen+watch_chromium.org, victorhsieh+watch_chromium.org, fukino+watch_chromium.org, yamaguchi+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org, qsr+mojo_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Use the MIME type returned by the container to handle content URLs
Currently, when displaying an ARC file specified by a content URL, we are using MIME type guessed by looking at the extension part of the URL.
This doesn't always work so we should ask the ContentResolver running in the container to get the actual MIME type.
- Add GetMimeType to FileSystemInstance interface.
- Add arc::PathToArcUrl to arc_content_file_system_url_util
- Use GetMimeType and arc::PathToArcUrl in filesystem_api_util.cc
BUG=704701
TEST=Download a image file to the device's Downloads directory. Open chrome://settings/androidApps/details -> "Manage Android
preferences" -> Storage -> Images -> Download -> <the downloaded image file>.
Verify that the image file is displayed correctly, not as garbage text.
Review-Url: https://codereview.chromium.org/2914433002
Cr-Commit-Position: refs/heads/master@{#476953}
Committed: https://chromium.googlesource.com/chromium/src/+/aa692120241e0bfd072ac7b1b34ff26fcc6aa838
Patch Set 1 #Patch Set 2 : Fix tests #
Total comments: 2
Patch Set 3 : Use ArcFileSystemOperationRunner #
Total comments: 2
Patch Set 4 : Profile check #
Total comments: 6
Patch Set 5 : Address comments #
Total comments: 13
Patch Set 6 : rebase #Patch Set 7 : Address comments #
Total comments: 8
Patch Set 8 : Comment drop. #Messages
Total messages: 45 (25 generated)
The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== arc: Use the MIME type returned by the container to handle content URLs BUG=704701 TEST=Download a image file to the device's Downloads directory. Open chrome://settings/androidApps/details -> "Manage Android preferences" -> Storage -> Images -> Download -> <the downloaded image file>. Verify that the image file is displayed correctly, not as garbage text. ========== to ========== arc: Use the MIME type returned by the container to handle content URLs Currently, when displaying an ARC file specified by a content URL, we are using MIME type guessed by looking at the extension part of the URL. This doesn't always work so we should ask the ContentResolver running in the container to get the actual MIME type. BUG=704701 TEST=Download a image file to the device's Downloads directory. Open chrome://settings/androidApps/details -> "Manage Android preferences" -> Storage -> Images -> Download -> <the downloaded image file>. Verify that the image file is displayed correctly, not as garbage text. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== arc: Use the MIME type returned by the container to handle content URLs Currently, when displaying an ARC file specified by a content URL, we are using MIME type guessed by looking at the extension part of the URL. This doesn't always work so we should ask the ContentResolver running in the container to get the actual MIME type. BUG=704701 TEST=Download a image file to the device's Downloads directory. Open chrome://settings/androidApps/details -> "Manage Android preferences" -> Storage -> Images -> Download -> <the downloaded image file>. Verify that the image file is displayed correctly, not as garbage text. ========== to ========== arc: Use the MIME type returned by the container to handle content URLs Currently, when displaying an ARC file specified by a content URL, we are using MIME type guessed by looking at the extension part of the URL. This doesn't always work so we should ask the ContentResolver running in the container to get the actual MIME type. - Add GetMimeType to FileSystemInstance interface. - Add arc::PathToArcUrl to arc_content_file_system_url_util - Use GetMimeType and arc::PathToArcUrl in filesystem_api_util.cc BUG=704701 TEST=Download a image file to the device's Downloads directory. Open chrome://settings/androidApps/details -> "Manage Android preferences" -> Storage -> Images -> Download -> <the downloaded image file>. Verify that the image file is displayed correctly, not as garbage text. ==========
hashimoto@chromium.org changed reviewers: + nya@chromium.org
nya@, could you review this change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nya@google.com changed reviewers: + nya@google.com
https://codereview.chromium.org/2914433002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:205: auto* file_system_instance = ARC_GET_INSTANCE_FOR_METHOD( Please add ArcFileSystemOperationRunner::GetMimeType() and call it, rather than invoking IPC method directly. ArcFileSystemOperationRunner is the canonical interface for file system operations. It provides operation deferring for example. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/fileapi/arc_...
Thank you for reviewing! PTAL https://codereview.chromium.org/2914433002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:205: auto* file_system_instance = ARC_GET_INSTANCE_FOR_METHOD( On 2017/05/30 09:30:17, nya wrote: > Please add ArcFileSystemOperationRunner::GetMimeType() and call it, rather than > invoking IPC method directly. > > ArcFileSystemOperationRunner is the canonical interface for file system > operations. It provides operation deferring for example. > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/fileapi/arc_... Ugh, I totally forgot about that deferring feature, and was only thinking about the threading functionality provided by file_system_operation_runner_util. Done.
nya@chromium.org changed reviewers: - nya@google.com
Mostly looks good, thanks for the change! Please send me Android changes later. Also I suppose you can send this change to IPC reviewers now. https://codereview.chromium.org/2914433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:204: auto* runner = arc::ArcServiceManager::GetGlobalService< GetGlobalService() returns an instance regardless of |profile|, so we should check ARC is actually enabled for |profile|. Please check arc::IsArcAllowedForProfile(). https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_util.h?t...
hashimoto@chromium.org changed reviewers: + dcheng@chromium.org
nya@: PTAL dcheng@: Please review components/arc/common/file_system.mojom as a security owner. https://codereview.chromium.org/2914433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:204: auto* runner = arc::ArcServiceManager::GetGlobalService< On 2017/05/31 03:22:55, Shuhei Takahashi wrote: > GetGlobalService() returns an instance regardless of |profile|, so we should > check ARC is actually enabled for |profile|. Please check > arc::IsArcAllowedForProfile(). > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_util.h?t... Done.
The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nya@google.com changed reviewers: + nya@google.com
lgtm. Thanks!
nya@google.com changed reviewers: - nya@google.com
lgtm I happened to use a wrong account...
https://codereview.chromium.org/2914433002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2914433002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:121: FROM_HERE, base::Bind(callback, base::Optional<std::string>())); Let's use base::nullopt instead of base::Optional<T>() https://codereview.chromium.org/2914433002/diff/60001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/60001/components/arc/common/f... components/arc/common/file_system.mojom:109: // URL. Nit: document what it means if nothing is returned (and if that's distinct from the empty string) https://codereview.chromium.org/2914433002/diff/60001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2914433002/diff/60001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:180: FROM_HERE, base::Bind(callback, base::Optional<std::string>())); Ditto: prefer base::nullopt
dcheng@, thank you for reviewing! PTAL https://codereview.chromium.org/2914433002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2914433002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:121: FROM_HERE, base::Bind(callback, base::Optional<std::string>())); On 2017/06/01 11:06:44, dcheng wrote: > Let's use base::nullopt instead of base::Optional<T>() Sounds good. Done. https://codereview.chromium.org/2914433002/diff/60001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/60001/components/arc/common/f... components/arc/common/file_system.mojom:109: // URL. On 2017/06/01 11:06:44, dcheng wrote: > Nit: document what it means if nothing is returned (and if that's distinct from > the empty string) Done. https://codereview.chromium.org/2914433002/diff/60001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2914433002/diff/60001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:180: FROM_HERE, base::Bind(callback, base::Optional<std::string>())); On 2017/06/01 11:06:44, dcheng wrote: > Ditto: prefer base::nullopt Done.
ipc lgtm https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:69: const base::Callback<void(bool, const std::string&)>& callback, I didn't notice this earlier, but it might be nice to clean this up in a future CL to just pass an Optional<std::string> to avoid the need to default construct a std::string() in the false case.
hashimoto@chromium.org changed reviewers: + hidehiko@chromium.org, mtomasz@chromium.org
hidehiko@: Please review as an arc owner. mtomasz@: Please review as an owner of chrome/browser/chromeos/fileapi https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:69: const base::Callback<void(bool, const std::string&)>& callback, On 2017/06/02 15:58:29, dcheng wrote: > I didn't notice this earlier, but it might be nice to clean this up in a future > CL to just pass an Optional<std::string> to avoid the need to default construct > a std::string() in the false case. Sounds good. Filed crbug.com/729442
https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.h (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.h:45: // container. nit: Shall we add a comment when this can return an empty GURL, like in #36? https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:121: FROM_HERE, base::Bind(callback, base::nullopt)); nit: Bind -> BindOnce https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:209: base::Bind(callback, false, std::string())); nit: BindOnce https://codereview.chromium.org/2914433002/diff/80001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/80001/components/arc/common/f... components/arc/common/file_system.mojom:110: // empty string). optional: How are directories handled? On Android it's "vnd.android.document/directory" which may not make sense on Chrome OS side. Do we need to care about it? https://codereview.chromium.org/2914433002/diff/80001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2914433002/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:179: base::ThreadTaskRunnerHandle::Get()->PostTask( nit: ditto here and below. @tzik has been refactoring these Bind to BindOnce all over the codebase (where applicable).
Thank you for reviewing! PTAL https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.h (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.h:45: // container. On 2017/06/05 04:29:55, mtomasz wrote: > nit: Shall we add a comment when this can return an empty GURL, like in #36? Done. https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:121: FROM_HERE, base::Bind(callback, base::nullopt)); On 2017/06/05 04:29:55, mtomasz wrote: > nit: Bind -> BindOnce Done. https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:209: base::Bind(callback, false, std::string())); On 2017/06/05 04:29:55, mtomasz wrote: > nit: BindOnce Done. https://codereview.chromium.org/2914433002/diff/80001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/80001/components/arc/common/f... components/arc/common/file_system.mojom:110: // empty string). On 2017/06/05 04:29:55, mtomasz wrote: > optional: How are directories handled? On Android it's > "vnd.android.document/directory" which may not make sense on Chrome OS side. Do > we need to care about it? Android-side impl just calls ContentResolver.getType() with the given URL, so it returns what ContentReoslver.getType() returns for a directory when it happens. IIUC no Chrome OS code should try to get MIME type of a directory (i.e. is-directory check should be done before calling GetNonNativeLocalPathMimeType()) so I think it's OK to leave the behavior undocumented. WDYT? https://codereview.chromium.org/2914433002/diff/80001/components/arc/test/fak... File components/arc/test/fake_file_system_instance.cc (right): https://codereview.chromium.org/2914433002/diff/80001/components/arc/test/fak... components/arc/test/fake_file_system_instance.cc:179: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2017/06/05 04:29:55, mtomasz wrote: > nit: ditto here and below. @tzik has been refactoring these Bind to BindOnce all > over the codebase (where applicable). Done.
lgtm! https://codereview.chromium.org/2914433002/diff/80001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/80001/components/arc/common/f... components/arc/common/file_system.mojom:110: // empty string). On 2017/06/05 04:55:37, hashimoto wrote: > On 2017/06/05 04:29:55, mtomasz wrote: > > optional: How are directories handled? On Android it's > > "vnd.android.document/directory" which may not make sense on Chrome OS side. > Do > > we need to care about it? > > Android-side impl just calls ContentResolver.getType() with the given URL, so it > returns what ContentReoslver.getType() returns for a directory when it happens. > > IIUC no Chrome OS code should try to get MIME type of a directory (i.e. > is-directory check should be done before calling > GetNonNativeLocalPathMimeType()) so I think it's OK to leave the behavior > undocumented. > WDYT? SGTM, thanks for explanation.
LGTM with optional comments. https://codereview.chromium.org/2914433002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:159: void GetNonNativeLocalPathMimeType( Optional: As this function gets bigger, and it looks to have a dup code, how about small refactoring, like; namespace { bool GetNonNativeLocalPathMimeTypeInteranl(...) { if (IsUnderDriveMountPoint(...)) { auto* file_system = GetFileSystemByProfile(profile); if (!file_system) return false; ... return true; } if (IsFileSystemProviderLocalPath(...)) { ... parser ... if (!parser.Parse()) return false; ... return true; } ... ditto for your new code ... return false; } } // namespace void GetNonNativeLocalPathMimeType(...) { if (GetNonNativeLocalPathMimeType(...)) { // callback is deferred to the async operation, so do nothing. return; } // PostTask once to be consistent with async operation. PostTask(UI, FROM_HERE, BindOnce(callback, false, std::string())); } https://codereview.chromium.org/2914433002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:213: arc_url, base::Bind(&GetMimeTypeAfterGetMimeTypeForArcContentFileSystem, style / optional: If you're starting to use BindOnce, this should be, too? (Though, for consistency, we have more Bind() in this file to be converted to BindOnce, I don't have strong opinion, in this CL). https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... components/arc/common/file_system.mojom:110: // empty string). Clarification: Can empty string be a valid returned value? How about comment an example case when it happens?
https://codereview.chromium.org/2914433002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right): https://codereview.chromium.org/2914433002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:159: void GetNonNativeLocalPathMimeType( On 2017/06/05 05:18:04, hidehiko wrote: > Optional: > As this function gets bigger, and it looks to have a dup code, how about small > refactoring, like; > > namespace { > > bool GetNonNativeLocalPathMimeTypeInteranl(...) { > if (IsUnderDriveMountPoint(...)) { > auto* file_system = GetFileSystemByProfile(profile); > if (!file_system) > return false; > ... > return true; > } > > if (IsFileSystemProviderLocalPath(...)) { > ... parser ... > if (!parser.Parse()) > return false; > ... > return true; > } > > ... ditto for your new code ... > return false; > } > > } // namespace > > void GetNonNativeLocalPathMimeType(...) { > if (GetNonNativeLocalPathMimeType(...)) { > // callback is deferred to the async operation, so do nothing. > return; > } > // PostTask once to be consistent with async operation. > PostTask(UI, FROM_HERE, BindOnce(callback, false, std::string())); > } Thanks, sounds like a good thing to do when working on crbug.com/729442 https://codereview.chromium.org/2914433002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/file_manager/filesystem_api_util.cc:213: arc_url, base::Bind(&GetMimeTypeAfterGetMimeTypeForArcContentFileSystem, On 2017/06/05 05:18:04, hidehiko wrote: > style / optional: If you're starting to use BindOnce, this should be, too? > (Though, for consistency, we have more Bind() in this file to be converted to > BindOnce, I don't have strong opinion, in this CL). Unfortunately, ArcFileSystemOperationRunner does not accept OnceCallback (probably we should fix this) so we cannot use BindOnce here. https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... components/arc/common/file_system.mojom:110: // empty string). On 2017/06/05 05:18:04, hidehiko wrote: > Clarification: Can empty string be a valid returned value? > How about comment an example case when it happens? This method returns the empty string when ContentResolver.getType() does that. I don't think it happens with a sane ContentProvider implementation, so I don't have a relevant example case in my mind. "not to be confused with ..." part was added upon dcheng@'s request, but if it has implications that the empty string is a valid MIME type, probably it's better to remove that part from the comment?
Thank you for updating the bug. still lgtm. https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... components/arc/common/file_system.mojom:110: // empty string). On 2017/06/05 05:40:53, hashimoto wrote: > On 2017/06/05 05:18:04, hidehiko wrote: > > Clarification: Can empty string be a valid returned value? > > How about comment an example case when it happens? > This method returns the empty string when ContentResolver.getType() does that. > I don't think it happens with a sane ContentProvider implementation, so I don't > have a relevant example case in my mind. > > "not to be confused with ..." part was added upon dcheng@'s request, but if it > has implications that the empty string is a valid MIME type, probably it's > better to remove that part from the comment? Just IMHO: Checked a bit more. According to RFC2045 Section 5.1, empty string is invalid mime type. It is common practice to use invalid data (like empty string, -1 or 0 for int, etc.) for representing an error. Though, if explicit optional type is preferred, I'm fine with it, too. If I were you, I'd drop it, because it looks as if empty string has some valid meaning, but actually not. Or, "When an error occurs, returns null value (rather than empty string)." to avoid empty string's validness implication? Up to you.
Thank you for the extensive research! https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2914433002/diff/120001/components/arc/common/... components/arc/common/file_system.mojom:110: // empty string). On 2017/06/05 05:52:53, hidehiko wrote: > On 2017/06/05 05:40:53, hashimoto wrote: > > On 2017/06/05 05:18:04, hidehiko wrote: > > > Clarification: Can empty string be a valid returned value? > > > How about comment an example case when it happens? > > This method returns the empty string when ContentResolver.getType() does that. > > I don't think it happens with a sane ContentProvider implementation, so I > don't > > have a relevant example case in my mind. > > > > "not to be confused with ..." part was added upon dcheng@'s request, but if it > > has implications that the empty string is a valid MIME type, probably it's > > better to remove that part from the comment? > > Just IMHO: > Checked a bit more. According to RFC2045 Section 5.1, empty string is invalid > mime type. > It is common practice to use invalid data (like empty string, -1 or 0 for int, > etc.) for representing an error. > Though, if explicit optional type is preferred, I'm fine with it, too. > > If I were you, I'd drop it, because it looks as if empty string has some valid > meaning, but actually not. Or, > > "When an error occurs, returns null value (rather than empty string)." > > to avoid empty string's validness implication? > Up to you. OK, dropped that part. Either in C++ (base::Optional<std::string>) or in Java (String), it should be clear that a null value is different from the empty string.
The CQ bit was checked by hashimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nya@chromium.org, nya@google.com, dcheng@chromium.org, hidehiko@chromium.org, mtomasz@chromium.org Link to the patchset: https://codereview.chromium.org/2914433002/#ps140001 (title: "Comment drop.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496642655171340,
"parent_rev": "1035c1f354818c779ebeac87639de33bd16075d1", "commit_rev":
"aa692120241e0bfd072ac7b1b34ff26fcc6aa838"}
Message was sent while issue was closed.
Description was changed from ========== arc: Use the MIME type returned by the container to handle content URLs Currently, when displaying an ARC file specified by a content URL, we are using MIME type guessed by looking at the extension part of the URL. This doesn't always work so we should ask the ContentResolver running in the container to get the actual MIME type. - Add GetMimeType to FileSystemInstance interface. - Add arc::PathToArcUrl to arc_content_file_system_url_util - Use GetMimeType and arc::PathToArcUrl in filesystem_api_util.cc BUG=704701 TEST=Download a image file to the device's Downloads directory. Open chrome://settings/androidApps/details -> "Manage Android preferences" -> Storage -> Images -> Download -> <the downloaded image file>. Verify that the image file is displayed correctly, not as garbage text. ========== to ========== arc: Use the MIME type returned by the container to handle content URLs Currently, when displaying an ARC file specified by a content URL, we are using MIME type guessed by looking at the extension part of the URL. This doesn't always work so we should ask the ContentResolver running in the container to get the actual MIME type. - Add GetMimeType to FileSystemInstance interface. - Add arc::PathToArcUrl to arc_content_file_system_url_util - Use GetMimeType and arc::PathToArcUrl in filesystem_api_util.cc BUG=704701 TEST=Download a image file to the device's Downloads directory. Open chrome://settings/androidApps/details -> "Manage Android preferences" -> Storage -> Images -> Download -> <the downloaded image file>. Verify that the image file is displayed correctly, not as garbage text. Review-Url: https://codereview.chromium.org/2914433002 Cr-Commit-Position: refs/heads/master@{#476953} Committed: https://chromium.googlesource.com/chromium/src/+/aa692120241e0bfd072ac7b1b34f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/aa692120241e0bfd072ac7b1b34f... |
