Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24039002:
Pepper API implementation for output protection. (Closed)
|
Created:
7 years, 3 months ago by kcwu Modified:
6 years, 11 months ago Reviewers:
Daniel Erat, Cris Neckar, cpu_(ooo_6.6-7.5), Elliot Glaysher, piman, dmichael (off chromium), wuchengli, DaleCurtis, marcheu, binji CC:
chromium-reviews, joi+watch-content_chromium.org, jam, stevenjb+watch_chromium.org, oshima+watch_chromium.org, darin-cc_chromium.org, Pawel Osciak, Sean Paul, Elliot Glaysher Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPepper API implementation for output protection.
BUG=256538
R=cpu@chromium.org, dalecurtis@chromium.org, dmichael@chromium.org, marcheu@chromium.org, wuchengli@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225849
Patch Set 1 #
Total comments: 4
Patch Set 2 : avoid PostTask methods in dtor #
Total comments: 88
Patch Set 3 : #Patch Set 4 : nits #Patch Set 5 : tweaks conditional compile conditions #
Total comments: 77
Patch Set 6 : modify pepper host as message filter. rewrite methods in real_output_configurator_delegate. #
Total comments: 16
Patch Set 7 : all host code run in UI thread #Patch Set 8 : fix header dependency #Patch Set 9 : add test #
Total comments: 3
Patch Set 10 : nit, enum value #Patch Set 11 : fix test on chromeos #
Total comments: 24
Patch Set 12 : revise init & terminate of PepperOutputProtectionMessageFilter #
Total comments: 39
Patch Set 13 : revise output configurator related code. rebased. #
Total comments: 49
Patch Set 14 : revise according to comments #Patch Set 15 : use cached_outputs_ in output_configurator #
Total comments: 20
Patch Set 16 : revise chromeos/display according to review comments #Patch Set 17 : upload again #Patch Set 18 : something wrong with 'git cl upload'. upload again #
Total comments: 2
Patch Set 19 : nit #Patch Set 20 : rebase #Patch Set 21 : fix build for non-chromeos #Messages
Total messages: 62 (0 generated)
Please let me know if you think splitting this CL will make it easier to review. Reviewers, cpu@ for chrome/browser/component_updater/ jochen@ for chrome/chrome_browser.gypi marcheu@, please review chromeos/display dmichael@, dalecurtis@, ddorwin@, could you please review the full CL? Thanks! p.s. I'm not sure my usage of USE_ASH, OS_CHROMEOS, and OS_NACL. Please pay attention of them when you review. https://codereview.chromium.org/24039002/diff/1/chromeos/display/real_output_... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/1/chromeos/display/real_output_... chromeos/display/real_output_configurator_delegate.h:14: #include "ppapi/c/private/ppb_output_protection_private.h" I included this for enums. But presubmit error on this line. It complains You added one or more #includes that violate checkdeps rules. chromeos/display/real_output_configurator_delegate.h Illegal include: "ppapi/c/private/ppb_output_protection_private.h" Because of no rule applying. How to fix this?
https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:40: base::Unretained(this), Can't post tasks to "this" during destruction -- the object is already in the process of being destroyed.
lgtm on component_updater/ppapi_utils.cc
https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:40: base::Unretained(this), On 2013/09/07 01:14:38, DaleCurtis wrote: > Can't post tasks to "this" during destruction -- the object is already in the > process of being destroyed. Done. post tasks to the underlying call.
https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:77: #if defined(USE_ASH) I don't see windows implementations so this is not enough to block windows from reaching here.
https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:77: #if defined(USE_ASH) Why USE_ASH? Should this be OS_CHROMEOS? (And I guess this implies that this API is ChromeOS-only? I would have guessed it could be supported on Windows at least and probably also Mac.) https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:35: #if defined(USE_ASH) && defined(OS_CHROMEOS) Why not use gyp or ifdef out the whole file? (and also the .h contents) https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:44: base::Unretained(this), I don't know what EnableOutputProtection actually does, but it seems dangerous to pass "this" from the destructor to PostTask... https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:69: ash::Shell::GetInstance()->output_configurator(); can this ever be NULL? https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:85: nit: excess whitespace https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:92: nit: excess whitespace https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:116: PepperOutputProtectionHost::OnEnableProtection( nit: it looks like this line should fit with the return type https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:119: nit: excess whitespace in this function body too. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... chromeos/display/output_configurator.cc:247: delegate_->UngrabServer(); I don't know anything about that delegate or what GrabServer/UngrabServer does, but it would be nice if that delegate defined a scoped sentinel to make sure UngrabServer is called when exiting scope. To prevent mistakes if functions can return early. I.e. something you could use like this: { ScopedServerGrabber grab_server(delegate_); // Do something with delegate_ } // UngrabServer gets called automatically. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... chromeos/display/output_configurator.h:374: chrome::PepperOutputProtectionHost* client, should this be const? Same question for |client| in EnableOutputProtection.. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:387: int success; please initialize these (same for other variables below in your patch) https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:429: value = XInternAtom(display_, "Undesired", False); It looks like this function returns type "Atom"... why are you using unsigned long? https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:438: unsigned char *data = (unsigned char*)&value; Please do not use C-style casts. I think you want reinterpret_cast. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:444: bool DetermineLinkType(const XRROutputInfo* output_info, const-reference? https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:462: bool RealOutputConfiguratorDelegate::GetOutputLinkType( Type->Types? It looks like you can retrieve more than one? https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:463: std::vector<PP_OutputProtectionLinkType_Private>& link_type) { out-params should be by pointer: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu... https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:468: XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_, this_id); Can this return NULL? It looks like it can: http://sourcecodebrowser.com/libxrandr/1.2.2/_xrr_output_8c.html#a41ed0191cd9... https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:470: XRRFreeOutputInfo(output_info); optional suggestion: You could use scoped_ptr<XRROutputInfo, XRROutputInfoDeleter> Where XRROutputInfoDeleter looks like this: struct XRROutputInfoDeleter { inline void operator()(void* ptr) const { XRRFreeOutputInfo(static_cast<XRRFreeOutputInfo*>(ptr)); } }; If you end up with more than 1 or 2 places you have to call XRRFreeOutputInfo in any function, it's probably a good idea to use the RAII idiom (a scoper that cleans up after you). After the overhead of defining the Deleter functor, it should shorten your code and make it harder to forget to free. That said... in this case, it's probably simpler to just do my next suggestion... https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:472: continue; Why not just use an else? And I'd maybe flip the cases. I.e: if (output_info->connection == RR_COnnected) { // stuff form 475-480 } else { // stuff from 470-471 } This also means you can do XRRFreeOutputInfo in one place, after the else. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:508: DCHECK(screen_->noutput == (int)link_type.size()); no C-style casts http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:512: PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) { optional nit: I would find it more readable as: if (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI) || (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) { Maybe that's just me. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:523: *protection_mask = enabled & ~unfulfiled; I don't really understand the APIs we're using... but this reads to me that if there are 2 or more HDMI/DISPLAYPORTs, but any one of them is not HDCP-enabled, that GetProtectionMethods will indicate that there is no HDCP support. Is that what we want? https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:547: if (!GetOutputLinkType(link_type)) You do this a lot. Might it make sense to have something like a member that gets populated during GrabScreen()? Or is it just not used everywhere GrabScreen is? https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:554: PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) { ditto, I somewhat prefer separate bit masking and logical ||. The intent is clearer to me. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:573: GetOutputLinkMask(link_mask); nit: 4-space indent https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.h:72: ProtectionRequests; You might want IDMap instead. That would get rid of the weird-looking usage of |this| in the host destructor (which I think I understand now that I've gotten this far). https://codereview.chromium.org/24039002/diff/8001/ppapi/cpp/private/output_p... File ppapi/cpp/private/output_protection_private.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/cpp/private/output_p... ppapi/cpp/private/output_protection_private.cc:9: #include "ppapi/c/ppb_var.h" ^^ You don't appear to use this https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:23: : PluginResource(connection, instance) { please initialize link_mask_ and protection_mask_ https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:46: return PP_ERROR_INPROGRESS; Suggestion: You could instead use Bind to bind |link_mask|, |protection_mask|, and |callback| below when you bind OnPluginMsgQueryStatusReply. Then, those don't have to be members at all, and you can support multiple in-flight queries. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:67: if (link_mask_ == NULL || protection_mask_ == NULL) NOTREACHED()? https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.h:23: virtual ~OutputProtectionResource(); nit: the destructor and below can be private. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/ppapi_messages... ppapi/proxy/ppapi_messages.h:1758: uint32_t /* link_mas */, mas->mask https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/resource_creat... File ppapi/proxy/resource_creation_proxy.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/resource_creat... ppapi/proxy/resource_creation_proxy.cc:313: ->GetReference(); nit: "->" should go on the end of 312 https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb... File ppapi/thunk/interfaces_ppb_private_no_permissions.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb... ppapi/thunk/interfaces_ppb_private_no_permissions.h:57: PPB_OutputProtection_Private_0_1) See the comment on 10 and 11... is that how we want this to work? That this is only available for whitelisted origins/plugins? I think you probably want this in interfaces_ppb_private.h. Otherwise the check you have in the host-side factory is inconsistent. (That, or the host-side factory should check that the plugin is in a whitelist, instead of looking at PRIVATE permissions.)
https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:40: base::Unretained(this), On 2013/09/09 07:58:16, kcwu wrote: > On 2013/09/07 01:14:38, DaleCurtis wrote: > > Can't post tasks to "this" during destruction -- the object is already in the > > process of being destroyed. > > Done. > post tasks to the underlying call. I don't think this is fixed? You can't post task to "this" at all in the destructor.
On Mon, Sep 9, 2013 at 2:53 PM, <dalecurtis@chromium.org> wrote: > > https://codereview.chromium.**org/24039002/diff/1/chrome/** > browser/renderer_host/pepper/**pepper_output_protection_host.**cc<https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc> > File > chrome/browser/renderer_host/**pepper/pepper_output_**protection_host.cc > (right): > > https://codereview.chromium.**org/24039002/diff/1/chrome/** > browser/renderer_host/pepper/**pepper_output_protection_host.** > cc#newcode40<https://codereview.chromium.org/24039002/diff/1/chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc#newcode40> > chrome/browser/renderer_host/**pepper/pepper_output_** > protection_host.cc:40: > base::Unretained(this), > On 2013/09/09 07:58:16, kcwu wrote: > >> On 2013/09/07 01:14:38, DaleCurtis wrote: >> > Can't post tasks to "this" during destruction -- the object is >> > already in the > >> > process of being destroyed. >> > > Done. >> post tasks to the underlying call. >> > > I don't think this is fixed? You can't post task to "this" at all in the > destructor. I noticed the same... it's actually a parameter now, and is only used as a key in a map (not dereferenced). So it's actually safe in practice... for now. But it's fishy, and prone to future errors, so I agree it should be something else (I suggested using IDMap instead of treating the pointer as a key). > > > https://codereview.chromium.**org/24039002/<https://codereview.chromium.org/2... > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@**chromium.org<chromium-reviews%2Bunsubscribe@ch... > . > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:77: #if defined(USE_ASH) On 2013/09/09 17:30:24, cpu wrote: > I don't see windows implementations so this is not enough to block windows from > reaching here. Do you mean ASH is available on Windows? I think ASH is chromeos only. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:77: #if defined(USE_ASH) On 2013/09/09 20:37:15, dmichael wrote: > Why USE_ASH? Should this be OS_CHROMEOS? (And I guess this implies that this API > is ChromeOS-only? I would have guessed it could be supported on Windows at least > and probably also Mac.) Yes, this API is ChromeOS-only. This API is designed in current simple form because it is running in secure environment, i.e. ChromeOS. To support other open platforms, the API must be extended to complex API/protocol for authentication, verification, etc. which is not covered in this CL. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:35: #if defined(USE_ASH) && defined(OS_CHROMEOS) On 2013/09/09 20:37:15, dmichael wrote: > Why not use gyp or ifdef out the whole file? (and also the .h contents) If conditional build the whole file, this means I have to use ifdef OS_CHROMEOS in ppapi/proxy/output_protection_resource.cc to return failure there, right? https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:44: base::Unretained(this), On 2013/09/09 20:37:15, dmichael wrote: > I don't know what EnableOutputProtection actually does, but it seems dangerous > to pass "this" from the destructor to PostTask... I just pass the pointer "value" as client id and never deference it. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:69: ash::Shell::GetInstance()->output_configurator(); On 2013/09/09 20:37:15, dmichael wrote: > can this ever be NULL? output_configurator will not be NULL if shell instance is alive. Regarding to shell instance, I'm not sure, all caller don't check NULL as I know. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:85: On 2013/09/09 20:37:15, dmichael wrote: > nit: excess whitespace Done. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:92: On 2013/09/09 20:37:15, dmichael wrote: > nit: excess whitespace Done. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:116: PepperOutputProtectionHost::OnEnableProtection( On 2013/09/09 20:37:15, dmichael wrote: > nit: it looks like this line should fit with the return type Done. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:119: On 2013/09/09 20:37:15, dmichael wrote: > nit: excess whitespace in this function body too. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... chromeos/display/output_configurator.cc:247: delegate_->UngrabServer(); On 2013/09/09 20:37:15, dmichael wrote: > I don't know anything about that delegate or what GrabServer/UngrabServer does, > but it would be nice if that delegate defined a scoped sentinel to make sure > UngrabServer is called when exiting scope. To prevent mistakes if functions can > return early. > > I.e. something you could use like this: > { > ScopedServerGrabber grab_server(delegate_); > // Do something with delegate_ > } // UngrabServer gets called automatically. Agree. However there are 4 existing GrabServer() in this file works this way. Do you suggest me refactor them or just follow them? https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... chromeos/display/output_configurator.h:374: chrome::PepperOutputProtectionHost* client, On 2013/09/09 20:37:15, dmichael wrote: > should this be const? Same question for |client| in EnableOutputProtection.. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:387: int success; On 2013/09/09 20:37:15, dmichael wrote: > please initialize these (same for other variables below in your patch) Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:429: value = XInternAtom(display_, "Undesired", False); On 2013/09/09 20:37:15, dmichael wrote: > It looks like this function returns type "Atom"... why are you using unsigned > long? Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:438: unsigned char *data = (unsigned char*)&value; On 2013/09/09 20:37:15, dmichael wrote: > Please do not use C-style casts. I think you want reinterpret_cast. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:438: unsigned char *data = (unsigned char*)&value; On 2013/09/09 20:37:15, dmichael wrote: > Please do not use C-style casts. I think you want reinterpret_cast. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:444: bool DetermineLinkType(const XRROutputInfo* output_info, On 2013/09/09 20:37:15, dmichael wrote: > const-reference? Existing code, IsInternalOutput() takes const-pointer. Should I use const-reference? https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:462: bool RealOutputConfiguratorDelegate::GetOutputLinkType( On 2013/09/09 20:37:15, dmichael wrote: > Type->Types? It looks like you can retrieve more than one? Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:463: std::vector<PP_OutputProtectionLinkType_Private>& link_type) { On 2013/09/09 20:37:15, dmichael wrote: > out-params should be by pointer: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu... Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:468: XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_, this_id); On 2013/09/09 20:37:15, dmichael wrote: > Can this return NULL? It looks like it can: > http://sourcecodebrowser.com/libxrandr/1.2.2/_xrr_output_8c.html#a41ed0191cd9... Done. BTW, this is copied from existing code, line 121. It didn't check NULL. I will address this in another CL. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:470: XRRFreeOutputInfo(output_info); On 2013/09/09 20:37:15, dmichael wrote: > optional suggestion: You could use scoped_ptr<XRROutputInfo, > XRROutputInfoDeleter> > > Where XRROutputInfoDeleter looks like this: > struct XRROutputInfoDeleter { > inline void operator()(void* ptr) const { > XRRFreeOutputInfo(static_cast<XRRFreeOutputInfo*>(ptr)); > } > }; > > If you end up with more than 1 or 2 places you have to call XRRFreeOutputInfo in > any function, it's probably a good idea to use the RAII idiom (a scoper that > cleans up after you). After the overhead of defining the Deleter functor, it > should shorten your code and make it harder to forget to free. > > That said... in this case, it's probably simpler to just do my next > suggestion... Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:472: continue; On 2013/09/09 20:37:15, dmichael wrote: > Why not just use an else? And I'd maybe flip the cases. I.e: > if (output_info->connection == RR_COnnected) { > // stuff form 475-480 > } else { > // stuff from 470-471 > } > > This also means you can do XRRFreeOutputInfo in one place, after the else. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:508: DCHECK(screen_->noutput == (int)link_type.size()); On 2013/09/09 20:37:15, dmichael wrote: > no C-style casts > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:512: PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) { On 2013/09/09 20:37:15, dmichael wrote: > optional nit: I would find it more readable as: > if (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI) || > (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) { > Maybe that's just me. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:523: *protection_mask = enabled & ~unfulfiled; On 2013/09/09 20:37:15, dmichael wrote: > I don't really understand the APIs we're using... but this reads to me that if > there are 2 or more HDMI/DISPLAYPORTs, but any one of them is not HDCP-enabled, > that GetProtectionMethods will indicate that there is no HDCP support. Is that > what we want? Yes, content owner would like to playback their content only if protections are enabled on all links. That bit means "fully enabled" on all applicable output links. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:547: if (!GetOutputLinkType(link_type)) On 2013/09/09 20:37:15, dmichael wrote: > You do this a lot. Might it make sense to have something like a member that gets > populated during GrabScreen()? Or is it just not used everywhere GrabScreen is? Right. It is not used otherwhere GrabServer is. Anyway, I moved GetOutputLinkType() calls and reduced one. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:554: PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) { On 2013/09/09 20:37:15, dmichael wrote: > ditto, I somewhat prefer separate bit masking and logical ||. The intent is > clearer to me. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:573: GetOutputLinkMask(link_mask); On 2013/09/09 20:37:15, dmichael wrote: > nit: 4-space indent Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.h:72: ProtectionRequests; On 2013/09/09 20:37:15, dmichael wrote: > You might want IDMap instead. That would get rid of the weird-looking usage of > |this| in the host destructor (which I think I understand now that I've gotten > this far). IDMap looks good. What do you suggest how to use IDMap? How about: - The delegate holds two table, one is IDMap (pointer -> id) and the other is (id -> request). - Pepper hosts have to register to delegate first and get the id. - Pepper hosts call QueryStatus(), Enable(), and Unregister in dtor with id. https://codereview.chromium.org/24039002/diff/8001/ppapi/cpp/private/output_p... File ppapi/cpp/private/output_protection_private.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/cpp/private/output_p... ppapi/cpp/private/output_protection_private.cc:9: #include "ppapi/c/ppb_var.h" On 2013/09/09 20:37:15, dmichael wrote: > ^^ You don't appear to use this Done. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:23: : PluginResource(connection, instance) { On 2013/09/09 20:37:15, dmichael wrote: > please initialize link_mask_ and protection_mask_ Done. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:46: return PP_ERROR_INPROGRESS; On 2013/09/09 20:37:15, dmichael wrote: > Suggestion: > You could instead use Bind to bind |link_mask|, |protection_mask|, and > |callback| below when you bind OnPluginMsgQueryStatusReply. Then, those don't > have to be members at all, and you can support multiple in-flight queries. If so, do I still need to check IsPending() at the beginning of the function since the callback will not be aborted as current dtor? https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:67: if (link_mask_ == NULL || protection_mask_ == NULL) On 2013/09/09 20:37:15, dmichael wrote: > NOTREACHED()? Done. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.h:23: virtual ~OutputProtectionResource(); On 2013/09/09 20:37:15, dmichael wrote: > nit: the destructor and below can be private. Done. It compiles but I don't understand. This class want to implement PluginResource and PPB_OutputProtection_API's public methods. Why make them private? https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/ppapi_messages... ppapi/proxy/ppapi_messages.h:1758: uint32_t /* link_mas */, On 2013/09/09 20:37:15, dmichael wrote: > mas->mask Done. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/resource_creat... File ppapi/proxy/resource_creation_proxy.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/resource_creat... ppapi/proxy/resource_creation_proxy.cc:313: ->GetReference(); On 2013/09/09 20:37:15, dmichael wrote: > nit: "->" should go on the end of 312 Done. https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb... File ppapi/thunk/interfaces_ppb_private_no_permissions.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb... ppapi/thunk/interfaces_ppb_private_no_permissions.h:57: PPB_OutputProtection_Private_0_1) On 2013/09/09 20:37:15, dmichael wrote: > See the comment on 10 and 11... is that how we want this to work? That this is > only available for whitelisted origins/plugins? > > I think you probably want this in interfaces_ppb_private.h. Otherwise the check > you have in the host-side factory is inconsistent. (That, or the host-side > factory should check that the plugin is in a whitelist, instead of looking at > PRIVATE permissions.) Good question. I'm not sure how CDM will use this api. dalecurtis@, could you give comment on this?
+cdn@ for ppapi/proxy/ppapi_messages.h
>>Do you mean ASH is available on Windows? I think ASH is >> chromeos only. Nope. http://www.theverge.com/2013/9/6/4701504/chrome-os-in-windows-8-app
IPC message changes LGTM
I tweaked the conditional compile conditions. New behavior is:
if OS_CHROMEOS:
if USE_ASH && USE_X11:
if xrandr supports content protection:
it works
else:
callback(PP_ERROR_FAILED)
else:
// This is unexpected combination.
NOTIMPLEMENTED();
callback(PP_ERROR_NOTSUPPORTED)
else:
// failed to create pepper host. plugin will receive
callback(PP_ERROR_BADRESOURCE)
https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc
(right):
https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:77:
#if defined(USE_ASH)
On 2013/09/09 20:37:15, dmichael wrote:
> Why USE_ASH? Should this be OS_CHROMEOS? (And I guess this implies that this
API
> is ChromeOS-only? I would have guessed it could be supported on Windows at
least
> and probably also Mac.)
Done.
https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:35: #if defined(USE_ASH) && defined(OS_CHROMEOS) On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > Why not use gyp or ifdef out the whole file? (and also the .h contents) > > If conditional build the whole file, this means I have to use ifdef OS_CHROMEOS > in ppapi/proxy/output_protection_resource.cc to return failure there, right? I guess I was thinking that it would be ideal if the interface wasn't even visible to the plugin if it's on a platform where the API is unsupported. In that world, you would ifdef out the whole ppapi/proxy "Resource" too. But maybe that's too hard to pull off with the way we have resources defined. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... chromeos/display/output_configurator.cc:247: delegate_->UngrabServer(); On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > I don't know anything about that delegate or what GrabServer/UngrabServer > does, > > but it would be nice if that delegate defined a scoped sentinel to make sure > > UngrabServer is called when exiting scope. To prevent mistakes if functions > can > > return early. > > > > I.e. something you could use like this: > > { > > ScopedServerGrabber grab_server(delegate_); > > // Do something with delegate_ > > } // UngrabServer gets called automatically. > > Agree. However there are 4 existing GrabServer() in this file works this way. Do > you suggest me refactor them or just follow them? Following them is fine. But it's worth considering a follow-up CL. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:444: bool DetermineLinkType(const XRROutputInfo* output_info, On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > const-reference? > > Existing code, IsInternalOutput() takes const-pointer. Should I use > const-reference? Yes. It looks like IsInternalOutput assumes that the pointer is valid, so const& makes the most sense there too. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:508: DCHECK(screen_->noutput == (int)link_type.size()); On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > no C-style casts > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... > > Done. nit: It's usually better to static_cast the type that has smaller range. I.e., static_cast<size_t>(screen_->noutput) instead. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.h:72: ProtectionRequests; On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > You might want IDMap instead. That would get rid of the weird-looking usage of > > |this| in the host destructor (which I think I understand now that I've gotten > > this far). > > IDMap looks good. What do you suggest how to use IDMap? > How about: > - The delegate holds two table, one is IDMap (pointer -> id) and the other is > (id -> request). > - Pepper hosts have to register to delegate first and get the id. > - Pepper hosts call QueryStatus(), Enable(), and Unregister in dtor with id. I was suggesting using IDMap here, and you can have an initial "RegisterClient" method that adds an entry to the map and returns the client ID. EnableOutputProtection and EnableOutputProtection would use the same ID instead of a raw pointer. It's not functionally any better, but probably safer in the long run. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:46: return PP_ERROR_INPROGRESS; On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > Suggestion: > > You could instead use Bind to bind |link_mask|, |protection_mask|, and > > |callback| below when you bind OnPluginMsgQueryStatusReply. Then, those don't > > have to be members at all, and you can support multiple in-flight queries. > > If so, do I still need to check IsPending() at the beginning of the function > since the callback will not be aborted as current dtor? Yes, it may still be aborted via the CallbackTracker. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.h:23: virtual ~OutputProtectionResource(); On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > nit: the destructor and below can be private. > > Done. > It compiles but I don't understand. > This class want to implement PluginResource and PPB_OutputProtection_API's > public methods. Why make them private? Not everyone in Chromium likes to do it this way, but it's come up a couple of times in chromium-dev. E.g.: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl... Basically, these are methods (including the destructor) that we expect by-design to only be called through the interface. If somebody tries to call them _directly_, I want the compiler to yell at them so they stop and think about whether they're doing the right thing. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:473: if (output_info->connection == RR_Connected) { you could use "else if" instead of break. I find breaks a little harder to reason about.
https://codereview.chromium.org/24039002/diff/12001/ppapi/cpp/private/output_... File ppapi/cpp/private/output_protection_private.h (right): https://codereview.chromium.org/24039002/diff/12001/ppapi/cpp/private/output_... ppapi/cpp/private/output_protection_private.h:23: const CompletionCallback& callback); This is a comment about the documentation in the IDL, which has already been committed. This can be addressed in separate CL, but I wanted to record this. Currently, the comment says: * @param[in] callback A <code>PP_CompletionCallback</code> to be called when * the protection request has been made. This may be before the protection * have actually been applied. Call QueryStatus to get protection status. The above text assumes a successful request, in which case the status will be PP_SUCCESS. We should also cover the failure case. If the callback is called with PP_FAILED, there is no need to call QueryStatus().
https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:79: return scoped_ptr<ResourceHost>(new PepperOutputProtectionHost( While reviewing your code, I'm beginning to think that given the work both of our APIs are doing on the UI thread, maybe we should be using a MessageFilterHost w/ the UI thread instead of ResourceHost with hopping. MessageFilterHost allows us to set OverrideTaskRunnerForMessage() to the UI thread. Which would mean we can get rid of the paired BlahBlah and BlahBlahOnUIThread methods. See PepperBrokerMessageFilter for an example of this. dmichael, WDYT? https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:36: chromeos::OutputConfigurator* configurator = Is it safe to call this from any thread? I don't see any DCHECKs in ash::Shell::GetInstance(), but I'm not familiar with that code. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:44: base::Unretained(this), A comment indicating that this is only used as a key and not dereferenced should be added. Otherwise this looks like a potential for use-after-free. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:80: host()->SendReply(reply_context, invalid wrapping. reply_context must go on the next line per style guide. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:111: host()->SendReply(reply_context, Wrapping. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.h (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.h:4: Extra space. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.h:42: content::BrowserPpapiHost* renderer_ppapi_host_; Unused? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:228: const chrome::PepperOutputProtectionHost* client, Why not just use a void* if you don't intend to access the class. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:376: uint32_t* protection_mask); OVERRIDE? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:384: uint32_t desired_method_mask); Ditto? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:381: CHECK(screen_) << "Server not grabbed"; This will crash the browser process. Do you want to instead return false / dcheck? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:435: DCHECK(0) << "Invalid HDCP state: " << state; NOTREACHED() ? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:468: XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_, this_id); How expensive is this query? I ask since it'll be queried pretty frequently on the UI thread. As an example, we see delays in the hundreds of milliseconds when querying for webcams, audio devices, etc. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:478: result = false; Do you want to break here? Should link_types be cleared on failure? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:506: DCHECK(screen_->noutput == static_cast<int>(link_types.size())); DCHECK_EQ ?
High-level review of chrome/ and chromeos/ https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:35: #if defined(OS_CHROMEOS) && defined(USE_ASH) && defined(USE_X11) What is the purpose of ASH and X11? Is there a better value we could define? At least in this file since it appears multiple times. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:44: base::Unretained(this), Passing |this| in the destructor seems dangerous. Can we pass NULL if we assume this will be gone? https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:47: NOTIMPLEMENTED(); Wouldn't it be better to indicate this when the object is created too? https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:72: uint32_t link_mask = 0, protection_mask = 0; These need to be defined outside the #if block or you'll get a compile error at 82. Should these be defined on separate lines? https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.h (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.h:39: void Close(); Not implemented. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:232: // Set desired protection methods. Returns true when the protection request s/Set/Request the/ https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:368: // |link_mask| is the type of connected output links, which is a bit-mask nit: "bit mask" is 5x more common in the Chromium code and "bitmask" is even more common, though that includes variable names. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:33: // Prefixes of putput name output https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:487: bool RealOutputConfiguratorDelegate::GetOutputLinkMask( This is really a "Convert" function. Also, it could be a local function instead of class member. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:499: bool RealOutputConfiguratorDelegate::GetProtectionMethods( This function could easily return the link_mask, simplifying callers. (If you don't want to always populate the mask, you could handle a NULL output parameter. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:546: if (!GetOutputLinkTypes(&link_types)) It's odd that we iterate through the outputs to get this vector then iterate through the outputs again. (Also, in the meantime, the values of outputs could change.) It might be better to extract 467-482 into a function that gets the link type for a single output. That could be called for each iteration of 549. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:561: if (!ok) "&& ok" could just be a condition in the for loop then you'd return ok below. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:577: ProtectionRequests::iterator it = client_protection_request_.find(client); This should be with the code below (separated by an empty line). https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:581: *protection_mask &= it->second; It might help to comment or assign |second| to a const ref. I assume this is the requested_mask. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:583: *protection_mask = 0; Should this be a NOTREACHED() or can it actually happen? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:71: typedef std::map<const chrome::PepperOutputProtectionHost*, uint32_t> Maybe explain the two values. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:142: bool GetProtectionMethods( While the above method might be useful on its own (though probably currently unused), it seems this is only useful when returning both outputs. Is there a reason these are separate? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:145: // Get the bit-mask of all requested protection methods. I didn't understand what this function did (from the comment or name) until I looked at the implementation. It should somehow indicate that it's the OR of all clients. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:158: ProtectionRequests client_protection_request_; ...requests_ ^
simple changes to gypi files don't require a review from an OWNER
https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:79: return scoped_ptr<ResourceHost>(new PepperOutputProtectionHost( On 2013/09/12 01:23:13, DaleCurtis wrote: > While reviewing your code, I'm beginning to think that given the work both of > our APIs are doing on the UI thread, maybe we should be using a > MessageFilterHost w/ the UI thread instead of ResourceHost with hopping. > > MessageFilterHost allows us to set OverrideTaskRunnerForMessage() to the UI > thread. Which would mean we can get rid of the paired BlahBlah and > BlahBlahOnUIThread methods. See PepperBrokerMessageFilter for an example of > this. > > dmichael, WDYT? Good point, if you don't have to do work on the IO thread before posting to the UI thread, that's probably a better approach.
Thanks for your review. Most of comments are done. Two remain issues will be addressed tomorrow. https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/8001/chrome/browser/renderer_ho... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:35: #if defined(USE_ASH) && defined(OS_CHROMEOS) On 2013/09/11 18:22:45, dmichael wrote: > On 2013/09/10 12:50:21, kcwu wrote: > > On 2013/09/09 20:37:15, dmichael wrote: > > > Why not use gyp or ifdef out the whole file? (and also the .h contents) > > > > If conditional build the whole file, this means I have to use ifdef > OS_CHROMEOS > > in ppapi/proxy/output_protection_resource.cc to return failure there, right? > I guess I was thinking that it would be ideal if the interface wasn't even > visible to the plugin if it's on a platform where the API is unsupported. In > that world, you would ifdef out the whole ppapi/proxy "Resource" too. > > But maybe that's too hard to pull off with the way we have resources defined. IIUC how pepper works, in order to make it true invisible, some generated code, i.e. pnacl_shim.c, are involved. I would prefer the simpler way. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/output_co... chromeos/display/output_configurator.cc:247: delegate_->UngrabServer(); On 2013/09/11 18:22:45, dmichael wrote: > On 2013/09/10 12:50:21, kcwu wrote: > > On 2013/09/09 20:37:15, dmichael wrote: > > > I don't know anything about that delegate or what GrabServer/UngrabServer > > does, > > > but it would be nice if that delegate defined a scoped sentinel to make sure > > > UngrabServer is called when exiting scope. To prevent mistakes if functions > > can > > > return early. > > > > > > I.e. something you could use like this: > > > { > > > ScopedServerGrabber grab_server(delegate_); > > > // Do something with delegate_ > > > } // UngrabServer gets called automatically. > > > > Agree. However there are 4 existing GrabServer() in this file works this way. > Do > > you suggest me refactor them or just follow them? > Following them is fine. But it's worth considering a follow-up CL. Thanks https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:444: bool DetermineLinkType(const XRROutputInfo* output_info, On 2013/09/11 18:22:45, dmichael wrote: > On 2013/09/10 12:50:21, kcwu wrote: > > On 2013/09/09 20:37:15, dmichael wrote: > > > const-reference? > > > > Existing code, IsInternalOutput() takes const-pointer. Should I use > > const-reference? > Yes. It looks like IsInternalOutput assumes that the pointer is valid, so const& > makes the most sense there too. Done. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.cc:508: DCHECK(screen_->noutput == (int)link_type.size()); On 2013/09/11 18:22:45, dmichael wrote: > On 2013/09/10 12:50:21, kcwu wrote: > > On 2013/09/09 20:37:15, dmichael wrote: > > > no C-style casts > > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... > > > > Done. > nit: It's usually better to static_cast the type that has smaller range. I.e., > static_cast<size_t>(screen_->noutput) instead. Use DCHECK_EQ() instead as Dale suggested. https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/8001/chromeos/display/real_outp... chromeos/display/real_output_configurator_delegate.h:72: ProtectionRequests; On 2013/09/11 18:22:45, dmichael wrote: > On 2013/09/10 12:50:21, kcwu wrote: > > On 2013/09/09 20:37:15, dmichael wrote: > > > You might want IDMap instead. That would get rid of the weird-looking usage > of > > > |this| in the host destructor (which I think I understand now that I've > gotten > > > this far). > > > > IDMap looks good. What do you suggest how to use IDMap? > > How about: > > - The delegate holds two table, one is IDMap (pointer -> id) and the other is > > (id -> request). > > - Pepper hosts have to register to delegate first and get the id. > > - Pepper hosts call QueryStatus(), Enable(), and Unregister in dtor with id. > > I was suggesting using IDMap here, and you can have an initial "RegisterClient" > method that adds an entry to the map and returns the client ID. > EnableOutputProtection and EnableOutputProtection would use the same ID instead > of a raw pointer. > > It's not functionally any better, but probably safer in the long run. How about void* as Dale suggested? I would modify it to use IDMap tomorrow. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.cc (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.cc:46: return PP_ERROR_INPROGRESS; On 2013/09/11 18:22:45, dmichael wrote: > On 2013/09/10 12:50:21, kcwu wrote: > > On 2013/09/09 20:37:15, dmichael wrote: > > > Suggestion: > > > You could instead use Bind to bind |link_mask|, |protection_mask|, and > > > |callback| below when you bind OnPluginMsgQueryStatusReply. Then, those > don't > > > have to be members at all, and you can support multiple in-flight queries. > > > > If so, do I still need to check IsPending() at the beginning of the function > > since the callback will not be aborted as current dtor? > Yes, it may still be aborted via the CallbackTracker. I will address this tomorrow. https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... File ppapi/proxy/output_protection_resource.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/proxy/output_protect... ppapi/proxy/output_protection_resource.h:23: virtual ~OutputProtectionResource(); On 2013/09/11 18:22:45, dmichael wrote: > On 2013/09/10 12:50:21, kcwu wrote: > > On 2013/09/09 20:37:15, dmichael wrote: > > > nit: the destructor and below can be private. > > > > Done. > > It compiles but I don't understand. > > This class want to implement PluginResource and PPB_OutputProtection_API's > > public methods. Why make them private? > Not everyone in Chromium likes to do it this way, but it's come up a couple of > times in chromium-dev. E.g.: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl... > Basically, these are methods (including the destructor) that we expect by-design > to only be called through the interface. If somebody tries to call them > _directly_, I want the compiler to yell at them so they stop and think about > whether they're doing the right thing. Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc:79: return scoped_ptr<ResourceHost>(new PepperOutputProtectionHost( On 2013/09/12 16:49:10, dmichael wrote: > On 2013/09/12 01:23:13, DaleCurtis wrote: > > While reviewing your code, I'm beginning to think that given the work both of > > our APIs are doing on the UI thread, maybe we should be using a > > MessageFilterHost w/ the UI thread instead of ResourceHost with hopping. > > > > MessageFilterHost allows us to set OverrideTaskRunnerForMessage() to the UI > > thread. Which would mean we can get rid of the paired BlahBlah and > > BlahBlahOnUIThread methods. See PepperBrokerMessageFilter for an example of > > this. > > > > dmichael, WDYT? > Good point, if you don't have to do work on the IO thread before posting to the > UI thread, that's probably a better approach. Done. However, I still need to post task in dtor since the dtor is not invoked in UI thread. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:35: #if defined(OS_CHROMEOS) && defined(USE_ASH) && defined(USE_X11) On 2013/09/12 04:22:13, ddorwin wrote: > What is the purpose of ASH and X11? Is there a better value we could define? At > least in this file since it appears multiple times. USE_X11: Output protection implementation requires xrandr USE_ASH: output_configurator() is only available if defined(USE_ASH) https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:44: base::Unretained(this), On 2013/09/12 04:22:13, ddorwin wrote: > Passing |this| in the destructor seems dangerous. Can we pass NULL if we assume > this will be gone? No. That will be a small memory leak. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:44: base::Unretained(this), On 2013/09/12 01:23:13, DaleCurtis wrote: > A comment indicating that this is only used as a key and not dereferenced should > be added. Otherwise this looks like a potential for use-after-free. Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:47: NOTIMPLEMENTED(); On 2013/09/12 04:22:13, ddorwin wrote: > Wouldn't it be better to indicate this when the object is created too? Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:72: uint32_t link_mask = 0, protection_mask = 0; On 2013/09/12 04:22:13, ddorwin wrote: > These need to be defined outside the #if block or you'll get a compile error at > 82. > > Should these be defined on separate lines? Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:80: host()->SendReply(reply_context, On 2013/09/12 01:23:13, DaleCurtis wrote: > invalid wrapping. reply_context must go on the next line per style guide. Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:111: host()->SendReply(reply_context, On 2013/09/12 01:23:13, DaleCurtis wrote: > Wrapping. Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.h (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.h:4: On 2013/09/12 01:23:13, DaleCurtis wrote: > Extra space. Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.h:39: void Close(); On 2013/09/12 04:22:13, ddorwin wrote: > Not implemented. Done. https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.h:42: content::BrowserPpapiHost* renderer_ppapi_host_; On 2013/09/12 01:23:13, DaleCurtis wrote: > Unused? Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:228: const chrome::PepperOutputProtectionHost* client, On 2013/09/12 01:23:13, DaleCurtis wrote: > Why not just use a void* if you don't intend to access the class. Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:232: // Set desired protection methods. Returns true when the protection request On 2013/09/12 04:22:13, ddorwin wrote: > s/Set/Request the/ Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:368: // |link_mask| is the type of connected output links, which is a bit-mask On 2013/09/12 04:22:13, ddorwin wrote: > nit: "bit mask" is 5x more common in the Chromium code and "bitmask" is even > more common, though that includes variable names. Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:376: uint32_t* protection_mask); On 2013/09/12 01:23:13, DaleCurtis wrote: > OVERRIDE? Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:384: uint32_t desired_method_mask); On 2013/09/12 01:23:13, DaleCurtis wrote: > Ditto? Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:33: // Prefixes of putput name On 2013/09/12 04:22:13, ddorwin wrote: > output Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:381: CHECK(screen_) << "Server not grabbed"; On 2013/09/12 01:23:13, DaleCurtis wrote: > This will crash the browser process. Do you want to instead return false / > dcheck? All checks of screen_ in this file are using CHECK(). I just follow them. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:435: DCHECK(0) << "Invalid HDCP state: " << state; On 2013/09/12 01:23:13, DaleCurtis wrote: > NOTREACHED() ? Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:468: XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_, this_id); On 2013/09/12 01:23:13, DaleCurtis wrote: > How expensive is this query? I ask since it'll be queried pretty frequently on > the UI thread. > > As an example, we see delays in the hundreds of milliseconds when querying for > webcams, audio devices, etc. This query should be very fast. I can measure the number tomorrow. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:473: if (output_info->connection == RR_Connected) { On 2013/09/11 18:22:45, dmichael wrote: > you could use "else if" instead of break. I find breaks a little harder to > reason about. These logic are refactored into separate function, QueryOutputProtectionLinkType(). https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:478: result = false; On 2013/09/12 01:23:13, DaleCurtis wrote: > Do you want to break here? Should link_types be cleared on failure? These logic are refactored into separate function, QueryOutputProtectionLinkType(). I didn't define the value of link_types for the case of failure. Should I? https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:487: bool RealOutputConfiguratorDelegate::GetOutputLinkMask( On 2013/09/12 04:22:13, ddorwin wrote: > This is really a "Convert" function. Also, it could be a local function instead > of class member. These methods are restructured. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:499: bool RealOutputConfiguratorDelegate::GetProtectionMethods( On 2013/09/12 04:22:13, ddorwin wrote: > This function could easily return the link_mask, simplifying callers. (If you > don't want to always populate the mask, you could handle a NULL output > parameter. Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:506: DCHECK(screen_->noutput == static_cast<int>(link_types.size())); On 2013/09/12 01:23:13, DaleCurtis wrote: > DCHECK_EQ ? Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:546: if (!GetOutputLinkTypes(&link_types)) On 2013/09/12 04:22:13, ddorwin wrote: > It's odd that we iterate through the outputs to get this vector then iterate > through the outputs again. > (Also, in the meantime, the values of outputs could change.) > > It might be better to extract 467-482 into a function that gets the link type > for a single output. That could be called for each iteration of 549. Done. Per this suggestion. These methods are refactored. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:561: if (!ok) On 2013/09/12 04:22:13, ddorwin wrote: > "&& ok" could just be a condition in the for loop then you'd return ok below. Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:577: ProtectionRequests::iterator it = client_protection_request_.find(client); On 2013/09/12 04:22:13, ddorwin wrote: > This should be with the code below (separated by an empty line). Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:581: *protection_mask &= it->second; On 2013/09/12 04:22:13, ddorwin wrote: > It might help to comment or assign |second| to a const ref. I assume this is the > requested_mask. Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:583: *protection_mask = 0; On 2013/09/12 04:22:13, ddorwin wrote: > Should this be a NOTREACHED() or can it actually happen? This happens if the client Enable(none) and QueryStatus(). https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:71: typedef std::map<const chrome::PepperOutputProtectionHost*, uint32_t> On 2013/09/12 04:22:13, ddorwin wrote: > Maybe explain the two values. Done. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:142: bool GetProtectionMethods( On 2013/09/12 04:22:13, ddorwin wrote: > While the above method might be useful on its own (though probably currently > unused), it seems this is only useful when returning both outputs. Is there a > reason these are separate? These methods are rewritten and restructured. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:145: // Get the bit-mask of all requested protection methods. On 2013/09/12 04:22:13, ddorwin wrote: > I didn't understand what this function did (from the comment or name) until I > looked at the implementation. It should somehow indicate that it's the OR of all > clients. These methods are rewritten. https://codereview.chromium.org/24039002/diff/12001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:158: ProtectionRequests client_protection_request_; On 2013/09/12 04:22:13, ddorwin wrote: > ...requests_ > ^ Done. https://codereview.chromium.org/24039002/diff/12001/ppapi/cpp/private/output_... File ppapi/cpp/private/output_protection_private.h (right): https://codereview.chromium.org/24039002/diff/12001/ppapi/cpp/private/output_... ppapi/cpp/private/output_protection_private.h:23: const CompletionCallback& callback); On 2013/09/11 20:43:01, ddorwin wrote: > This is a comment about the documentation in the IDL, which has already been > committed. This can be addressed in separate CL, but I wanted to record this. > > Currently, the comment says: > * @param[in] callback A <code>PP_CompletionCallback</code> to be called when > * the protection request has been made. This may be before the protection > * have actually been applied. Call QueryStatus to get protection status. > > The above text assumes a successful request, in which case the status will be > PP_SUCCESS. We should also cover the failure case. If the callback is called > with PP_FAILED, there is no need to call QueryStatus(). Thanks.
https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb... File ppapi/thunk/interfaces_ppb_private_no_permissions.h (right): https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb... ppapi/thunk/interfaces_ppb_private_no_permissions.h:57: PPB_OutputProtection_Private_0_1) On 2013/09/10 12:50:21, kcwu wrote: > On 2013/09/09 20:37:15, dmichael wrote: > > See the comment on 10 and 11... is that how we want this to work? That this > is > > only available for whitelisted origins/plugins? > > > > I think you probably want this in interfaces_ppb_private.h. Otherwise the > check > > you have in the host-side factory is inconsistent. (That, or the host-side > > factory should check that the plugin is in a whitelist, instead of looking at > > PRIVATE permissions.) > > Good question. I'm not sure how CDM will use this api. > dalecurtis@, could you give comment on this? Per ddorwin@ the CDM has private permissions, so it should go under private.
https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c... chromeos/display/output_configurator.h:228: const chrome::PepperOutputProtectionHost* client, On 2013/09/12 18:22:08, kcwu wrote: > On 2013/09/12 01:23:13, DaleCurtis wrote: > > Why not just use a void* if you don't intend to access the class. > > Done. This is better, but my preference is still for an IDMap (or similar). If what you want is a unique ID, I think your intent is clearer if you use a unique ID rather than treat a pointer as one. Also, there's a slight chance the allocator could reuse that pointer for a new client before the task runs, which would make the "new" client disappear.
Can you add a test under ppapi/tests ? Even if you don't check the values, it's nice to have the plumbing testing. https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc (right): https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:115: host_->SendReply( SendReply() works w/o host_-> so you can stop storing host_. https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h (right): https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:26: protected: Can all be private? https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:42: ppapi::host::PpapiHost* host_; Not necessary. https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:435: DCHECK(0) << "Invalid HDCP state: " << state; NOTREACHED() << "blah blah" https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:449: if (output_info == NULL) { !output_info https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:465: DCHECK(0) << "Unknown link type: " << name; NOTREACHED() << "blah blah" https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:517: uint32_t unfulfiled = 0; unfulfilled. https://codereview.chromium.org/24039002/diff/27029/ppapi/proxy/output_protec... File ppapi/proxy/output_protection_resource.h (right): https://codereview.chromium.org/24039002/diff/27029/ppapi/proxy/output_protec... ppapi/proxy/output_protection_resource.h:45: uint32_t* link_mask_; Instead of storing these, you can just bind them into the reply function; up to you. Also all the scoepd_refptrs can be const &.
1. add binji@ for native_client_sdk/src/libraries/ppapi_cpp_private/library.dsc
2. Added test.
It works on my linux box.
But I don't know how to run browser_tests on chromeos. I copied it to
/opt/google/chrome and run
./browser_tests --gtest_filter="*OutputProtection*"
It failed on
../../chrome/test/ppapi/ppapi_test.cc:298: Failure
Value of: base::PathExists(plugin_lib)
Actual: false
Expected: true
../../chrome/test/ppapi/ppapi_test.cc:148: Failure
Value of: base::PathExists(test_path)
Actual: false
Expected: true
and then timed out.
3. My code had dependency problem.
output_configurator in chromeos/ includes ppapi/c header file for the enum.
Now I defined another copy of enum in output_configurator.h and COMPILE_ASSERT
them in pepper_output_protection_message_filter.cc
Please review these two files.
https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb...
File ppapi/thunk/interfaces_ppb_private_no_permissions.h (right):
https://codereview.chromium.org/24039002/diff/8001/ppapi/thunk/interfaces_ppb...
ppapi/thunk/interfaces_ppb_private_no_permissions.h:57:
PPB_OutputProtection_Private_0_1)
On 2013/09/12 18:32:38, DaleCurtis wrote:
> On 2013/09/10 12:50:21, kcwu wrote:
> > On 2013/09/09 20:37:15, dmichael wrote:
> > > See the comment on 10 and 11... is that how we want this to work? That
this
> > is
> > > only available for whitelisted origins/plugins?
> > >
> > > I think you probably want this in interfaces_ppb_private.h. Otherwise the
> > check
> > > you have in the host-side factory is inconsistent. (That, or the host-side
> > > factory should check that the plugin is in a whitelist, instead of looking
> at
> > > PRIVATE permissions.)
> >
> > Good question. I'm not sure how CDM will use this api.
> > dalecurtis@, could you give comment on this?
>
> Per ddorwin@ the CDM has private permissions, so it should go under private.
Done.
https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc
(right):
https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:36:
chromeos::OutputConfigurator* configurator =
On 2013/09/12 01:23:13, DaleCurtis wrote:
> Is it safe to call this from any thread? I don't see any DCHECKs in
> ash::Shell::GetInstance(), but I'm not familiar with that code.
After check the code, I feel they assume all run in UI thread. So I added two
ipc messages to make my init and termination code running on UI thread as well.
https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c...
File chromeos/display/output_configurator.h (right):
https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c...
chromeos/display/output_configurator.h:228: const
chrome::PepperOutputProtectionHost* client,
On 2013/09/12 20:10:15, dmichael wrote:
> On 2013/09/12 18:22:08, kcwu wrote:
> > On 2013/09/12 01:23:13, DaleCurtis wrote:
> > > Why not just use a void* if you don't intend to access the class.
> >
> > Done.
> This is better, but my preference is still for an IDMap (or similar). If what
> you want is a unique ID, I think your intent is clearer if you use a unique ID
> rather than treat a pointer as one. Also, there's a slight chance the
allocator
> could reuse that pointer for a new client before the task runs, which would
make
> the "new" client disappear.
Done.
I changed my code to register for an integer as id. I don't use IDMap because I
don't need the reverse mapping.
https://codereview.chromium.org/24039002/diff/12001/chromeos/display/output_c...
chromeos/display/output_configurator.h:376: uint32_t* protection_mask);
On 2013/09/12 18:22:08, kcwu wrote:
> On 2013/09/12 01:23:13, DaleCurtis wrote:
> > OVERRIDE?
>
> Done.
I found these methods are not override, so I reverted. Also removed "virtual"
keyword.
https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):
https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:115:
host_->SendReply(
On 2013/09/13 22:08:33, DaleCurtis wrote:
> SendReply() works w/o host_-> so you can stop storing host_.
Done.
https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h
(right):
https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:26:
protected:
On 2013/09/13 22:08:33, DaleCurtis wrote:
> Can all be private?
Done.
https://codereview.chromium.org/24039002/diff/27029/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:42:
ppapi::host::PpapiHost* host_;
On 2013/09/13 22:08:33, DaleCurtis wrote:
> Not necessary.
Done.
https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out...
File chromeos/display/real_output_configurator_delegate.cc (right):
https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:435: DCHECK(0) << "Invalid
HDCP state: " << state;
On 2013/09/13 22:08:33, DaleCurtis wrote:
> NOTREACHED() << "blah blah"
Done.
https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:449: if (output_info ==
NULL) {
On 2013/09/13 22:08:33, DaleCurtis wrote:
> !output_info
Done.
https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:465: DCHECK(0) << "Unknown
link type: " << name;
On 2013/09/13 22:08:33, DaleCurtis wrote:
> NOTREACHED() << "blah blah"
No, this may be reached when new link type is invented in the future.
https://codereview.chromium.org/24039002/diff/27029/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:517: uint32_t unfulfiled =
0;
On 2013/09/13 22:08:33, DaleCurtis wrote:
> unfulfilled.
Done.
https://codereview.chromium.org/24039002/diff/27029/ppapi/proxy/output_protec...
File ppapi/proxy/output_protection_resource.h (right):
https://codereview.chromium.org/24039002/diff/27029/ppapi/proxy/output_protec...
ppapi/proxy/output_protection_resource.h:45: uint32_t* link_mask_;
On 2013/09/13 22:08:33, DaleCurtis wrote:
> Instead of storing these, you can just bind them into the reply function; up
to
> you. Also all the scoepd_refptrs can be const &.
Done.
https://codereview.chromium.org/24039002/diff/52029/chrome/test/ppapi/ppapi_b...
File chrome/test/ppapi/ppapi_browsertest.cc (right):
https://codereview.chromium.org/24039002/diff/52029/chrome/test/ppapi/ppapi_b...
chrome/test/ppapi/ppapi_browsertest.cc:1533:
TEST_PPAPI_OUT_OF_PROCESS(OutputProtectionPrivate)
I'm not sure what kind of tests (in process, out of process, nacl, etc.) to run.
https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc (right): https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:36: chromeos::OutputConfigurator* configurator = On 2013/09/16 11:57:37, kcwu wrote: > On 2013/09/12 01:23:13, DaleCurtis wrote: > > Is it safe to call this from any thread? I don't see any DCHECKs in > > ash::Shell::GetInstance(), but I'm not familiar with that code. > > After check the code, I feel they assume all run in UI thread. So I added two > ipc messages to make my init and termination code running on UI thread as well. I don't understand why you added those two new IPCs? Why not just post a task to the UI thread during construction like you were doing during destruction?
On 2013/09/16 18:07:08, DaleCurtis wrote: > https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... > File chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc > (right): > > https://codereview.chromium.org/24039002/diff/12001/chrome/browser/renderer_h... > chrome/browser/renderer_host/pepper/pepper_output_protection_host.cc:36: > chromeos::OutputConfigurator* configurator = > On 2013/09/16 11:57:37, kcwu wrote: > > On 2013/09/12 01:23:13, DaleCurtis wrote: > > > Is it safe to call this from any thread? I don't see any DCHECKs in > > > ash::Shell::GetInstance(), but I'm not familiar with that code. > > > > After check the code, I feel they assume all run in UI thread. So I added two > > ipc messages to make my init and termination code running on UI thread as > well. > > I don't understand why you added those two new IPCs? Why not just post a task to > the UI thread during construction like you were doing during destruction? I feel this make the code simpler. ctor and dtor both run at IO thread. And to make GetInstance() safe, both need separated static functions for posting tasks to UI thread. And in ctor, I have to assign |client_id_| back to the class. If assigning back in the task, I have to take care race conditions, for example, client calling QueryStatus() before |client_id_| assigned. Adding states or locking may fix the race condition, but it makes the code more complex. And it makes code consistent -- all in UI thread look better than some in IO thread and some in UI thread.
The one downside to this approach is that if the renderer crashes you never get the Terminate message and that clients protections will stay enabled until shutdown. I don't know enough about the display process to say if this is a problem though. Otherwise this all lgtm, but I'm not an owner on any of the code here :)
dmichael@ and binji@, could you please take another look? thanks.
I think you can get rid of the Init and Terminate messages (and you should if you can). Otherwise just nits. lgtm https://chromiumcodereview.appspot.com/24039002/diff/52029/chrome/test/ppapi/... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://chromiumcodereview.appspot.com/24039002/diff/52029/chrome/test/ppapi/... chrome/test/ppapi/ppapi_browsertest.cc:1533: TEST_PPAPI_OUT_OF_PROCESS(OutputProtectionPrivate) On 2013/09/16 11:57:37, kcwu wrote: > I'm not sure what kind of tests (in process, out of process, nacl, etc.) to run. What you have seems right to me. If it's not being used by NaCl, it should only be supported and tested out-of-process. https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc (right): https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:8: #include "content/public/browser/browser_ppapi_host.h" You may not need this include? https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:10: #include "ipc/ipc_message.h" nit: you don't need this include https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:109: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); nit: please be consistent about usage of BrowserThread. Here you fully specify it, elsewhere you rely on the "using content::BrowserThread". I slightly prefer removing the "using" and just fully specifying the namespace, but I'll be happy so long as you choose one way and do it consistently. https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:113: client_id_ = configurator->RegisterOutputProtectionClient(); suggestion: Can you just do this in the constructor and get rid of the Init message? https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:115: NOTIMPLEMENTED(); Maybe return PP_ERROR_FAILED here, and move the "return PP_OK;" in to the #if section above? (Same for other methods) https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:126: configurator->UnregisterOutputProtectionClient(client_id_); suggestion: I think you could do this Unregister in the destructor, and get rid of the Terminate message entirely. In fact, you should probably Unregister in the destructor anyway. If the plugin crashes, the message to "Terminate" won't get there, but we'll still clean up Hosts and Filters for the plugin instance. https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h (right): https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/ren... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:24: explicit PepperOutputProtectionMessageFilter(); nit: no need for explicit on a default constructor. https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/test/ppapi/... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/test/ppapi/... chrome/test/ppapi/ppapi_browsertest.cc:1533: TEST_PPAPI_OUT_OF_PROCESS(OutputProtectionPrivate) Do you want to only run this on OS_CHROMEOS? https://chromiumcodereview.appspot.com/24039002/diff/80001/chromeos/display/o... File chromeos/display/output_configurator.h (right): https://chromiumcodereview.appspot.com/24039002/diff/80001/chromeos/display/o... chromeos/display/output_configurator.h:56: OUTPUT_PROTECTION_METHOD_NONE, = 0? https://chromiumcodereview.appspot.com/24039002/diff/80001/ppapi/tests/test_o... File ppapi/tests/test_output_protection_private.h (right): https://chromiumcodereview.appspot.com/24039002/diff/80001/ppapi/tests/test_o... ppapi/tests/test_output_protection_private.h:18: // TestCase implementation. nit/suggestion: Everything here down could be private.
Sorry, I just noticed dalecurtis had the same suggestion about Terminate
and Init.
Sending IPC messages to help with thread-hopping is a little bit
heavy-weight and unusual.
For the init step, what about if you initialize client_id_ to 0 (you should
be doing that anyway), and then lazily call
"RegisterOutputProtectionClient" in each method where an id is necessary.
Maybe add a helper method. E.g.:
uint64_t PepperOutputProtectionMessageFilter::GetClientId() {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
if (client_id_ == 0) {
chromeos::OutputConfigurator* configurator =
ash::Shell::GetInstance()->output_configurator();
client_id_ = configurator->RegisterOutputProtectionClient();
}
return client_id_;
}
And then, inside OnQueryStatus for example:
// ...
bool result = configurator->QueryOutputProtectionStatus(
GetClientId(), &link_mask, &protection_mask);
// ...
For the terminate step, it looks like you can have a simple method in an
unnamed namespace in the .cc file that does the unregistration, and then
from the destructor, you can use BrowserThread to bind the client ID and
post a task to run that function on the UI thread. Since the callback
doesn't need to refer to any members of the
PepperOutputProtectionMessageFilter, it should be pretty simple.
i.e.,
namespace {
UnregisterClientOnUIThread(uint64_t client_id) {
chromeos::OutputConfigurator* configurator =
ash::Shell::GetInstance()->output_configurator();
configurator->UnregisterOutputProtectionClient(client_id);
}
} // namespace
And then, in the destructor you can:
PepperOutputProtectionMessageFilter::~PepperOutputProtectionMessageFilter()
{
if (client_id_ != 0) {
content::BrowserThread::PostTask(
content::BrowserThread::UI,
FROM_HERE,
base::Bind(&UnregisterClientOnUIThread, client_id_));
}
}
WDYT?
On Tue, Sep 17, 2013 at 11:25 AM, <dmichael@chromium.org> wrote:
> I think you can get rid of the Init and Terminate messages (and you should
> if
> you can).
>
> Otherwise just nits.
>
> lgtm
>
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
52029/chrome/test/ppapi/ppapi_**browsertest.cc<https://chromiumcodereview.appspot.com/24039002/diff/52029/chrome/test/ppapi/ppapi_browsertest.cc>
> File chrome/test/ppapi/ppapi_**browsertest.cc (right):
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
52029/chrome/test/ppapi/ppapi_**browsertest.cc#newcode1533<https://chromiumcodereview.appspot.com/24039002/diff/52029/chrome/test/ppapi/ppapi_browsertest.cc#newcode1533>
> chrome/test/ppapi/ppapi_**browsertest.cc:1533:
> TEST_PPAPI_OUT_OF_PROCESS(**OutputProtectionPrivate)
>
> On 2013/09/16 11:57:37, kcwu wrote:
>
>> I'm not sure what kind of tests (in process, out of process, nacl,
>>
> etc.) to run.
> What you have seems right to me. If it's not being used by NaCl, it
> should only be supported and tested out-of-process.
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.cc<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc>
> File
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.cc#**newcode8<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc#newcode8>
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.cc:**8:
> #include "content/public/browser/**browser_ppapi_host.h"
> You may not need this include?
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.cc#**newcode10<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc#newcode10>
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.cc:**10:
> #include "ipc/ipc_message.h"
> nit: you don't need this include
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.cc#**newcode109<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc#newcode109>
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.cc:**109:
> DCHECK(content::BrowserThread:**:CurrentlyOn(content::**
> BrowserThread::UI));
> nit: please be consistent about usage of BrowserThread. Here you fully
> specify it, elsewhere you rely on the "using content::BrowserThread".
>
> I slightly prefer removing the "using" and just fully specifying the
> namespace, but I'll be happy so long as you choose one way and do it
> consistently.
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.cc#**newcode113<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc#newcode113>
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.cc:**113:
> client_id_ = configurator->**RegisterOutputProtectionClient**();
> suggestion: Can you just do this in the constructor and get rid of the
> Init message?
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.cc#**newcode115<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc#newcode115>
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.cc:**115:
> NOTIMPLEMENTED();
> Maybe return PP_ERROR_FAILED here, and move the "return PP_OK;" in to
> the #if section above?
>
> (Same for other methods)
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.cc#**newcode126<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc#newcode126>
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.cc:**126:
> configurator->**UnregisterOutputProtectionClie**nt(client_id_);
> suggestion: I think you could do this Unregister in the destructor, and
> get rid of the Terminate message entirely. In fact, you should probably
> Unregister in the destructor anyway. If the plugin crashes, the message
> to "Terminate" won't get there, but we'll still clean up Hosts and
> Filters for the plugin instance.
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.h<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h>
> File
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.h
> (right):
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
> 80001/chrome/browser/renderer_**host/pepper/pepper_output_**
>
protection_message_filter.h#**newcode24<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h#newcode24>
> chrome/browser/renderer_host/**pepper/pepper_output_**
> protection_message_filter.h:**24:
> explicit PepperOutputProtectionMessageF**ilter();
> nit: no need for explicit on a default constructor.
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
80001/chrome/test/ppapi/ppapi_**browsertest.cc<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/test/ppapi/ppapi_browsertest.cc>
> File chrome/test/ppapi/ppapi_**browsertest.cc (right):
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
80001/chrome/test/ppapi/ppapi_**browsertest.cc#newcode1533<https://chromiumcodereview.appspot.com/24039002/diff/80001/chrome/test/ppapi/ppapi_browsertest.cc#newcode1533>
> chrome/test/ppapi/ppapi_**browsertest.cc:1533:
> TEST_PPAPI_OUT_OF_PROCESS(**OutputProtectionPrivate)
> Do you want to only run this on OS_CHROMEOS?
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
80001/chromeos/display/output_**configurator.h<https://chromiumcodereview.appspot.com/24039002/diff/80001/chromeos/display/output_configurator.h>
> File chromeos/display/output_**configurator.h (right):
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
80001/chromeos/display/output_**configurator.h#newcode56<https://chromiumcodereview.appspot.com/24039002/diff/80001/chromeos/display/output_configurator.h#newcode56>
> chromeos/display/output_**configurator.h:56:
> OUTPUT_PROTECTION_METHOD_NONE,
> = 0?
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
80001/ppapi/tests/test_output_**protection_private.h<https://chromiumcodereview.appspot.com/24039002/diff/80001/ppapi/tests/test_output_protection_private.h>
> File ppapi/tests/test_output_**protection_private.h (right):
>
> https://chromiumcodereview.**appspot.com/24039002/diff/**
>
80001/ppapi/tests/test_output_**protection_private.h#newcode18<https://chromiumcodereview.appspot.com/24039002/diff/80001/ppapi/tests/test_output_protection_private.h#newcode18>
> ppapi/tests/test_output_**protection_private.h:18: // TestCase
> implementation.
> nit/suggestion: Everything here down could be private.
>
>
https://chromiumcodereview.**appspot.com/24039002/<https://chromiumcodereview...
>
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
chromium-reviews+unsubscribe@**chromium.org<chromium-reviews%2Bunsubscribe@ch...
> .
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
lgtm
https://codereview.chromium.org/24039002/diff/52029/chrome/test/ppapi/ppapi_b... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/24039002/diff/52029/chrome/test/ppapi/ppapi_b... chrome/test/ppapi/ppapi_browsertest.cc:1533: TEST_PPAPI_OUT_OF_PROCESS(OutputProtectionPrivate) On 2013/09/17 17:25:33, dmichael wrote: > On 2013/09/16 11:57:37, kcwu wrote: > > I'm not sure what kind of tests (in process, out of process, nacl, etc.) to > run. > What you have seems right to me. If it's not being used by NaCl, it should only > be supported and tested out-of-process. Thanks. https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc (right): https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:8: #include "content/public/browser/browser_ppapi_host.h" On 2013/09/17 17:25:33, dmichael wrote: > You may not need this include? Done. https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:10: #include "ipc/ipc_message.h" On 2013/09/17 17:25:33, dmichael wrote: > nit: you don't need this include Done. https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:109: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2013/09/17 17:25:33, dmichael wrote: > nit: please be consistent about usage of BrowserThread. Here you fully specify > it, elsewhere you rely on the "using content::BrowserThread". > > I slightly prefer removing the "using" and just fully specifying the namespace, > but I'll be happy so long as you choose one way and do it consistently. Done. https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:113: client_id_ = configurator->RegisterOutputProtectionClient(); On 2013/09/17 17:25:33, dmichael wrote: > suggestion: Can you just do this in the constructor and get rid of the Init > message? Done. https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:115: NOTIMPLEMENTED(); On 2013/09/17 17:25:33, dmichael wrote: > Maybe return PP_ERROR_FAILED here, and move the "return PP_OK;" in to the #if > section above? > > (Same for other methods) I extracted GetOutputConfigurator() to make it less #if checks. Should it be PP_ERROR_FAILED or PP_ERROR_NOTSUPPORTED ? https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:126: configurator->UnregisterOutputProtectionClient(client_id_); On 2013/09/17 17:25:33, dmichael wrote: > suggestion: I think you could do this Unregister in the destructor, and get rid > of the Terminate message entirely. In fact, you should probably Unregister in > the destructor anyway. If the plugin crashes, the message to "Terminate" won't > get there, but we'll still clean up Hosts and Filters for the plugin instance. Done. https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h (right): https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.h:24: explicit PepperOutputProtectionMessageFilter(); On 2013/09/17 17:25:33, dmichael wrote: > nit: no need for explicit on a default constructor. Done. https://codereview.chromium.org/24039002/diff/80001/chrome/test/ppapi/ppapi_b... File chrome/test/ppapi/ppapi_browsertest.cc (right): https://codereview.chromium.org/24039002/diff/80001/chrome/test/ppapi/ppapi_b... chrome/test/ppapi/ppapi_browsertest.cc:1533: TEST_PPAPI_OUT_OF_PROCESS(OutputProtectionPrivate) On 2013/09/17 17:25:33, dmichael wrote: > Do you want to only run this on OS_CHROMEOS? Done. https://codereview.chromium.org/24039002/diff/80001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/80001/chromeos/display/output_c... chromeos/display/output_configurator.h:56: OUTPUT_PROTECTION_METHOD_NONE, On 2013/09/17 17:25:33, dmichael wrote: > = 0? Done. https://codereview.chromium.org/24039002/diff/80001/ppapi/tests/test_output_p... File ppapi/tests/test_output_protection_private.h (right): https://codereview.chromium.org/24039002/diff/80001/ppapi/tests/test_output_p... ppapi/tests/test_output_protection_private.h:18: // TestCase implementation. On 2013/09/17 17:25:33, dmichael wrote: > nit/suggestion: Everything here down could be private. Done.
https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc (right): https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:115: NOTIMPLEMENTED(); On 2013/09/17 23:15:32, kcwu wrote: > On 2013/09/17 17:25:33, dmichael wrote: > > Maybe return PP_ERROR_FAILED here, and move the "return PP_OK;" in to the #if > > section above? > > > > (Same for other methods) > > I extracted GetOutputConfigurator() to make it less #if checks. > Should it be PP_ERROR_FAILED or PP_ERROR_NOTSUPPORTED ? (please ignore the first sentence)
https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc (right): https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:71: PepperOutputProtectionMessageFilter::PepperOutputProtectionMessageFilter() { Please initialize client_id_ https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:115: NOTIMPLEMENTED(); On 2013/09/17 23:17:05, kcwu wrote: > On 2013/09/17 23:15:32, kcwu wrote: > > On 2013/09/17 17:25:33, dmichael wrote: > > > Maybe return PP_ERROR_FAILED here, and move the "return PP_OK;" in to the > #if > > > section above? > > > > > > (Same for other methods) > > > > I extracted GetOutputConfigurator() to make it less #if checks. > > Should it be PP_ERROR_FAILED or PP_ERROR_NOTSUPPORTED ? > (please ignore the first sentence) Good point, PP_ERROR_NOTSUPPORTED is probably better
https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... File chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc (right): https://codereview.chromium.org/24039002/diff/80001/chrome/browser/renderer_h... chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:71: PepperOutputProtectionMessageFilter::PepperOutputProtectionMessageFilter() { On 2013/09/17 23:48:57, dmichael wrote: > Please initialize client_id_ It's already done in next patch.
https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.cc:260: delegate_->GrabServer(); the delegate isn't checking these server grabs. are they necessary? i'm concerned about all of these calls to GrabServer(). each of them calls XRRGetScreenResources(), which will block the UI thread for 60 seconds. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:29: class PepperOutputProtectionHost; this doesn't appear to be used here https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:243: // Register a client for output protection and request a client id. Returns nit: make these verbs match the tense in other comments (Registers, Unregisters, Queries, etc.) https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:245: virtual uint64_t RegisterOutputProtectionClient() = 0; mind typedef-ing uint64_t to something like OutputProtectionClientId? also document that the server doesn't need to be grabbed for this. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:246: // Unregister the client. nit: please add blank lines between methods https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:247: virtual void UnregisterOutputProtectionClient(uint64_t client_id) = 0; document that the server _does_ need to be grabbed for this. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:258: uint32_t desired_method_mask) = 0; what does |desired_method_mask| contain? if it's or-ed OutputProtectionMethod values, please rename it to |protection_mask| to match QueryOutputProtectionStatus() https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:391: uint64_t RegisterOutputProtectionClient(); nit: please add comments between these methods too https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:398: // PP_OutputProtectionMethod_Private values. are the masks actually PP_*_Private values, or are they the enums that are defined above? https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:34: const char kOutputPortName_VGA[] = "VGA"; nit: remove underscores from constant identifiers https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:392: success = XRRGetOutputProperty(display_, id, prop, 0, 100, False, please add a TODO to move this to x11_util (similar method calls in this file and output_util.cc) https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:57: nit: delete blank line https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:73: HDCP_State_Undesired, nit: HDCP_STATE_UNDESIRED etc. to match the enum naming style elsewhere https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:129: bool GetHDCPState(RROutput id, HDCPState* state); nit: add blank lines between methods https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:150: uint64_t next_client_id_; nit: next_output_protection_client_id_ but does this actually need to live in the delegate? unless i'm missing something, the ID creation/destruction can just be handled in OutputConfigurator, which'd eliminate a few wrapper methods.
https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.cc:260: delegate_->GrabServer(); On 2013/09/18 22:39:16, Daniel Erat wrote: > the delegate isn't checking these server grabs. are they necessary? > > i'm concerned about all of these calls to GrabServer(). each of them calls > XRRGetScreenResources(), which will block the UI thread for 60 seconds. Yup this grab isn't strictly required, as we are just checking properties. The other grab below is probably required though, to ensure consistency between the outputs which exist and the ones we want to set HDCP on. However "enable" doesn't happen nearly as often as "query" so that should be ok. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:43: // Video output link types. The terminology "link" isn't used in this code, can we replace this with just "output"? https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:53: Same here, we just use the term "output", not "output link". https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:37: const char kOutputPortName_DisplayPort[] = "DP"; I think just "OutputName_*" is fine, the "OutputPort" terminology is never used, just "Output". Also, we already have const char kInternal_LVDS[] = "LVDS"; const char kInternal_eDP[] = "eDP"; const char kInternal_DSI[] = "DSI"; in output_util.cc that we might want to reuse/extend. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:441: PropModeReplace, data, 1); Hmm, we probably shouldn't return true here without checking for X errors. See ui/base/x/x11_util.cc:372 for an example of how to do this: X11ErrorTracker err_tracker; bool result = XShmAttach(dpy, &shminfo); if (result) VLOG(1) << "X got shared memory segment " << shmkey; else LOG(WARNING) << "X failed to attach to shared memory segment " << shmkey; if (err_tracker.FoundNewError()) result = false; shmdt(address); if (!result) { LOG(WARNING) << "X failed to attach to shared memory segment " << shmkey; return SHARED_MEMORY_NONE; }
https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.cc:260: delegate_->GrabServer(); On 2013/09/18 23:21:35, marcheu wrote: > On 2013/09/18 22:39:16, Daniel Erat wrote: > > the delegate isn't checking these server grabs. are they necessary? > > > > i'm concerned about all of these calls to GrabServer(). each of them calls > > XRRGetScreenResources(), which will block the UI thread for 60 seconds. > > Yup this grab isn't strictly required, as we are just checking properties. The > other grab below is probably required though, to ensure consistency between the > outputs which exist and the ones we want to set HDCP on. However "enable" > doesn't happen nearly as often as "query" so that should be ok. Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:29: class PepperOutputProtectionHost; On 2013/09/18 22:39:16, Daniel Erat wrote: > this doesn't appear to be used here Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:43: // Video output link types. On 2013/09/18 23:21:35, marcheu wrote: > The terminology "link" isn't used in this code, can we replace this with just > "output"? Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:53: On 2013/09/18 23:21:35, marcheu wrote: > Same here, we just use the term "output", not "output link". Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:243: // Register a client for output protection and request a client id. Returns On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: make these verbs match the tense in other comments (Registers, Unregisters, > Queries, etc.) Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:245: virtual uint64_t RegisterOutputProtectionClient() = 0; On 2013/09/18 22:39:16, Daniel Erat wrote: > mind typedef-ing uint64_t to something like OutputProtectionClientId? > > also document that the server doesn't need to be grabbed for this. Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:246: // Unregister the client. On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: please add blank lines between methods Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:247: virtual void UnregisterOutputProtectionClient(uint64_t client_id) = 0; On 2013/09/18 22:39:16, Daniel Erat wrote: > document that the server _does_ need to be grabbed for this. This is done in output_configurator.cc https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:258: uint32_t desired_method_mask) = 0; On 2013/09/18 22:39:16, Daniel Erat wrote: > what does |desired_method_mask| contain? if it's or-ed OutputProtectionMethod > values, please rename it to |protection_mask| to match > QueryOutputProtectionStatus() This follows the naming in ppapi/api/private/ppb_output_protection_private.idl https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:391: uint64_t RegisterOutputProtectionClient(); On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: please add comments between these methods too Do you mean blank line? https://codereview.chromium.org/24039002/diff/95001/chromeos/display/output_c... chromeos/display/output_configurator.h:398: // PP_OutputProtectionMethod_Private values. On 2013/09/18 22:39:16, Daniel Erat wrote: > are the masks actually PP_*_Private values, or are they the enums that are > defined above? Done. Ah, forgot to change this. PP_*_Private values are identical to enums defined here. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:34: const char kOutputPortName_VGA[] = "VGA"; On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: remove underscores from constant identifiers This naming style follows kInternal_* as marcheu mentioned. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:37: const char kOutputPortName_DisplayPort[] = "DP"; On 2013/09/18 23:21:35, marcheu wrote: > I think just "OutputName_*" is fine, the "OutputPort" terminology is never used, > just "Output". > > Also, we already have > > const char kInternal_LVDS[] = "LVDS"; > const char kInternal_eDP[] = "eDP"; > const char kInternal_DSI[] = "DSI"; > > in output_util.cc that we might want to reuse/extend. Done. I changed they as kOutputName_*. These names are only used in this file now. Should I move them to output_util.cc? https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:392: success = XRRGetOutputProperty(display_, id, prop, 0, 100, False, On 2013/09/18 22:39:16, Daniel Erat wrote: > please add a TODO to move this to x11_util (similar method calls in this file > and output_util.cc) Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:441: PropModeReplace, data, 1); On 2013/09/18 23:21:35, marcheu wrote: > Hmm, we probably shouldn't return true here without checking for X errors. > > See ui/base/x/x11_util.cc:372 for an example of how to do this: > > X11ErrorTracker err_tracker; > bool result = XShmAttach(dpy, &shminfo); > if (result) > VLOG(1) << "X got shared memory segment " << shmkey; > else > LOG(WARNING) << "X failed to attach to shared memory segment " << shmkey; > if (err_tracker.FoundNewError()) > result = false; > shmdt(address); > if (!result) { > LOG(WARNING) << "X failed to attach to shared memory segment " << shmkey; > return SHARED_MEMORY_NONE; > } Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:57: On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: delete blank line Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:73: HDCP_State_Undesired, On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: HDCP_STATE_UNDESIRED etc. to match the enum naming style elsewhere Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:129: bool GetHDCPState(RROutput id, HDCPState* state); On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: add blank lines between methods Done. https://codereview.chromium.org/24039002/diff/95001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:150: uint64_t next_client_id_; On 2013/09/18 22:39:16, Daniel Erat wrote: > nit: next_output_protection_client_id_ > > but does this actually need to live in the delegate? unless i'm missing > something, the ID creation/destruction can just be handled in > OutputConfigurator, which'd eliminate a few wrapper methods. Done.
https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS File chromeos/display/DEPS (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS#new... chromeos/display/DEPS:2: "+ui/base/x", per elliot's https://codereview.chromium.org/23536057/, chromeos probably shouldn't depend on ui/. i'll let him weigh in on this, but perhaps the error handler should be moved under base/, as he's doing with other ui stuff in his CL. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.cc:299: return result; just do: return delegate_->Query... https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:245: // Query link status and protection status. Returns true on success. nit: s/Query/Queries/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:251: // Request the desired protection methods. Returns true when the protection nit: s/Request/Requests/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:255: uint32_t desired_method_mask) = 0; you said that this matches the naming in the idl file, but as far as i can see, that's not the case: EnableProtection has a |desired_protection_mask| parameter there. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:392: // Registers a client for output protection and request a client id. Returns nit: s/request/requests/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:411: // of the PP_OutputProtectionMethod_Private values. s/PP_OutputProtectionMethod_Private/OutputProtectionMethod/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:415: uint32_t desired_method_mask); s/method/protection/ here too https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:325: Atom prop = XInternAtom(display_, "Content Protection", False); please move this to a constant at the top of the file (e.g. kContentProtectionAtomName) that you can share across other methods and error messages https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:340: if (value == XInternAtom(display_, "Undesired", False)) { please make constants for these too so they aren't duplicated https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:347: LOG(ERROR) << "Unknown property value: " << value; nit: mind changing this to "Unknown " << kContentProtectionAtomName << " value: " so it's obvious what this is referring to if it shows up in the logs? https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:411: DCHECK(0) << "Unknown link type: " << name; use NOTREACHED() or maybe just LOG(ERROR) instead of DCHECK(0) https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:427: client_protection_requests_[client_id] = desired_method_mask; should you hold off on updating the request map until the state has been successfully set? https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:445: HDCPState new_desired_state; nit: HDCPState new_desired_state = (all_desires & OUTPUT_PROTECTION_METHOD_HDCP) ? HDCP_STATE_DESIRED : HDCP_STATE_UNDESIRED; https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:461: uint32_t* protection_mask) { mind adding a CHECK(screen_) here too? but this seems fishy to me: OutputConfigurator::QueryOutputProtectionStatus() isn't grabbing the server and fetching the xrandr resources now (which is a good thing if this gets called while the user is interacting with the browser, since that'd block the UI thread), but this method uses |screen_|. am i missing something? https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:476: RROutput this_id = screen_->outputs[i]; i think you're shadowing the earlier |this_id| variable, which should already have the same value https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:62: uint32_t desired_method_mask) OVERRIDE; s/method/protection/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:105: // Get HDCP state of output. nit: s/Get/Gets/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:108: // Set HDCP state of output. nit: s/Set/Sets/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:111: // Query output protection link type of output. nit: s/Query/Queries/ https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:114: OutputType *link_type); nit: move '*' to left side of space
https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:461: uint32_t* protection_mask) { On 2013/09/20 15:00:24, Daniel Erat wrote: > mind adding a CHECK(screen_) here too? > > but this seems fishy to me: OutputConfigurator::QueryOutputProtectionStatus() > isn't grabbing the server and fetching the xrandr resources now (which is a good > thing if this gets called while the user is interacting with the browser, since > that'd block the UI thread), but this method uses |screen_|. am i missing > something? moreover, is there any reason why you can't save the output type (and maybe also the protection methods?) in the OutputSnapshot structs that are cached by OutputConfigurator? that would probably let you move some code out of the delegate and write tests for it in output_configurator_unittest.cc.
https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS File chromeos/display/DEPS (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS#new... chromeos/display/DEPS:2: "+ui/base/x", On 2013/09/20 15:00:24, Daniel Erat wrote: > per elliot's https://codereview.chromium.org/23536057/, chromeos probably > shouldn't depend on ui/. i'll let him weigh in on this, but perhaps the error > handler should be moved under base/, as he's doing with other ui stuff in his > CL. It's a combination of dependency aesthetics (chromeos/ shouldn't depend on ui/ because they do very different things) and the problem that this won't link in some configurations. I'd REALLY prefer if chromeos/ didn't depend on ui/, but if you do go that route, make sure that it depends on ui, via gyp.
https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS File chromeos/display/DEPS (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS#new... chromeos/display/DEPS:2: "+ui/base/x", On 2013/09/20 16:28:20, Elliot Glaysher wrote: > On 2013/09/20 15:00:24, Daniel Erat wrote: > > per elliot's https://codereview.chromium.org/23536057/, chromeos probably > > shouldn't depend on ui/. i'll let him weigh in on this, but perhaps the error > > handler should be moved under base/, as he's doing with other ui stuff in his > > CL. > > It's a combination of dependency aesthetics (chromeos/ shouldn't depend on ui/ > because they do very different things) and the problem that this won't link in > some configurations. I'd REALLY prefer if chromeos/ didn't depend on ui/, but if > you do go that route, make sure that it depends on ui, via gyp. It's nice that these common things moved to base/. Let me move ui/base/x/x11_error_tracker.* to base/x11, how do you feel?
https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.cc:299: return result; On 2013/09/20 15:00:24, Daniel Erat wrote: > just do: > > return delegate_->Query... Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:325: Atom prop = XInternAtom(display_, "Content Protection", False); On 2013/09/20 15:00:24, Daniel Erat wrote: > please move this to a constant at the top of the file (e.g. > kContentProtectionAtomName) that you can share across other methods and error > messages Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:340: if (value == XInternAtom(display_, "Undesired", False)) { On 2013/09/20 15:00:24, Daniel Erat wrote: > please make constants for these too so they aren't duplicated Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:347: LOG(ERROR) << "Unknown property value: " << value; On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: mind changing this to "Unknown " << kContentProtectionAtomName << " value: > " so it's obvious what this is referring to if it shows up in the logs? Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:411: DCHECK(0) << "Unknown link type: " << name; On 2013/09/20 15:00:24, Daniel Erat wrote: > use NOTREACHED() or maybe just LOG(ERROR) instead of DCHECK(0) Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:427: client_protection_requests_[client_id] = desired_method_mask; On 2013/09/20 15:00:24, Daniel Erat wrote: > should you hold off on updating the request map until the state has been > successfully set? Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:445: HDCPState new_desired_state; On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: > > HDCPState new_desired_state = > (all_desires & OUTPUT_PROTECTION_METHOD_HDCP) ? > HDCP_STATE_DESIRED : HDCP_STATE_UNDESIRED; Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:461: uint32_t* protection_mask) { On 2013/09/20 15:00:24, Daniel Erat wrote: > mind adding a CHECK(screen_) here too? > > but this seems fishy to me: OutputConfigurator::QueryOutputProtectionStatus() > isn't grabbing the server and fetching the xrandr resources now (which is a good > thing if this gets called while the user is interacting with the browser, since > that'd block the UI thread), but this method uses |screen_|. am i missing > something? I'm confused. marcheu said grabbing is not strictly required. Does that mean I can cache screen_->outputs first and call XRRGetOutputProperty without grabbing later? Before that, I added grab back. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:461: uint32_t* protection_mask) { On 2013/09/20 15:08:26, Daniel Erat wrote: > On 2013/09/20 15:00:24, Daniel Erat wrote: > > mind adding a CHECK(screen_) here too? > > > > but this seems fishy to me: OutputConfigurator::QueryOutputProtectionStatus() > > isn't grabbing the server and fetching the xrandr resources now (which is a > good > > thing if this gets called while the user is interacting with the browser, > since > > that'd block the UI thread), but this method uses |screen_|. am i missing > > something? > > moreover, is there any reason why you can't save the output type (and maybe also > the protection methods?) in the OutputSnapshot structs that are cached by > OutputConfigurator? that would probably let you move some code out of the > delegate and write tests for it in output_configurator_unittest.cc. Ah, I didn't notice OutputSnapshot was added recently. I will use it later today. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:476: RROutput this_id = screen_->outputs[i]; On 2013/09/20 15:00:24, Daniel Erat wrote: > i think you're shadowing the earlier |this_id| variable, which should already > have the same value Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:62: uint32_t desired_method_mask) OVERRIDE; On 2013/09/20 15:00:24, Daniel Erat wrote: > s/method/protection/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:105: // Get HDCP state of output. On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: s/Get/Gets/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:108: // Set HDCP state of output. On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: s/Set/Sets/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:111: // Query output protection link type of output. On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: s/Query/Queries/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.h:114: OutputType *link_type); On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: move '*' to left side of space Done.
https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:461: uint32_t* protection_mask) { On 2013/09/20 20:29:30, kcwu wrote: > On 2013/09/20 15:00:24, Daniel Erat wrote: > > mind adding a CHECK(screen_) here too? > > > > but this seems fishy to me: OutputConfigurator::QueryOutputProtectionStatus() > > isn't grabbing the server and fetching the xrandr resources now (which is a > good > > thing if this gets called while the user is interacting with the browser, > since > > that'd block the UI thread), but this method uses |screen_|. am i missing > > something? > I'm confused. marcheu said grabbing is not strictly required. Does that mean I > can cache screen_->outputs first and call XRRGetOutputProperty without grabbing > later? > Before that, I added grab back. screen_ is only valid while the server is grabbed (in fact, it's NULL the rest of the time). i think that stephane meant that it's okay for you to call XRRGetOutputProperty() while the server isn't grabbed if you already know the RROutput that you're interested in. OutputSnapshot contains the output's RROutput, but it's probably cleaner to just save the output type to the OutputSnapshot in InitOutputSnapshot(). that way, it'll already be available from OutputConfigurator::cached_outputs_. if the HDCP state is cached too, you may not need to communicate with the X server at all to return the protection status.
https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS File chromeos/display/DEPS (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/DEPS#new... chromeos/display/DEPS:2: "+ui/base/x", On 2013/09/20 17:55:13, kcwu wrote: > On 2013/09/20 16:28:20, Elliot Glaysher wrote: > > On 2013/09/20 15:00:24, Daniel Erat wrote: > > > per elliot's https://codereview.chromium.org/23536057/, chromeos probably > > > shouldn't depend on ui/. i'll let him weigh in on this, but perhaps the > error > > > handler should be moved under base/, as he's doing with other ui stuff in > his > > > CL. > > > > It's a combination of dependency aesthetics (chromeos/ shouldn't depend on ui/ > > because they do very different things) and the problem that this won't link in > > some configurations. I'd REALLY prefer if chromeos/ didn't depend on ui/, but > if > > you do go that route, make sure that it depends on ui, via gyp. > > It's nice that these common things moved to base/. Let me move > ui/base/x/x11_error_tracker.* to base/x11, how do you feel? I'm fine with that. (However, there currently isn't a base/x11/ directory; the first patch that adds files is currently under review. That said, since you aren't adding the same code, I don't see a reason to block on me.)
I modified this CL to use cached_outputs_ in output_configurator. However, some code flows won't set cached_outputs_ (ex. some early returns in OutputConfigurator::EnterState). I am not sure they are harmless or not. I will check it again tomorrow. BTW, I'm moving x11_error_tracker to base/x11/. Please give comments about the moving at https://codereview.chromium.org/24160005/ (so you could ignore changes in base/* and ui/* of this CL) https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:245: // Query link status and protection status. Returns true on success. On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: s/Query/Queries/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:251: // Request the desired protection methods. Returns true when the protection On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: s/Request/Requests/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:255: uint32_t desired_method_mask) = 0; On 2013/09/20 15:00:24, Daniel Erat wrote: > you said that this matches the naming in the idl file, but as far as i can see, > that's not the case: EnableProtection has a |desired_protection_mask| parameter > there. Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:392: // Registers a client for output protection and request a client id. Returns On 2013/09/20 15:00:24, Daniel Erat wrote: > nit: s/request/requests/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:411: // of the PP_OutputProtectionMethod_Private values. On 2013/09/20 15:00:24, Daniel Erat wrote: > s/PP_OutputProtectionMethod_Private/OutputProtectionMethod/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:411: // of the PP_OutputProtectionMethod_Private values. On 2013/09/20 15:00:24, Daniel Erat wrote: > s/PP_OutputProtectionMethod_Private/OutputProtectionMethod/ Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/output_c... chromeos/display/output_configurator.h:415: uint32_t desired_method_mask); On 2013/09/20 15:00:24, Daniel Erat wrote: > s/method/protection/ here too Done. https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/42001/chromeos/display/real_out... chromeos/display/real_output_configurator_delegate.cc:461: uint32_t* protection_mask) { On 2013/09/20 20:40:59, Daniel Erat wrote: > On 2013/09/20 20:29:30, kcwu wrote: > > On 2013/09/20 15:00:24, Daniel Erat wrote: > > > mind adding a CHECK(screen_) here too? > > > > > > but this seems fishy to me: > OutputConfigurator::QueryOutputProtectionStatus() > > > isn't grabbing the server and fetching the xrandr resources now (which is a > > good > > > thing if this gets called while the user is interacting with the browser, > > since > > > that'd block the UI thread), but this method uses |screen_|. am i missing > > > something? > > I'm confused. marcheu said grabbing is not strictly required. Does that mean I > > can cache screen_->outputs first and call XRRGetOutputProperty without > grabbing > > later? > > Before that, I added grab back. > > screen_ is only valid while the server is grabbed (in fact, it's NULL the rest > of the time). i think that stephane meant that it's okay for you to call > XRRGetOutputProperty() while the server isn't grabbed if you already know the > RROutput that you're interested in. > > OutputSnapshot contains the output's RROutput, but it's probably cleaner to just > save the output type to the OutputSnapshot in InitOutputSnapshot(). that way, > it'll already be available from OutputConfigurator::cached_outputs_. if the HDCP > state is cached too, you may not need to communicate with the X server at all to > return the protection status. Done.
Thanks, I'm glad to see the server grabs go away! This basically looks okay to me now, with some nits. I don't have strong feelings about where X11 utility code should live, so I'll let you work that out with Nico and Elliot. Regarding cached_outputs_, I'll take a look at updating those in OutputConfigurator::GetOutputs() as well. It should be safe to do that, and it'll make sure that we don't end up with stale data if EnterState() fails for some reason. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... chromeos/display/output_configurator.cc:348: uint32_t all_desires = desired_method_mask; nit: s/all_desires/all_desired/ https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... chromeos/display/output_configurator.cc:353: if (it->first != client_id) { nit: remove curly brackets here https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... chromeos/display/output_configurator.h:124: bool is_internal; mind adding a TODO to remove this, since it should probably be possible to just check type == OUTPUT_TYPE_INTERNAL instead now? https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:345: Atom prop = XInternAtom(display_, kContentProtectionAtomName, False); nit: please add a TODO to update all of this code to use X11AtomCache instead of making a bunch of round trips to the server every time this is called https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:356: "' does not exist"; nit: indent multi-line logging statements like this: LOG(ERROR) << "Some string " << a_variable << " another string " << ...; https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:370: LOG(ERROR) << "Unknown " << kContentProtectionAtomName << " value: " << nit: fix indenting here too https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:387: Atom name = XInternAtom(display_, "Content Protection", False); kContentProtectionAtomName https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:388: Atom value; nit: initialize this to None in case so it doesn't contain garbage if someone introduces a bug later https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.h:8: #include <map> nit: i don't think you need this here anymore https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.h:57: // Gets HDCP state of output. nit: get rid of comments and squash these up against the other overrides (these methods are already documented in the interface)
After talking to thakis and oshima, I'm going to take another stab at getting the chromeos/ depending on ui/base/x/. Previous attempts broke, but I'm going to try one more time before I throw my hands up and say that it has to live in base/.
High-level question: What happens when a new display is connected? Doesn't the original requestor need to be notified that their previously-requested protection isn't enabled on it? (Otherwise, you could probably try to turn on protection automatically on the new display, but it might be unsupported or otherwise fail...)
On 2013/09/23 22:26:39, Daniel Erat wrote: > High-level question: What happens when a new display is connected? Doesn't the > original requestor need to be notified that their previously-requested > protection isn't enabled on it? (Otherwise, you could probably try to turn on > protection automatically on the new display, but it might be unsupported or > otherwise fail...) Never mind this comment; Stephane pointed out that the requestor will be polling periodically to notice changes to the status.
https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... chromeos/display/output_configurator.cc:348: uint32_t all_desires = desired_method_mask; On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: s/all_desires/all_desired/ Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... chromeos/display/output_configurator.cc:353: if (it->first != client_id) { On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: remove curly brackets here Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... File chromeos/display/output_configurator.h (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/output_... chromeos/display/output_configurator.h:124: bool is_internal; On 2013/09/23 19:31:43, Daniel Erat wrote: > mind adding a TODO to remove this, since it should probably be possible to just > check type == OUTPUT_TYPE_INTERNAL instead now? Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:345: Atom prop = XInternAtom(display_, kContentProtectionAtomName, False); On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: please add a TODO to update all of this code to use X11AtomCache instead of > making a bunch of round trips to the server every time this is called Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:356: "' does not exist"; On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: indent multi-line logging statements like this: > > LOG(ERROR) << "Some string " << a_variable > << " another string " << ...; Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:370: LOG(ERROR) << "Unknown " << kContentProtectionAtomName << " value: " << On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: fix indenting here too Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:387: Atom name = XInternAtom(display_, "Content Protection", False); On 2013/09/23 19:31:43, Daniel Erat wrote: > kContentProtectionAtomName Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:388: Atom value; On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: initialize this to None in case so it doesn't contain garbage if someone > introduces a bug later Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... File chromeos/display/real_output_configurator_delegate.h (right): https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.h:8: #include <map> On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: i don't think you need this here anymore Done. https://codereview.chromium.org/24039002/diff/121001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.h:57: // Gets HDCP state of output. On 2013/09/23 19:31:43, Daniel Erat wrote: > nit: get rid of comments and squash these up against the other overrides (these > methods are already documented in the interface) Done.
LGTM for chromeos/ https://codereview.chromium.org/24039002/diff/135001/chromeos/display/real_ou... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/135001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:402: unsigned char *data = reinterpret_cast<unsigned char*>(&value); nit: 'unsigned char* data'
https://codereview.chromium.org/24039002/diff/135001/chromeos/display/real_ou... File chromeos/display/real_output_configurator_delegate.cc (right): https://codereview.chromium.org/24039002/diff/135001/chromeos/display/real_ou... chromeos/display/real_output_configurator_delegate.cc:402: unsigned char *data = reinterpret_cast<unsigned char*>(&value); On 2013/09/24 16:26:44, Daniel Erat wrote: > nit: 'unsigned char* data' Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kcwu@chromium.org/24039002/141001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kcwu@chromium.org/24039002/167001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kcwu@chromium.org/24039002/167001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kcwu@chromium.org/24039002/167001
Message was sent while issue was closed.
Change committed as 225849 |
