Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827098:
Update the number of fetched files more frequently. (Closed)
|
Created:
8 years, 4 months ago by hidehiko Modified:
8 years, 4 months ago Reviewers:
satorux1 CC:
chromium-reviews, achuith+watch_chromium.org, oshima+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. |
DescriptionUpdate the number of fetched files more frequently.
BUG=128141
TEST=ran manually, ran browser_tests, and ran unit_tests --gtest_filter=GData*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149893
Patch Set 1 #Patch Set 2 : Update the number of fetched files more frequently. #
Total comments: 14
Patch Set 3 : Update the number of fetched files more frequently. #Patch Set 4 : Update the number of fetched files more frequently. #
Total comments: 18
Patch Set 5 : Update the number of fetched files more frequently. #
Messages
Total messages: 12 (0 generated)
Hi Satoru, I cleaned up the patch. Would you mind to take a look?
sorry for the belated response. http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:58: const int kNumUiUpdateStepsPerFetch = 50; With kMaxDocumentsPerFeed = 500 defined elsewhere, and 50 here, we update the fetch progress UI per every 10 feeds. If we change kMaxDocumentsPerFeed to 700, we'll end up counting per every 14 feeds, which would look bad. That said, Wouldn't it be better to define something like // Update the fetch progress UI per every 10 feeds, though it's giving illusion to users. The feeds are actually processed per kMaxDocumentsPerFeed const int kFetchUiUpdateStep = 10; http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:615: // GDataFileSystem::GetDocumentsUIState struct implementation. UI -> Ui http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:637: void Run() { I'm guessing that this method is unnecessary. see below. http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2918: ui_weak_ptr_)); I have a feeling that this base::Bind() is unnecessary. http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2918: ui_weak_ptr_)); I'm guessing that this callback parameter is unnecessary. see below. http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2921: DCHECK(ui_state != NULL); remove != NULL, to be consistent with elsewhere. http://codereview.chromium.org/10827098/diff/2001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2927: base::Bind(&GetDocumentsUIState::Run, ui_state->ui_weak_ptr)); Cannot we just do the following here? base::Bind(&GDataFileSystem::OnNotifyDocumentFeedFetched, ui_weak_ptr_, ui_state)
Thank you for your review. Could you take another look? https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:58: const int kNumUiUpdateStepsPerFetch = 50; On 2012/08/02 08:03:14, satorux1 wrote: > With kMaxDocumentsPerFeed = 500 defined elsewhere, and 50 here, we update the > fetch progress UI per every 10 feeds. If we change kMaxDocumentsPerFeed to 700, > we'll end up counting per every 14 feeds, which would look bad. > > That said, Wouldn't it be better to define something like > > // Update the fetch progress UI per every 10 feeds, though it's giving illusion > to users. The feeds are actually processed per kMaxDocumentsPerFeed > const int kFetchUiUpdateStep = 10; Done. https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:615: // GDataFileSystem::GetDocumentsUIState struct implementation. On 2012/08/02 08:03:14, satorux1 wrote: > UI -> Ui Done. https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:637: void Run() { On 2012/08/02 08:03:14, satorux1 wrote: > I'm guessing that this method is unnecessary. see below. Done. https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2918: ui_weak_ptr_)); On 2012/08/02 08:03:14, satorux1 wrote: > I have a feeling that this base::Bind() is unnecessary. Done. https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2918: ui_weak_ptr_)); On 2012/08/02 08:03:14, satorux1 wrote: > I'm guessing that this callback parameter is unnecessary. see below. Done. https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2921: DCHECK(ui_state != NULL); On 2012/08/02 08:03:14, satorux1 wrote: > remove != NULL, to be consistent with elsewhere. Done. https://chromiumcodereview.appspot.com/10827098/diff/2001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2927: base::Bind(&GetDocumentsUIState::Run, ui_state->ui_weak_ptr)); On 2012/08/02 08:03:14, satorux1 wrote: > Cannot we just do the following here? > > base::Bind(&GDataFileSystem::OnNotifyDocumentFeedFetched, ui_weak_ptr_, > ui_state) > Done.
http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:58: const int kFetchUiUpdateStep = 10; Please put some comment like // Update the fetch progress UI per every this number of feeds. http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:640: // How long duration the actual fetching takes in total. What about: // Time elapsed since the the feed fetching was started. http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:642: base::TimeDelta fetched_duration; maybe: feed_fetching_elapsed_time ? http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:2908: // Currently the UI update is stopped. Start UI periodical callback. periodic http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3675: return; I think managing lifetime of ui_state using weak ptr is tricky. What about: 1) Make ui_state a member of GDataWapiLoader (sorry I submitted a large refactoring, and this part is now in a separate class) 2) Create ui_state_ in OnGetDocuments. 3) Delete it in OnGetDocuments, once all feeds are processed. http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3678: base::TimeDelta consumed_duration = maybe: elapsed_time http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3686: int num_remaining_ui_update = num_remaining_ui_updates
Thank you for your review. I'll address all your remaining comments, but before editing the code, please let me ask a question about the life cycle of ui state params. http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3675: return; On 2012/08/02 16:31:03, satorux1 wrote: > I think managing lifetime of ui_state using weak ptr is tricky. What about: > > 1) Make ui_state a member of GDataWapiLoader (sorry I submitted a large > refactoring, and this part is now in a separate class) > 2) Create ui_state_ in OnGetDocuments. > 3) Delete it in OnGetDocuments, once all feeds are processed. It looks slightly danger to me, because; When OnGetDocuments with all feed fetch done is called, a pending task to call OnNotifyDocumentFeedFetched may be in the message queue. In that case, once the ui_state_ would be deleted. However, it is possible to re-run another data fetching before pending OnNotifyDocumentFeedFetched is run (I guess it should be quite rare in the real world, though), then ui_state_ will be re-created and another OnNotifyDocumentFeedFetched task would be enqueued. We here would need to ignore first OnNofityDocumentFeedFetched invocation somehow, and to process the second invocation normally (otherwise the callback sequence would get buggy state). If we make ui_state_ the member of GDataWapiLoader, it'd be very hard to distinguish the first and second invocation. I put GetDocumentUiState into GetDocumentsParams because GetDocumentsParams holds a data for one whole fetching process and I thought the UI state is also a part of it. What do you think?
http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3675: return; On 2012/08/02 19:06:33, hidehiko wrote: > On 2012/08/02 16:31:03, satorux1 wrote: > > I think managing lifetime of ui_state using weak ptr is tricky. What about: > > > > 1) Make ui_state a member of GDataWapiLoader (sorry I submitted a large > > refactoring, and this part is now in a separate class) > > 2) Create ui_state_ in OnGetDocuments. > > 3) Delete it in OnGetDocuments, once all feeds are processed. > > It looks slightly danger to me, because; > > When OnGetDocuments with all feed fetch done is called, a pending task to call > OnNotifyDocumentFeedFetched may be in the message queue. > In that case, once the ui_state_ would be deleted. However, it is possible to > re-run another data fetching before pending OnNotifyDocumentFeedFetched is run > (I guess it should be quite rare in the real world, though), then ui_state_ will > be re-created and another OnNotifyDocumentFeedFetched task would be enqueued. > We here would need to ignore first OnNofityDocumentFeedFetched invocation > somehow, and to process the second invocation normally (otherwise the callback > sequence would get buggy state). If we make ui_state_ the member of > GDataWapiLoader, it'd be very hard to distinguish the first and second > invocation. > > I put GetDocumentUiState into GetDocumentsParams because GetDocumentsParams > holds a data for one whole fetching process and I thought the UI state is also a > part of it. > > What do you think? > > Are you are right, can we delete it in OnNotifyDocumentFeedFetched when the last feed is processed? WeakPtr is tricky so I'd like to minimize use of it.
http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3675: return; On 2012/08/02 19:37:16, satorux1 wrote: > On 2012/08/02 19:06:33, hidehiko wrote: > > On 2012/08/02 16:31:03, satorux1 wrote: > > > I think managing lifetime of ui_state using weak ptr is tricky. What about: > > > > > > 1) Make ui_state a member of GDataWapiLoader (sorry I submitted a large > > > refactoring, and this part is now in a separate class) > > > 2) Create ui_state_ in OnGetDocuments. > > > 3) Delete it in OnGetDocuments, once all feeds are processed. > > > > It looks slightly danger to me, because; > > > > When OnGetDocuments with all feed fetch done is called, a pending task to call > > OnNotifyDocumentFeedFetched may be in the message queue. > > In that case, once the ui_state_ would be deleted. However, it is possible to > > re-run another data fetching before pending OnNotifyDocumentFeedFetched is run > > (I guess it should be quite rare in the real world, though), then ui_state_ > will > > be re-created and another OnNotifyDocumentFeedFetched task would be enqueued. > > We here would need to ignore first OnNofityDocumentFeedFetched invocation > > somehow, and to process the second invocation normally (otherwise the callback > > sequence would get buggy state). If we make ui_state_ the member of > > GDataWapiLoader, it'd be very hard to distinguish the first and second > > invocation. > > > > I put GetDocumentUiState into GetDocumentsParams because GetDocumentsParams > > holds a data for one whole fetching process and I thought the UI state is also > a > > part of it. > > > > What do you think? > > > > > > Are you are right, can we delete it in OnNotifyDocumentFeedFetched when the last > feed is processed? > > WeakPtr is tricky so I'd like to minimize use of it. > No, unfortunately, because it's potential memory leaking. When GDataWapiLoader is deleted, there may be a pending task to invoke OnNotifyDocumentFeedFetched. In that case, OnNotifyDocumentFeedFetched wouldn't be actually invoked, and then delete operation wouldn't be executed. If we keep ui_state as a member of GDataWapiLoader, there is another (but probably rare case) issue that; in the scenario in my previous reply, it is impossible to store the secondary created ui-state instance into ui_state_ field, because there remains the older ui_state instance. What we actually need here is cancelling the pending OnNotifyDocumentFeedFetched conceptually, and WeakPtr seems the commonly used implementation of the (pseudo) cancelling? (Though, I agree WeakPtr looks tricky in general...)
http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10827098/diff/9001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_file_system.cc:3675: return; On 2012/08/02 19:59:06, hidehiko wrote: > On 2012/08/02 19:37:16, satorux1 wrote: > > On 2012/08/02 19:06:33, hidehiko wrote: > > > On 2012/08/02 16:31:03, satorux1 wrote: > > > > I think managing lifetime of ui_state using weak ptr is tricky. What > about: > > > > > > > > 1) Make ui_state a member of GDataWapiLoader (sorry I submitted a large > > > > refactoring, and this part is now in a separate class) > > > > 2) Create ui_state_ in OnGetDocuments. > > > > 3) Delete it in OnGetDocuments, once all feeds are processed. > > > > > > It looks slightly danger to me, because; > > > > > > When OnGetDocuments with all feed fetch done is called, a pending task to > call > > > OnNotifyDocumentFeedFetched may be in the message queue. > > > In that case, once the ui_state_ would be deleted. However, it is possible > to > > > re-run another data fetching before pending OnNotifyDocumentFeedFetched is > run > > > (I guess it should be quite rare in the real world, though), then ui_state_ > > will > > > be re-created and another OnNotifyDocumentFeedFetched task would be > enqueued. > > > We here would need to ignore first OnNofityDocumentFeedFetched invocation > > > somehow, and to process the second invocation normally (otherwise the > callback > > > sequence would get buggy state). If we make ui_state_ the member of > > > GDataWapiLoader, it'd be very hard to distinguish the first and second > > > invocation. > > > > > > I put GetDocumentUiState into GetDocumentsParams because GetDocumentsParams > > > holds a data for one whole fetching process and I thought the UI state is > also > > a > > > part of it. > > > > > > What do you think? > > > > > > > > > > Are you are right, can we delete it in OnNotifyDocumentFeedFetched when the > last > > feed is processed? > > > > WeakPtr is tricky so I'd like to minimize use of it. > > > > No, unfortunately, because it's potential memory leaking. > When GDataWapiLoader is deleted, there may be a pending task to invoke > OnNotifyDocumentFeedFetched. In that case, OnNotifyDocumentFeedFetched wouldn't > be actually invoked, and then delete operation wouldn't be executed. > > If we keep ui_state as a member of GDataWapiLoader, there is another (but > probably rare case) issue that; in the scenario in my previous reply, it is > impossible to store the secondary created ui-state instance into ui_state_ > field, because there remains the older ui_state instance. > > What we actually need here is cancelling the pending OnNotifyDocumentFeedFetched > conceptually, and WeakPtr seems the commonly used implementation of the (pseudo) > cancelling? (Though, I agree WeakPtr looks tricky in general...) Ah I see. it's complicated. :) I thought about it again, but could not come up with a good idea. Let's go with the weak ptr approach for now.
Merged to the head, and adapted the code style a bit. Could you take another look? https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:58: const int kFetchUiUpdateStep = 10; On 2012/08/02 16:31:03, satorux1 wrote: > Please put some comment like > > // Update the fetch progress UI per every this number of feeds. Done. https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:640: // How long duration the actual fetching takes in total. On 2012/08/02 16:31:03, satorux1 wrote: > What about: > > // Time elapsed since the the feed fetching was started. Done. https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:642: base::TimeDelta fetched_duration; On 2012/08/02 16:31:03, satorux1 wrote: > maybe: feed_fetching_elapsed_time ? Done. https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:2908: // Currently the UI update is stopped. Start UI periodical callback. On 2012/08/02 16:31:03, satorux1 wrote: > periodic Done. https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:3675: return; On 2012/08/02 21:14:48, satorux1 wrote: > On 2012/08/02 19:59:06, hidehiko wrote: > > On 2012/08/02 19:37:16, satorux1 wrote: > > > On 2012/08/02 19:06:33, hidehiko wrote: > > > > On 2012/08/02 16:31:03, satorux1 wrote: > > > > > I think managing lifetime of ui_state using weak ptr is tricky. What > > about: > > > > > > > > > > 1) Make ui_state a member of GDataWapiLoader (sorry I submitted a large > > > > > refactoring, and this part is now in a separate class) > > > > > 2) Create ui_state_ in OnGetDocuments. > > > > > 3) Delete it in OnGetDocuments, once all feeds are processed. > > > > > > > > It looks slightly danger to me, because; > > > > > > > > When OnGetDocuments with all feed fetch done is called, a pending task to > > call > > > > OnNotifyDocumentFeedFetched may be in the message queue. > > > > In that case, once the ui_state_ would be deleted. However, it is possible > > to > > > > re-run another data fetching before pending OnNotifyDocumentFeedFetched is > > run > > > > (I guess it should be quite rare in the real world, though), then > ui_state_ > > > will > > > > be re-created and another OnNotifyDocumentFeedFetched task would be > > enqueued. > > > > We here would need to ignore first OnNofityDocumentFeedFetched invocation > > > > somehow, and to process the second invocation normally (otherwise the > > callback > > > > sequence would get buggy state). If we make ui_state_ the member of > > > > GDataWapiLoader, it'd be very hard to distinguish the first and second > > > > invocation. > > > > > > > > I put GetDocumentUiState into GetDocumentsParams because > GetDocumentsParams > > > > holds a data for one whole fetching process and I thought the UI state is > > also > > > a > > > > part of it. > > > > > > > > What do you think? > > > > > > > > > > > > > > Are you are right, can we delete it in OnNotifyDocumentFeedFetched when the > > last > > > feed is processed? > > > > > > WeakPtr is tricky so I'd like to minimize use of it. > > > > > > > No, unfortunately, because it's potential memory leaking. > > When GDataWapiLoader is deleted, there may be a pending task to invoke > > OnNotifyDocumentFeedFetched. In that case, OnNotifyDocumentFeedFetched > wouldn't > > be actually invoked, and then delete operation wouldn't be executed. > > > > If we keep ui_state as a member of GDataWapiLoader, there is another (but > > probably rare case) issue that; in the scenario in my previous reply, it is > > impossible to store the secondary created ui-state instance into ui_state_ > > field, because there remains the older ui_state instance. > > > > What we actually need here is cancelling the pending > OnNotifyDocumentFeedFetched > > conceptually, and WeakPtr seems the commonly used implementation of the > (pseudo) > > cancelling? (Though, I agree WeakPtr looks tricky in general...) > > Ah I see. it's complicated. :) > > I thought about it again, but could not come up with a good idea. Let's go with > the weak ptr approach for now. Done. https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:3678: base::TimeDelta consumed_duration = On 2012/08/02 16:31:03, satorux1 wrote: > maybe: elapsed_time Done. https://chromiumcodereview.appspot.com/10827098/diff/9001/chrome/browser/chro... chrome/browser/chromeos/gdata/gdata_file_system.cc:3686: int num_remaining_ui_update = On 2012/08/02 16:31:03, satorux1 wrote: > num_remaining_ui_updates Done.
LGTM. Thanks!!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/10827098/11003
Change committed as 149893 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
