Chromium Code Reviews
Help | Chromium Project | Sign in
(309)

Issue 847004: Implement SyncerThread::RequestPause and SyncerThread::RequestResume (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 5 months ago by skrul
Modified:
4 years, 3 months ago
CC:
chromium-reviews, ncarter, idana, John Grabowski, ben+cc_chromium.org, pam+watch_chromium.org, PaweĊ‚ Hajdan Jr., tim (not reviewing)
Visibility:
Public.

Description

Implement SyncerThread::RequestPause and SyncerThread::RequestResume Basic functionality with unit test. More plumbing later :) Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41628

Patch Set 1 #

Total comments: 9

Patch Set 2 : Address review comments. #

Total comments: 7

Patch Set 3 : Address comments. #

Patch Set 4 : Fix method name. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -2 lines) Patch
M chrome/browser/sync/engine/all_status.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/sync/engine/syncer_thread.h View 1 2 3 5 chunks +26 lines, -1 line 0 comments Download
M chrome/browser/sync/engine/syncer_thread.cc View 1 2 3 4 chunks +46 lines, -1 line 0 comments Download
M chrome/browser/sync/engine/syncer_thread_unittest.cc View 1 2 2 chunks +68 lines, -0 lines 0 comments Download
M chrome/browser/sync/engine/syncer_types.h View 1 chunk +8 lines, -0 lines 0 comments Download
Project "None" does not have a commit queue.

Messages

Total messages: 7 (0 generated)
skrul
5 years, 5 months ago (2010-03-12 18:59:02 UTC) #1
tim (not reviewing)
http://codereview.chromium.org/847004/diff/1/3 File chrome/browser/sync/engine/syncer_thread.cc (right): http://codereview.chromium.org/847004/diff/1/3#newcode167 chrome/browser/sync/engine/syncer_thread.cc:167: if (!thread_.IsRunning()) while this is fine for now, is ...
5 years, 5 months ago (2010-03-12 22:10:32 UTC) #2
tim (not reviewing)
chromium-reviews bounced.. tweaked addy and tried again?
5 years, 5 months ago (2010-03-12 22:13:27 UTC) #3
skrul
http://codereview.chromium.org/847004/diff/1/3 File chrome/browser/sync/engine/syncer_thread.cc (right): http://codereview.chromium.org/847004/diff/1/3#newcode245 chrome/browser/sync/engine/syncer_thread.cc:245: if (vault_.pause_) { On 2010/03/12 22:10:32, timsteele wrote: > ...
5 years, 5 months ago (2010-03-13 01:04:47 UTC) #4
tim (not reviewing)
LGTM once nits and test comment addressed. http://codereview.chromium.org/847004/diff/7001/8002 File chrome/browser/sync/engine/syncer_thread.cc (right): http://codereview.chromium.org/847004/diff/7001/8002#newcode246 chrome/browser/sync/engine/syncer_thread.cc:246: PauseIfRequested(); cool, ...
5 years, 5 months ago (2010-03-13 01:23:06 UTC) #5
skrul
http://codereview.chromium.org/847004/diff/7001/8002 File chrome/browser/sync/engine/syncer_thread.cc (right): http://codereview.chromium.org/847004/diff/7001/8002#newcode246 chrome/browser/sync/engine/syncer_thread.cc:246: PauseIfRequested(); On 2010/03/13 01:23:06, timsteele wrote: > cool, should ...
5 years, 5 months ago (2010-03-15 17:48:52 UTC) #6
tim (not reviewing)
5 years, 5 months ago (2010-03-15 17:58:48 UTC) #7
http://codereview.chromium.org/847004/diff/7001/8002
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/847004/diff/7001/8002#newcode246
chrome/browser/sync/engine/syncer_thread.cc:246: PauseIfRequested();
On 2010/03/15 17:48:52, skrul wrote:
> On 2010/03/13 01:23:06, timsteele wrote:
> > cool, should we move the check above in this method too? It just looks odd
to
> > see if (pause) PauseIfRequested.
> 
> I've changed it to the other way, but I think it looks a bit odd as well.  The
> pattern in this method seems to be to check some condition, then while/wait
loop
> for a signal, then continue to the top of the outer loop. 
> 
> Let me know what you like better or if you have any other suggestions :)
> 

my prev comment was ignorant of the fact that we had to continue; if the pause
happened.  I'm fine with what you have though, but I should have suggested 
 if (pause) HandlePause or if (pause) PauseUntilResumedOrQuit  :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 020f378