Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2597853002:
Unit test for PresentationDispatcher (Closed)
|
Created:
4 years ago by takumif Modified:
3 years, 11 months ago Reviewers:
mark a. foltz CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnit test for PresentationDispatcher
This CL adds test cases for methods in the blink::WebPresentationClient
interface implemented by PresentationDispatcher.
BUG=576808
Review-Url: https://codereview.chromium.org/2597853002
Cr-Commit-Position: refs/heads/master@{#441752}
Committed: https://chromium.googlesource.com/chromium/src/+/75acd15fe1c4b43755f8f41df04661e663860ca2
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebase #Patch Set 3 : Address Mark's comments #
Total comments: 16
Patch Set 4 : Address Mark's comments #
Messages
Total messages: 30 (21 generated)
Description was changed from ========== Unit tests for PresentationDispatcher BUG=576808 ========== to ========== Unit test for PresentationDispatcher This CL adds test cases for methods in the blink::WebPresentationClient interface implemented by PresentationDispatcher. BUG=576808 ==========
Description was changed from ========== Unit test for PresentationDispatcher This CL adds test cases for methods in the blink::WebPresentationClient interface implemented by PresentationDispatcher. BUG=576808 ========== to ========== Unit test for PresentationDispatcher This CL adds test cases for methods in the blink::WebPresentationClient interface implemented by PresentationDispatcher. BUG=576808 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Unit test for PresentationDispatcher This CL adds test cases for methods in the blink::WebPresentationClient interface implemented by PresentationDispatcher. BUG=576808 ========== to ========== Unit test for PresentationDispatcher This CL adds test cases for methods in the blink::WebPresentationClient interface implemented by PresentationDispatcher. BUG=576808 ==========
takumif@chromium.org changed reviewers: + mfoltz@chromium.org
Please take a look, thanks!
LGTM with minor comments. Thanks for getting this put together :) Optional cases to add to this patch: - Tests for state changes - Test for messages received - Test for discarding queued messages when the frame navigates - Test for screen availability not supported - Test for default presentation starting If you don't get to them, then add a TODO at the top of the file with test cases to add later. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:175: urls_, base::MakeUnique<WebPresentationConnectionClientCallbacks>()); Can you add verification that the callback passed into startSession is invoked with the content of |session_info| above? https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:178: Can you add a test case for returning an error from startSession()? https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:193: base::MakeUnique<WebPresentationConnectionClientCallbacks>()); Similar comment to above. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:196: Test error case. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:261: It looks like we aren't handling the boolean return value from SendConnectionMessage in PresentationDispatcher yet, so there's no need to test it ... :-/
https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:261: On 2016/12/26 at 20:15:15, mark a. foltz wrote: > It looks like we aren't handling the boolean return value from SendConnectionMessage in PresentationDispatcher yet, so there's no need to test it ... :-/ Can you please add a TODO to handle (or remove) the boolean parameter to PresentationDispatcher::HandleSendMessageRequests
Patchset #2 (id:100001) has been deleted
Thanks for reviewing! I'll need to better understand PresentationDispatcher and the related classes to implement the additional test cases you mentioned, so I'm leaving a TODO comment as you suggested for now. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:175: urls_, base::MakeUnique<WebPresentationConnectionClientCallbacks>()); On 2016/12/26 20:15:15, mark a. foltz wrote: > Can you add verification that the callback passed into startSession is invoked > with the content of |session_info| above? Done. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:178: On 2016/12/26 20:15:15, mark a. foltz wrote: > Can you add a test case for returning an error from startSession()? Done. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:193: base::MakeUnique<WebPresentationConnectionClientCallbacks>()); On 2016/12/26 20:15:15, mark a. foltz wrote: > Similar comment to above. Done. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:196: On 2016/12/26 20:15:15, mark a. foltz wrote: > Test error case. Done. https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:261: On 2016/12/26 20:17:12, mark a. foltz wrote: > On 2016/12/26 at 20:15:15, mark a. foltz wrote: > > It looks like we aren't handling the boolean return value from > SendConnectionMessage in PresentationDispatcher yet, so there's no need to test > it ... :-/ > > Can you please add a TODO to handle (or remove) the boolean parameter to > PresentationDispatcher::HandleSendMessageRequests Sorry, I don't understand why the boolean argument for HandleSendMessageRequests() has to be handled differently or removed.
LGTM with comments addressed Nice test cases :) https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:53: FRIEND_TEST_ALL_PREFIXES(PresentationDispatcherTest, TestStartSession); Do these test cases actually use private members of PresentationDispatcher? https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 :) https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:117: *callback_called_ = true; It seems like the only time you would instantiate this callback is when you expected it to be called. Maybe callback_called could be a private member, and assert it's true in the callback dtor? https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:141: *callback_called_ = true; See comment above about making this a private member. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:188: presentation_service_ = base::MakeUnique<MockPresentationService>(); I don't see a reason why these can't be instantiated/destroyed in the test ctor/dtor (so no need for unique_ptr), which would simplify this test a bit. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:201: PresentationDispatcher* dispatcher() { return dispatcher_.get(); } It's common to declare objects used in test cases as protected, versus adding trivial getter methods. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:250: [&](const std::vector<GURL>& presentation_urls, I slightly prefer explicit captures to default capture, as it makes the code easier to refactor. But it would make this test more verbose. I don't feel strongly. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:343: static_cast<const uint8_t*>(array_buffer_.data()), Consider computing this pointer in the test base class to avoid code duplication.
https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2597853002/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher_unittest.cc:261: On 2016/12/28 at 00:59:18, takumif wrote: > On 2016/12/26 20:17:12, mark a. foltz wrote: > > On 2016/12/26 at 20:15:15, mark a. foltz wrote: > > > It looks like we aren't handling the boolean return value from > > SendConnectionMessage in PresentationDispatcher yet, so there's no need to test > > it ... :-/ > > > > Can you please add a TODO to handle (or remove) the boolean parameter to > > PresentationDispatcher::HandleSendMessageRequests > > Sorry, I don't understand why the boolean argument for HandleSendMessageRequests() has to be handled differently or removed. Ignore this comment, we can discuss offline; I think this API could be improved but need to understand the requirements a bit better.
Thanks for reviewing! https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:53: FRIEND_TEST_ALL_PREFIXES(PresentationDispatcherTest, TestStartSession); On 2017/01/03 18:39:19, mfoltz_ooo_until_jan_3 wrote: > Do these test cases actually use private members of PresentationDispatcher? Yes, PresentationDispatcher implements the methods of the WebPresentationClient interface as private methods. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher_unittest.cc (right): https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/03 18:39:20, mfoltz_ooo_until_jan_3 wrote: > 2017 :) Done. :) https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:117: *callback_called_ = true; On 2017/01/03 18:39:20, mfoltz_ooo_until_jan_3 wrote: > It seems like the only time you would instantiate this callback is when you > expected it to be called. Maybe callback_called could be a private member, and > assert it's true in the callback dtor? Done. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:141: *callback_called_ = true; On 2017/01/03 18:39:20, mfoltz_ooo_until_jan_3 wrote: > See comment above about making this a private member. Done. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:188: presentation_service_ = base::MakeUnique<MockPresentationService>(); On 2017/01/03 18:39:20, mfoltz_ooo_until_jan_3 wrote: > I don't see a reason why these can't be instantiated/destroyed in the test > ctor/dtor (so no need for unique_ptr), which would simplify this test a bit. Done. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:201: PresentationDispatcher* dispatcher() { return dispatcher_.get(); } On 2017/01/03 18:39:20, mfoltz_ooo_until_jan_3 wrote: > It's common to declare objects used in test cases as protected, versus adding > trivial getter methods. Done. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:250: [&](const std::vector<GURL>& presentation_urls, On 2017/01/03 18:39:20, mfoltz_ooo_until_jan_3 wrote: > I slightly prefer explicit captures to default capture, as it makes the code > easier to refactor. But it would make this test more verbose. I don't feel > strongly. > Done. Although I can capture |this| but not individual member variables, so it's not much more informative. https://codereview.chromium.org/2597853002/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher_unittest.cc:343: static_cast<const uint8_t*>(array_buffer_.data()), On 2017/01/03 18:39:20, mfoltz_ooo_until_jan_3 wrote: > Consider computing this pointer in the test base class to avoid code > duplication. Done.
The CQ bit was checked by takumif@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2597853002/#ps160001 (title: "Address Mark's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483650626168720,
"parent_rev": "01b8456652e46cc732b7d9282f07fca6e4c99d00", "commit_rev":
"75acd15fe1c4b43755f8f41df04661e663860ca2"}
Message was sent while issue was closed.
Description was changed from ========== Unit test for PresentationDispatcher This CL adds test cases for methods in the blink::WebPresentationClient interface implemented by PresentationDispatcher. BUG=576808 ========== to ========== Unit test for PresentationDispatcher This CL adds test cases for methods in the blink::WebPresentationClient interface implemented by PresentationDispatcher. BUG=576808 Review-Url: https://codereview.chromium.org/2597853002 Cr-Commit-Position: refs/heads/master@{#441752} Committed: https://chromium.googlesource.com/chromium/src/+/75acd15fe1c4b43755f8f41df046... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/chromium/src/+/75acd15fe1c4b43755f8f41df046... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
