Skip to content

Commit

Permalink
Reland "Add std::move() to local base::Callback instances in //content"
Browse files Browse the repository at this point in the history
This reverts commit 9b4af06.

Reason for revert: This does not seem like the root cause.

Original change's description:
> Revert "Add std::move() to local base::Callback instances in //content"
> 
> This reverts commit c55a5a4.
> 
> Reason for revert: Speculative revert because of renderer crashes across
> the board on Android with:
> [FATAL:weak_ptr.cc(26)] Check failed: sequence_checker_.CalledOnValidSequence(). WeakPtrs must be checked on the same sequenced thread
> 
> Original change's description:
> > Add std::move() to local base::Callback instances in //content
> > 
> > This adds std::move() around base::Callback instances where it looks
> > relevant, by applying `base_bind_rewriters -rewriter=add_std_move`.
> > https://crrev.com/c/970143, plus manual fixes.
> > 
> > Example:
> >   // Before:
> >   void set_callback(base::Closure cb) { g_cb = cb; }
> >   void RunCallback(base::Callback<void(int)> cb) { cb.Run(42); }
> >   void Post() {
> >     base::Closure task = base::Bind(&Foo);
> >     PostTask(FROM_HERE, task);
> >   }
> > 
> >   // After:
> >   void set_callback(base::Closure cb) { g_cb = std::move(cb); }
> >   void RunCallback(base::Callback<void(int)> cb) { std::move(cb).Run(42); }
> >   void Post() {
> >     base::Closure task = base::Bind(&Foo);
> >     PostTask(FROM_HERE, std::move(task));
> >   }
> > 
> > Specifically, it inserts std::move() if:
> >  - it's a pass-by-value parameter or non-const local variable.
> >  - the occurrence is the latest in its control flow.
> >  - no pointer is taken for the variable.
> >  - no capturing lambda exists for the variable.
> > 
> > Change-Id: I53853f9b9c8604994e2065af66ed4607af9c12ed
> > Reviewed-on: https://chromium-review.googlesource.com/970056
> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> > Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#544356}
> 
> TBR=kinuko@chromium.org,tzik@chromium.org
> 
> Change-Id: Ie7392a2229e1ef0f740d8958f8fe43d99b0460e9
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/972321
> Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544527}

TBR=kinuko@chromium.org,nyquist@chromium.org,tzik@chromium.org

Change-Id: I0aabd6032a070a28d0e5a4f796f37fe18f1e5cd4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/972302
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Commit-Queue: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544536}
  • Loading branch information
tommynyquist authored and Commit Bot committed Mar 20, 2018
1 parent b5e2e22 commit 4b749d0
Show file tree
Hide file tree
Showing 141 changed files with 443 additions and 378 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class AccessibilityEventRecorder {
}

void ListenToEvents(AccessibilityEventCallback callback) {
callback_ = callback;
callback_ = std::move(callback);
}

// Access the vector of human-readable event logs, one string per event.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ bool BrowserAccessibilityStateImpl::IsAccessibleBrowser() {

void BrowserAccessibilityStateImpl::AddHistogramCallback(
base::Closure callback) {
histogram_callbacks_.push_back(callback);
histogram_callbacks_.push_back(std::move(callback));
}

void BrowserAccessibilityStateImpl::UpdateHistogramsForTesting() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ void DidCreateRegistration(
blink::mojom::BackgroundFetchError error,
std::unique_ptr<BackgroundFetchRegistration> registration) {
*out_error = error;
quit_closure.Run();
std::move(quit_closure).Run();
}

void DidGetError(base::Closure quit_closure,
blink::mojom::BackgroundFetchError* out_error,
blink::mojom::BackgroundFetchError error) {
*out_error = error;
quit_closure.Run();
std::move(quit_closure).Run();
}

