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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by skrul
Modified:
2 years, 11 months ago
Reviewers:
timsteele
CC:
chromium-reviews_chromium.org, ncarter, idana, John Grabowski, ben+cc_chromium.org, pam+watch_chromium.org, PaweĊ‚ Hajdan Jr., timsteele
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) Lint Patch
M chrome/browser/sync/engine/all_status.cc View 1 chunk +6 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/sync/engine/syncer_thread.h View 1 2 3 5 chunks +26 lines, -1 line 0 comments 0 errors Download
M chrome/browser/sync/engine/syncer_thread.cc View 1 2 3 4 chunks +46 lines, -1 line 0 comments 0 errors Download
M chrome/browser/sync/engine/syncer_thread_unittest.cc View 1 2 2 chunks +68 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/sync/engine/syncer_types.h View 1 chunk +8 lines, -0 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 7
skrul
4 years, 1 month ago #1
timsteele
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 ...
4 years, 1 month ago #2
timsteele
chromium-reviews bounced.. tweaked addy and tried again?
4 years, 1 month ago #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: > ...
4 years, 1 month ago #4
timsteele
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, ...
4 years, 1 month ago #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 ...
4 years, 1 month ago #6
timsteele
4 years, 1 month ago #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 1280:2d3e6564b7b6