Update base::DoNothing() / base::NullCallback() to use a type tag. [chromium/src : main]
Daniel Cheng (Gerrit)
Attention is currently required from: danakj.
Daniel Cheng would like danakj to review this change.
Update base::DoNothing() / base::NullCallback() to use a type tag.
This allows both helpers to be used in contexts where the original
helper required templated arguments to be explicitly specified,
generally leading to more concise code.
However, since base::DoNothing() only returns a type tag now, it can no
longer be used as a functor with base::BindOnce/base::BindRepeating.
In general, this seems to be a net neutral to slight positive change:
the lambda version is actually shorter—though some might argue less
readable—and it generates more efficient code since running the callback
now jumps through one less thunk.
Change-Id: I6b72230fe9d583865ff3d40fc51025f6195ebbc4
---
M ash/login/security_token_request_controller.cc
M ash/login/ui/pin_request_view_unittest.cc
M ash/services/recording/recording_service.cc
M ash/system/accessibility/floating_accessibility_controller_unittest.cc
M ash/wallpaper/wallpaper_controller_impl.cc
M base/barrier_callback_unittest.cc
M base/callback.h
M base/callback_helpers.h
M base/fuchsia/test_log_listener_safe.cc
M base/task/single_thread_task_executor_unittest.cc
M base/task/thread_pool/delayed_task_manager_unittest.cc
M base/test/test_mock_time_task_runner_unittest.cc
M base/timer/mock_timer_unittest.cc
M cc/tiles/image_controller_unittest.cc
M chrome/browser/ash/child_accounts/time_limits/app_time_controller_unittest.cc
M chrome/browser/ash/crostini/crostini_manager.cc
M chrome/browser/ash/crostini/crostini_manager_unittest.cc
M chrome/browser/ash/login/security_token_session_controller.cc
M chrome/browser/ash/net/rollback_network_config/rollback_network_config.cc
M chrome/browser/ash/settings/device_settings_provider_unittest.cc
M chrome/browser/chromeos/fileapi/file_change_service_unittest.cc
M chrome/browser/client_hints/client_hints_browsertest.cc
M chrome/browser/component_updater/first_party_sets_component_installer_unittest.cc
M chrome/browser/extensions/active_tab_permission_granter.cc
M chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc
M chrome/browser/extensions/permissions_updater.cc
M chrome/browser/extensions/permissions_updater_unittest.cc
M chrome/browser/extensions/scripting_permissions_modifier.cc
M chrome/browser/gpu/gpu_mode_manager.cc
M chrome/browser/optimization_guide/android/android_push_notification_manager.cc
M chrome/browser/signin/chrome_signin_helper.cc
M chrome/browser/ui/ash/assistant/assistant_setup.cc
M chrome/browser/ui/ash/clipboard_util.cc
M chrome/browser/ui/ash/media_client_impl.cc
M chrome/browser/ui/commander/commander_controller_unittest.cc
M chrome/browser/ui/hats/hats_service.cc
M chrome/browser/ui/hung_renderer/hung_renderer_interactive_uitest.cc
M chrome/browser/ui/user_education/reopen_tab_in_product_help_trigger_unittest.cc
M chrome/browser/ui/views/extensions/extension_install_friction_dialog_view_browsertest.cc
M chrome/browser/ui/views/hung_renderer_view_browsertest.cc
M chrome/browser/ui/views/user_education/feature_promo_bubble_view_unittest.cc
M chrome/browser/ui/webui/settings/chromeos/crostini_handler.cc
M chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc
M chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
M chrome/browser/ui/webui/settings/site_settings_helper_unittest.cc
M chrome/browser/usb/usb_chooser_context_unittest.cc
M chrome/browser/vr/webxr_vr_indicators_browser_test.cc
M chrome/browser/vr/webxr_vr_permission_request_browser_test.cc
M chrome/browser/web_applications/extensions/web_app_extension_shortcut_mac.mm
M chrome/browser/web_applications/os_integration_manager.cc
M chrome/browser/web_applications/web_app_file_handler_manager_unittest.cc
M chrome/browser/web_applications/web_app_file_handler_registration_win.cc
M chrome/browser/web_applications/web_app_handler_registration_utils_win_unittest.cc
M chrome/browser/web_applications/web_app_shortcut_win.cc
M chrome/browser/webshare/win/share_operation.cc
M chrome/chrome_cleaner/engines/broker/sandbox_setup.cc
M chrome/chrome_cleaner/engines/target/sandboxed_test_helpers.h
M chrome/chrome_cleaner/ipc/sandbox.cc
M chrome/chrome_cleaner/os/file_remover_unittest.cc
M chromeos/dbus/shill/fake_shill_device_client.cc
M chromeos/dbus/shill/fake_shill_service_client.cc
M chromeos/network/network_metadata_store.cc
M components/account_manager_core/account_manager_facade_impl.cc
M components/autofill/core/browser/pattern_provider/pattern_configuration_parser.h
M components/autofill_assistant/browser/basic_interactions.cc
M components/autofill_assistant/browser/controller.cc
M components/autofill_assistant/browser/starter_unittest.cc
M components/browsing_data/content/database_helper.cc
M components/cast_channel/cast_message_handler_unittest.cc
M components/component_updater/android/component_loader_policy_unittests.cc
M components/nacl/browser/pnacl_host.cc
M components/offline_pages/core/model/offline_page_model_taskified.cc
M components/optimization_guide/core/optimization_guide_store.cc
M components/policy/core/common/cloud/external_policy_data_fetcher.cc
M components/segmentation_platform/internal/database/database_maintenance_impl.cc
M components/segmentation_platform/internal/database/signal_storage_config.cc
M components/services/app_service/public/cpp/icon_coalescer.cc
M components/services/storage/indexed_db/scopes/disjoint_range_lock_manager_unittest.cc
M components/services/storage/service_worker/service_worker_disk_cache_unittest.cc
M components/signin/core/browser/cookie_reminter.cc
M components/viz/service/display_embedder/skia_output_device_buffer_queue_unittest.cc
M components/viz/service/display_embedder/skia_output_surface_impl_unittest.cc
M content/browser/aggregation_service/aggregation_service_network_fetcher_impl_unittest.cc
M content/browser/background_sync/background_sync_manager.cc
M content/browser/cache_storage/legacy/legacy_cache_storage.cc
M content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
M content/browser/conversions/conversion_network_sender_impl_unittest.cc
M content/browser/file_system_access/file_system_access_file_delegate_host_impl.h
M content/browser/file_system_access/file_system_access_file_writer_impl_unittest.cc
M content/browser/file_system_access/file_system_access_handle_base.h
M content/browser/interest_group/ad_auction_service_impl.cc
M content/browser/media/capture/fake_video_capture_stack.cc
M content/browser/network_service_instance_impl.cc
M content/browser/quota/quota_change_browsertest.cc
M content/browser/renderer_host/media/in_process_launched_video_capture_device.cc
M content/browser/renderer_host/media/video_capture_browsertest.cc
M content/browser/renderer_host/policy_container_host.cc
M content/browser/scheduler/responsiveness/jank_monitor_impl_unittest.cc
M content/browser/xr/service/browser_xr_runtime_impl.cc
M content/public/test/browser_test_utils.cc
M content/services/shared_storage_worklet/shared_storage_worklet_global_scope_unittest.cc
M content/shell/browser/shell.cc
M dbus/test_service.cc
M device/vr/openxr/openxr_device.cc
M extensions/browser/api/lock_screen_data/data_item_unittest.cc
M extensions/browser/content_verifier.cc
M extensions/browser/extension_util.cc
M extensions/browser/renderer_startup_helper.cc
M fuchsia/base/legacymetrics_client_unittest.cc
M fuchsia/engine/browser/web_engine_browser_main_parts.cc
M google_apis/gcm/engine/gcm_store_impl.cc
M gpu/command_buffer/client/gl_helper.cc
M gpu/ipc/service/image_transport_surface_overlay_mac.mm
M ios/web_view/internal/autofill/cwv_credit_card_verifier_unittest.mm
M media/audio/android/aaudio_output.cc
M media/audio/audio_input_device.cc
M media/base/audio_buffer.cc
M media/base/video_frame.cc
M media/base/video_frame_unittest.cc
M media/base/video_util.cc
M media/capture/video/chromeos/camera_app_device_bridge_impl.cc
M media/capture/video/video_capture_device_unittest.cc
M media/filters/dav1d_video_decoder.cc
M media/gpu/android/codec_allocator_unittest.cc
M media/gpu/chromeos/image_processor.cc
M media/gpu/chromeos/mailbox_video_frame_converter.cc
M media/gpu/v4l2/v4l2_image_processor_backend.cc
M media/gpu/vaapi/vaapi_dmabuf_video_frame_mapper.cc
M media/gpu/vaapi/vaapi_video_encode_accelerator_unittest.cc
M media/mojo/clients/mojo_video_encode_accelerator.cc
M media/video/video_encode_accelerator_adapter.cc
M net/base/prioritized_task_runner_unittest.cc
M net/disk_cache/simple/simple_index.cc
M net/http/http_transaction_test_util.cc
M net/url_request/report_sender_unittest.cc
M remoting/base/oauth_token_getter_proxy_unittest.cc
M remoting/base/protobuf_http_client_unittest.cc
M remoting/host/it2me/it2me_host.cc
M remoting/protocol/webrtc_data_stream_adapter.cc
M remoting/signaling/ftl_message_reception_channel_unittest.cc
M remoting/signaling/ftl_registration_manager.cc
M remoting/test/ftl_services_playground.cc
M services/device/generic_sensor/generic_sensor_service_unittest.cc
M services/device/media_transfer_protocol/mtp_device_manager.cc
M services/device/public/cpp/test/fake_usb_device.cc
M services/device/usb/usb_device_handle_impl.cc
M services/device/usb/usb_device_handle_win.cc
M services/network/url_loader.cc
M services/network/url_loader_unittest.cc
M services/tracing/public/cpp/perfetto/perfetto_traced_process.cc
M storage/browser/blob/blob_builder_from_stream_unittest.cc
M third_party/blink/renderer/modules/file_system_access/file_system_access_incognito_file_delegate.cc
M third_party/blink/renderer/modules/mediastream/local_video_capturer_source.cc
M third_party/blink/renderer/modules/webcodecs/video_encoder.cc
M third_party/blink/renderer/platform/graphics/video_frame_image_util_test.cc
M third_party/blink/renderer/platform/webrtc/convert_to_webrtc_video_frame_buffer.cc
M ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc
157 files changed, 413 insertions(+), 530 deletions(-)
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: danakj.
12 comments:
Patchset:
Most of the changes are fairly trivial, so I'd like to Owners-Override after getting specific OWNERS to review more interesting changes. I've tried to highlight the interesting ones below.
File chrome/browser/ash/login/security_token_session_controller.cc:
Patch Set #4, Line 108:
base::RepeatingClosure(base::DoNothing())),This is needed to disambiguate, since the constructor has two overloads with different callback signatures.
File chrome/browser/ash/net/rollback_network_config/rollback_network_config.cc:
Patch Set #4, Line 95:
base::BindOnce([](const std::string&, const std::string&) {I think the previous version was a bit nicer, but this is also the only one I've found in tree.
File chrome/browser/gpu/gpu_mode_manager.cc:
Patch Set #4, Line 50:
base::RepeatingClosure(base::DoNothing()));Similarly, this is needed to disambiguate between two overloads with different callback types.
File chrome/browser/signin/chrome_signin_helper.cc:
Patch Set #4, Line 121:
// TODO(dcheng): Should ReleaseSoon() support this use case?This is the only one I saw in the entire codebase so...
File chrome/browser/ui/ash/media_client_impl.cc:
Patch Set #4, Line 606:
base::RepeatingClosure(base::DoNothing())),Needed for disambiguation.
File chrome/browser/ui/webui/settings/chromeos/crostini_handler.cc:
Patch Set #4, Line 635:
base::DoNothing());This change is because clang now warns about pessimizing move, whereas it didn't before.
File chrome/browser/web_applications/extensions/web_app_extension_shortcut_mac.mm:
Patch Set #4, Line 50:
return base::BindRepeating([](Latch*) {}, base::RetainedRef(this));This feels a lot like BarrierClosure...
File components/autofill_assistant/browser/basic_interactions.cc:
Patch Set #4, Line 547:
user_actions->back().SetCallback(base::OnceClosure(base::DoNothing()));I'm not actually sure this change is safe. The reason is whether or not a callback is attached is exposed via HasCallback(), and that is consulted here: https://source.chromium.org/chromium/chromium/src/+/main:components/autofill_assistant/browser/script_executor.cc;l=463;drc=cb65284bde8d00949f8b51e6b6c58f2502d8af2d?q=UserAction::HasCallback&ss=chromium
I'm seeing if any tests fail ^_^
(Assuming this change is OK, I will run this one explicitly by the autofill owners. And if it's not, I will probably still run it by them with whatever updated form I use.)
File gpu/ipc/service/image_transport_surface_overlay_mac.mm:
Patch Set #4, Line 404:
base::ThreadTaskRunnerHandle::Get()->ReleaseSoon(This is technically a behavior change, as ReleaseSoon posts a non-nestable task. I think it's OK though?
File media/audio/android/aaudio_output.cc:
Patch Set #4, Line 140:
base::BindOnce([](std::unique_ptr<AAudioDestructionHelper>) {},Ah I guess there's one more version of DeleteSoon after a delay here...
File remoting/protocol/webrtc_data_stream_adapter.cc:
Patch Set #4, Line 37:
// TODO(dcheng): This could probably be ReleaseSoon.The reason I didn't make it ReleaseSoon() is because rtc::scoped_refptr. I think there's a helper somewhere that converts, but... I'm trying to limit the yak shave size here.
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Daniel Cheng.
15 comments:
File base/barrier_callback_unittest.cc:
Patch Set #8, Line 29:
BindRepeatingOnce
Patch Set #8, Line 122:
BindRepeatingOnce
File chrome/browser/ash/login/security_token_session_controller.cc:
Patch Set #4, Line 108:
base::RepeatingClosure(base::DoNothing())),
This is needed to disambiguate, since the constructor has two overloads with different callback sign […]
Give DoNothingType a method to do this? Or DoNothingClosure()? Too confusing? This is also kinda confusing without a comment.
File chrome/browser/ash/net/rollback_network_config/rollback_network_config.cc:
Patch Set #4, Line 95:
base::BindOnce([](const std::string&, const std::string&) {I think the previous version was a bit nicer, but this is also the only one I've found in tree.
The above and this are the same problem.
DoNothing().As<Args...>() ?
File chrome/browser/gpu/gpu_mode_manager.cc:
Patch Set #4, Line 50:
base::RepeatingClosure(base::DoNothing()));Similarly, this is needed to disambiguate between two overloads with different callback types.
.As<>()?
File chrome/browser/signin/chrome_signin_helper.cc:
Patch Set #4, Line 121:
// TODO(dcheng): Should ReleaseSoon() support this use case?This is the only one I saw in the entire codebase so...
Could also use .As<scoped_refptr<AccountReconcilorLockWrapper>()?
Though I like lambda here anyway myself.
File chrome/browser/ui/ash/media_client_impl.cc:
Patch Set #4, Line 606:
base::RepeatingClosure(base::DoNothing())),Needed for disambiguation.
.As<>()?
or .As<void>() depending what is allowed
File chrome/browser/ui/webui/settings/chromeos/crostini_handler.cc:
Patch Set #4, Line 635:
base::DoNothing());This change is because clang now warns about pessimizing move, whereas it didn't before.
Ack
File components/autofill_assistant/browser/basic_interactions.cc:
Patch Set #8, Line 547:
user_actions->back().SetCallback(Please do in another CL
File gpu/ipc/service/image_transport_surface_overlay_mac.mm:
Patch Set #4, Line 404:
base::ThreadTaskRunnerHandle::Get()->ReleaseSoon(
This is technically a behavior change, as ReleaseSoon posts a non-nestable task. […]
Separate CL would be best
File media/base/video_frame.cc:
Patch Set #8, Line 911:
framenow use-after-move?
File media/gpu/v4l2/v4l2_image_processor_backend.cc:
Patch Set #8, Line 435:
DeleteSoonSeparate CL?
File remoting/base/protobuf_http_client_unittest.cc:
Patch Set #8, Line 113:
base::RepeatingCallback<void(std::unique_ptr<EchoResponse>)>(DoNothing().As<unique_ptr<EchoResponse>>()?
Or maybe .With() is the play here.. do nothing with these args...
File remoting/protocol/webrtc_data_stream_adapter.cc:
Patch Set #4, Line 37:
// TODO(dcheng): This could probably be ReleaseSoon.
The reason I didn't make it ReleaseSoon() is because rtc::scoped_refptr. […]
Yes this is a big CL, no behaviour changes would be nice.
File third_party/blink/renderer/platform/webrtc/convert_to_webrtc_video_frame_buffer.cc:
Patch Set #8, Line 508:
const scoped_refptr<rtc::RefCountInterface>&nit: by value
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: danakj.
13 comments:
Patchset:
As a possible compromise, I guess we could add more complexity. Rather than .As<> returning a Callback directly, it would return a templated type tag that would be used as a hint for which Callback constructor to pick if there are multiple.
Let me see what it looks like when I try implementing it—that being said, I'm not super enthused about it, because it feels like a lot of complexity for relatively few uses.
File base/barrier_callback_unittest.cc:
Patch Set #8, Line 29:
BindRepeatingOnce
Done
Patch Set #8, Line 122:
BindRepeatingOnce
Done
File chrome/browser/ash/login/security_token_session_controller.cc:
Patch Set #4, Line 108:
base::RepeatingClosure(base::DoNothing())),
Give DoNothingType a method to do this? Or DoNothingClosure()? Too confusing? This is also kinda con […]
Hmm... I'd kind of rather not. This leads back into
base::BindOnce(base::DoNothingClosure(), ...)
territory. What comment would you suggest?(From my perspective, it's not super confusing—it'd be like trying to use a tag constructor for any overload where it's ambiguous. For example, if this had an overload where one took an optional<F> and the other took an optional<G>, we'd have the same problem with in_place_t)
File chrome/browser/ash/net/rollback_network_config/rollback_network_config.cc:
Patch Set #4, Line 95:
base::BindOnce([](const std::string&, const std::string&) {
The above and this are the same problem. […]
I'd strongly prefer to avoid this, as this leads to:
base::BindOnce(base::DoNothing().As<...>(), ...)
territory, and I feel that in most cases, the lambda version is strictly better.
File components/autofill_assistant/browser/basic_interactions.cc:
Patch Set #8, Line 547:
user_actions->back().SetCallback(Please do in another CL
Ack
Patch Set #8, Line 547:
user_actions->back().SetCallback(Please do in another CL
Done.
(Though the chances of another CL happening are quite low ^_^)
File gpu/ipc/service/image_transport_surface_overlay_mac.mm:
Patch Set #4, Line 404:
base::ThreadTaskRunnerHandle::Get()->ReleaseSoon(
Separate CL would be best
Done
File media/base/video_frame.cc:
Patch Set #8, Line 911:
framenow use-after-move?
Good catch—fixed.
Patch Set #8, Line 911:
framenow use-after-move?
Done
File media/gpu/v4l2/v4l2_image_processor_backend.cc:
Patch Set #8, Line 435:
DeleteSoonSeparate CL?
Done
File remoting/base/protobuf_http_client_unittest.cc:
Patch Set #8, Line 113:
base::RepeatingCallback<void(std::unique_ptr<EchoResponse>)>(
DoNothing().As<unique_ptr<EchoResponse>>()? […]
This is one of those overload disambiguation issues again.
File third_party/blink/renderer/platform/webrtc/convert_to_webrtc_video_frame_buffer.cc:
Patch Set #8, Line 508:
const scoped_refptr<rtc::RefCountInterface>&nit: by value
This is the mechanical version of the original though :)
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Xiaohan Wang (Gerrit)
Attention is currently required from: Daniel Cheng, danakj.
1 comment:
File media/gpu/chromeos/image_processor.cc:
Patch Set #10, Line 109:
std::move(backend_)));drive-by nit:
I hope we have a dedicated helper for this case instead of having a lamdbda which is harder to read; something like
base::BindOnce(base::TakeOwnership(std::move(back_end_)));
or
base::BindOnce(base::Release(std::move(back_end_)));
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: danakj, Xiaohan Wang.
1 comment:
File media/gpu/chromeos/image_processor.cc:
Patch Set #10, Line 109:
std::move(backend_)));
drive-by nit: […]
DeleteSoon and ReleaseSoon already work for this use case.
(They don't work for other types of RAII scopers though)
I originally tried changing this to DeleteSoon, but for a change like this, danakj pointed out (and I agree) that trying to make it match behavior is the right approach.
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Xiaohan Wang (Gerrit)
Attention is currently required from: Daniel Cheng, danakj.
1 comment:
File media/gpu/chromeos/image_processor.cc:
Patch Set #10, Line 109:
std::move(backend_)));
DeleteSoon and ReleaseSoon already work for this use case. […]
Got it. Thanks!
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: danakj.
3 comments:
Patchset:
OK, I uploaded a new version with the As<> helper. It's not used a lot, but I guess it does make a few places more readable.
Assuming the general approach is reasonable, how about this for landing:
- preland the base::BindOnce/base::BindRepeating using base::DoNothing as a functor as a prerequisite CL
- preland any DeleteSoon/ReleaseSoon as a prerequisite CL
- any other miscellaneous cleanups
Then hopefully this CL will just be a much more straightforward base::DoNothing::Repeatedly / base::DoNothing::Once -> base::DoNothing CL?
File media/gpu/v4l2/v4l2_video_decode_accelerator.cc:
Patch Set #13, Line 1928:
cancelable_service_device_task_callback_ = {};Another new one that popped up. There's no need to reset here, since Cancel() already does that.
File remoting/base/protobuf_http_client_unittest.cc:
Patch Set #8, Line 113:
base::RepeatingCallback<void(std::unique_ptr<EchoResponse>)>(
This is one of those overload disambiguation issues again.
Ah, I can't use As<> here. The problem is SetMessageCallback() is templated...
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Daniel Cheng.
3 comments:
File base/callback.h:
Patch Set #14, Line 97:
RunTypeShould this be `Args...`?
R has to be void right?
File remoting/base/protobuf_http_client_unittest.cc:
Patch Set #8, Line 113:
base::RepeatingCallback<void(std::unique_ptr<EchoResponse>)>(
Ah, I can't use As<> here. The problem is SetMessageCallback() is templated...
Awkward :P
RepeatingCallback(DoNothing().As<std::unique_ptr<EchoResponse>>()) is the other option I guess?
sshrug
File third_party/blink/renderer/platform/webrtc/convert_to_webrtc_video_frame_buffer.cc:
Patch Set #8, Line 508:
const scoped_refptr<rtc::RefCountInterface>&
This is the mechanical version of the original though :)
It's true T_T
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: danakj.
5 comments:
File base/callback.h:
Patch Set #14, Line 97:
RunType
Should this be `Args...`? […]
In our current implementation, it has to be. But I think the templates are easier to write out if they just match exactly, rather than synthesizing things.
(I also think As<void(T1, T2, ...)> is easier to read than As<T1, T2>. The former I can easily tell is a function signature, whereas the latter is less straightforward IMO)
Btw one kind of unfortunate side effect of this is that the documentation of DoNothing/NullCallback is split between callback_helpers.h (where DoNothing and NullCallback live) and here (where the helper types have to live for the callback templates to use them).
The only way I can think of to fix this is... to move the helpers themselves into callback.h. Thoughts?
File chrome/browser/ash/login/security_token_session_controller.cc:
Patch Set #4, Line 108:
base::RepeatingClosure(base::DoNothing())),
Hmm... I'd kind of rather not. This leads back into […]
To circle back on this, I thought of a potential implementation that would allow this to work.
With<> or As<> would return a lambda of the appropriate type, derived from the template params.
However:
- I didn't want two ways to write essentially the same thing (base::DoNothing().As<...>() is ultimately the same as writing out the equivalent lambda)
- I wanted to minimize the inconsistency between DoNothing and NullCallback. If I had As()/With() return a lambda, the NullCallback implementation would have to do something different.
File components/autofill_assistant/browser/basic_interactions.cc:
Patch Set #4, Line 547:
user_actions->back().SetCallback(base::OnceClosure(base::DoNothing()));
I'm not actually sure this change is safe. […]
(I've abandoned any attempts at cleanup)
File media/gpu/v4l2/v4l2_video_decode_accelerator.cc:
Patch Set #13, Line 1928:
cancelable_service_device_task_callback_ = {};Another new one that popped up. There's no need to reset here, since Cancel() already does that.
I'll pull this one out into a separate CL.
File remoting/base/protobuf_http_client_unittest.cc:
Patch Set #8, Line 113:
base::RepeatingCallback<void(std::unique_ptr<EchoResponse>)>(
Awkward :P […]
We would need MakeRepeatingCallback, since RepeatingCallback won't deduce the class template arguments. At that point, I'm not sure if we're gaining.
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Daniel Cheng.
6 comments:
File base/callback.h:
Patch Set #22, Line 66:
constexpr NullCallbackWithSignatureHintTag<Signature> As() {Regarding docs.. the other option is DoNothing() or DoNothingAs<void()>() which moves the callable-stuff/API all over to one place.
Tiny suggestion, a nested WithSignature struct here instead of a wholly separate type?
ie NullCallbackTag::WithSignature?
Patch Set #22, Line 91:
constexpr OnceCallback(internal::NullCallbackTag) : OnceCallback() {}Please have a comment explaining this is the constructor from NullCallback(), etc for each constructor, and explaining the WithSignature one is used when the type can't be deduced because it's a templated type.
File base/callback.h:
Patch Set #14, Line 97:
RunType
In our current implementation, it has to be. […]
Toward my ultimate goal of just having one callback header... :evil laugh:
For docs, I think pointing to the DoNothing docs is sufficient on this side.
File base/callback_helpers.h:
Patch Set #22, Line 146:
// Creates a null callback.Perhaps this should be expanded to explain it's a type that will convert into a null callback on demand, similar to nullptr/nullopt?
Also another though is maybe this shouldn't be a function. This is like std::nullptr or std::nullopt, which is just a global singleton, like base::null_callback which is-a base::NullCallbackTag, and base::do_nothing?
File chrome/browser/ash/login/security_token_session_controller.cc:
Patch Set #4, Line 108:
base::RepeatingClosure(base::DoNothing())),
I didn't want two ways to write essentially the same thing (base::DoNothing().As<...>() is ultimately the same as writing out the equivalent lambda)
This is the same for all DoNothings though: DoNothing() == BindOnce([](){})
I wanted to minimize the inconsistency between DoNothing and NullCallback. If I had As()/With() return a lambda, the NullCallback implementation would have to do something different.
I think that makes good sense, though perhaps the link between these two is weaker when you're writing a CL changing them both. I don't recall seeing any cases where DoNothing needed type parameters
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: danakj.
6 comments:
File base/callback.h:
constexpr OnceCallback(internal::NullCallbackTag::WithSignature<RunType>)
: OnceCallback(internal::NullCallbackTag()) {}
constexpr OnceCallback& operator=(
internal::NullCallbackTag::WithSignature<RunType>) {
*this = internal::NullCallbackTag();
return *this;
}I change all the WithSignature variants to delegate. I had a bug in the original which would cause DoNothing::WithSignature to behave like NullCallback. Oops...
File base/callback.h:
Patch Set #22, Line 66:
constexpr NullCallbackWithSignatureHintTag<Signature> As() {
Regarding docs.. […]
Done. I like this suggestion—it's a bit shorter and fewer parentheses.
Tiny suggestion, a nested WithSignature struct here instead of a wholly separate type? […]
Done
Patch Set #22, Line 91:
constexpr OnceCallback(internal::NullCallbackTag) : OnceCallback() {}
Please have a comment explaining this is the constructor from NullCallback(), etc for each construct […]
Done
File base/callback_helpers.h:
Patch Set #22, Line 146:
// Creates a null callback.
Perhaps this should be expanded to explain it's a type that will convert into a null callback on dem […]
Possibly, but I'm trying to minimize how much breaks in one change :)
File chrome/browser/ash/login/security_token_session_controller.cc:
Patch Set #4, Line 108:
base::RepeatingClosure(base::DoNothing())),
> I didn't want two ways to write essentially the same thing (base::DoNothing().As<... […]
You mean NullCallback?
We don't have any atm, but it's not really out of the realm of possibility: it'd be needed to disambiguate in cases where there are multiple callback overloads.
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Daniel Cheng.
Patch set 25:Code-Review +1
3 comments:
Patchset:
LGTM
File base/callback_helpers.h:
nit: void(int) instead? this won't compile right?
File chrome/browser/ash/login/security_token_session_controller.cc:
Patch Set #4, Line 108:
base::RepeatingClosure(base::DoNothing())),
You mean NullCallback? […]
I did mean NullCallback, thanks.
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Gabriel Charette (Gerrit)
Attention is currently required from: Daniel Cheng.
Daniel Cheng has uploaded this change for review.
Update base::DoNothing() / base::NullCallback() to use a type tag.
This allows both helpers to be used in contexts where the original
helper required templated arguments to be explicitly specified,
generally leading to more concise code.
However, since base::DoNothing() only returns a type tag now, it can no
longer be used as a functor with base::BindOnce/base::BindRepeating.
In general, this seems to be a net neutral to slight positive change:
the lambda version is actually shorter—though some might argue less
readable—and it generates more efficient code since running the callback
now jumps through one less thunk.
Change-Id: I6b72230fe9d583865ff3d40fc51025f6195ebbc4
---
M chrome/browser/ash/crostini/crostini_manager_unittest.cc
M base/fuchsia/test_log_listener_safe.cc
M components/autofill/core/browser/pattern_provider/pattern_configuration_parser.h
M chrome/browser/web_applications/os_integration_manager.cc
M services/tracing/public/cpp/perfetto/perfetto_traced_process.cc
M chrome/chrome_cleaner/engines/target/sandboxed_test_helpers.h
M services/network/transitional_url_loader_factory_owner.cc
M chrome/chrome_cleaner/engines/broker/sandbox_setup.cc
M components/viz/service/display_embedder/skia_output_surface_impl_unittest.cc
M dbus/test_service.cc
M gpu/command_buffer/client/gl_helper.cc
M media/audio/audio_input_device.cc
M chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
M chromeos/dbus/shill/fake_shill_service_client.cc
M ash/wallpaper/wallpaper_controller_impl.cc
M remoting/test/ftl_services_playground.cc
M base/sequenced_task_runner_helpers.h
M chrome/browser/extensions/active_tab_permission_granter.cc
M content/browser/renderer_host/media/video_capture_browsertest.cc
M chrome/browser/ui/ash/clipboard_util.cc
M chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc
M chrome/browser/extensions/permissions_updater_unittest.cc
M chrome/browser/vr/webxr_vr_permission_request_browser_test.cc
M services/device/generic_sensor/generic_sensor_service_unittest.cc
M media/gpu/chromeos/image_processor.cc
M ash/login/security_token_request_controller.cc
M components/cast_channel/cast_message_handler_unittest.cc
M components/component_updater/android/component_loader_policy_unittests.cc
M content/browser/xr/service/browser_xr_runtime_impl.cc
M base/task/single_thread_task_executor_unittest.cc
M components/offline_pages/core/model/offline_page_model_taskified.cc
M media/gpu/v4l2/v4l2_image_processor_backend.cc
M chrome/browser/ui/ash/media_client_impl.cc
M content/browser/quota/quota_change_browsertest.cc
M chrome/browser/web_applications/web_app_file_handler_manager_unittest.cc
M components/segmentation_platform/internal/database/database_maintenance_impl.cc
M chrome/browser/client_hints/client_hints_browsertest.cc
M fuchsia/engine/browser/web_engine_browser_main_parts.cc
M media/gpu/vaapi/vaapi_video_encode_accelerator_unittest.cc
M content/browser/cache_storage/legacy/legacy_cache_storage.cc
M fuchsia/base/legacymetrics_client_unittest.cc
M device/vr/openxr/openxr_device.cc
M chrome/browser/gpu/gpu_mode_manager.cc
M extensions/browser/extension_util.cc
M chrome/browser/component_updater/first_party_sets_component_installer_unittest.cc
M components/autofill_assistant/browser/controller.cc
M chrome/browser/optimization_guide/android/android_push_notification_manager.cc
M net/http/http_transaction_test_util.cc
M components/autofill_assistant/browser/starter_unittest.cc
M chrome/browser/web_applications/web_app_sync_bridge.cc
M net/url_request/report_sender_unittest.cc
M media/capture/video/chromeos/camera_app_device_bridge_impl.cc
M extensions/browser/renderer_startup_helper.cc
M chrome/browser/usb/usb_chooser_context_unittest.cc
M chrome/browser/extensions/permissions_updater.cc
M extensions/browser/api/lock_screen_data/data_item_unittest.cc
M content/public/test/browser_test_utils.cc
M components/viz/service/display_embedder/skia_output_device_buffer_queue_unittest.cc
M google_apis/gcm/engine/gcm_store_impl.cc
M components/optimization_guide/core/optimization_guide_store.cc
M chrome/browser/web_applications/web_app_handler_registration_utils_win_unittest.cc
M content/browser/background_sync/background_sync_manager.cc
M components/signin/core/browser/cookie_reminter.cc
M remoting/base/protobuf_http_client_unittest.cc
M remoting/signaling/ftl_registration_manager.cc
M chrome/browser/ash/login/security_token_session_controller.cc
M chromeos/network/network_metadata_store.cc
M ash/login/ui/pin_request_view_unittest.cc
M content/public/test/slow_http_response.cc
M chrome/browser/ui/views/user_education/feature_promo_bubble_view_unittest.cc
M remoting/host/it2me/it2me_host.cc
M chrome/browser/ash/settings/device_settings_provider_unittest.cc
M net/base/prioritized_task_runner_unittest.cc
M chrome/browser/web_applications/web_app_file_handler_registration_win.cc
M chrome/browser/extensions/api/developer_private/extension_info_generator_unittest.cc
M chrome/browser/ui/views/hung_renderer_view_browsertest.cc
M chrome/browser/extensions/scripting_permissions_modifier.cc
M chromeos/dbus/shill/fake_shill_device_client.cc
M chrome/browser/ui/user_education/reopen_tab_in_product_help_trigger_unittest.cc
M third_party/blink/renderer/modules/webcodecs/video_encoder.cc
M media/capture/video/video_capture_device_unittest.cc
M ash/system/accessibility/floating_accessibility_controller_unittest.cc
M base/sequenced_task_runner.h
M chrome/browser/ui/views/extensions/extension_install_friction_dialog_view_browsertest.cc
M chrome/browser/ash/crostini/crostini_manager.cc
M remoting/signaling/ftl_message_reception_channel_unittest.cc
M content/browser/attribution_reporting/conversion_manager_impl_unittest.cc
M remoting/base/oauth_token_getter_proxy_unittest.cc
M chrome/chrome_cleaner/os/file_remover_unittest.cc
M chrome/browser/ui/ash/assistant/assistant_setup.cc
M media/gpu/android/codec_allocator_unittest.cc
M base/test/test_mock_time_task_runner_unittest.cc
M ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc
M base/callback.h
M chrome/browser/ash/child_accounts/time_limits/app_time_controller_unittest.cc
M components/services/storage/service_worker/service_worker_disk_cache_unittest.cc
M services/device/public/cpp/test/fake_usb_device.cc
M media/gpu/v4l2/v4l2_video_decode_accelerator.cc
M third_party/blink/renderer/platform/graphics/video_frame_image_util_test.cc
M components/account_manager_core/account_manager_facade_impl.cc
M components/browsing_data/content/database_helper.cc
M chrome/browser/ui/hung_renderer/hung_renderer_interactive_uitest.cc
M chrome/browser/ui/commander/commander_controller_unittest.cc
M base/callback_helpers.h
M components/services/storage/indexed_db/scopes/disjoint_range_lock_manager_unittest.cc
M content/services/shared_storage_worklet/shared_storage_worklet_global_scope_unittest.cc
M media/base/video_frame_unittest.cc
M services/network/url_loader_unittest.cc
M media/gpu/v4l2/v4l2_image_processor_backend.h
M base/task/thread_pool/delayed_task_manager_unittest.cc
M chrome/browser/ui/hats/hats_service.cc
M content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
M content/browser/scheduler/responsiveness/jank_monitor_impl_unittest.cc
M chrome/browser/signin/chrome_signin_helper.cc
M components/autofill_assistant/browser/basic_interactions.cc
M third_party/blink/renderer/modules/mediastream/local_video_capturer_source.cc
M ash/services/recording/recording_service.cc
M cc/tiles/image_controller_unittest.cc
M services/device/media_transfer_protocol/mtp_device_manager.cc
M chrome/browser/ui/webui/settings/site_settings_helper_unittest.cc
M chrome/browser/vr/webxr_vr_indicators_browser_test.cc
121 files changed, 451 insertions(+), 427 deletions(-)
Gabriel Charette (Gerrit)
Attention is currently required from: Daniel Cheng.
1 comment:
Patchset:
drive-by: CC pkasting who worked on the previous iteration of base::DoNothing (no comment from me other than thanks for looking to improve base APIs once again 😊!)
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Patchset:
Do you want an OO?
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: danakj.
2 comments:
Patchset:
Do you want an OO?
If you are comfortable providing one, it would be helpful. I have tried to move all the not no-op changes out (e.g. anything that's not simply base::DoNothing::Once/base::DoNothing::Repeating -> base::DoNothing), but I think one or two may have snuck back in in the course of rebasing.
File base/callback_helpers.h:
nit: void(int) instead? this won't compile right?
Normally we can't overload on function return types. But we're not overloading function return types here; we're overloading the callback type that a function accepts, so it's not a problem.
(I mostly did this because it was the most concise, but I can change this if desired)
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
File base/sequenced_task_runner.h:
Patch Set #25, Line 132:
from_here, &DeleteUniquePtrHelper<T>::DoDelete, object.release());Hmm... my rebase must have gotten messed up. I'll fix this.
File base/sequenced_task_runner_helpers.h:
Patch Set #25, Line 33:
class DeleteUniquePtrHelper {??? I'm so confused. Checking my rebase.
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Daniel Cheng.
2 comments:
File base/callback_helpers.h:
Normally we can't overload on function return types. […]
I just mean DoNothingAs<int()>() has to generate a callback that returns an int, but it cant?
File base/sequenced_task_runner.h:
Patch Set #25, Line 132:
from_here, &DeleteUniquePtrHelper<T>::DoDelete, object.release());Hmm... my rebase must have gotten messed up. I'll fix this.
I will do a quick pass once youve checked on rebasing
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Attention is currently required from: Daniel Cheng.
Daniel Cheng uploaded patch set #27 to this change.
Update base::DoNothing() / base::NullCallback() to use a type tag.
This allows both helpers to be used in contexts where the original
helper required templated arguments to be explicitly specified,
generally leading to more concise code.
However, since base::DoNothing() only returns a type tag now, it can no
longer be used as a functor with base::BindOnce/base::BindRepeating.
In general, this seems to be a net neutral to slight positive change:
the lambda version is actually shorter—though some might argue less
readable—and it generates more efficient code since running the callback
now jumps through one less thunk.
Bug: 1252980
Change-Id: I6b72230fe9d583865ff3d40fc51025f6195ebbc4
---
M chrome/browser/ash/crostini/crostini_manager_unittest.cc
M base/fuchsia/test_log_listener_safe.cc
M components/autofill/core/browser/pattern_provider/pattern_configuration_parser.h
M chrome/browser/web_applications/os_integration_manager.cc
M services/tracing/public/cpp/perfetto/perfetto_traced_process.cc
M chrome/chrome_cleaner/engines/target/sandboxed_test_helpers.h
M chrome/chrome_cleaner/engines/broker/sandbox_setup.cc
M components/viz/service/display_embedder/skia_output_surface_impl_unittest.cc
M dbus/test_service.cc
M gpu/command_buffer/client/gl_helper.cc
M media/audio/audio_input_device.cc
M chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc
M chromeos/dbus/shill/fake_shill_service_client.cc
M ash/wallpaper/wallpaper_controller_impl.cc
M remoting/test/ftl_services_playground.cc
M chrome/browser/extensions/active_tab_permission_granter.cc
M content/browser/renderer_host/media/video_capture_browsertest.cc
M chrome/browser/ui/ash/clipboard_util.cc
M chrome/browser/ui/webui/settings/settings_cookies_view_handler.cc
M chrome/browser/extensions/permissions_updater_unittest.cc
M chrome/browser/vr/webxr_vr_permission_request_browser_test.cc
M services/device/generic_sensor/generic_sensor_service_unittest.cc
M ash/login/security_token_request_controller.cc
M components/cast_channel/cast_message_handler_unittest.cc
M components/component_updater/android/component_loader_policy_unittests.cc
M content/browser/xr/service/browser_xr_runtime_impl.cc
M base/task/single_thread_task_executor_unittest.cc
M components/offline_pages/core/model/offline_page_model_taskified.cc
M base/task/thread_pool/delayed_task_manager_unittest.cc
M chrome/browser/ui/hats/hats_service.cc
M content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc
M content/browser/scheduler/responsiveness/jank_monitor_impl_unittest.cc
M chrome/browser/signin/chrome_signin_helper.cc
M components/autofill_assistant/browser/basic_interactions.cc
M third_party/blink/renderer/modules/mediastream/local_video_capturer_source.cc
M ash/services/recording/recording_service.cc
M cc/tiles/image_controller_unittest.cc
M services/device/media_transfer_protocol/mtp_device_manager.cc
M chrome/browser/ui/webui/settings/site_settings_helper_unittest.cc
M chrome/browser/vr/webxr_vr_indicators_browser_test.cc
115 files changed, 422 insertions(+), 411 deletions(-)
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Patchset:
Please Owners-Override if it LG.
I do need to land a presubmit change before this, but that shouldn't be blocking here:
** Presubmit ERRORS **
The following files use std::unique_ptr<T>(). Use nullptr instead.
components/autofill_assistant/browser/basic_interactions.cc:548
base::DoNothingAs<void(std::unique_ptr<TriggerContext>)>());
File base/callback.h:
Patch Set #14, Line 97:
RunType
Toward my ultimate goal of just having one callback header... :evil laugh: […]
Ack
File base/callback_helpers.h:
I just mean DoNothingAs<int()>() has to generate a callback that returns an int, but it cant?
Got it. Done.
File base/fuchsia/test_log_listener_safe.cc:
Patch Set #27, Line 92:
on_log_message_ = NullCallback();This is a no-op change. I tried to make this unambiguous using various tricks (such as restricting who could construct the tag types), but in the end, I still couldn't disambiguate it.
I think this is more readable anyway.
File chrome/browser/gpu/gpu_mode_manager.cc:
Patch Set #4, Line 50:
base::RepeatingClosure(base::DoNothing()));
.As<>()?
Done
File chrome/browser/signin/chrome_signin_helper.cc:
Patch Set #4, Line 121:
// TODO(dcheng): Should ReleaseSoon() support this use case?
Could also use .As<scoped_refptr<AccountReconcilorLockWrapper>()? […]
Ack
File chrome/browser/ui/ash/media_client_impl.cc:
Patch Set #4, Line 606:
base::RepeatingClosure(base::DoNothing())),
.As<>()? […]
Done
File content/browser/background_sync/background_sync_manager.cc:
Patch Set #27, Line 2308:
return op_scheduler_.WrapCallbackToRunNext(base::BindOnce([] {}));DoNothingAs<> doesn't work here because WrapCallbackToRunNext() is a templated function.
File content/browser/cache_storage/legacy/legacy_cache_storage_cache.cc:
Patch Set #27, Line 2532:
scheduler_->WrapCallbackToRunNext(id, base::BindOnce([] {})))));Similar problem here.
File media/capture/video/chromeos/camera_app_device_bridge_impl.cc:
Patch Set #27, Line 114:
camera_info_getter_ = base::NullCallback();These are the ambiguity problem from earlier. I was unable to resolve them through sufficient C++ cleverness—and I think writing it out is clearer anyway.
File media/gpu/v4l2/v4l2_video_decode_accelerator.cc:
Patch Set #13, Line 1928:
cancelable_service_device_task_callback_ = {};
I'll pull this one out into a separate CL.
Oops I forgot do pull this one into a separate CL 😞
But I've reverted this to setting cancelable_service_device_task_callback_ since I don't really understand this code enough, so now it's a trivial conversion.
File remoting/base/protobuf_http_client_unittest.cc:
Patch Set #8, Line 113:
base::RepeatingCallback<void(std::unique_ptr<EchoResponse>)>(
We would need MakeRepeatingCallback, since RepeatingCallback won't deduce the class template argumen […]
As mentioned, I was unable to improve upon this. We could add something to help with this, but there's very few instances where it's an issue, and I'd like to prevent backsliding on the base::BindOnce(base::DoNothing...) problems.
I changed this back to using the base::BindRepeating() pattern that we use elsewhere though.
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Daniel Cheng.
Patch set 29:Code-Review +1
6 comments:
Patchset:
Some bots are unhappy but it LGTM and I can OO when the comment is fixed.. and the bots..?
File components/segmentation_platform/internal/database/database_maintenance_impl.cc:
remove
remove
File content/browser/background_sync/background_sync_manager.cc:
Patch Set #27, Line 2308:
return op_scheduler_.WrapCallbackToRunNext(base::BindOnce([] {}));DoNothingAs<> doesn't work here because WrapCallbackToRunNext() is a templated function.
FWIW rust would solve this by calling DoNothing().into() here which would construct whatever was needed from the DoNothing(). We could have a once() and repeatedly() methods on the DoNothing types to get that sort of thing here (without the into() inferrence). Probably not worth it.
File media/capture/video/chromeos/camera_app_device_bridge_impl.cc:
Patch Set #27, Line 114:
camera_info_getter_ = base::NullCallback();
These are the ambiguity problem from earlier. […]
Agree.
Patch Set #27, Line 114:
camera_info_getter_ = base::NullCallback();
These are the ambiguity problem from earlier. […]
Ack
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
3 comments:
Patchset:
I'm pretty sure the bot failures were due to a bad CL that got reverted.
File components/segmentation_platform/internal/database/database_maintenance_impl.cc:
remove
Done
remove
Done
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
danakj (Gerrit)
Attention is currently required from: Daniel Cheng.
Patch set 30:Owners-Override +1Code-Review +1Commit-Queue +2
1 comment:
Patchset:
Godspeed
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Update base::DoNothing() / base::NullCallback() to use a type tag.
This allows both helpers to be used in contexts where the original
helper required templated arguments to be explicitly specified,
generally leading to more concise code.
However, since base::DoNothing() only returns a type tag now, it can no
longer be used as a functor with base::BindOnce/base::BindRepeating.
In general, this seems to be a net neutral to slight positive change:
the lambda version is actually shorter—though some might argue less
readable—and it generates more efficient code since running the callback
now jumps through one less thunk.
Bug: 1252980
Change-Id: I6b72230fe9d583865ff3d40fc51025f6195ebbc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3180101
Commit-Queue: Daniel Cheng <dch...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Reviewed-by: danakj <dan...@chromium.org>
Owners-Override: danakj <dan...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#927031}
115 files changed, 429 insertions(+), 411 deletions(-)
Sameh Moha (Gerrit)
1 comment:
Patchset:
hi , what I must doing
To view, visit change 3180101. To unsubscribe, or for help writing mail filters, visit settings.