void DidGetRegistrationUserDataByKeyPrefix(base::Closure quit_closure,
Expand All @@ -69,7 +69,7 @@ void DidGetRegistrationUserDataByKeyPrefix(base::Closure quit_closure,
DCHECK(out_data);
DCHECK_EQ(SERVICE_WORKER_OK, status);
*out_data = data;
quit_closure.Run();
std::move(quit_closure).Run();
}

} // namespace
Expand Down Expand Up @@ -217,7 +217,7 @@ class BackgroundFetchDataManagerTest
*out_error = error;
*out_registration = std::move(registration);

quit_closure.Run();
std::move(quit_closure).Run();
}

void DidGetDeveloperIds(base::Closure quit_closure,
Expand All @@ -228,7 +228,7 @@ class BackgroundFetchDataManagerTest
*out_error = error;
*out_ids = ids;

quit_closure.Run();
std::move(quit_closure).Run();
}

BackgroundFetchRegistrationStorage registration_storage_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,10 @@ void BackgroundFetchEventDispatcher::StartActiveWorkerForDispatch(
DCHECK(service_worker_version);

service_worker_version->RunAfterStartWorker(
event, base::BindOnce(&BackgroundFetchEventDispatcher::DispatchEvent,
event, std::move(finished_closure), loaded_callback,
base::WrapRefCounted(service_worker_version)));
event,
base::BindOnce(&BackgroundFetchEventDispatcher::DispatchEvent, event,
std::move(finished_closure), std::move(loaded_callback),
base::WrapRefCounted(service_worker_version)));
}

void BackgroundFetchEventDispatcher::DispatchEvent(
Expand All @@ -221,7 +222,7 @@ void BackgroundFetchEventDispatcher::DispatchEvent(
base::BindOnce(&BackgroundFetchEventDispatcher::DidDispatchEvent, event,
std::move(finished_closure), DispatchPhase::DISPATCHING));

loaded_callback.Run(std::move(service_worker_version), request_id);
std::move(loaded_callback).Run(std::move(service_worker_version), request_id);
}

void BackgroundFetchEventDispatcher::DidDispatchEvent(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,16 @@ class BackgroundFetchSchedulerTest
void PostQuitAfterRepeatingBarriers(base::Closure quit_closure,
int number_of_barriers) {
if (--number_of_barriers == 0) {
quit_closure.Run();
std::move(quit_closure).Run();
return;
}

BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::BindOnce(
&BackgroundFetchSchedulerTest::PostQuitAfterRepeatingBarriers,
base::Unretained(this), quit_closure, number_of_barriers));
base::Unretained(this), std::move(quit_closure),
number_of_barriers));
}

void PopNextRequest(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,15 +215,15 @@ class BackgroundFetchServiceTest : public BackgroundFetchTestBase {
*out_registration =
registration ? registration.value() : BackgroundFetchRegistration();

quit_closure.Run();
std::move(quit_closure).Run();
}

void DidGetError(base::Closure quit_closure,
blink::mojom::BackgroundFetchError* out_error,
blink::mojom::BackgroundFetchError error) {
*out_error = error;

quit_closure.Run();
std::move(quit_closure).Run();
}

void DidGetDeveloperIds(base::Closure quit_closure,
Expand All @@ -234,7 +234,7 @@ class BackgroundFetchServiceTest : public BackgroundFetchTestBase {
*out_error = error;
*out_developer_ids = developer_ids;

quit_closure.Run();
std::move(quit_closure).Run();
}

scoped_refptr<BackgroundFetchContext> context_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void DidRegisterServiceWorker(int64_t* out_service_worker_registration_id,

*out_service_worker_registration_id = service_worker_registration_id;

quit_closure.Run();
std::move(quit_closure).Run();
}

void DidFindServiceWorkerRegistration(
Expand All @@ -57,7 +57,7 @@ void DidFindServiceWorkerRegistration(

*out_service_worker_registration = service_worker_registration;

quit_closure.Run();
std::move(quit_closure).Run();
}

} // namespace
Expand Down
2 changes: 1 addition & 1 deletion content/browser/background_sync/background_sync_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ void BackgroundSyncManager::DispatchSyncEvent(
active_version->event_dispatcher()->DispatchSyncEvent(
tag, last_chance, parameters_->max_sync_event_duration,
base::BindOnce(&OnSyncEventFinished, std::move(active_version),
request_id, repeating_callback));
request_id, std::move(repeating_callback)));
}

