| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
assert_array_equals: expected property 7 to be "committed fulfilled 2" but got "navigatesuccess" (expected array ["navigate", "AbortSignal abort", "navigateerror", "navigate", "currententrychange", "committed rejected 1", "finished rejected 1", "committed fulfilled 2", "promise microtask", "navigatesuccess", "finished fulfilled 2"] got ["navigate", "AbortSignal abort", "navigateerror", "navigate", "currententrychange", "committed rejected 1", "finished rejected 1", "navigatesuccess", "committed fulfilled 2", "promise microtask", "finished fulfilled 2"])This looks like a test that got missed in the WPT update.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
base::OnceCallback<void(const VectorType&)> resolve_callback_;These would be mutually exclusive, right? Can we do with just one instead of two resolve callbacks and define the type conditionally of the underlying value type?
values_.clear();Consider making callback accept values vector by value and std::move()'ing here.
std::move(resolve_undefined_callback).Run();Note that per // https://webidl.spec.whatwg.org/#wait-for-all
Step 6:
```
If total is 0, then:
Queue a microtask to perform successSteps given « ».
```
resolver->Resolve();We don't really need a lambda here, do we? Can we just bind `ScriptPromiseResolver<IDLUndefined>::Resolve` directly?
Also, wanna give [HeapBind](third_party/blink/renderer/platform/heap/heap_bind.h) a try?
static ScriptPromise<ResolverType> GetPromiseForWaitingForAll(nit: s/Get/Create/?
base::OnceCallback<void(const VectorType&)> resolve_callback_;These would be mutually exclusive, right? Can we do with just one instead of two resolve callbacks and define the type conditionally of the underlying value type?
Done
values_.clear();Consider making callback accept values vector by value and std::move()'ing here.
Done
std::move(resolve_undefined_callback).Run();Note that per // https://webidl.spec.whatwg.org/#wait-for-all
Step 6:
```
If total is 0, then:Queue a microtask to perform successSteps given « ».
```
Done
resolver->Resolve();We don't really need a lambda here, do we? Can we just bind `ScriptPromiseResolver<IDLUndefined>::Resolve` directly?
Also, wanna give [HeapBind](third_party/blink/renderer/platform/heap/heap_bind.h) a try?
HeapBind: Done
No lambdas: Doesn't work because of function name overloads.
static ScriptPromise<ResolverType> GetPromiseForWaitingForAll(nit: s/Get/Create/?
`Get` matches the spec text - i agree that normally we'd prefee `Create`, but I think in this case it's worth sticking with the spec name.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
static ScriptPromise<ResolverType> GetPromiseForWaitingForAll(Nate Chapinnit: s/Get/Create/?
`Get` matches the spec text - i agree that normally we'd prefee `Create`, but I think in this case it's worth sticking with the spec name.
Acknowledged
nit: perhaps add some explicit coverage for WaitForAll() too?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapinnit: perhaps add some explicit coverage for WaitForAll() too?
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
EXPECT_EQ(wait_for_all_result->resolve_value[0], "first");nit: this can be just `EXPECT_THAT(wait_for_all_result->resolve_value, testing::ValuesAre("first", "second", "third"));`
EXPECT_EQ(wait_for_all_result->resolve_value[0], "first");nit: this can be just `EXPECT_THAT(wait_for_all_result->resolve_value, testing::ValuesAre("first", "second", "third"));`
s/ValuesAre/ElementsAre/ done 😊
Also fixed the case that I copied this logic from.
assert_array_equals: expected property 7 to be "committed fulfilled 2" but got "navigatesuccess" (expected array ["navigate", "AbortSignal abort", "navigateerror", "navigate", "currententrychange", "committed rejected 1", "finished rejected 1", "committed fulfilled 2", "promise microtask", "navigatesuccess", "finished fulfilled 2"] got ["navigate", "AbortSignal abort", "navigateerror", "navigate", "currententrychange", "committed rejected 1", "finished rejected 1", "navigatesuccess", "committed fulfilled 2", "promise microtask", "finished fulfilled 2"])This looks like a test that got missed in the WPT update.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/bindings/core/v8/promise_all_test.cc
Insertions: 4, Deletions: 6.
The diff is too large to show. Please review the diff.
```
PromiseAll should support both "wait for all" and "get a promise for waiting for all"
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |