sync: make ServerConnectionManager explicitly NonThreadSafe

sync: make ServerConnectionManager explicitly NonThreadSafe

This is now possible because there's no core thread to contend with -- the SCM is only used from the sync thread.

BUG=19757, 93829
TEST=sync_unit_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98486

[lipalani1, 2011-08-24 20:35:28]

Here are the comments. But I feel comfortable with a LGTM assuming you are ok
with the comments.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:140: if
(conn_mgr_->server_status_ != response_->server_status) {
It is not a big deal if it is not easy. But we mush have thread checking inside
this class too. Particularly inside the destructor.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:166:
ServerConnectionManager::~ServerConnectionManager() {
Can you add a CHECK here?(rather than a DCHECK. Issues similar to this has been
hard to debug in the past.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:301: bool use_ssl) {
Add dcheck here too.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:335:
ServerConnectionEventListener* listener) {
here too.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:340:
ServerConnectionEventListener* listener) {
Here too.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
File chrome/browser/sync/engine/net/server_connection_manager.h (right):

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.h:185: bool* use_ssl)
const {
Add a dcheck for calledonvalidthread here.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/internal_ap...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/internal_ap...
chrome/browser/sync/internal_api/sync_manager.cc:1291: const
ServerConnectionEvent& event) {
You can add a dcheck here too to be on parity with the above.

http://codereview.chromium.org/7731002/diff/6/chrome/test/sync/engine/mock_co...
File chrome/test/sync/engine/mock_connection_manager.cc (right):

http://codereview.chromium.org/7731002/diff/6/chrome/test/sync/engine/mock_co...
chrome/test/sync/engine/mock_connection_manager.cc:613: OnServerConnectionEvent(
You can simply call NotifyStatusChanged

http://codereview.chromium.org/7731002/diff/6/chrome/test/sync/engine/mock_co...
chrome/test/sync/engine/mock_connection_manager.cc:622: OnServerConnectionEvent(
Same as above.

[tim (not reviewing), 2011-08-25 16:21:46]

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
File chrome/browser/sync/engine/net/server_connection_manager.cc (right):

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:140: if
(conn_mgr_->server_status_ != response_->server_status) {
On 2011/08/24 20:35:28, lipalani1 wrote:
> It is not a big deal if it is not easy. But we mush have thread checking
inside
> this class too. Particularly inside the destructor.

Good idea, looking into it.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:166:
ServerConnectionManager::~ServerConnectionManager() {
On 2011/08/24 20:35:28, lipalani1 wrote:
> Can you add a CHECK here?(rather than a DCHECK. Issues similar to this has
been
> hard to debug in the past.

My issue with this is that NonThreadSafe by default has an assertion in the dtor
(which we get by inheriting), but it's a dcheck in accordance with the note
about CHECK vs DCHECK at http://dev.chromium.org/developers/coding-style 

I can add it if you insist, but I'm not sure we want / need to deviate here.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:301: bool use_ssl) {
On 2011/08/24 20:35:28, lipalani1 wrote:
> Add dcheck here too.

Done.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:335:
ServerConnectionEventListener* listener) {
On 2011/08/24 20:35:28, lipalani1 wrote:
> here too.

Done.

http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/...
chrome/browser/sync/engine/net/server_connection_manager.cc:340:
ServerConnectionEventListener* listener) {
On 2011/08/24 20:35:28, lipalani1 wrote:
> Here too.

Done.

[lipalani, 2011-08-25 18:35:54]

Agree on all the points.
For check vs dcheck dcheck is fine as it is used only by the developers.
Thanks!
regards,
Lingesh
On Thu, Aug 25, 2011 at 9:21 AM, <tim@chromium.org> wrote:

>
> http://codereview.chromium.**org/7731002/diff/6/chrome/**
>
browser/sync/engine/net/**server_connection_manager.cc<http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/server_connection_manager.cc>
> File chrome/browser/sync/engine/**net/server_connection_manager.**cc
> (right):
>
> http://codereview.chromium.**org/7731002/diff/6/chrome/**
>
browser/sync/engine/net/**server_connection_manager.cc#**newcode140<http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/server_connection_manager.cc#newcode140>
> chrome/browser/sync/engine/**net/server_connection_manager.**cc:140: if
> (conn_mgr_->server_status_ != response_->server_status) {
> On 2011/08/24 20:35:28, lipalani1 wrote:
>
>> It is not a big deal if it is not easy. But we mush have thread
>>
> checking inside
>
>> this class too. Particularly inside the destructor.
>>
>
> Good idea, looking into it.
>
>
> http://codereview.chromium.**org/7731002/diff/6/chrome/**
>
browser/sync/engine/net/**server_connection_manager.cc#**newcode166<http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/server_connection_manager.cc#newcode166>
> chrome/browser/sync/engine/**net/server_connection_manager.**cc:166:
> ServerConnectionManager::~**ServerConnectionManager() {
> On 2011/08/24 20:35:28, lipalani1 wrote:
>
>> Can you add a CHECK here?(rather than a DCHECK. Issues similar to this
>>
> has been
>
>> hard to debug in the past.
>>
>
> My issue with this is that NonThreadSafe by default has an assertion in
> the dtor (which we get by inheriting), but it's a dcheck in accordance
> with the note about CHECK vs DCHECK at
>
http://dev.chromium.org/**developers/coding-style<http://dev.chromium.org/dev...
>
> I can add it if you insist, but I'm not sure we want / need to deviate
> here.
>
> http://codereview.chromium.**org/7731002/diff/6/chrome/**
>
browser/sync/engine/net/**server_connection_manager.cc#**newcode301<http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/server_connection_manager.cc#newcode301>
> chrome/browser/sync/engine/**net/server_connection_manager.**cc:301: bool
> use_ssl) {
> On 2011/08/24 20:35:28, lipalani1 wrote:
>
>> Add dcheck here too.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7731002/diff/6/chrome/**
>
browser/sync/engine/net/**server_connection_manager.cc#**newcode335<http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/server_connection_manager.cc#newcode335>
> chrome/browser/sync/engine/**net/server_connection_manager.**cc:335:
> ServerConnectionEventListener* listener) {
> On 2011/08/24 20:35:28, lipalani1 wrote:
>
>> here too.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7731002/diff/6/chrome/**
>
browser/sync/engine/net/**server_connection_manager.cc#**newcode340<http://codereview.chromium.org/7731002/diff/6/chrome/browser/sync/engine/net/server_connection_manager.cc#newcode340>
> chrome/browser/sync/engine/**net/server_connection_manager.**cc:340:
> ServerConnectionEventListener* listener) {
> On 2011/08/24 20:35:28, lipalani1 wrote:
>
>> Here too.
>>
>
> Done.
>
>
>
http://codereview.chromium.**org/7731002/<http://codereview.chromium.org/7731...
>

[tim (not reviewing), 2011-08-25 18:45:00]

Made ScopedServerStatusWatcher inherit NonThreadSafe, which adds thread checking
on destruction.  Sent to trybots.

[commit-bot: I haz the power, 2011-08-26 21:02:39]

Change committed as 98486