Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2580713004:
mediaview: Implement ArcDocumentsProviderBackendDelegate. (Closed)
|
Created:
4 years ago by Shuhei Takahashi Modified:
3 years, 11 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tzik, hidehiko+watch_chromium.org, nhiroki, lhchavez+watch_chromium.org, oshima+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmediaview: Implement ArcDocumentsProviderBackendDelegate.
BUG=chromium:671511
TEST=unit_tests --gtest_filter='Arc*'
TEST=trybot
Review-Url: https://codereview.chromium.org/2580713004
Cr-Commit-Position: refs/heads/master@{#441891}
Committed: https://chromium.googlesource.com/chromium/src/+/ec0ad68ca0447e40e6a2396ed5742d009c23641c
Patch Set 1 #Patch Set 2 : Constructors are called on the UI thread. #Patch Set 3 : ThreadChecker failure fix. #
Total comments: 6
Patch Set 4 : Too long; use auto. #Patch Set 5 : Rebased to ToT. #Patch Set 6 : Remove cache layer. #
Total comments: 8
Patch Set 7 : Rebased. #Patch Set 8 : Addressed hashimoto's comments. #Patch Set 9 : Removed unused #include. #
Total comments: 10
Patch Set 10 : Addressed hashimoto's comments. #
Total comments: 4
Patch Set 11 : Remove WeakPtrFactory & add TODO for unit tests. #Dependent Patchsets: Messages
Total messages: 50 (36 generated)
nya@chromium.org changed reviewers: + hashimoto@chromium.org, lhchavez@chromium.org
lhchavez, hashimoto: PTAL
The CQ bit was checked by nya@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: 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_...)
The CQ bit was checked by nya@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: 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_...)
The CQ bit was checked by nya@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.
lgtm with nit. https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:52: return std::move(reader); Huh, why did the pessimizing move warning not trigger? Anyways, remove the std::move on return statements.
https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:52: return std::move(reader); On 2016/12/17 00:05:26, Luis Héctor Chávez wrote: > Huh, why did the pessimizing move warning not trigger? Anyways, remove the > std::move on return statements. I'm not sure why, but returning std::unique_ptr<Derived> in a function with return value type std::unique<Base> gives a compile error on g++ 4.x, but not on g++ 5.x. http://melpon.org/wandbox/permlink/B2S4qXDpmznhtNR9 http://melpon.org/wandbox/permlink/fLZveKGpfiwgSJcU
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Just FYI. https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:45: std::unique_ptr<ArcDocumentsProviderFileStreamReader> reader = Optional: I'd recommend to use "auto", because of - consistency that auto + MakeUnique is used in many places in ARC code, and - simplicity that the type appears only once. https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:52: return std::move(reader); On 2016/12/17 00:23:09, Shuhei Takahashi wrote: > On 2016/12/17 00:05:26, Luis Héctor Chávez wrote: > > Huh, why did the pessimizing move warning not trigger? Anyways, remove the > > std::move on return statements. > > I'm not sure why, but returning std::unique_ptr<Derived> in a function with > return value type std::unique<Base> gives a compile error on g++ 4.x, but not on > g++ 5.x. > > http://melpon.org/wandbox/permlink/B2S4qXDpmznhtNR9 > http://melpon.org/wandbox/permlink/fLZveKGpfiwgSJcU IIUC, it's C++11 spec about copy elision. cf) http://stackoverflow.com/questions/22018115/converting-stdunique-ptrderived-t...
https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:45: std::unique_ptr<ArcDocumentsProviderFileStreamReader> reader = On 2016/12/18 14:29:18, hidehiko wrote: > Optional: I'd recommend to use "auto", because of > - consistency that auto + MakeUnique is used in many places in ARC code, and > - simplicity that the type appears only once. Sure, changed to use auto. https://codereview.chromium.org/2580713004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:52: return std::move(reader); On 2016/12/18 14:29:18, hidehiko wrote: > On 2016/12/17 00:23:09, Shuhei Takahashi wrote: > > On 2016/12/17 00:05:26, Luis Héctor Chávez wrote: > > > Huh, why did the pessimizing move warning not trigger? Anyways, remove the > > > std::move on return statements. > > > > I'm not sure why, but returning std::unique_ptr<Derived> in a function with > > return value type std::unique<Base> gives a compile error on g++ 4.x, but not > on > > g++ 5.x. > > > > http://melpon.org/wandbox/permlink/B2S4qXDpmznhtNR9 > > http://melpon.org/wandbox/permlink/fLZveKGpfiwgSJcU > > IIUC, it's C++11 spec about copy elision. > cf) > http://stackoverflow.com/questions/22018115/converting-stdunique-ptrderived-t... I know that spec but I was wondering why gcc 5.x allows it even with -std=c++11... Anyway, I think we need std::move() here.
The CQ bit was checked by nya@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.
The CQ bit was checked by nya@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...
PTAL Rebased on top of the latest patch of https://codereview.chromium.org/2574173002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please rebase on top of the landed changes. https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:77: void ArcDocumentsProviderBackendDelegate::GetRedirectURLForContents( qq: When is this method used, and when is the FileStreamReader impl used? Do we really need both? https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:96: GetRedirectURLForContentsWithContentUrl( nit: "GetRedirectURLFor..." sounds like this method triggers some action while this is actually a callback. How about something like "OnRedirectURLReady" or "ConvertArcUrlToExternalFileUrlAndRunCallback"? https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:62: net::EscapeQueryParamValue(authority, false).c_str(), What does this false mean? Please make it a constant, or add a comment. https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right): https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:40: GURL BuildDocumentUrl(const std::string& authority, Please add a test for this function, at least to ensure that this function does not crash.
The CQ bit was checked by nya@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 checked by nya@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...
PTAL Rebased to ToT and addressed comments. https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:77: void ArcDocumentsProviderBackendDelegate::GetRedirectURLForContents( On 2016/12/22 06:20:01, hashimoto wrote: > qq: When is this method used, and when is the FileStreamReader impl used? > Do we really need both? I realized this function is never called by chromeos::FileSystemBackend. Let's make this function empty, and revisit if it turns out we need to implement it. https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:96: GetRedirectURLForContentsWithContentUrl( On 2016/12/22 06:20:01, hashimoto wrote: > nit: "GetRedirectURLFor..." sounds like this method triggers some action while > this is actually a callback. > How about something like "OnRedirectURLReady" or > "ConvertArcUrlToExternalFileUrlAndRunCallback"? Removed the unused function. https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:62: net::EscapeQueryParamValue(authority, false).c_str(), On 2016/12/22 06:20:01, hashimoto wrote: > What does this false mean? > Please make it a constant, or add a comment. Added comments. https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right): https://codereview.chromium.org/2580713004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:40: GURL BuildDocumentUrl(const std::string& authority, On 2016/12/22 06:20:01, hashimoto wrote: > Please add a test for this function, at least to ensure that this function does > not crash. Added a simple test.
https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc (right): https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc:42: pending_operations_.clear(); How about std::move'ing pending_operations_ to a temporary variable before the for loop, instead of calling clear() here? This way, you don't have to worry about what happens when pending_operations_ gets modified as a result of running callbacks (it doesn't happen with the current code, but someone may carelessly modify this file in future). https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc:58: base::IgnoreResult(&ArcDocumentsProviderFileStreamReader::Read), IgnoreResult here feels a bit dangerous to me. ATM ArcDocumentsProviderFileStreamReader::Read() doesn't return values other than ERR_IO_PENDING, but it's not guaranteed to be true for future. The same goes for GetLength(). https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc:59: weak_ptr_factory_.GetWeakPtr(), buffer, buffer_length, callback)); Without holding a refptr, this buffer may die before ArcDocumentsProviderFileStreamReader::Read gets called. https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h (right): https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h:32: base::WeakPtr<ArcDocumentsProviderFileStreamReader> GetWeakPtr(); How about calling ParseAndLookup() and ArcDocumentsProviderRoot::ResolveToContentUrl() in this class's ctor so that we don't need to expose SetContentUrl() and GetWeakPtr() as public methods? https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h:42: bool resolved_; What "resolve" means is not clear from just looking at this class. How about giving this variable a name like content_url_is_set_ as this variable is only modified in SetContentUrl()?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nya@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...
PTAL https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc (right): https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc:42: pending_operations_.clear(); On 2017/01/05 09:21:22, hashimoto wrote: > How about std::move'ing pending_operations_ to a temporary variable before the > for loop, instead of calling clear() here? > This way, you don't have to worry about what happens when pending_operations_ > gets modified as a result of running callbacks (it doesn't happen with the > current code, but someone may carelessly modify this file in future). Well, since |resolved_| = true at this moment, further elements are never added to |pending_operations_|. But anyway it sounds okay to me to use a temporary variable here. Note that I did use .swap() instead of std::move() because std::move() puts a vector valid but unspecified state, thus usually should be used for soon-expiring objects. https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc:58: base::IgnoreResult(&ArcDocumentsProviderFileStreamReader::Read), On 2017/01/05 09:21:22, hashimoto wrote: > IgnoreResult here feels a bit dangerous to me. > ATM ArcDocumentsProviderFileStreamReader::Read() doesn't return values other > than ERR_IO_PENDING, but it's not guaranteed to be true for future. > The same goes for GetLength(). True. I added proper handling. https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc:59: weak_ptr_factory_.GetWeakPtr(), buffer, buffer_length, callback)); On 2017/01/05 09:21:22, hashimoto wrote: > Without holding a refptr, this buffer may die before > ArcDocumentsProviderFileStreamReader::Read gets called. Good catch! https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h (right): https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h:32: base::WeakPtr<ArcDocumentsProviderFileStreamReader> GetWeakPtr(); On 2017/01/05 09:21:22, hashimoto wrote: > How about calling ParseAndLookup() and > ArcDocumentsProviderRoot::ResolveToContentUrl() in this class's ctor so that we > don't need to expose SetContentUrl() and GetWeakPtr() as public methods? Sounds good. https://codereview.chromium.org/2580713004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h:42: bool resolved_; On 2017/01/05 09:21:22, hashimoto wrote: > What "resolve" means is not clear from just looking at this class. > How about giving this variable a name like content_url_is_set_ as this variable > is only modified in SetContentUrl()? renamed to content_url_resolved.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:22: : async_file_util_(&roots_), weak_ptr_factory_(this) {} This WeakPtrFactory is unused. https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h (right): https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h:30: class ArcDocumentsProviderFileStreamReader : public storage::FileStreamReader { It'd be nice to have a test for this class (not necessarily in this CL). Sorry for not noticing this earlier.
Thanks for reviewing. https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc (right): https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:22: : async_file_util_(&roots_), weak_ptr_factory_(this) {} On 2017/01/06 04:16:51, hashimoto wrote: > This WeakPtrFactory is unused. Thanks for catching, done. https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h (right): https://codereview.chromium.org/2580713004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h:30: class ArcDocumentsProviderFileStreamReader : public storage::FileStreamReader { On 2017/01/06 04:16:51, hashimoto wrote: > It'd be nice to have a test for this class (not necessarily in this CL). > Sorry for not noticing this earlier. Sure, added a TODO.
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2580713004/#ps200001 (title: "Remove WeakPtrFactory & add TODO for unit tests.")
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": 200001, "attempt_start_ts": 1483681098192370,
"parent_rev": "e12c3a936186cf3d3f9e388ae2be6a484e40096e", "commit_rev":
"ec0ad68ca0447e40e6a2396ed5742d009c23641c"}
Message was sent while issue was closed.
Description was changed from ========== mediaview: Implement ArcDocumentsProviderBackendDelegate. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot ========== to ========== mediaview: Implement ArcDocumentsProviderBackendDelegate. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot Review-Url: https://codereview.chromium.org/2580713004 Cr-Commit-Position: refs/heads/master@{#441891} Committed: https://chromium.googlesource.com/chromium/src/+/ec0ad68ca0447e40e6a2396ed574... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/ec0ad68ca0447e40e6a2396ed574... |
