Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
567123002:
Cleanup template in CdmPromise (Closed)
DescriptionCdmPromise to use CdmPromiseTraits so specializations not needed
Due to GetResolveParameterType(), the template was specialized for all
the cases used. Adding CdmPromiseTraits so that the specializations
are not needed, and the type set in the constructor appropriately.
BUG=358271
TEST=existing EME tests still pass
Committed: https://crrev.com/d2fa738da06c08fe136dedc2212e76174de94772
Cr-Commit-Position: refs/heads/master@{#296112}
Patch Set 1 #
Total comments: 1
Patch Set 2 : back to promise #
Total comments: 22
Patch Set 3 : ReportResultToUMA #
Total comments: 4
Messages
Total messages: 17 (3 generated)
jrummell@chromium.org changed reviewers: + ddorwin@chromium.org, sandersd@chromium.org, xhwang@chromium.org
PTAL. Will override these classes to hold a WebCDMResult in order to simplify my other CL.
https://codereview.chromium.org/567123002/diff/1/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/1/media/base/cdm_promise.h#new... media/base/cdm_promise.h:75: void ResolveBase(); As discussed, rename as ReportResolution().
I skimmed this CL. It seems the we have to specialize all derived classes of
CdmPromise because we need to specialize GetResolveParameterType().
Maybe we can introduce a traits class (e.g. CdmPromiseTraits) which defines a
static const variable kParameterType, then we can do:
CdmPromise::ResolveParameterType
CdmPromiseTemplate<T>::GetResolveParameterType() const {
return CdmPromiseTraits<T>::kParameterType;
}
I still likes the templated version because it's more scalable: adding a new
CdmPromise type is super easy. But if we are sure we won't add more CdmPromise
types the current CL may be fine.
+ddorwin: any comments?
On 2014/09/16 00:58:00, xhwang wrote:
> I skimmed this CL. It seems the we have to specialize all derived classes of
> CdmPromise because we need to specialize GetResolveParameterType().
>
> Maybe we can introduce a traits class (e.g. CdmPromiseTraits) which defines a
> static const variable kParameterType, then we can do:
>
> CdmPromise::ResolveParameterType
> CdmPromiseTemplate<T>::GetResolveParameterType() const {
> return CdmPromiseTraits<T>::kParameterType;
> }
>
> I still likes the templated version because it's more scalable: adding a new
> CdmPromise type is super easy. But if we are sure we won't add more CdmPromise
> types the current CL may be fine.
>
> +ddorwin: any comments?
As discussed offline, we should return to the templated non-specialized classes.
Basically, revert some of this change:
https://codereview.chromium.org/515753002/diff/60001/media/base/cdm_promise.h.
We'll then use specialization here and in content/renderer (in
https://codereview.chromium.org/555223004/) to get the correct value for
GetResolveParameterType() and call the correct method on the Blink API object,
respectively. A traits class probably makes sense here, but we may need a
specialized function in the other CL.
Patchset #2 (id:20001) has been deleted
Updated to switch back to template without all the specialization.
Thanks! https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:98: base::LinearHistogram::FactoryGet( As discussed offline, it might make sense to extract ReportResult() so we only have one location for the UMA call. (This might also allow us to use the macro - discuss with sandersd.) https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:125: CdmPromiseTemplate<T>::CdmPromiseTemplate() { hopefully remove https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:130: DCHECK(!is_pending_); The parent's destructor is already doing this. We don't even need to define a destructor. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:70: // Called by all resolve() methods to do the check on |is_pending_| ... to report UMA, if applicable, and update |is_pending_|. ^ Start with the important thing (and the reason for its name); the check is a DCHECK, so focus on the unconditional behavior. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:72: void ReportResolution(); nit: "Resolution" has many meanings. How about ReportResolved? https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:95: virtual ResolveParameterType GetResolveParameterType() const OVERRIDE; Rather than overriding this, can we make the base class non-virtual and have its constructor take the type? We can use the same traits call as in the override in this class's constructor. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:98: // Allow subclasses to completely override the implementation. Is this still appropriate? I don't think it was use in the base either. Ditto below.
https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:19: return CdmPromise::VOID_TYPE; You can simply do: static const CdmPromise::ResolveParameterType kType = CdmPromise::VOID_TYPE; https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:72: void ReportResolution(); ReportXxxToUMA() to be explicit? https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:79: // UMA to report result to. s/UMA/UMA name/ Can you give an example here? Sometimes I see a UMA on the server, but it's hard to find where it's reported.
Updated. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:19: return CdmPromise::VOID_TYPE; On 2014/09/20 00:34:50, xhwang wrote: > You can simply do: > > static const CdmPromise::ResolveParameterType kType = CdmPromise::VOID_TYPE; Done. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:98: base::LinearHistogram::FactoryGet( On 2014/09/19 20:56:46, ddorwin wrote: > As discussed offline, it might make sense to extract ReportResult() so we only > have one location for the UMA call. (This might also allow us to use the macro - > discuss with sandersd.) Done. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:125: CdmPromiseTemplate<T>::CdmPromiseTemplate() { On 2014/09/19 20:56:46, ddorwin wrote: > hopefully remove See comment in .h file. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:130: DCHECK(!is_pending_); On 2014/09/19 20:56:46, ddorwin wrote: > The parent's destructor is already doing this. We don't even need to define a > destructor. Done. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:70: // Called by all resolve() methods to do the check on |is_pending_| On 2014/09/19 20:56:46, ddorwin wrote: > ... to report UMA, if applicable, and update |is_pending_|. > > ^ Start with the important thing (and the reason for its name); the check is a > DCHECK, so focus on the unconditional behavior. Done. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:72: void ReportResolution(); On 2014/09/19 20:56:46, ddorwin wrote: > nit: "Resolution" has many meanings. How about ReportResolved? Done. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:72: void ReportResolution(); On 2014/09/20 00:34:50, xhwang wrote: > ReportXxxToUMA() to be explicit? Done. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:79: // UMA to report result to. On 2014/09/20 00:34:50, xhwang wrote: > s/UMA/UMA name/ > > Can you give an example here? Sometimes I see a UMA on the server, but it's hard > to find where it's reported. Comment changed. As for an example, the names are generated by whoever creates the promise. The names include the component, so they are something like "Media.EME.ClearKey.CreateSession". However, this class doesn't care about what names are used. But I do agree with you -- it would be nice to simply search on the UMA name and find where it comes from. Maybe the names should be included near the callers. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:95: virtual ResolveParameterType GetResolveParameterType() const OVERRIDE; On 2014/09/19 20:56:46, ddorwin wrote: > Rather than overriding this, can we make the base class non-virtual and have its > constructor take the type? We can use the same traits call as in the override in > this class's constructor. Done. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:98: // Allow subclasses to completely override the implementation. On 2014/09/19 20:56:46, ddorwin wrote: > Is this still appropriate? I don't think it was use in the base either. Ditto > below. It is used by SessionUpdatedPromise and SessionLoadedPromise (in ppapi_decryptor.cc). Although they will go away, I expect the new overrides in WebContentDecryptionModuleSessionImpl will need them.
LGTM with comment in PS2 and pending xhwang's review. Thanks! https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:98: // Allow subclasses to completely override the implementation. On 2014/09/22 19:21:10, jrummell wrote: > On 2014/09/19 20:56:46, ddorwin wrote: > > Is this still appropriate? I don't think it was use in the base either. Ditto > > below. > > It is used by SessionUpdatedPromise and SessionLoadedPromise (in > ppapi_decryptor.cc). Although they will go away, I expect the new overrides in > WebContentDecryptionModuleSessionImpl will need them. Maybe a TODO to consider removing these in October (timelines are desirable; if there is a relevant bug, you can use that instead). Actually, the TODO should be at new line 64, which can also be removed and would cause the others to be removed too.
lgtm % comments for future CL https://codereview.chromium.org/567123002/diff/60001/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/60001/media/base/cdm_promise.h... media/base/cdm_promise.h:65: CdmPromise(ResolveParameterType parameter_type, PromiseRejectedCB reject_cb); In another CL, pass callbacks by const-ref? https://codereview.chromium.org/567123002/diff/60001/media/base/cdm_promise.h... media/base/cdm_promise.h:94: PromiseRejectedCB rejected_cb); ditto: pass both callbacks in const-ref?
Thanks for the reviews. https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/40001/media/base/cdm_promise.h... media/base/cdm_promise.h:98: // Allow subclasses to completely override the implementation. On 2014/09/22 20:46:30, ddorwin wrote: > On 2014/09/22 19:21:10, jrummell wrote: > > On 2014/09/19 20:56:46, ddorwin wrote: > > > Is this still appropriate? I don't think it was use in the base either. > Ditto > > > below. > > > > It is used by SessionUpdatedPromise and SessionLoadedPromise (in > > ppapi_decryptor.cc). Although they will go away, I expect the new overrides in > > WebContentDecryptionModuleSessionImpl will need them. > > Maybe a TODO to consider removing these in October (timelines are desirable; if > there is a relevant bug, you can use that instead). Actually, the TODO should be > at new line 64, which can also be removed and would cause the others to be > removed too. I'll wait until the new overrides are implemented. https://codereview.chromium.org/567123002/diff/60001/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/567123002/diff/60001/media/base/cdm_promise.h... media/base/cdm_promise.h:65: CdmPromise(ResolveParameterType parameter_type, PromiseRejectedCB reject_cb); On 2014/09/22 20:55:31, xhwang wrote: > In another CL, pass callbacks by const-ref? Acknowledged. https://codereview.chromium.org/567123002/diff/60001/media/base/cdm_promise.h... media/base/cdm_promise.h:94: PromiseRejectedCB rejected_cb); On 2014/09/22 20:55:31, xhwang wrote: > ditto: pass both callbacks in const-ref? Acknowledged.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/567123002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as eab6221c14097d4b4c9f25702bc1b741d64f5048
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d2fa738da06c08fe136dedc2212e76174de94772 Cr-Commit-Position: refs/heads/master@{#296112} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
