New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 789805 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Add test-only base::Bind overload that works with capturing lambdas

Project Member Reported by dcheng@chromium.org, Nov 30 2017

Issue description

I'm not sure how we should implement this yet, but we should add something like this for tests.
 

Comment 1 by jam@chromium.org, Nov 30 2017

Cc: roc...@chromium.org
Here's an example I hit https://cs.chromium.org/chromium/src/content/test/url_loader_interceptor_test.cc?q=url_loader_interce&sq=package:chromium&l=1 that would be a lot nicer with capturing.

Comment 2 by tzik@chromium.org, Nov 30 2017

Here is a WIP CL containing base::BindLambdaForTesting(). Does it work for you guys?
http://crrev.com/c/798817
http://crrev.com/c/798817/2/base/bind_unittest.cc#1282
Owner: tzik@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f98654bbf86a0d88a72ed9a6fcef80931fd5e977

commit f98654bbf86a0d88a72ed9a6fcef80931fd5e977
Author: tzik <tzik@chromium.org>
Date: Sat Dec 02 03:28:58 2017

Extend accepted functors on base::Bind{Once,Repeating}

This CL allows empty functors on base::BindOnce and base::Repeating, and
adds base::BindLambdaForTesting that is a test-only variant of Bind that
binds any callable object to a RepeatingCallback.

Bug:  789805 
Change-Id: I7fa34acd2a6b79e8c26d3f6ded67e91bd8d6b966
Reviewed-on: https://chromium-review.googlesource.com/798817
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521192}
[modify] https://crrev.com/f98654bbf86a0d88a72ed9a6fcef80931fd5e977/base/bind_internal.h
[modify] https://crrev.com/f98654bbf86a0d88a72ed9a6fcef80931fd5e977/base/bind_unittest.cc
[modify] https://crrev.com/f98654bbf86a0d88a72ed9a6fcef80931fd5e977/base/bind_unittest.nc
[modify] https://crrev.com/f98654bbf86a0d88a72ed9a6fcef80931fd5e977/base/test/BUILD.gn
[add] https://crrev.com/f98654bbf86a0d88a72ed9a6fcef80931fd5e977/base/test/bind_test_util.h

Comment 5 by tzik@chromium.org, Dec 2 2017

Status: Fixed (was: Available)

Comment 6 by jam@chromium.org, Dec 4 2017

Thanks!

Comment 7 by jam@chromium.org, Dec 4 2017

Status: Assigned (was: Fixed)
Ok I tried this out the example from comment 1, but my Bind() method has one parameter because the InteceptCallback signature has a parameter.

Comment 8 by tzik@chromium.org, Dec 4 2017

Oh... right. Extra parameters are not handled correctly by it.
Updating: http://crrev.com/c/804908
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b83dce526b8fe5b738eef646965d3aefec725c7

commit 4b83dce526b8fe5b738eef646965d3aefec725c7
Author: tzik <tzik@chromium.org>
Date: Mon Dec 04 19:04:27 2017

Fix extra parameter support of BindLambdaForTesting

This CL fixes the extra parameter handling on BindLambdaForTesting.
On the previous implementation, it had dropped parameters on the
resulting Callback::Run. So it used to work only for no-parameter ones.

Bug:  789805 
Change-Id: I0b708382e206444d1ed9fe8cc932dd7826b02b00
Reviewed-on: https://chromium-review.googlesource.com/804908
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521401}
[modify] https://crrev.com/4b83dce526b8fe5b738eef646965d3aefec725c7/base/bind_unittest.cc
[modify] https://crrev.com/4b83dce526b8fe5b738eef646965d3aefec725c7/base/test/bind_test_util.h
[modify] https://crrev.com/4b83dce526b8fe5b738eef646965d3aefec725c7/content/test/url_loader_interceptor_test.cc

Comment 10 by tzik@chromium.org, Dec 5 2017

Status: Fixed (was: Assigned)
Cc: sunn...@chromium.org gab@chromium.org
 Issue 721555  has been merged into this issue.

Sign in to add a comment