Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2316573004:
Refactor async_test in RTCPeerConnection-ice layout test (Closed)
DescriptionRefactor async_test in RTCPeerConnection-ice layout test
In https://codereview.chromium.org/2273363004/ qyearsley pointed out that this test wasn't running in the browser. This refactors the test to pass a function argument to async_test. I was hoping to solve the browser problem via this refactor, but it is still unsolved.
BUG=614963
Committed: https://crrev.com/f2c961c0712e93b1c0c7a559415f71e838390ff6
Cr-Commit-Position: refs/heads/master@{#417659}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Refactor RTCPeerConnection-ice to use async_test and step_func #Patch Set 3 : Fix bugs and match Google JavaScript style #Patch Set 4 : Add step_func_done to end of 2nd test #Messages
Total messages: 12 (4 generated)
qyearsley@chromium.org changed reviewers: + qyearsley@chromium.org
https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:66: }), unexpectedCallback); I'm ambivalent about whether this is easier to read with anonymous functions than with a series of functions called addIceCandidateSuccess1, addIceCandidateSuccess2, etc. On one hand, those function names don't have a lot of meaning to me; on the other hand, this way makes the code very nested, which usually makes it harder to read. I think using a series of named functions with 1,2,3 might actually still be easier to read in this case.
https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:66: }), unexpectedCallback); On 2016/09/07 at 18:17:03, qyearsley wrote: > I'm ambivalent about whether this is easier to read with anonymous functions than with a series of functions called addIceCandidateSuccess1, addIceCandidateSuccess2, etc. On one hand, those function names don't have a lot of meaning to me; on the other hand, this way makes the code very nested, which usually makes it harder to read. I think using a series of named functions with 1,2,3 might actually still be easier to read in this case. Agreed - I think I'm leaning toward named functions. I wanted to try this out, but there are just too many layers of nesting :/ The main contribution of this PR is to add the t.step_func calls - I'll make a patch that only adds those.
On 2016/09/07 at 18:27:21, jeffcarp wrote: > https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): > > https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:66: }), unexpectedCallback); > On 2016/09/07 at 18:17:03, qyearsley wrote: > > I'm ambivalent about whether this is easier to read with anonymous functions than with a series of functions called addIceCandidateSuccess1, addIceCandidateSuccess2, etc. On one hand, those function names don't have a lot of meaning to me; on the other hand, this way makes the code very nested, which usually makes it harder to read. I think using a series of named functions with 1,2,3 might actually still be easier to read in this case. > > Agreed - I think I'm leaning toward named functions. I wanted to try this out, but there are just too many layers of nesting :/ > > The main contribution of this PR is to add the t.step_func calls - I'll make a patch that only adds those. Ok, I re-refactored the PR to use named functions. Lmk what you think. The testing-in-browser issue is still unresolved. Would be curious if there's a flag that needs to be enabled for it to work.
On 2016/09/09 at 04:23:14, jeffcarp wrote: > On 2016/09/07 at 18:27:21, jeffcarp wrote: > > https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): > > > > https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:66: }), unexpectedCallback); > > On 2016/09/07 at 18:17:03, qyearsley wrote: > > > I'm ambivalent about whether this is easier to read with anonymous functions than with a series of functions called addIceCandidateSuccess1, addIceCandidateSuccess2, etc. On one hand, those function names don't have a lot of meaning to me; on the other hand, this way makes the code very nested, which usually makes it harder to read. I think using a series of named functions with 1,2,3 might actually still be easier to read in this case. > > > > Agreed - I think I'm leaning toward named functions. I wanted to try this out, but there are just too many layers of nesting :/ > > > > The main contribution of this PR is to add the t.step_func calls - I'll make a patch that only adds those. > > Ok, I re-refactored the PR to use named functions. Lmk what you think. > > The testing-in-browser issue is still unresolved. Would be curious if there's a flag that needs to be enabled for it to work. LGTM Note, in Chromium we don't usually use the term PR or pull request; the term CL or changelist is used more often.
On 2016/09/09 at 16:24:11, qyearsley wrote: > On 2016/09/09 at 04:23:14, jeffcarp wrote: > > On 2016/09/07 at 18:27:21, jeffcarp wrote: > > > https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... > > > File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): > > > > > > https://codereview.chromium.org/2316573004/diff/1/third_party/WebKit/LayoutTe... > > > third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:66: }), unexpectedCallback); > > > On 2016/09/07 at 18:17:03, qyearsley wrote: > > > > I'm ambivalent about whether this is easier to read with anonymous functions than with a series of functions called addIceCandidateSuccess1, addIceCandidateSuccess2, etc. On one hand, those function names don't have a lot of meaning to me; on the other hand, this way makes the code very nested, which usually makes it harder to read. I think using a series of named functions with 1,2,3 might actually still be easier to read in this case. > > > > > > Agreed - I think I'm leaning toward named functions. I wanted to try this out, but there are just too many layers of nesting :/ > > > > > > The main contribution of this PR is to add the t.step_func calls - I'll make a patch that only adds those. > > > > Ok, I re-refactored the PR to use named functions. Lmk what you think. > > > > The testing-in-browser issue is still unresolved. Would be curious if there's a flag that needs to be enabled for it to work. > > LGTM > > Note, in Chromium we don't usually use the term PR or pull request; the term CL or changelist is used more often. Oops haha, force of habit from GitHub. Thanks!
The CQ bit was checked by jeffcarp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2316573004/#ps60001 (title: "Add step_func_done to end of 2nd test")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Refactor async_test in RTCPeerConnection-ice layout test In https://codereview.chromium.org/2273363004/ qyearsley pointed out that this test wasn't running in the browser. This refactors the test to pass a function argument to async_test. I was hoping to solve the browser problem via this refactor, but it is still unsolved. BUG=614963 ========== to ========== Refactor async_test in RTCPeerConnection-ice layout test In https://codereview.chromium.org/2273363004/ qyearsley pointed out that this test wasn't running in the browser. This refactors the test to pass a function argument to async_test. I was hoping to solve the browser problem via this refactor, but it is still unsolved. BUG=614963 Committed: https://crrev.com/f2c961c0712e93b1c0c7a559415f71e838390ff6 Cr-Commit-Position: refs/heads/master@{#417659} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f2c961c0712e93b1c0c7a559415f71e838390ff6 Cr-Commit-Position: refs/heads/master@{#417659} |
