Skip to content

Commit

Permalink
Fix PRESUBMIT list of base::Bind/Callback exceptions and new violators
Browse files Browse the repository at this point in the history
The PRESUBMIT.py rule checking for base::Bind and base::Callback usage
was not working, so new usage crept into places that were previously
marked clean.

This CL updates the list in PRESUBMIT.py. A lot of places listed in the
list are now clean or no longer exist. For places that were previously
clean and now violate the rule, we mostly convert them over.

3 exceptions that we add to the PRESUBMIT.py list are:
  //jingle
  //media/blink
  //weblayer

R=sky@chromium.org

Bug: 1028284
Change-Id: I2e34f57879265c4894be1d1c4d132bf17fb973a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1934767
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Erik Staab <estaab@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719249}
  • Loading branch information
danakj authored and Commit Bot committed Nov 26, 2019
1 parent f5acb98 commit c857609
Show file tree
Hide file tree
Showing 27 changed files with 95 additions and 135 deletions.
46 changes: 5 additions & 41 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@

# Directories that contain deprecated Bind() or Callback types.
# Find sub-directories from a given directory by running:
# for i in `find . -maxdepth 1 -type d`; do
# for i in `find . -maxdepth 1 -type d|sort`; do
# echo "-- $i"
# (cd $i; git grep -P 'base::(Bind\(|(Callback<|Closure))'|wc -l)
# done
Expand All @@ -272,7 +272,6 @@
'^chrome/chrome_watcher/',
'^chrome/common/',
'^chrome/installer/',
'^chrome/notification_helper/',
'^chrome/renderer/',
'^chrome/services/',
'^chrome/test/',
Expand All @@ -285,46 +284,36 @@
'^chromecast/metrics/',
'^chromecast/net/',
'^chromeos/attestation/',
'^chromeos/audio/',
'^chromeos/components/',
'^chromeos/cryptohome/',
'^chromeos/dbus/',
'^chromeos/geolocation/',
'^chromeos/login/',
'^chromeos/network/',
'^chromeos/process_proxy/',
'^chromeos/services/',
'^chromeos/settings/',
'^chromeos/timezone/',
'^chromeos/tpm/',
'^components/arc/',
'^components/assist_ranker/',
'^components/autofill/',
'^components/autofill_assistant/',
'^components/bookmarks/',
'^components/browser_sync/',
'^components/browser_watcher/',
'^components/browsing_data/',
'^components/cast_channel/',
'^components/certificate_transparency/',
'^components/chromeos_camera/',
'^components/component_updater/',
'^components/content_settings/',
'^components/crash/',
'^components/cronet/',
'^components/data_reduction_proxy/',
'^components/discardable_memory/',
'^components/dom_distiller/',
'^components/domain_reliability/',
'^components/dom_distiller/',
'^components/download/',
'^components/drive/',
'^components/exo/',
'^components/favicon/',
'^components/feature_engagement/',
'^components/feedback/',
'^components/flags_ui/',
'^components/gcm_driver/',
'^components/google/',
'^components/guest_view/',
'^components/heap_profiling/',
'^components/history/',
Expand All @@ -340,11 +329,9 @@
'^components/network_time/',
'^components/ntp_snippets/',
'^components/ntp_tiles/',
'^components/offline_items_collection/',
'^components/offline_pages/',
'^components/omnibox/',
'^components/ownership/',
'^components/pairing/',
'^components/password_manager/',
'^components/payments/',
'^components/plugins/',
Expand All @@ -355,7 +342,6 @@
'^components/quirks/',
'^components/rappor/',
'^components/remote_cocoa/',
'^components/renderer_context_menu/',
'^components/rlz/',
'^components/safe_browsing/',
'^components/search_engines/',
Expand All @@ -369,10 +355,7 @@
'^components/storage_monitor/',
'^components/subresource_filter/',
'^components/suggestions/',
'^components/supervised_user_error_page/',
'^components/sync/',
'^components/sync_bookmarks/',
'^components/sync_device_info/',
'^components/sync_preferences/',
'^components/sync_sessions/',
'^components/test/',
Expand All @@ -383,24 +366,14 @@
'^components/upload_list/',
'^components/variations/',
'^components/visitedlink/',
'^components/web_cache/',
'^components/webcrypto/',
'^components/webdata/',
'^components/webdata_services/',
'^components/wifi/',
'^components/zoom/',
'^content/browser/',
'^content/child/',
'^content/public/',
'^content/utility/',
'^dbus/',
'^device/base/',
'^device/bluetooth/',
'^device/fido/',
'^device/gamepad/',
'^device/vr/',
'^extensions/',
'^gin/',
'^google_apis/dive/',
'^google_apis/gaia/',
'^google_apis/gcm/',
Expand All @@ -411,8 +384,10 @@
'^ios/web/',
'^ios/web_view/',
'^ipc/',
'^jingle/',
'^media/audio/',
'^media/base/',
'^media/blink/',
'^media/capture/',
'^media/cast/',
'^media/cdm/',
Expand All @@ -426,7 +401,6 @@
'^media/remoting/',
'^media/renderers/',
'^media/test/',
'^mojo/core/',
'^mojo/public/',
'^net/',
'^ppapi/proxy/',
Expand All @@ -442,11 +416,8 @@
'^remoting/protocol/',
'^remoting/signaling/',
'^remoting/test/',
'^sandbox/linux/',
'^sandbox/win/',
'^services/',
'^storage/browser/',
'^testing/libfuzzer/',
'^third_party/blink/',
'^third_party/crashpad/crashpad/test/gtest_main.cc',
'^third_party/leveldatabase/leveldb_chrome.cc',
Expand All @@ -459,17 +430,10 @@
'^tools/clang/base_bind_rewriters/', # Intentional.
'^tools/gdb/gdb_chrome.py', # Intentional.
'^ui/accelerated_widget_mac/',
'^ui/android/',
'^ui/aura/',
'^ui/base/',
'^ui/compositor/',
'^ui/display/',
'^ui/events/',
'^ui/gfx/',
'^ui/message_center/',
'^ui/snapshot/',
'^ui/views_content_client/',
'^ui/wm/',
'^weblayer/',
))

# Format: Sequence of tuples containing:
Expand Down
2 changes: 1 addition & 1 deletion chrome/common/service_process_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class ServiceProcessState {
// |task_runner| must be of type IO and is the loop that POSIX uses
// to monitor the service process.
bool SignalReady(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
const base::Closure& terminate_task);
base::OnceClosure terminate_task);

// Signal that the service process is stopped.
void SignalStopped();
Expand Down
14 changes: 6 additions & 8 deletions chrome/common/service_process_util_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,8 @@ MultiProcessLock* TakeNamedLock(const std::string& name, bool waiting) {
}

ServiceProcessTerminateMonitor::ServiceProcessTerminateMonitor(
const base::Closure& terminate_task)
: terminate_task_(terminate_task) {
}
base::OnceClosure terminate_task)
: terminate_task_(std::move(terminate_task)) {}

ServiceProcessTerminateMonitor::~ServiceProcessTerminateMonitor() {
}
Expand All @@ -206,8 +205,7 @@ void ServiceProcessTerminateMonitor::OnFileCanReadWithoutBlocking(int fd) {
int buffer;
int length = read(fd, &buffer, sizeof(buffer));
if ((length == sizeof(buffer)) && (buffer == kTerminateMessage)) {
terminate_task_.Run();
terminate_task_.Reset();
std::move(terminate_task_).Run();
} else if (length > 0) {
DLOG(ERROR) << "Unexpected read: " << buffer;
} else if (length == 0) {
Expand Down Expand Up @@ -321,7 +319,7 @@ void ServiceProcessState::CreateState() {

bool ServiceProcessState::SignalReady(
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
const base::Closure& terminate_task) {
base::OnceClosure terminate_task) {
DCHECK(task_runner);
DCHECK(state_);

Expand All @@ -331,8 +329,8 @@ bool ServiceProcessState::SignalReady(
return false;
}
#endif
state_->terminate_monitor.reset(
new ServiceProcessTerminateMonitor(terminate_task));
state_->terminate_monitor = std::make_unique<ServiceProcessTerminateMonitor>(
std::move(terminate_task));
if (pipe(state_->sockets) < 0) {
DPLOG(ERROR) << "pipe";
return false;
Expand Down
4 changes: 2 additions & 2 deletions chrome/common/service_process_util_posix.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ class ServiceProcessTerminateMonitor
kTerminateMessage = 0xdecea5e
};

explicit ServiceProcessTerminateMonitor(const base::Closure& terminate_task);
explicit ServiceProcessTerminateMonitor(base::OnceClosure terminate_task);
~ServiceProcessTerminateMonitor() override;

// MessagePumpForIO::FdWatcher overrides
void OnFileCanReadWithoutBlocking(int fd) override;
void OnFileCanWriteWithoutBlocking(int fd) override;

private:
base::Closure terminate_task_;
base::OnceClosure terminate_task_;
};

struct ServiceProcessState::StateData {
Expand Down
20 changes: 9 additions & 11 deletions chrome/common/service_process_util_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,8 @@ std::string GetObsoleteServiceProcessAutoRunKey() {
class ServiceProcessTerminateMonitor
: public base::win::ObjectWatcher::Delegate {
public:
explicit ServiceProcessTerminateMonitor(const base::Closure& terminate_task)
: terminate_task_(terminate_task) {
}
explicit ServiceProcessTerminateMonitor(base::OnceClosure terminate_task)
: terminate_task_(std::move(terminate_task)) {}
void Start() {
base::string16 event_name = GetServiceProcessTerminateEventName();
DCHECK(event_name.length() <= MAX_PATH);
Expand All @@ -71,16 +70,14 @@ class ServiceProcessTerminateMonitor

// base::ObjectWatcher::Delegate implementation.
void OnObjectSignaled(HANDLE object) override {
if (!terminate_task_.is_null()) {
terminate_task_.Run();
terminate_task_.Reset();
}
if (!terminate_task_.is_null())
std::move(terminate_task_).Run();
}

private:
base::win::ScopedHandle terminate_event_;
base::win::ObjectWatcher watcher_;
base::Closure terminate_task_;
base::OnceClosure terminate_task_;
};

} // namespace
Expand Down Expand Up @@ -225,15 +222,16 @@ bool ServiceProcessState::TakeSingletonLock() {

bool ServiceProcessState::SignalReady(
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
const base::Closure& terminate_task) {
base::OnceClosure terminate_task) {
DCHECK(state_);
DCHECK(state_->ready_event.IsValid());
if (!SetEvent(state_->ready_event.Get())) {
return false;
}
if (!terminate_task.is_null()) {
state_->terminate_monitor.reset(
new ServiceProcessTerminateMonitor(terminate_task));
state_->terminate_monitor =
std::make_unique<ServiceProcessTerminateMonitor>(
std::move(terminate_task));
state_->terminate_monitor->Start();
}
return true;
Expand Down
8 changes: 4 additions & 4 deletions chrome/service/service_ipc_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ ServiceIPCServer::ServiceIPCServer(
DCHECK(client);
DCHECK(shutdown_event);
binder_registry_.AddInterface(
base::Bind(&ServiceIPCServer::HandleServiceProcessConnection,
base::Unretained(this)));
base::BindRepeating(&ServiceIPCServer::HandleServiceProcessConnection,
base::Unretained(this)));
}

bool ServiceIPCServer::Init() {
Expand All @@ -34,8 +34,8 @@ void ServiceIPCServer::CreateChannel() {
receiver_.Bind(
mojo::PendingReceiver<service_manager::mojom::InterfaceProvider>(
client_->CreateChannelMessagePipe()));
receiver_.set_disconnect_handler(
base::Bind(&ServiceIPCServer::OnChannelError, base::Unretained(this)));
receiver_.set_disconnect_handler(base::BindOnce(
&ServiceIPCServer::OnChannelError, base::Unretained(this)));
}

ServiceIPCServer::~ServiceIPCServer() = default;
Expand Down
5 changes: 2 additions & 3 deletions chrome/service/service_ipc_server_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,8 @@ void ServiceIPCServerTest::TearDown() {

void ServiceIPCServerTest::PumpLoops() {
base::RunLoop run_loop;
io_thread_.task_runner()->PostTaskAndReply(FROM_HERE,
base::Bind(&PumpCurrentLoop),
run_loop.QuitClosure());
io_thread_.task_runner()->PostTaskAndReply(
FROM_HERE, base::BindOnce(&PumpCurrentLoop), run_loop.QuitClosure());
run_loop.Run();
PumpCurrentLoop();
}
Expand Down
6 changes: 3 additions & 3 deletions chrome/service/service_process.cc
Original file line number Diff line number Diff line change
Expand Up @@ -238,15 +238,15 @@ bool ServiceProcess::Initialize(base::OnceClosure quit_closure,

ipc_server_.reset(new ServiceIPCServer(this /* client */, io_task_runner(),
&shutdown_event_));
ipc_server_->binder_registry().AddInterface(
base::Bind(&cloud_print::CloudPrintMessageHandler::Create, this));
ipc_server_->binder_registry().AddInterface(base::BindRepeating(
&cloud_print::CloudPrintMessageHandler::Create, this));
ipc_server_->Init();

// After the IPC server has started we signal that the service process is
// ready.
if (!service_process_state_->SignalReady(
io_task_runner().get(),
base::Bind(&ServiceProcess::Terminate, base::Unretained(this)))) {
base::BindOnce(&ServiceProcess::Terminate, base::Unretained(this)))) {
return false;
}

Expand Down
8 changes: 4 additions & 4 deletions chrome/service/service_utility_process_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -438,10 +438,10 @@ void ServiceUtilityProcessHost::OnRenderPDFPagesToMetafilesPageDone(

base::PostTaskAndReplyWithResult(
client_task_runner_.get(), FROM_HERE,
base::Bind(&Client::MetafileAvailable, client_.get(), scale_factor,
base::Passed(&emf_region)),
base::Bind(&ServiceUtilityProcessHost::OnMetafileSpooled,
weak_ptr_factory_.GetWeakPtr()));
base::BindOnce(&Client::MetafileAvailable, client_.get(), scale_factor,
base::Passed(&emf_region)),
base::BindOnce(&ServiceUtilityProcessHost::OnMetafileSpooled,
weak_ptr_factory_.GetWeakPtr()));
}

void ServiceUtilityProcessHost::OnPDFToEmfFinished(bool success) {
Expand Down
4 changes: 2 additions & 2 deletions chrome/updater/mac/net/network_unittest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ void DownloadCallback(int net_error, int64_t content_size) {
auto fetcher = base::MakeRefCounted<NetworkFetcherFactory>()->Create();

net::EmbeddedTestServer test_server;
test_server.RegisterRequestHandler(base::Bind(
test_server.RegisterRequestHandler(base::BindRepeating(
&ChromeUpdaterNetworkMacTest::HandleRequest, base::Unretained(this)));
ASSERT_TRUE(test_server.Start());
const GURL url = test_server.GetURL("/echo");
Expand All @@ -130,7 +130,7 @@ void DownloadCallback(int net_error, int64_t content_size) {
auto fetcher = base::MakeRefCounted<NetworkFetcherFactory>()->Create();

net::EmbeddedTestServer test_server;
test_server.RegisterRequestHandler(base::Bind(
test_server.RegisterRequestHandler(base::BindRepeating(
&ChromeUpdaterNetworkMacTest::HandleRequest, base::Unretained(this)));
ASSERT_TRUE(test_server.Start());
const GURL url = test_server.GetURL("/echo");
Expand Down
2 changes: 1 addition & 1 deletion chrome/updater/win/installer/run_all_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ int main(int argc, char** argv) {

return base::LaunchUnitTestsSerially(
argc, argv,
base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
base::BindOnce(&base::TestSuite::Run, base::Unretained(&test_suite)));
}
2 changes: 1 addition & 1 deletion chromeos/memory/kstaled.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ void InitializeKstaled() {
DCHECK(debugd_client);

debugd_client->SetKstaledRatio(static_cast<uint8_t>(feature_ratio),
base::Bind(&OnRatioSet));
base::BindOnce(&OnRatioSet));
}

} // namespace chromeos
Loading

0 comments on commit c857609

Please sign in to comment.