Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829088:
Fix Precise build. (Closed)
|
Created:
8 years, 4 months ago by dcheng Modified:
8 years, 4 months ago Reviewers:
rlarocque CC:
chromium-reviews, Raghu Simha, ncarter (slow), akalin, tim (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix Precise build.
Precise's gcc warns about variables that are set but never used.
BUG=none
TEST=compiles on Precise.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149073
Patch Set 1 #
Total comments: 2
Messages
Total messages: 11 (0 generated)
On 2012/07/30 20:42:34, dcheng wrote: LGTM. If you want, you could investigate removing the if block, as I mentioned in the comment. If you'd rather just fix the error and get back to developing, that's fine too. I can try to clean it up later.
http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... File chrome/browser/sync/test_profile_sync_service.cc (left): http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... chrome/browser/sync/test_profile_sync_service.cc:121: if (!directory->initial_sync_ended_for_type(syncer::NIGORI)) { It's possible this entire if block could be removed. I'd have to try it to be sure, though.
http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... File chrome/browser/sync/test_profile_sync_service.cc (left): http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... chrome/browser/sync/test_profile_sync_service.cc:121: if (!directory->initial_sync_ended_for_type(syncer::NIGORI)) { On 2012/07/30 20:55:33, rlarocque wrote: > It's possible this entire if block could be removed. I'd have to try it to be > sure, though. What would I need to do to verify that removing this block is safe? Would running sync_integration_tests be enough?
On 2012/07/30 20:58:19, dcheng wrote: > http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... > File chrome/browser/sync/test_profile_sync_service.cc (left): > > http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... > chrome/browser/sync/test_profile_sync_service.cc:121: if > (!directory->initial_sync_ended_for_type(syncer::NIGORI)) { > On 2012/07/30 20:55:33, rlarocque wrote: > > It's possible this entire if block could be removed. I'd have to try it to be > > sure, though. > > What would I need to do to verify that removing this block is safe? Would > running sync_integration_tests be enough? sync_unit_tests with --gtest_filter='*Sync*' should do it.
On 2012/07/30 21:13:28, rlarocque wrote: > On 2012/07/30 20:58:19, dcheng wrote: > > > http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... > > File chrome/browser/sync/test_profile_sync_service.cc (left): > > > > > http://codereview.chromium.org/10829088/diff/1/chrome/browser/sync/test_profi... > > chrome/browser/sync/test_profile_sync_service.cc:121: if > > (!directory->initial_sync_ended_for_type(syncer::NIGORI)) { > > On 2012/07/30 20:55:33, rlarocque wrote: > > > It's possible this entire if block could be removed. I'd have to try it to > be > > > sure, though. > > > > What would I need to do to verify that removing this block is safe? Would > > running sync_integration_tests be enough? > > sync_unit_tests with --gtest_filter='*Sync*' should do it. Actually, I meant unit_tests with --gtest_filter='*Sync*'.
I tried removing the if block and this is what happened:
[16996:17011:0730/145158:1632633380410:FATAL:sync_manager_impl.cc(606)] Check
failed: false.
Backtrace:
base::debug::StackTrace::StackTrace() [0x7fb99f11fc3e]
logging::LogMessage::~LogMessage() [0x7fb99f150db5]
syncer::SyncManagerImpl::UpdateCryptographerAndNigoriCallback()
[0x2a7c739]
base::internal::RunnableAdapter<>::Run() [0x2a8dfe5]
base::internal::InvokeHelper<>::MakeItSo() [0x2a8c875]
base::internal::Invoker<>::Run() [0x2a89de2]
base::Callback<>::Run() [0x25cec46]
syncer::(anonymous namespace)::OnSessionNameFilled() [0x2a61fee]
base::internal::RunnableAdapter<>::Run() [0x2a62610]
base::internal::InvokeHelper<>::MakeItSo() [0x2a62584]
base::internal::Invoker<>::Run() [0x2a624fa]
base::Callback<>::Run() [0x7fb99f1183e5]
base::(anonymous
namespace)::PostTaskAndReplyRelay::RunReplyAndSelfDestruct() [0x7fb99f1b80f
7]
base::internal::RunnableAdapter<>::Run() [0x7fb99f1b8502]
base::internal::InvokeHelper<>::MakeItSo() [0x7fb99f1b8487]
base::internal::Invoker<>::Run() [0x7fb99f1b8433]
base::Callback<>::Run() [0x7fb99f1183e5]
I'll just make the simple compile fix for now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dcheng@chromium.org/10829088/1
On 2012/07/30 21:53:30, dcheng wrote: > I tried removing the if block and this is what happened: > [16996:17011:0730/145158:1632633380410:FATAL:sync_manager_impl.cc(606)] Check > failed: false. > Backtrace: > base::debug::StackTrace::StackTrace() [0x7fb99f11fc3e] > logging::LogMessage::~LogMessage() [0x7fb99f150db5] > syncer::SyncManagerImpl::UpdateCryptographerAndNigoriCallback() > [0x2a7c739] > base::internal::RunnableAdapter<>::Run() [0x2a8dfe5] > base::internal::InvokeHelper<>::MakeItSo() [0x2a8c875] > base::internal::Invoker<>::Run() [0x2a89de2] > base::Callback<>::Run() [0x25cec46] > syncer::(anonymous namespace)::OnSessionNameFilled() [0x2a61fee] > base::internal::RunnableAdapter<>::Run() [0x2a62610] > base::internal::InvokeHelper<>::MakeItSo() [0x2a62584] > base::internal::Invoker<>::Run() [0x2a624fa] > base::Callback<>::Run() [0x7fb99f1183e5] > base::(anonymous > namespace)::PostTaskAndReplyRelay::RunReplyAndSelfDestruct() [0x7fb99f1b80f > 7] > base::internal::RunnableAdapter<>::Run() [0x7fb99f1b8502] > base::internal::InvokeHelper<>::MakeItSo() [0x7fb99f1b8487] > base::internal::Invoker<>::Run() [0x7fb99f1b8433] > base::Callback<>::Run() [0x7fb99f1183e5] > > I'll just make the simple compile fix for now. Sounds good. Sorry for the distraction.
Change committed as 149073 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