void BackgroundSyncManager::ScheduleDelayedTask(base::OnceClosure callback,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ void BluetoothDeviceChooserController::GetDevice(

if (WebContentsDelegate* delegate = web_contents_->GetDelegate()) {
chooser_ = delegate->RunBluetoothChooser(render_frame_host_,
chooser_event_handler);
std::move(chooser_event_handler));
}

if (!chooser_.get()) {
Expand Down
10 changes: 5 additions & 5 deletions content/browser/browser_main_loop.cc
Original file line number Diff line number Diff line change
Expand Up @@ -924,23 +924,23 @@ void BrowserMainLoop::CreateStartupTasks() {
#endif
StartupTask pre_create_threads =
base::Bind(&BrowserMainLoop::PreCreateThreads, base::Unretained(this));
startup_task_runner_->AddTask(pre_create_threads);
startup_task_runner_->AddTask(std::move(pre_create_threads));

StartupTask create_threads =
base::Bind(&BrowserMainLoop::CreateThreads, base::Unretained(this));
startup_task_runner_->AddTask(create_threads);
startup_task_runner_->AddTask(std::move(create_threads));

StartupTask post_create_threads =
base::Bind(&BrowserMainLoop::PostCreateThreads, base::Unretained(this));
startup_task_runner_->AddTask(post_create_threads);
startup_task_runner_->AddTask(std::move(post_create_threads));

StartupTask browser_thread_started = base::Bind(
&BrowserMainLoop::BrowserThreadsStarted, base::Unretained(this));
startup_task_runner_->AddTask(browser_thread_started);
startup_task_runner_->AddTask(std::move(browser_thread_started));

StartupTask pre_main_message_loop_run = base::Bind(
&BrowserMainLoop::PreMainMessageLoopRun, base::Unretained(this));
startup_task_runner_->AddTask(pre_main_message_loop_run);
startup_task_runner_->AddTask(std::move(pre_main_message_loop_run));

#if defined(OS_ANDROID)
if (parameters_.ui_task) {
Expand Down
12 changes: 6 additions & 6 deletions content/browser/browsing_data/browsing_data_remover_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,9 @@ bool BrowsingDataRemoverImpl::DoesOriginMatchMask(
if (embedder_delegate_)
embedder_matcher = embedder_delegate_->GetOriginTypeMatcher();

return DoesOriginMatchMaskAndURLs(origin_type_mask,
base::Callback<bool(const GURL&)>(),
embedder_matcher, origin, policy);
return DoesOriginMatchMaskAndURLs(
origin_type_mask, base::Callback<bool(const GURL&)>(),
std::move(embedder_matcher), origin, policy);
}

void BrowsingDataRemoverImpl::Remove(const base::Time& delete_begin,
Expand Down Expand Up @@ -451,8 +451,8 @@ void BrowsingDataRemoverImpl::RemoveImpl(
storage_partition->ClearData(
storage_partition_remove_mask, quota_storage_remove_mask,
base::BindRepeating(&DoesOriginMatchMaskAndURLs, origin_type_mask_,
filter, embedder_matcher),
cookie_matcher, delete_begin_, delete_end_,
filter, std::move(embedder_matcher)),
std::move(cookie_matcher), delete_begin_, delete_end_,
CreatePendingTaskCompletionClosure());
}

Expand Down Expand Up @@ -480,7 +480,7 @@ void BrowsingDataRemoverImpl::RemoveImpl(
storage_partition->ClearHttpAndMediaCaches(
delete_begin, delete_end,
filter_builder.IsEmptyBlacklist() ? base::Callback<bool(const GURL&)>()
: filter,
: std::move(filter),
CreatePendingTaskCompletionClosureForMojo());

// When clearing cache, wipe accumulated network related data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ class ClearSiteDataThrottleBrowserTest : public ContentBrowserTest {
base::Closure callback) {
cookie_store_ =
request_context_getter->GetURLRequestContext()->cookie_store();
callback.Run();
std::move(callback).Run();
}

// Adds a cookie for the |url|. Used in the cookie integration tests.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ IN_PROC_BROWSER_TEST_F(ConditionalCacheDeletionHelperBrowserTest,
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::BindOnce(&ConditionalCacheDeletionHelperBrowserTest::DeleteEntries,
base::Unretained(this), base::ConstRef(condition)));
base::Unretained(this), std::move(condition)));
WaitForTasksOnIOThread();

// Expect that only "icon2.png" and "icon3.png" were deleted.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ void CacheStorageBlobToDiskCache::OnDataPipeReadable(MojoResult unused) {
buffer.get(), bytes_to_read, cache_write_callback,
true /* truncate */);
if (rv != net::ERR_IO_PENDING)
cache_write_callback.Run(rv);
std::move(cache_write_callback).Run(rv);
}

} // namespace content
23 changes: 12 additions & 11 deletions content/browser/cache_storage/cache_storage_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ void ReadMetadata(disk_cache::Entry* entry, MetadataCallback callback) {
buffer->size(), read_header_callback);

