Fix HDCP in mirror mode.

Fix HDCP in mirror mode.

BUG=chromium:371878
TEST=verified with pepper plugin manually

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

[Daniel Erat, 2014-05-15 14:04:25]

i'm out for the next few days but will check mail occasionally. adding dnicoara
as a reviewer.

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
ash/display/display_manager.h:256: virtual bool software_mirroring_enabled()
const {
making an accessor virtual seems strange...

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:226: (mirroring_controller_ &&
wrapped line should be indented four spaces, not two

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:242: ++request_it)
fix indenting

[dnicoara, 2014-05-15 14:39:49]

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:268: // No protections for these
types. Do nothing.
Perhaps I've misunderstood the bug, but is this correct in the bug's context? If
we're supposed to have HDCP protection, shouldn't these cases return false when
HDCP protection is requested?

[kcwu1, 2014-05-15 15:02:42]

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
ash/display/display_manager.h:256: virtual bool software_mirroring_enabled()
const {
On 2014/05/15 14:04:25, Daniel Erat (OOO until May 20) wrote:
> making an accessor virtual seems strange...

If not, how do you suggest to expose this method for
SoftwareMirroringController?

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:268: // No protections for these
types. Do nothing.
On 2014/05/15 14:39:49, dnicoara wrote:
> Perhaps I've misunderstood the bug, but is this correct in the bug's context?
If
> we're supposed to have HDCP protection, shouldn't these cases return false
when
> HDCP protection is requested?

This method could be understood as "protect if you can". The caller will call
QueryContentProtectionStatus to verify the it's successful or not.

[dnicoara, 2014-05-15 19:38:48]

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
ash/display/display_manager.h:256: virtual bool software_mirroring_enabled()
const {
On 2014/05/15 15:02:42, kcwu1 wrote:
> On 2014/05/15 14:04:25, Daniel Erat (OOO until May 20) wrote:
> > making an accessor virtual seems strange...
> 
> If not, how do you suggest to expose this method for
> SoftwareMirroringController?

I think the confusion comes from the fact that the unix_hacker_style should not
be used for virtual functions (as per coding guidelines). Or maybe because
OVERRIDE is missing (so it isn't obvious that this function is implementing the
interface).

Also, should it be in the #ifdef above?

[kcwu, 2014-05-16 04:36:38]

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/20001/ash/display/display_mana...
ash/display/display_manager.h:256: virtual bool software_mirroring_enabled()
const {
On 2014/05/15 19:38:48, dnicoara wrote:
> On 2014/05/15 15:02:42, kcwu1 wrote:
> > On 2014/05/15 14:04:25, Daniel Erat (OOO until May 20) wrote:
> > > making an accessor virtual seems strange...
> > 
> > If not, how do you suggest to expose this method for
> > SoftwareMirroringController?
> 
> I think the confusion comes from the fact that the unix_hacker_style should
not
> be used for virtual functions (as per coding guidelines). Or maybe because
> OVERRIDE is missing (so it isn't obvious that this function is implementing
the
> interface).
> 
> Also, should it be in the #ifdef above?

Done.
It should not be in #ifdef because it is also used by this class itself.

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:226: (mirroring_controller_ &&
On 2014/05/15 14:04:25, Daniel Erat (OOO until May 20) wrote:
> wrapped line should be indented four spaces, not two

Done.

https://codereview.chromium.org/284853002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:242: ++request_it)
On 2014/05/15 14:04:25, Daniel Erat (OOO until May 20) wrote:
> fix indenting

Done.

[dnicoara, 2014-05-16 12:42:40]

Other than my next comment lgtm.

https://codereview.chromium.org/284853002/diff/40001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/40001/ash/display/display_mana...
ash/display/display_manager.h:256: virtual bool SoftwareMirroringEnabled() const
OVERRIDE {
I think the clang builders will have an issue with this on !CrOS builds since
this is virtual and overwritten from SoftwareMirrorController and
SoftwareMirrorController isn't inherited in the !CrOS case.

[Daniel Erat, 2014-05-16 14:34:17]

lgtm w/nits

https://codereview.chromium.org/284853002/diff/40001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/284853002/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:227:
mirroring_controller_->SoftwareMirroringEnabled());
nit: is this what clang-format recommends? i think that the most common
indenting style i've seen here is to line this up with |mirroring_controller_|
from the previous line

https://codereview.chromium.org/284853002/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:242: request_it != requests.end();
nit: fix indenting (this and the next line should line up with |request_it| from
the previous line)

[kcwu1, 2014-05-16 15:49:53]

Please take another look for how I handle OVERRIDE keyword issue.

https://codereview.chromium.org/284853002/diff/40001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/40001/ash/display/display_mana...
ash/display/display_manager.h:256: virtual bool SoftwareMirroringEnabled() const
OVERRIDE {
On 2014/05/16 12:42:41, dnicoara wrote:
> I think the clang builders will have an issue with this on !CrOS builds since
> this is virtual and overwritten from SoftwareMirrorController and
> SoftwareMirrorController isn't inherited in the !CrOS case.

Done.
Thanks for pointing out this issue. I keep original method and added a new one
to  fix this issue.

https://codereview.chromium.org/284853002/diff/40001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/284853002/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:227:
mirroring_controller_->SoftwareMirroringEnabled());
On 2014/05/16 14:34:17, Daniel Erat (OOO until May 20) wrote:
> nit: is this what clang-format recommends? i think that the most common
> indenting style i've seen here is to line this up with |mirroring_controller_|
> from the previous line

Done.

https://codereview.chromium.org/284853002/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:242: request_it != requests.end();
On 2014/05/16 14:34:17, Daniel Erat (OOO until May 20) wrote:
> nit: fix indenting (this and the next line should line up with |request_it|
from
> the previous line)

Done.

[dnicoara, 2014-05-16 17:42:00]

lgtm unless Daniel has something else in mind.

https://codereview.chromium.org/284853002/diff/60001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/60001/ash/display/display_mana...
ash/display/display_manager.h:255: virtual bool SoftwareMirroringEnabled() const
OVERRIDE {
nit: Please move the implementation to the source file (as per coding guidelines
virtual functions should be implemented in the source file).

[kcwu1, 2014-05-16 18:18:29]

https://codereview.chromium.org/284853002/diff/60001/ash/display/display_mana...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/284853002/diff/60001/ash/display/display_mana...
ash/display/display_manager.h:255: virtual bool SoftwareMirroringEnabled() const
OVERRIDE {
On 2014/05/16 17:42:00, dnicoara wrote:
> nit: Please move the implementation to the source file (as per coding
guidelines
> virtual functions should be implemented in the source file).

Done.

[kcwu1, 2014-05-16 18:19:00]

The CQ bit was checked by kcwu@google.com

[commit-bot: I haz the power, 2014-05-16 18:19:52]

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

[kcwu1, 2014-05-17 04:00:07]

The CQ bit was unchecked by kcwu@google.com

[kcwu1, 2014-05-17 04:00:55]

The CQ bit was checked by kcwu@google.com

[commit-bot: I haz the power, 2014-05-17 04:02:16]

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

[commit-bot: I haz the power, 2014-05-17 10:05:36]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-17 11:17:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 11:17:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[kcwu1, 2014-05-17 11:21:18]

The CQ bit was checked by kcwu@google.com

[commit-bot: I haz the power, 2014-05-17 11:22:28]

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

[commit-bot: I haz the power, 2014-05-17 18:21:07]

Change committed as 271252