Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10352004:
gdata: Implement periodic file system update checks. (Closed)
|
Created:
8 years, 7 months ago by hshi Modified:
8 years, 7 months ago CC:
chromium-reviews, achuith+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptiongdata: Implement periodic file system update checks.
We should run periodic checks for updates on server as long as we
have presence of a file watcher for /gdata.
BUG=chromium-os:29411
TEST=locally tested on lumpy device.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135251
Patch Set 1 : Only start timer when file watcher is present on GData. #Patch Set 2 : Fix compile error in mock_gdata_file_system.h #
Total comments: 14
Patch Set 3 : Address Zel & Satoru's comments regarding Patch Set #2 #Patch Set 4 : Bind the ui_weak_ptr_ in a callback to Timer::Start #
Total comments: 12
Patch Set 5 : Use base timer class and other miscellaneous fixes #
Total comments: 8
Patch Set 6 : Move num_update_requests_ logic to FileBrowserEventRouter. Handle start/stop update requests in UI … #
Total comments: 3
Patch Set 7 : Add 2 more DCHECKs to make sure we're not starting/stopping timer when it is already started/stoppe… #
Total comments: 4
Messages
Total messages: 30 (0 generated)
Uploaded Patch Set #1. This appears to be working correctly on my lumpy device. Please review this and let me know which area should I improve. Thanks!
http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1047: this, &GDataFileSystem::CheckForUpdates); this should be ui_weak_ptr_ instead of 'this' http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.h:408: virtual void AddFileWatch(const FilePath& watch_path) OVERRIDE; Please add StartUpdates() and StopUpdates() methods instead.
http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.h (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.h:159: gdata::GDataFileSystem* GetFileSystem() const; function comment is missing. one line comment would suffice. http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1043: if (!(num_file_watches_++)) { This looks tricky. maybe: ++num_file_watches_; if (num_file_watches_ == 1) { // first time. http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1047: this, &GDataFileSystem::CheckForUpdates); On 2012/05/03 03:41:07, zel wrote: > this should be ui_weak_ptr_ instead of 'this' Looking at base/timer.h, there is another Start() method that takes a callback rather than |this| + |method|. Can you use that version instead? http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1054: } else if (!(--num_file_watches_)) { This also looks tricky. maybe else { --num_file_watches_; if (num_file_watches_ == 0) { http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.h:1313: // Check for updates on the server. Check -> Checks per our style guide
Uploaded Patch Set #3 for review, thanks! http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.h (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.h:159: gdata::GDataFileSystem* GetFileSystem() const; On 2012/05/03 16:49:29, satorux1 wrote: > function comment is missing. one line comment would suffice. Done. http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1043: if (!(num_file_watches_++)) { On 2012/05/03 16:49:29, satorux1 wrote: > This looks tricky. maybe: > > ++num_file_watches_; > if (num_file_watches_ == 1) { // first time. Done. http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1047: this, &GDataFileSystem::CheckForUpdates); Zel & Satoru, the documentation in base/timer.h actually recommends to pass the "this" pointer directly to base::RepeatingTimer.Start(). This is also how this is invoked in many other places. The definition of BaseTimerMethodPointer::Start already handles the binding of the method and the "this" pointer as a non-refcounted class: void Start(const tracked_objects::Location& posted_from, TimeDelta delay, Receiver* receiver, ReceiverMethod method) { Timer::Start(posted_from, delay, base::Bind(method, base::Unretained(receiver))); } http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1054: } else if (!(--num_file_watches_)) { On 2012/05/03 16:49:29, satorux1 wrote: > This also looks tricky. maybe > > else { > --num_file_watches_; > if (num_file_watches_ == 0) { Done. http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.h:408: virtual void AddFileWatch(const FilePath& watch_path) OVERRIDE; On 2012/05/03 03:41:07, zel wrote: > Please add StartUpdates() and StopUpdates() methods instead. Done. http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.h:1313: // Check for updates on the server. On 2012/05/03 16:49:29, satorux1 wrote: > Check -> Checks per our style guide Done.
http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1047: this, &GDataFileSystem::CheckForUpdates); On 2012/05/03 17:46:35, hshi wrote: > Zel & Satoru, the documentation in base/timer.h actually recommends to pass the > "this" pointer directly to base::RepeatingTimer.Start(). This is also how this > is invoked in many other places. > > The definition of BaseTimerMethodPointer::Start already handles the binding of > the method and the "this" pointer as a non-refcounted class: > > void Start(const tracked_objects::Location& posted_from, > TimeDelta delay, > Receiver* receiver, > ReceiverMethod method) { > Timer::Start(posted_from, delay, > base::Bind(method, base::Unretained(receiver))); > } This is worrisome. Is this safe? I don't know how Timer classes are implemented, so this may be safe, but asynchronous API that takes raw |this| pointer is scary. Generally we avoid using base::Unretained(this) as a bad thing (use-after-free) happens if the callback is run after |this| object is deleted. If we can pass base::Bind(method, weak_ptr_ui) instead of |this| + |method|, it'd be safer.
Satoru,
For some strange reason, I'm unable to directly invoke the base-class method
base::Timer.Start(const tracked_objects::Location&, TimeDelta delay, const
base::Closure&). Doing so will cause a compile error.
However, I can explicitly make the Start call to the base class, like this:
update_timer_.Timer::Start(FROM_HERE,
base::TimeDelta::FromSeconds(
kGDataUpdateCheckIntervalInSec),
base::Bind(&GDataFileSystem::CheckForUpdates,
ui_weak_ptr_));
Does that look ok?
On 2012/05/03 18:09:29, satorux1 wrote:
>
http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):
>
>
http://codereview.chromium.org/10352004/diff/5004/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_file_system.cc:1047: this,
> &GDataFileSystem::CheckForUpdates);
> On 2012/05/03 17:46:35, hshi wrote:
> > Zel & Satoru, the documentation in base/timer.h actually recommends to pass
> the
> > "this" pointer directly to base::RepeatingTimer.Start(). This is also how
this
> > is invoked in many other places.
> >
> > The definition of BaseTimerMethodPointer::Start already handles the binding
of
> > the method and the "this" pointer as a non-refcounted class:
> >
> > void Start(const tracked_objects::Location& posted_from,
> > TimeDelta delay,
> > Receiver* receiver,
> > ReceiverMethod method) {
> > Timer::Start(posted_from, delay,
> > base::Bind(method, base::Unretained(receiver)));
> > }
>
> This is worrisome. Is this safe? I don't know how Timer classes are
implemented,
> so this may be safe, but asynchronous API that takes raw |this| pointer is
> scary.
>
> Generally we avoid using base::Unretained(this) as a bad thing
(use-after-free)
> happens if the callback is run after |this| object is deleted.
>
> If we can pass base::Bind(method, weak_ptr_ui) instead of |this| + |method|,
> it'd be safer.
http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1050: ui_weak_ptr_)); Unfortunately I have to explicitly call base class (Timer::Start) here. Some compiler problem with C++ templates and inheritance prevents me from directly calling the Start method with the callback as the 3rd argument.
Why don't you just use that base class as it is? On Thu, May 3, 2012 at 11:26 AM, <hshi@google.com> wrote: > > http://codereview.chromium.**org/10352004/diff/12006/** > chrome/browser/chromeos/gdata/**gdata_file_system.cc<http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gdata/gdata_file_system.cc> > File chrome/browser/chromeos/gdata/**gdata_file_system.cc (right): > > http://codereview.chromium.**org/10352004/diff/12006/** > chrome/browser/chromeos/gdata/**gdata_file_system.cc#**newcode1050<http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gdata/gdata_file_system.cc#newcode1050> > chrome/browser/chromeos/gdata/**gdata_file_system.cc:1050: ui_weak_ptr_)); > Unfortunately I have to explicitly call base class (Timer::Start) here. > Some compiler problem with C++ templates and inheritance prevents me > from directly calling the Start method with the callback as the 3rd > argument. > > http://codereview.chromium.**org/10352004/<http://codereview.chromium.org/103... >
http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1050: ui_weak_ptr_)); On 2012/05/03 18:26:37, hshi wrote: > Unfortunately I have to explicitly call base class (Timer::Start) here. Some > compiler problem with C++ templates and inheritance prevents me from directly > calling the Start method with the callback as the 3rd argument. I just found this comment in timer.h: // OneShotTimer and RepeatingTimer both cancel the timer when they go out of // scope, which makes it easy to ensure that you do not get called when your // object has gone out of scope. Passing |this| is guaranteed to be safe. So I think your original code is correct. The API looks rather scary, but should be safe. http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1058: } you can replace the four lines with: DCHECK_GE(0, num_update_requests); http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:180: virtual void StartUpdates() = 0; Please also explain a bit more about these functions. For instance, StartUpdates() will start periodic updates only if it's not yet started, right? Likewise, StopUpdates() stops it only if the number of update requests becomes zero, right? that said, RequestStartPeriodicUpdates(), RequestStopPeriodicUpdates() may be better names.
I can use the base class as it is. I personally think this extra layer of "RepeatingTimer" is not a very useful design, even though it is well intended. On 2012/05/03 18:40:51, zel wrote: > Why don't you just use that base class as it is?
http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1050: ui_weak_ptr_)); How about I just use the base::Timer class as Zel suggested? I find that it is actually cleaner. Passing in |this| is scary and confusing. On 2012/05/03 18:46:20, satorux1 wrote: > On 2012/05/03 18:26:37, hshi wrote: > > Unfortunately I have to explicitly call base class (Timer::Start) here. Some > > compiler problem with C++ templates and inheritance prevents me from directly > > calling the Start method with the callback as the 3rd argument. > > I just found this comment in timer.h: > > // OneShotTimer and RepeatingTimer both cancel the timer when they go out of > // scope, which makes it easy to ensure that you do not get called when your > // object has gone out of scope. > > Passing |this| is guaranteed to be safe. So I think your original code is > correct. The API looks rather scary, but should be safe. http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1058: } Did you mean DCHECK_GE(num_update_requests, 0)? On 2012/05/03 18:46:20, satorux1 wrote: > you can replace the four lines with: > > DCHECK_GE(0, num_update_requests);
http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1050: ui_weak_ptr_)); On 2012/05/03 18:55:54, hshi wrote: > How about I just use the base::Timer class as Zel suggested? I find that it is > actually cleaner. Passing in |this| is scary and confusing. > > On 2012/05/03 18:46:20, satorux1 wrote: > > On 2012/05/03 18:26:37, hshi wrote: > > > Unfortunately I have to explicitly call base class (Timer::Start) here. Some > > > compiler problem with C++ templates and inheritance prevents me from > directly > > > calling the Start method with the callback as the 3rd argument. > > > > I just found this comment in timer.h: > > > > // OneShotTimer and RepeatingTimer both cancel the timer when they go out of > > > // scope, which makes it easy to ensure that you do not get called when your > > > // object has gone out of scope. > > > > Passing |this| is guaranteed to be safe. So I think your original code is > > correct. The API looks rather scary, but should be safe. > Sounds good. http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1058: } On 2012/05/03 18:55:54, hshi wrote: > Did you mean DCHECK_GE(num_update_requests, 0)? > > On 2012/05/03 18:46:20, satorux1 wrote: > > you can replace the four lines with: > > > > DCHECK_GE(0, num_update_requests); > oh I meant DCHECK_LE(). not a big deal at all, but seems to me that a constant (in this case 0) is often the first parameter in DCHECK_XX().
http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1050: ui_weak_ptr_)); On 2012/05/03 18:59:54, satorux1 wrote: > On 2012/05/03 18:55:54, hshi wrote: > > How about I just use the base::Timer class as Zel suggested? I find that it is > > actually cleaner. Passing in |this| is scary and confusing. > > > > On 2012/05/03 18:46:20, satorux1 wrote: > > > On 2012/05/03 18:26:37, hshi wrote: > > > > Unfortunately I have to explicitly call base class (Timer::Start) here. > Some > > > > compiler problem with C++ templates and inheritance prevents me from > > directly > > > > calling the Start method with the callback as the 3rd argument. > > > > > > I just found this comment in timer.h: > > > > > > // OneShotTimer and RepeatingTimer both cancel the timer when they go out of > > > > > > // scope, which makes it easy to ensure that you do not get called when your > > > > > > // object has gone out of scope. > > > > > > Passing |this| is guaranteed to be safe. So I think your original code is > > > correct. The API looks rather scary, but should be safe. > > > > Sounds good. Done. http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1058: } On 2012/05/03 18:59:54, satorux1 wrote: > On 2012/05/03 18:55:54, hshi wrote: > > Did you mean DCHECK_GE(num_update_requests, 0)? > > > > On 2012/05/03 18:46:20, satorux1 wrote: > > > you can replace the four lines with: > > > > > > DCHECK_GE(0, num_update_requests); > > > > oh I meant DCHECK_LE(). not a big deal at all, but seems to me that a constant > (in this case 0) is often the first parameter in DCHECK_XX(). Done. http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:180: virtual void StartUpdates() = 0; On 2012/05/03 18:46:20, satorux1 wrote: > Please also explain a bit more about these functions. For instance, > StartUpdates() will start periodic updates only if it's not yet started, right? > > Likewise, StopUpdates() stops it only if the number of update requests becomes > zero, right? > > that said, RequestStartPeriodicUpdates(), RequestStopPeriodicUpdates() may be > better names. Done. Although I think "Periodic" may be redundant.
LGTM http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/10352004/diff/12006/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.h:180: virtual void StartUpdates() = 0; On 2012/05/03 19:09:19, hshi wrote: > On 2012/05/03 18:46:20, satorux1 wrote: > > Please also explain a bit more about these functions. For instance, > > StartUpdates() will start periodic updates only if it's not yet started, > right? > > > > Likewise, StopUpdates() stops it only if the number of update requests becomes > > zero, right? > > > > that said, RequestStartPeriodicUpdates(), RequestStopPeriodicUpdates() may be > > better names. > > Done. Although I think "Periodic" may be redundant. Agreed.
Sorry but there is one more issue. It turns out that I must invoke gdata::GdataSystemServiceFactory::GetForProfile() from the UI thread. If I do it in browser thread I will trigger a DCHECk in ProfileKeyedBaseFactory::GetProfileToUse. So it looks like I must post the task to UI thread and add two more functions in file_browser_event_router.cc to handle the RequestStartUpdates and RequestStopUpdates calls. Any idea to do it in a simpler way? Thanks.
On 2012/05/03 19:29:36, hshi wrote: > Sorry but there is one more issue. It turns out that I must invoke > gdata::GdataSystemServiceFactory::GetForProfile() from the UI thread. If I do it > in browser thread I will trigger a DCHECk in > ProfileKeyedBaseFactory::GetProfileToUse. what "browser thread" are you talking about? maybe IO thread? I thought file_browser_event_router.cc ran on UI thread? > So it looks like I must post the task to UI thread and add two more functions in > file_browser_event_router.cc to handle the RequestStartUpdates and > RequestStopUpdates calls. Any idea to do it in a simpler way? Thanks. If you have a weak pointer bound to UI thread (one created on UI thread), you can post a task to UI thread using BrowserThread::PostTask and the weak ptr.
http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:170: gdata::GDataFileSystem* file_system = GetFileSystem(); Per your comment, we are not on UI thread here? If so, on what thread we are now? Please annotate with DCHECK(BrowserThread::CurrentlyOn(BrowserThread::XXX)); at the beginning of this function.
http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:172: file_system->RequestStartUpdates(); I have another idea. Wouldn't it be simpler just to call this when FileBrowserEventRouter is created, and call RequestStopUpdates() when the router is deleted?
http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:172: file_system->RequestStartUpdates(); On 2012/05/03 19:44:54, satorux1 wrote: > I have another idea. > > Wouldn't it be simpler just to call this when FileBrowserEventRouter is created, > and call RequestStopUpdates() when the router is deleted? I was assuming that FileBrowserEventRouter is created when the file manager is opened, and deleted when the file manager is closed. I may be wrong.
http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:172: file_system->RequestStartUpdates(); On 2012/05/03 19:47:41, satorux1 wrote: > On 2012/05/03 19:44:54, satorux1 wrote: > > I have another idea. > > > > Wouldn't it be simpler just to call this when FileBrowserEventRouter is > created, > > and call RequestStopUpdates() when the router is deleted? > > I was assuming that FileBrowserEventRouter is created when the file manager is > opened, and deleted when the file manager is closed. I may be wrong. Lifetime of FileBrowserEventRouter is RefcountedProfileKeyedService - it's lifetime is linked to the Profile, so we surely don't want to start monitoring updates as soon as it gets created. http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1043: ++num_update_requests_; You should move num_update_requests_ logic to FileBrowserEventRouter.
> You should move num_update_requests_ logic to FileBrowserEventRouter. My intention with the current CL is to always call RequestStartUpdates which would perform an immediate check for updates, in addition to starting the timer when num_update_requests_ == 1. This ensures that when someone presses "refresh" or opens a 2nd file manager window he/she will get immediate refresh instead of waiting for the timer interval to elapse. It can still work this way if I move num_updates_requests_ logic to FileBrowserEventRouter, but then I have to declare GDataFileSystem::CheckForUpdates as public.
Uploaded Patch Set #6: moved num_update_requests_ logic to FileBrowserEventRouter; moved the start/stop update requests to a separate handler on the UI thread. Thanks! http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:170: gdata::GDataFileSystem* file_system = GetFileSystem(); On 2012/05/03 19:41:34, satorux1 wrote: > Per your comment, we are not on UI thread here? If so, on what thread we are > now? Please annotate with > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::XXX)); > > at the beginning of this function. Done. http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:172: file_system->RequestStartUpdates(); I have moved this logic out to a separate task on the UI thread. http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/4006/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1043: ++num_update_requests_; On 2012/05/03 19:54:30, zel wrote: > You should move num_update_requests_ logic to FileBrowserEventRouter. Done.
http://codereview.chromium.org/10352004/diff/12015/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/12015/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/file_browser_event_router.cc:189: base::Bind(&FileBrowserEventRouter::HandleRemoteUpdateRequestOnUIThread, why are you routing this to another thread? this function should be already running on UI thread - no?
I'm afraid that these two functions seem to be running on the File thread. From debugger: FileWatchBrowserFunctionBase::RunFileWatchOperationOnFileThread --> Add/RemoveFileWatchBrowserFunction::PerformFileWatchOperation--> event_router->Add/RemoveFileWatch
lgtm http://codereview.chromium.org/10352004/diff/12015/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/12015/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1040: update_timer_.Start(FROM_HERE, you should DCHECK to make sure we are not calling this if the timer is already running
http://codereview.chromium.org/10352004/diff/12015/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/12015/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_file_system.cc:1040: update_timer_.Start(FROM_HERE, On 2012/05/03 21:40:41, zel wrote: > you should DCHECK to make sure we are not calling this if the timer is already > running Done. http://codereview.chromium.org/10352004/diff/3019/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10352004/diff/3019/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:1049: DCHECK(update_timer_.IsRunning()); Ditto.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@google.com/10352004/3019
Change committed as 135251
http://codereview.chromium.org/10352004/diff/3019/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/3019/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:144: const std::string& extension_id) { I thought we wanted to have DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); http://codereview.chromium.org/10352004/diff/3019/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:179: const std::string& extension_id) { ditto.
http://codereview.chromium.org/10352004/diff/3019/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right): http://codereview.chromium.org/10352004/diff/3019/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_browser_event_router.cc:144: const std::string& extension_id) { Sorry, the CL is already committed. I'll try to squeeze it in in a separate CL. On 2012/05/04 00:42:20, satorux1 wrote: > I thought we wanted to have > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