if (read_rv != net::ERR_IO_PENDING)
read_header_callback.Run(read_rv);
std::move(read_header_callback).Run(read_rv);
}

void ReadMetadataDidReadMetadata(disk_cache::Entry* entry,
Expand Down Expand Up @@ -615,7 +615,7 @@ void CacheStorageCache::BatchDidGetUsageAndQuota(
weak_ptr_factory_.GetWeakPtr(), callback_copy));
auto completion_callback = base::BindRepeating(
&CacheStorageCache::BatchDidOneOperation, weak_ptr_factory_.GetWeakPtr(),
barrier_closure, callback_copy);
std::move(barrier_closure), std::move(callback_copy));

// Operations may synchronously invoke |callback| which could release the
// last reference to this instance. Hold a handle for the duration of this
Expand Down Expand Up @@ -806,7 +806,7 @@ void CacheStorageCache::QueryCache(
int rv = backend_->OpenEntry(request_ptr->url.spec(), entry_ptr,
open_entry_callback);
if (rv != net::ERR_IO_PENDING)
open_entry_callback.Run(rv);
std::move(open_entry_callback).Run(rv);
return;
}

Expand Down Expand Up @@ -853,7 +853,7 @@ void CacheStorageCache::QueryCacheOpenNextEntry(
int rv = iterator.OpenNextEntry(enumerated_entry, open_entry_callback);

if (rv != net::ERR_IO_PENDING)
open_entry_callback.Run(rv);
std::move(open_entry_callback).Run(rv);
}

