Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715493002:
mediaview: Support watchers in ArcFileSystemOperationRunner. (Closed)
|
Created:
3 years, 10 months ago by Shuhei Takahashi Modified:
3 years, 9 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: Support watchers in ArcFileSystemOperationRunner.
AddWatcher() and RemoveWatcher() are added.
Also, ArcFileSystemOperationRunner implements FileSystemHost to
receive watcher callbacks from the Android container.
BUG=chromium:684233
TEST=trybot
Review-Url: https://codereview.chromium.org/2715493002
Cr-Commit-Position: refs/heads/master@{#454589}
Committed: https://chromium.googlesource.com/chromium/src/+/1cf2cd5fd31329f2cb7de2cd2df919648b5e331b
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Review ready. #
Total comments: 21
Patch Set 4 : Addressed hidehiko's comments. #Patch Set 5 : Rebased to ToT. #
Total comments: 13
Patch Set 6 : . #Patch Set 7 : Introduced ObserverIOThreadWrapper. #Patch Set 8 : Avoid UI/IO thread confusion in util. #
Total comments: 4
Patch Set 9 : s/LOG/DLOG/ #
Total comments: 5
Patch Set 10 : Moved ObserverIOThreadWrapper to arc::file_system_operation_runner_util namespace. #
Total comments: 8
Patch Set 11 : Addressed dcheng's comments. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 54 (32 generated)
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...
Description was changed from ========== mediaview: Support watchers in ArcFileSystemOperationRunner. BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: Support watchers in ArcFileSystemOperationRunner. AddWatcher() and RemoveWatcher() are added. Also, ArcFileSystemOperationRunner implements FileSystemHost to receive watcher callbacks from the Android container. BUG=chromium:684233 TEST=trybot ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nya@chromium.org changed reviewers: + hidehiko@chromium.org
hidehiko: PTAL
https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:32: NOTREACHED(); Could you move this block after switch so that (some) compiler warns if some value is missing, which helps specifically if a new value is added in mojom::ChangeType. cf) ARC style guide. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:68: // ArcSessionManager may not exist in unit tests. auto* arc_session_manager = ArcSessionManager::Get(); if (arc_session_manager) arc_session_manager->AddObserver(this); looks more fit to the comment? If you'd like, you can DCHECK in delegating ctor, I think. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:213: watcher_callbacks_.erase(iter); This hides the OnDocumentChanged event between RemoveWatcher() and its callback, which means this is slightly different from the mojom::FileSystemInstance::RemoveWatcher behavior (in terms of timing). Could you; 1) update the code so that the both behavior is aligned, or 2) explicitly document the difference? https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:230: return; Could you comment when this happens? https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:233: if (type == mojom::ChangeType::DELETED) { Could you elide brace of one line if stmt, for consistency? https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:248: if (file_system_instance) { ditto https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:268: watcher_callbacks_.insert(std::make_pair(watcher_id, watcher_callback)); Optional: Just in case how about DCHECK_EQ(watcher_callbacks_.count(watcher_id), 0); https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:286: watcher_callbacks_.clear(); Could you comment why this is the timing for watcher_callbacks_ to be cleared? https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:113: void AddWatcherDone(const WatcherCallback& watcher_callback, Optional: OnWatcherAdded to be consistent with "OnStateChanged()" below? Up to you.
PTAL https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:32: NOTREACHED(); On 2017/02/23 11:55:28, hidehiko wrote: > Could you move this block after switch so that (some) compiler warns if some > value is missing, which helps specifically if a new value is added in > mojom::ChangeType. > cf) ARC style guide. Done. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:68: // ArcSessionManager may not exist in unit tests. On 2017/02/23 11:55:28, hidehiko wrote: > auto* arc_session_manager = ArcSessionManager::Get(); > if (arc_session_manager) > arc_session_manager->AddObserver(this); > > looks more fit to the comment? > If you'd like, you can DCHECK in delegating ctor, I think. Sounds good, done. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:213: watcher_callbacks_.erase(iter); On 2017/02/23 11:55:28, hidehiko wrote: > This hides the OnDocumentChanged event between RemoveWatcher() and its callback, > which means this is slightly different from the > mojom::FileSystemInstance::RemoveWatcher behavior (in terms of timing). > > Could you; > 1) update the code so that the both behavior is aligned, or > 2) explicitly document the difference? > https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:230: return; On 2017/02/23 11:55:28, hidehiko wrote: > Could you comment when this happens? Done. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:233: if (type == mojom::ChangeType::DELETED) { On 2017/02/23 11:55:28, hidehiko wrote: > Could you elide brace of one line if stmt, for consistency? Done. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:248: if (file_system_instance) { On 2017/02/23 11:55:28, hidehiko wrote: > ditto Done. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:268: watcher_callbacks_.insert(std::make_pair(watcher_id, watcher_callback)); On 2017/02/23 11:55:28, hidehiko wrote: > Optional: Just in case how about > > DCHECK_EQ(watcher_callbacks_.count(watcher_id), 0); Done. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:286: watcher_callbacks_.clear(); On 2017/02/23 11:55:28, hidehiko wrote: > Could you comment why this is the timing for watcher_callbacks_ to be cleared? Hm, I thought it's better to clear it because |should_defer_| = true means the container is booting and thus watchers have been cleared, but on a container crash Chrome browser process should restart, so we don't need this. Let's just remove it. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:113: void AddWatcherDone(const WatcherCallback& watcher_callback, On 2017/02/23 11:55:28, hidehiko wrote: > Optional: OnWatcherAdded to be consistent with "OnStateChanged()" below? Up to > you. Done.
nya@chromium.org changed reviewers: + dcheng@chromium.org
dcheng: PTAL for security review dcheng, here's a CL which implements FileSystemHost added in https://codereview.chromium.org/2714433003/.
https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:286: watcher_callbacks_.clear(); On 2017/02/24 06:10:39, Shuhei Takahashi wrote: > On 2017/02/23 11:55:28, hidehiko wrote: > > Could you comment why this is the timing for watcher_callbacks_ to be cleared? > > Hm, I thought it's better to clear it because |should_defer_| = true means the > container is booting and thus watchers have been cleared, but on a container > crash Chrome browser process should restart, so we don't need this. Let's just > remove it. Sorry, I'm confused. AFAIK, even if the container is crashed, Chrome browser process would be still alive. If the watcher ID is not persistent, it needs to be cleared somehow? BTW, if this is for ARC crashing case (I didn't come up with it in previous iteration. Thank you!), https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_... is the method to subscribe. OnArcPlayStoreEnabledChanged() is called only when the preference is changed as named. Also, I'm interested in the expected behavior in the case. Clearing watcher means the receivers will never receives the event. Is it acceptable practically? Or is there any more change is needed to cover the case?
FYI. https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:286: watcher_callbacks_.clear(); On 2017/02/24 16:16:17, hidehiko wrote: > On 2017/02/24 06:10:39, Shuhei Takahashi wrote: > > On 2017/02/23 11:55:28, hidehiko wrote: > > > Could you comment why this is the timing for watcher_callbacks_ to be > cleared? > > > > Hm, I thought it's better to clear it because |should_defer_| = true means the > > container is booting and thus watchers have been cleared, but on a container > > crash Chrome browser process should restart, so we don't need this. Let's just > > remove it. > > Sorry, I'm confused. AFAIK, even if the container is crashed, Chrome browser > process would be still alive. If the watcher ID is not persistent, it needs to > be cleared somehow? > > BTW, if this is for ARC crashing case (I didn't come up with it in previous > iteration. Thank you!), > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_... > is the method to subscribe. > OnArcPlayStoreEnabledChanged() is called only when the preference is changed as > named. Oh, one thing I was wrong here. You do not actually need ArcSessionObserver, because OnInstance{Ready,Close} can be used for that purpose. Sorry for confusing comment. > Also, I'm interested in the expected behavior in the case. Clearing watcher > means the receivers will never receives the event. Is it acceptable practically? > Or is there any more change is needed to cover the case?
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:31: return ArcFileSystemOperationRunner::ChangeType::DELETED; Let's add a typemap for this. https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:74: // ArcSessionManager may not exist in unit tests. Nit: in many other parts of Chrome, null for testing has made code harder to understand. If possible, let's make it so test code and real code don't have to differentiate on this. https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:87: arc_session_manager->AddObserver(this); RemoveObserver? https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:279: DCHECK_EQ(0u, watcher_callbacks_.count(watcher_id)); As this is coming from a less-trusted container, we can't make any assumptions. If it's important that watcher_id is not already in the container, we need to explicitly handle that.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:286: watcher_callbacks_.clear(); On 2017/02/25 14:41:30, hidehiko wrote: > On 2017/02/24 16:16:17, hidehiko wrote: > > On 2017/02/24 06:10:39, Shuhei Takahashi wrote: > > > On 2017/02/23 11:55:28, hidehiko wrote: > > > > Could you comment why this is the timing for watcher_callbacks_ to be > > cleared? > > > > > > Hm, I thought it's better to clear it because |should_defer_| = true means > the > > > container is booting and thus watchers have been cleared, but on a container > > > crash Chrome browser process should restart, so we don't need this. Let's > just > > > remove it. > > > > Sorry, I'm confused. AFAIK, even if the container is crashed, Chrome browser > > process would be still alive. If the watcher ID is not persistent, it needs to > > be cleared somehow? > > > > BTW, if this is for ARC crashing case (I didn't come up with it in previous > > iteration. Thank you!), > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_... > > is the method to subscribe. > > OnArcPlayStoreEnabledChanged() is called only when the preference is changed > as > > named. > > Oh, one thing I was wrong here. You do not actually need ArcSessionObserver, > because OnInstance{Ready,Close} can be used for that purpose. Sorry for > confusing comment. > > > Also, I'm interested in the expected behavior in the case. Clearing watcher > > means the receivers will never receives the event. Is it acceptable > practically? > > Or is there any more change is needed to cover the case? > Thanks for explanation. I had a misunderstanding that Chrome browser process will restart on Android container restart, but it's obviously false. Since watcher IDs are cleared on Android restart, we need to notify the event to the users of this class. I introduced Observer for that purpose. https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:31: return ArcFileSystemOperationRunner::ChangeType::DELETED; On 2017/02/28 06:56:32, dcheng wrote: > Let's add a typemap for this. Typemap sounds good and I tried adding one, but I could not resolve GN errors possibly due to weird configs in //storage/browser. Please let me address this later. https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:74: // ArcSessionManager may not exist in unit tests. On 2017/02/28 06:56:32, dcheng wrote: > Nit: in many other parts of Chrome, null for testing has made code harder to > understand. If possible, let's make it so test code and real code don't have to > differentiate on this. I totally agree checking null is not good, but we don't have working fakes of ArcSessionManager. Good news is that some folks are working on large refactoring around ArcSessionManager, so I hope we're in better situation after it is finished. https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:87: arc_session_manager->AddObserver(this); On 2017/02/28 06:56:32, dcheng wrote: > RemoveObserver? Oops! Thanks for catching. https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:279: DCHECK_EQ(0u, watcher_callbacks_.count(watcher_id)); On 2017/02/28 06:56:32, dcheng wrote: > As this is coming from a less-trusted container, we can't make any assumptions. > If it's important that watcher_id is not already in the container, we need to > explicitly handle that. Makes sense, done.
Patchset #8 (id:160001) has been deleted
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.
https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:31: return ArcFileSystemOperationRunner::ChangeType::DELETED; On 2017/03/01 09:10:08, Shuhei Takahashi wrote: > On 2017/02/28 06:56:32, dcheng wrote: > > Let's add a typemap for this. > > Typemap sounds good and I tried adding one, but I could not resolve GN errors > possibly due to weird configs in //storage/browser. Please let me address this > later. What's the error you're seeing?
https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:31: return ArcFileSystemOperationRunner::ChangeType::DELETED; On 2017/03/01 21:47:57, dcheng wrote: > On 2017/03/01 09:10:08, Shuhei Takahashi wrote: > > On 2017/02/28 06:56:32, dcheng wrote: > > > Let's add a typemap for this. > > > > Typemap sounds good and I tried adding one, but I could not resolve GN errors > > possibly due to weird configs in //storage/browser. Please let me address this > > later. > > What's the error you're seeing? Here's a WIP patch: https://codereview.chromium.org/2727113002/ I'm glad if you could help resolving the GN error. Anyway, the change to add typemaps will be not trivial, so I want to keep it a separate patch.
https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:293: LOG(ERROR) << "Conflicting watcher ID received. This must be a bug."; Nit: DLOG please (same comment below) https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h (right): https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h:20: : public base::RefCountedThreadSafe<ObserverIOThreadWrapper>, Reading through, I'm not quite sure why this is refcounted. It doesn't seem like any code actually keeps a reference to this, so how is this object supposed to stay alive?
https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:293: LOG(ERROR) << "Conflicting watcher ID received. This must be a bug."; On 2017/03/02 07:21:45, dcheng wrote: > Nit: DLOG please (same comment below) Done. https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h (right): https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h:20: : public base::RefCountedThreadSafe<ObserverIOThreadWrapper>, On 2017/03/02 07:21:45, dcheng wrote: > Reading through, I'm not quite sure why this is refcounted. It doesn't seem like > any code actually keeps a reference to this, so how is this object supposed to > stay alive? This object is refcounted because it does not stay in a single fixed thread (UI and IO). I added you to R= in https://codereview.chromium.org/2715473003/ which uses this class actually.
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...
On 2017/03/02 07:30:50, Shuhei Takahashi wrote: > https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... > cromo archivo / navegador / chromeos / arco / FileAPI / arc_file_system_operation_runner.cc</font></font><font></font><font><font> > (derecho):</font></font><font></font> > <font></font> > https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... > cromo / navegador / chromeos / arco / FileAPI / arc_file_system_operation_runner.cc: 293:</font></font><font></font><font><font> > LOG (ERROR) << "en conflicto ID vigilante recibió Esto debe ser un error..";</font></font><font></font><font><font> > En 03.02.2017 07:21:45, dcheng escribió:</font></font><font></font><font><font> > > Nit: DLOG favor (el mismo comentario a continuación)</font></font><font></font> > <font></font><font><font> > Hecho.</font></font><font></font> > <font></font> > https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... > cromo archivo / navegador / chromeos / arco / FileAPI / arc_file_system_operation_runner_util.h</font></font><font></font><font><font> > (derecho):</font></font><font></font> > <font></font> > https://codereview.chromium.org/2715493002/diff/180001/chrome/browser/chromeo... > cromo / navegador / chromeos / arco / FileAPI / arc_file_system_operation_runner_util.h: 20:</font></font><font></font><font><font> > : Base pública :: RefCountedThreadSafe <ObserverIOThreadWrapper>,</font></font><font></font><font><font> > En 03.02.2017 07:21:45, dcheng escribió:</font></font><font></font><font><font> > > Lectura a través de, no estoy muy seguro de por qué esto está refcounted. </font><font>No parece</font></font><font></font><font><font> > me gusta</font></font><font></font><font><font> > > Cualquier código en realidad mantiene una referencia a esto, así que ¿cómo se supone que este objeto</font></font><font></font><font><font> > > Seguir con vida?</font></font><font></font> > <font></font><font><font> > Este objeto se refcounted, ya que no se queda en un solo hilo fijo (IU</font></font><font></font><font><font> > e IO).</font></font><font></font> > <font></font><font><font> > Yo te añadió a R = en </font></font>https://codereview.chromium.org/2715473003/<font><font> que utiliza este</font></font><font></font><font><font> > clase realidad.</font></font>
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost LG. Minor comments only. https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:70: virtual ~Observer() = default; Could you move this into protected? cf) https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos... That will be added into our style guide (just not yet, though) as commented by Luis. https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h (right): https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h:19: class ObserverIOThreadWrapper Because here in namespace arc, and we have many Observer classes in ARC, ObserverIOThreadWrapper sounds too generic to me. Could you, either; 1) make it more generic class so that (if necessary) this can be used with other Observers. Or 2) rename it to more specific one? E.g. ArcFileSystemOperationRunnerObsereverIOThreadWrapper (sounds too long... though :-/).
https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:70: virtual ~Observer() = default; On 2017/03/02 14:43:47, hidehiko wrote: > Could you move this into protected? > > cf) > https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos... > > That will be added into our style guide (just not yet, though) as commented by > Luis. Done. https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h (right): https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h:19: class ObserverIOThreadWrapper On 2017/03/02 14:43:47, hidehiko wrote: > Because here in namespace arc, and we have many Observer classes in ARC, > ObserverIOThreadWrapper sounds too generic to me. > Could you, either; > 1) make it more generic class so that (if necessary) this can be used with other > Observers. Or > 2) rename it to more specific one? E.g. > ArcFileSystemOperationRunnerObsereverIOThreadWrapper (sounds too long... though > :-/). Makes sense. I've moved the class to arc::file_system_operation_runner_util namespace.
LGTM with some nits (The ObserverListThreadSafe and WeakPtr comments aren't required for this CL) https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:197: weak_ptr_factory_.GetWeakPtr(), authority, document_id, FWIW, this weak pointer is technically unnecessary (since this callback isn't posted, and the vector is owned by |this|) https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:293: DLOG(ERROR) << "Conflicting watcher ID received. This must be a bug."; Btw, if there are no possible races here, we usually like to annotate with NOTREACHED() and just close the binding (and not invoke the callback). https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc (right): https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc:96: LOG(ERROR) << "ArcFileSystemOperationRunner unavailable. " DLOG(ERROR) for all the other LOG(ERROR) introduced by this CL too. https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc:235: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, Btw... does ObserverListThreadSafe help with this at all? This seems pretty complicated =)
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...
Thanks dcheng for reviewing! https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:197: weak_ptr_factory_.GetWeakPtr(), authority, document_id, On 2017/03/03 09:31:14, dcheng wrote: > FWIW, this weak pointer is technically unnecessary (since this callback isn't > posted, and the vector is owned by |this|) Ah, that's true! But since the cost is negligible, please let me leave it as-is to avoid possible mistakes in the future. https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:293: DLOG(ERROR) << "Conflicting watcher ID received. This must be a bug."; On 2017/03/03 09:31:14, dcheng wrote: > Btw, if there are no possible races here, we usually like to annotate with > NOTREACHED() and just close the binding (and not invoke the callback). Yup, this code path should not be reached as long as the remote Android service is working correctly. Let's use NOTREACHED(). But I think we still need to invoke the callback because otherwise Android service can "attack" Chrome so that a callback is not invoked indefinitely. Closing the connection sounds good, but I'm afraid ARC code in Chrome is not prepared to actively close instance connections. Please let me just ignore invalid responses and keep the connection for now. https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc (right): https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc:96: LOG(ERROR) << "ArcFileSystemOperationRunner unavailable. " On 2017/03/03 09:31:14, dcheng wrote: > DLOG(ERROR) for all the other LOG(ERROR) introduced by this CL too. Sure, done. https://codereview.chromium.org/2715493002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc:235: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, On 2017/03/03 09:31:14, dcheng wrote: > Btw... does ObserverListThreadSafe help with this at all? This seems pretty > complicated =) I tried ObserverListThreadSafe actually, but I still needed RefCountedThreadSafe. I agree this is complicated. I welcome if you have any idea to make this simpler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2715493002/#ps240001 (title: "Addressed dcheng's comments.")
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": 240001, "attempt_start_ts": 1488555315347080,
"parent_rev": "42ab622f90f2f2c840ceae7137a3d9d66f04c5e6", "commit_rev":
"1cf2cd5fd31329f2cb7de2cd2df919648b5e331b"}
Message was sent while issue was closed.
Description was changed from ========== mediaview: Support watchers in ArcFileSystemOperationRunner. AddWatcher() and RemoveWatcher() are added. Also, ArcFileSystemOperationRunner implements FileSystemHost to receive watcher callbacks from the Android container. BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: Support watchers in ArcFileSystemOperationRunner. AddWatcher() and RemoveWatcher() are added. Also, ArcFileSystemOperationRunner implements FileSystemHost to receive watcher callbacks from the Android container. BUG=chromium:684233 TEST=trybot Review-Url: https://codereview.chromium.org/2715493002 Cr-Commit-Position: refs/heads/master@{#454589} Committed: https://chromium.googlesource.com/chromium/src/+/1cf2cd5fd31329f2cb7de2cd2df9... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/1cf2cd5fd31329f2cb7de2cd2df9...
Message was sent while issue was closed.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc (right): https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:33: NOTREACHED(); nit: mention that the above switch is exhaustive, but some compilers (old gcc?) still complain about it :/ https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:62: using AddWatcherCallback = base::Callback<void(int64_t watcher_id)>; nit: maybe sort lexicographically? I couldn't find any logical ordering. https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_unittest.cc (right): https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_unittest.cc:89: 123, can you make this a named constant? Maybe kInvalidDocumentId? https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h (right): https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:70: virtual ~Observer() = default; On 2017/03/03 04:22:15, Shuhei Takahashi wrote: > On 2017/03/02 14:43:47, hidehiko wrote: > > Could you move this into protected? > > > > cf) > > > https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos... > > > > That will be added into our style guide (just not yet, though) as commented by > > Luis. > > Done. It is now in the style guide :)
Message was sent while issue was closed.
On 2017/03/06 17:10:48, Luis Héctor Chávez wrote: > https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc > (right): > > https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:33: > NOTREACHED(); > nit: mention that the above switch is exhaustive, but some compilers (old gcc?) > still complain about it :/ > > https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h > (right): > > https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:62: using > AddWatcherCallback = base::Callback<void(int64_t watcher_id)>; > nit: maybe sort lexicographically? I couldn't find any logical ordering. > > https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... > File > chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_unittest.cc > (right): > > https://codereview.chromium.org/2715493002/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_unittest.cc:89: > 123, > can you make this a named constant? Maybe kInvalidDocumentId? > > https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h > (right): > > https://codereview.chromium.org/2715493002/diff/200001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:70: > virtual ~Observer() = default; > On 2017/03/03 04:22:15, Shuhei Takahashi wrote: > > On 2017/03/02 14:43:47, hidehiko wrote: > > > Could you move this into protected? > > > > > > cf) > > > > > > https://codereview.chromium.org/2720303002/diff/60001/chrome/browser/chromeos... > > > > > > That will be added into our style guide (just not yet, though) as commented > by > > > Luis. > > > > Done. > > It is now in the style guide :) Huh, apparently I had some old drafts that I forgot to send :( please ignore them. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
