Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514123004:
Fix range checking for PP_CdmKeyStatus (Closed)
DescriptionFix range checking for PP_CdmKeyStatus
The specified maximum value doesn't allow PP_CDMKEYSTATUS_RELEASED,
so update it.
BUG=666975
TEST=Tested with W3C EME tests
Committed: https://crrev.com/c24785e7ff8bf6ce47d903b598d813e1bbd17fab
Cr-Commit-Position: refs/heads/master@{#433754}
Patch Set 1 #
Total comments: 3
Patch Set 2 : add _MAX values #
Total comments: 2
Patch Set 3 : more _MAX values #
Messages
Total messages: 25 (12 generated)
jrummell@chromium.org changed reviewers: + dcheng@chromium.org, xhwang@chromium.org
PTAL. IPC_ENUM_TRAITS_MAX_VALUE for PP_CdmKeyStatus was set incorrectly.
The CQ bit was checked by jrummell@chromium.org to run a CQ dry run
xhwang@chromium.org changed reviewers: + bbudge@chromium.org
+bbudge from ppapi's perspective https://codereview.chromium.org/2514123004/diff/1/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/2514123004/diff/1/ppapi/proxy/ppapi_messages.... ppapi/proxy/ppapi_messages.h:98: IPC_ENUM_TRAITS_MAX_VALUE(PP_CdmKeyStatus, PP_CDMKEYSTATUS_RELEASED) The fix lg but is still error prone. Can we have something like PP_CDMKEYSTATUS_MAX = PP_CDMKEYSTATUS_RELEASED and use PP_CDMKEYSTATUS_MAX here? This is the pattern we use for IPC, but doesn't seem like widely used for ppapi messages.
BTW, nice catch!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2514123004/diff/1/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/2514123004/diff/1/ppapi/proxy/ppapi_messages.... ppapi/proxy/ppapi_messages.h:98: IPC_ENUM_TRAITS_MAX_VALUE(PP_CdmKeyStatus, PP_CDMKEYSTATUS_RELEASED) On 2016/11/19 01:27:51, xhwang wrote: > The fix lg but is still error prone. Can we have something like > > PP_CDMKEYSTATUS_MAX = PP_CDMKEYSTATUS_RELEASED > > and use PP_CDMKEYSTATUS_MAX here? > > This is the pattern we use for IPC, but doesn't seem like widely used for ppapi > messages. I think it's fine to add a PP_*_MAX value to a Pepper enum.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2514123004/diff/1/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/2514123004/diff/1/ppapi/proxy/ppapi_messages.... ppapi/proxy/ppapi_messages.h:98: IPC_ENUM_TRAITS_MAX_VALUE(PP_CdmKeyStatus, PP_CDMKEYSTATUS_RELEASED) On 2016/11/19 01:58:11, bbudge wrote: > On 2016/11/19 01:27:51, xhwang wrote: > > The fix lg but is still error prone. Can we have something like > > > > PP_CDMKEYSTATUS_MAX = PP_CDMKEYSTATUS_RELEASED > > > > and use PP_CDMKEYSTATUS_MAX here? > > > > This is the pattern we use for IPC, but doesn't seem like widely used for > ppapi > > messages. > > I think it's fine to add a PP_*_MAX value to a Pepper enum. Let's just add a _MAX value then =)
Updated to use PP_*_MAX values.
The CQ bit was checked by jrummell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2514123004/diff/20001/ppapi/api/private/pp_co... File ppapi/api/private/pp_content_decryptor.idl (right): https://codereview.chromium.org/2514123004/diff/20001/ppapi/api/private/pp_co... ppapi/api/private/pp_content_decryptor.idl:415: PP_SESSIONTYPE_PERSISTENT_RELEASE = 2 Can we fix this, as well as others in this file that are referred by IPC_ENUM_TRAITS_MAX_VALUE?
Now with more _MAX values. https://codereview.chromium.org/2514123004/diff/20001/ppapi/api/private/pp_co... File ppapi/api/private/pp_content_decryptor.idl (right): https://codereview.chromium.org/2514123004/diff/20001/ppapi/api/private/pp_co... ppapi/api/private/pp_content_decryptor.idl:415: PP_SESSIONTYPE_PERSISTENT_RELEASE = 2 On 2016/11/21 19:35:18, xhwang wrote: > Can we fix this, as well as others in this file that are referred by > IPC_ENUM_TRAITS_MAX_VALUE? Done.
Thanks! LGTM
ipc lgtm, thanks!
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/2514123004/#ps40001 (title: "more _MAX values")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479778451982870,
"parent_rev": "59581edbcee74117a87906f3b58061744a0c087a", "commit_rev":
"e10010f86bfd3f1332202387559b5847d097c0c2"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix range checking for PP_CdmKeyStatus The specified maximum value doesn't allow PP_CDMKEYSTATUS_RELEASED, so update it. BUG=666975 TEST=Tested with W3C EME tests ========== to ========== Fix range checking for PP_CdmKeyStatus The specified maximum value doesn't allow PP_CDMKEYSTATUS_RELEASED, so update it. BUG=666975 TEST=Tested with W3C EME tests Committed: https://crrev.com/c24785e7ff8bf6ce47d903b598d813e1bbd17fab Cr-Commit-Position: refs/heads/master@{#433754} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c24785e7ff8bf6ce47d903b598d813e1bbd17fab Cr-Commit-Position: refs/heads/master@{#433754} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