void CacheStorageCache::QueryCacheFilterEntry(
Expand Down Expand Up @@ -1132,7 +1132,7 @@ void CacheStorageCache::WriteSideDataImpl(ErrorCallback callback,

int rv = backend_->OpenEntry(url.spec(), entry_ptr, open_entry_callback);
if (rv != net::ERR_IO_PENDING)
open_entry_callback.Run(rv);
std::move(open_entry_callback).Run(rv);
}

void CacheStorageCache::WriteSideDataDidOpenEntry(
Expand Down Expand Up @@ -1189,7 +1189,7 @@ void CacheStorageCache::WriteSideDataDidReadMetaData(
write_side_data_callback, true /* truncate */);

if (rv != net::ERR_IO_PENDING)
write_side_data_callback.Run(rv);
std::move(write_side_data_callback).Run(rv);
}

void CacheStorageCache::WriteSideDataDidWrite(
Expand Down Expand Up @@ -1311,7 +1311,7 @@ void CacheStorageCache::PutDidDeleteEntry(
create_entry_callback);

if (rv != net::ERR_IO_PENDING)
create_entry_callback.Run(rv);
std::move(create_entry_callback).Run(rv);
}

void CacheStorageCache::PutDidCreateEntry(
Expand Down Expand Up @@ -1383,7 +1383,7 @@ void CacheStorageCache::PutDidCreateEntry(
true /* truncate */);

if (rv != net::ERR_IO_PENDING)
write_headers_callback.Run(rv);
std::move(write_headers_callback).Run(rv);
}

void CacheStorageCache::PutDidWriteHeaders(
Expand Down Expand Up @@ -1474,7 +1474,7 @@ void CacheStorageCache::CalculateCacheSizePadding(

int rv = backend_->CalculateSizeOfAllEntries(got_size_callback);
if (rv != net::ERR_IO_PENDING)
got_size_callback.Run(rv);
std::move(got_size_callback).Run(rv);
}

void CacheStorageCache::CalculateCacheSizePaddingGotSize(
Expand Down Expand Up @@ -1711,7 +1711,7 @@ void CacheStorageCache::CreateBackend(ErrorCallback callback) {
weak_ptr_factory_.GetWeakPtr()),
create_cache_callback);
if (rv != net::ERR_IO_PENDING)
create_cache_callback.Run(rv);
std::move(create_cache_callback).Run(rv);
}

void CacheStorageCache::CreateBackendDidCreate(
Expand Down Expand Up @@ -1756,7 +1756,8 @@ void CacheStorageCache::InitDidCreateBackend(
calculate_size_callback, cache_create_error));

if (rv != net::ERR_IO_PENDING)
InitGotCacheSize(calculate_size_callback, cache_create_error, rv);
InitGotCacheSize(std::move(calculate_size_callback), cache_create_error,
rv);
}

void CacheStorageCache::InitGotCacheSize(base::OnceClosure callback,
Expand Down
12 changes: 7 additions & 5 deletions content/browser/compositor/gpu_process_transport_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -497,15 +497,15 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
}
display_output_surface =
std::make_unique<SoftwareBrowserCompositorOutputSurface>(
CreateSoftwareOutputDevice(compositor->widget()), vsync_callback,
compositor->task_runner());
CreateSoftwareOutputDevice(compositor->widget()),
std::move(vsync_callback), compositor->task_runner());
} else {
DCHECK(context_provider);
const auto& capabilities = context_provider->ContextCapabilities();
if (data->surface_handle == gpu::kNullSurfaceHandle) {
display_output_surface =
std::make_unique<OffscreenBrowserCompositorOutputSurface>(
context_provider, vsync_callback,
context_provider, std::move(vsync_callback),
std::unique_ptr<viz::CompositorOverlayCandidateValidator>());
} else if (capabilities.surfaceless) {
#if defined(OS_MACOSX)
Expand All @@ -521,7 +521,8 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
#else
auto gpu_output_surface =
std::make_unique<GpuSurfacelessBrowserCompositorOutputSurface>(
context_provider, data->surface_handle, vsync_callback,
context_provider, data->surface_handle,
std::move(vsync_callback),
CreateOverlayCandidateValidator(compositor->widget()),
GL_TEXTURE_2D, GL_RGB,
display::DisplaySnapshot::PrimaryFormat(),
Expand All @@ -540,7 +541,8 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
#endif
auto gpu_output_surface =
std::make_unique<GpuBrowserCompositorOutputSurface>(
context_provider, vsync_callback, std::move(validator));
context_provider, std::move(vsync_callback),
std::move(validator));
gpu_vsync_control = gpu_output_surface.get();
display_output_surface = std::move(gpu_output_surface);
}
Expand Down
Loading

0 comments on commit 4b749d0

Please sign in to comment.