Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389393006:
Add test cases to BindTest for OnceCallback (Closed)
DescriptionAdd test cases to BindTest for OnceCallback
BindTest didn't have enough test cases for OnceCallback.
This CL replaces unannotated Callbacks with RepeatingCallback, and adds
equivalent tests for OnceCallback if needed. Also, this CL moves some test
cases to a typed test, BindVariantsTest, for smaller dups.
Committed: https://crrev.com/b9499fd94a664e91aac5b87874f8f618a0dca393
Cr-Commit-Position: refs/heads/master@{#424670}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 5
Patch Set 3 : typo fix #Messages
Total messages: 21 (14 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add test cases to BindTest for OnceCallback BUG= ========== to ========== Add test cases to BindTest for OnceCallback BindTest didn't have enough test cases for OnceCallback. This CL replaces unannotated Callbacks with RepeatingCallback, and adds equivalent tests for OnceCallback if needed. Also, this CL moves some test cases to a typed test, BindVariantsTest, for smaller dups. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tzik@chromium.org changed reviewers: + dcheng@chromium.org, yutak@chromium.org
PTAL
LGTM, I'd like dcheng to proofread, too, though. https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc#n... base/bind_unittest.cc:412: // RepeatingCallback can run via a rvalue-referenc. referenc. ^e https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc#n... base/bind_unittest.cc:540: int& ref_n = n; I think we still ban non-const ref like this, but this looks inevitable to me.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc#n... base/bind_unittest.cc:412: // RepeatingCallback can run via a rvalue-referenc. On 2016/10/07 08:36:56, Yuta Kitamura wrote: > referenc. > ^e Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM (Sorry for the delay, forgot to publish my comments) https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc#n... base/bind_unittest.cc:540: int& ref_n = n; On 2016/10/07 08:36:56, Yuta Kitamura wrote: > I think we still ban non-const ref like this, but this looks > inevitable to me. To clarify, the styleguide only bans mutable reference *arguments*. Since this is not an argument, this is actually OK =) https://codereview.chromium.org/2389393006/diff/20001/base/bind_unittest.cc#n... base/bind_unittest.cc:820: std::move(method_cb).Run(); I guess this would make the tests more complicated, but strictly speaking, this could also be templated ;-) (FWIW, I think the current test is fine... it would be nice to reduce the dupe, but parameterized tests are also a bit harder to read)
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org Link to the patchset: https://codereview.chromium.org/2389393006/#ps40001 (title: "typo fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add test cases to BindTest for OnceCallback BindTest didn't have enough test cases for OnceCallback. This CL replaces unannotated Callbacks with RepeatingCallback, and adds equivalent tests for OnceCallback if needed. Also, this CL moves some test cases to a typed test, BindVariantsTest, for smaller dups. ========== to ========== Add test cases to BindTest for OnceCallback BindTest didn't have enough test cases for OnceCallback. This CL replaces unannotated Callbacks with RepeatingCallback, and adds equivalent tests for OnceCallback if needed. Also, this CL moves some test cases to a typed test, BindVariantsTest, for smaller dups. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add test cases to BindTest for OnceCallback BindTest didn't have enough test cases for OnceCallback. This CL replaces unannotated Callbacks with RepeatingCallback, and adds equivalent tests for OnceCallback if needed. Also, this CL moves some test cases to a typed test, BindVariantsTest, for smaller dups. ========== to ========== Add test cases to BindTest for OnceCallback BindTest didn't have enough test cases for OnceCallback. This CL replaces unannotated Callbacks with RepeatingCallback, and adds equivalent tests for OnceCallback if needed. Also, this CL moves some test cases to a typed test, BindVariantsTest, for smaller dups. Committed: https://crrev.com/b9499fd94a664e91aac5b87874f8f618a0dca393 Cr-Commit-Position: refs/heads/master@{#424670} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b9499fd94a664e91aac5b87874f8f618a0dca393 Cr-Commit-Position: refs/heads/master@{#424670} |
