Unit tests for output protection in output configurator

Unit tests for output protection in output configurator

R=derat@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226414

[Daniel Erat, 2013-09-30 14:01:16]

Thanks for adding tests! This change basically looks fine to me; just a few
comments.

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator_unittest.cc (right):

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:77: return
base::StringPrintf("get_hdcp(id=%lu)", id);
i wouldn't bother with an action for this Get*() method; checking that
OutputConfigurator has the state that was previously set via set_hdcp_state() is
sufficient to make sure that it's being called. see e.g. GetOutputs(), which is
in a similar situation

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:82: return
base::StringPrintf("set_hdcp(id=%lu, state=%d)", id, state);
nit: remove space after the comma in the string to match other actions

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:125: void
set_hdcp_state(HDCPState state) {
nit: move this all to one line since it'll fit:

  void set_hdcp_state(HDCPState state) { hdcp_state_ = state; }

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:1119: uint32_t protection_mask;
nit: move these down to the point where they're first used and initialize them
to 0
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables)

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:1138: configurator_.Start(0);
calling Start() multiple times isn't expected behavior; UpdateOutputs(..., true)
should be enough to pick up the new display. note that doing so will generate
bunch of extra actions, but i'd recommend just adding a bare
GetActionsAndClear() call to clear them.

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:1198:
HDCP_STATE_DESIRED).c_str(),
please move the call to check the Set action up to just after you call
EnableOutputProtection()

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:1204: // Protections will be
disabled only if no more clients request for.
nit: s/request for/request them/

[kcwu, 2013-10-01 07:15:51]

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator_unittest.cc (right):

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:77: return
base::StringPrintf("get_hdcp(id=%lu)", id);
On 2013/09/30 14:01:16, Daniel Erat wrote:
> i wouldn't bother with an action for this Get*() method; checking that
> OutputConfigurator has the state that was previously set via set_hdcp_state()
is
> sufficient to make sure that it's being called. see e.g. GetOutputs(), which
is
> in a similar situation

Done.

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:82: return
base::StringPrintf("set_hdcp(id=%lu, state=%d)", id, state);
On 2013/09/30 14:01:16, Daniel Erat wrote:
> nit: remove space after the comma in the string to match other actions

Done.

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:125: void
set_hdcp_state(HDCPState state) {
On 2013/09/30 14:01:16, Daniel Erat wrote:
> nit: move this all to one line since it'll fit:
> 
>   void set_hdcp_state(HDCPState state) { hdcp_state_ = state; }

Done.

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:1119: uint32_t protection_mask;
On 2013/09/30 14:01:16, Daniel Erat wrote:
> nit: move these down to the point where they're first used and initialize them
> to 0
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables)

Done.

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:1138: configurator_.Start(0);
On 2013/09/30 14:01:16, Daniel Erat wrote:
> calling Start() multiple times isn't expected behavior; UpdateOutputs(...,
true)
> should be enough to pick up the new display. note that doing so will generate
> bunch of extra actions, but i'd recommend just adding a bare
> GetActionsAndClear() call to clear them.

Done.

https://codereview.chromium.org/25058009/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator_unittest.cc:1198:
HDCP_STATE_DESIRED).c_str(),
On 2013/09/30 14:01:16, Daniel Erat wrote:
> please move the call to check the Set action up to just after you call
> EnableOutputProtection()

Done.

[Daniel Erat, 2013-10-01 12:51:56]

lgtm

[commit-bot: I haz the power, 2013-10-02 03:46:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kcwu@chromium.org/25058009/6001

[commit-bot: I haz the power, 2013-10-02 07:21:15]

Change committed as 226414