Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1954293002:
Fixed a cc unittest for swap buffer change (Closed)
|
Created:
4 years, 7 months ago by shaktisahu Modified:
4 years, 7 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed a cc unittest for swap buffer change
BUG=603797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/8a00dfc9a4c82fece6036ccbf1c9664234a6a586
Cr-Commit-Position: refs/heads/master@{#392749}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 18 (5 generated)
Description was changed from ========== Fixed a cc unittest for swap buffer change BUG=603797 ========== to ========== Fixed a cc unittest for swap buffer change BUG=603797 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
shaktisahu@chromium.org changed reviewers: + khushalsagar@chromium.org
https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... File cc/trees/remote_channel_unittest.cc (right): https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... cc/trees/remote_channel_unittest.cc:13: : calls_received_(0), calls_received_on_both_server_and_client_(0) {} How about splitting this into calls_received_on_client_ and calls_received_on_server_ and using them for each test then?
https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... File cc/trees/remote_channel_unittest.cc (right): https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... cc/trees/remote_channel_unittest.cc:13: : calls_received_(0), calls_received_on_both_server_and_client_(0) {} On 2016/05/06 21:37:54, Khushal wrote: > How about splitting this into calls_received_on_client_ and > calls_received_on_server_ and using them for each test then? Yea, but it is hard to know if the call is received on client or engine since the method is identical. In that case, if I do calls_received_on_client_++ and calls_received_on_server_++, they both would be equal to 2, which is incorrect.
khushalsagar@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... File cc/trees/remote_channel_unittest.cc (right): https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... cc/trees/remote_channel_unittest.cc:13: : calls_received_(0), calls_received_on_both_server_and_client_(0) {} On 2016/05/07 00:46:12, shaktisahu wrote: > On 2016/05/06 21:37:54, Khushal wrote: > > How about splitting this into calls_received_on_client_ and > > calls_received_on_server_ and using them for each test then? > > Yea, but it is hard to know if the call is received on client or engine since > the method is identical. In that case, if I do calls_received_on_client_++ and > calls_received_on_server_++, they both would be equal to 2, which is incorrect. Sorry, I meant we could split it into calls_expected_on_server_only_, calls_expected_on_client_only_ and calls_expected_on_server_and_client_. Regarding knowing whether the call was made on the server or client, Its possible to add a bool to the LayerTreeHostClientForTesting to know whether it is created for server or client LayerTreeHost and pass it in the TestHooks callbacks. +vmpstr if that sounds cool.
A bit more description about why the test was breaking and how the change fixes it would be nice. :)
https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... File cc/trees/remote_channel_unittest.cc (right): https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... cc/trees/remote_channel_unittest.cc:13: : calls_received_(0), calls_received_on_both_server_and_client_(0) {} On 2016/05/09 09:06:33, Khushal wrote: > On 2016/05/07 00:46:12, shaktisahu wrote: > > On 2016/05/06 21:37:54, Khushal wrote: > > > How about splitting this into calls_received_on_client_ and > > > calls_received_on_server_ and using them for each test then? > > > > Yea, but it is hard to know if the call is received on client or engine since > > the method is identical. In that case, if I do calls_received_on_client_++ and > > calls_received_on_server_++, they both would be equal to 2, which is > incorrect. > > Sorry, I meant we could split it into calls_expected_on_server_only_, > calls_expected_on_client_only_ and calls_expected_on_server_and_client_. I would prefer that we don't have any "server and client" variables, since the test should know explicitly whether it's a server or a client and verify that state appropriately. > > Regarding knowing whether the call was made on the server or client, Its > possible to add a bool to the LayerTreeHostClientForTesting to know whether it > is created for server or client LayerTreeHost and pass it in the TestHooks > callbacks. +vmpstr if that sounds cool. Do we have ::CreateForServer or ::CreateForClient functions or something? How would this look for non-blimp LTHCFT /o\
https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... File cc/trees/remote_channel_unittest.cc (right): https://codereview.chromium.org/1954293002/diff/1/cc/trees/remote_channel_uni... cc/trees/remote_channel_unittest.cc:13: : calls_received_(0), calls_received_on_both_server_and_client_(0) {} On 2016/05/10 18:27:32, vmpstr wrote: > On 2016/05/09 09:06:33, Khushal wrote: > > On 2016/05/07 00:46:12, shaktisahu wrote: > > > On 2016/05/06 21:37:54, Khushal wrote: > > > > How about splitting this into calls_received_on_client_ and > > > > calls_received_on_server_ and using them for each test then? > > > > > > Yea, but it is hard to know if the call is received on client or engine > since > > > the method is identical. In that case, if I do calls_received_on_client_++ > and > > > calls_received_on_server_++, they both would be equal to 2, which is > > incorrect. > > > > Sorry, I meant we could split it into calls_expected_on_server_only_, > > calls_expected_on_client_only_ and calls_expected_on_server_and_client_. > > I would prefer that we don't have any "server and client" variables, since the > test should know explicitly whether it's a server or a client and verify that > state appropriately. The LayerTreeTests keep an instance of both the server and client and use a mock network layer to ship protos between the two. The RemoteChannelTests is verifying things for both ends. All the LTHC callbacks reach the tests using TestHooks, so there is not an easy way to distinctly identify where they came from. :/ > > > > > Regarding knowing whether the call was made on the server or client, Its > > possible to add a bool to the LayerTreeHostClientForTesting to know whether it > > is created for server or client LayerTreeHost and pass it in the TestHooks > > callbacks. +vmpstr if that sounds cool. > > Do we have ::CreateForServer or ::CreateForClient functions or something? How > would this look for non-blimp LTHCFT /o\ > Not yet, but that's one way to do it. The threaded tests don't really need that distinction, so they can just ignore that field. This would require changing all tests that use this though. Another option would be for classes extending LayerTreeTests to pass in their own LTHC for the remote client LTH, which can internally use a mini TestHooks like class to give the callbacks to the their test. That wouldn't be pretty either but wouldn't require changing the rest of the tests. Whichever is easier. . . Or for this change this is also fine.
It would be great if the tests verified where the calls came from, but that would need writing a lot of boilerplate code to hook in a different LTHC for the remote client LTH. Verifying that the calls expected on both come twice is fine by me too. lgtm.
lgtm
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954293002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fixed a cc unittest for swap buffer change BUG=603797 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Fixed a cc unittest for swap buffer change BUG=603797 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/8a00dfc9a4c82fece6036ccbf1c9664234a6a586 Cr-Commit-Position: refs/heads/master@{#392749} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/8a00dfc9a4c82fece6036ccbf1c9664234a6a586 Cr-Commit-Position: refs/heads/master@{#392749} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
