Bug 187260

<rdar://problem/41735280>

Created attachment 344127 [details]
Patch v1

Attachment 344127 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/CryptoAlgorithmIdentifier.h:33:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 344127 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=344127&action=review

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
>          Type type;

Should type have a default value as well?

Comment on attachment 344127 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=344127&action=review

>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
>>          Type type;
> 
> Should type have a default value as well?

Yes, but there is no good "Invalid" option to use here as a default value.

I'm torn between using Type::Codec (which would have a value of zero), adding a new "Invalid" enum, or doing nothing and letting the static analyzer warn when a subclass (substruct?) fails to set it.

Thoughts?

Comment on attachment 344127 [details]
Patch v1

LGTM, modulo the comment on VRDisplayEvent below.

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 344127 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344127&action=review
> 
> >> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
> >>          Type type;
> > 
> > Should type have a default value as well?
> 
> Yes, but there is no good "Invalid" option to use here as a default value.

And we cannot add one since it is bound to IDL.

> I'm torn between using Type::Codec (which would have a value of zero),
> adding a new "Invalid" enum, or doing nothing and letting the static
> analyzer warn when a subclass (substruct?) fails to set it.
> 
> Thoughts?

Ideally, the analyzer warning should be an error for that specific case.
If we cannot make it so, status quo seems fine to me.

View in context: https://bugs.webkit.org/attachment.cgi?id=344127&action=review

> Source/WebCore/Modules/webvr/VRDisplayEvent.h:39
> +        VRDisplayEventReason reason { VRDisplayEventReason::Mounted };

Looking at this one, reason is not required, so it should either be std::optional<VRDisplayEventReason> or something like VRDisplayEventReason::None.

Comment on attachment 344127 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=344127&action=review

>>>> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
>>>>          Type type;
>>> 
>>> Should type have a default value as well?
>> 
>> Yes, but there is no good "Invalid" option to use here as a default value.
>> 
>> I'm torn between using Type::Codec (which would have a value of zero), adding a new "Invalid" enum, or doing nothing and letting the static analyzer warn when a subclass (substruct?) fails to set it.
>> 
>> Thoughts?
> 
> And we cannot add one since it is bound to IDL.

I reran the clang static analyzer in deep mode, and I found that in JSRTCStatsReport.cpp, the various convertDictionary() methods can leave Stats.type and (below) IceCandidatePairStats.state undefined.

What are reasonable default values in each of these cases?  The ones that equate to the value zero?

> > And we cannot add one since it is bound to IDL.
> 
> I reran the clang static analyzer in deep mode, and I found that in
> JSRTCStatsReport.cpp, the various convertDictionary() methods can leave
> Stats.type and (below) IceCandidatePairStats.state undefined.
> 
> What are reasonable default values in each of these cases?  The ones that
> equate to the value zero?

Would std::optional<Type> type do the trick?

(In reply to youenn fablet from comment #8)
> > > And we cannot add one since it is bound to IDL.
> > 
> > I reran the clang static analyzer in deep mode, and I found that in
> > JSRTCStatsReport.cpp, the various convertDictionary() methods can leave
> > Stats.type and (below) IceCandidatePairStats.state undefined.
> > 
> > What are reasonable default values in each of these cases?  The ones that
> > equate to the value zero?
> 
> Would std::optional<Type> type do the trick?

Yes.  We'd have to update the IDL for that change, or just change the instance variables?  (Sorry, I'm not up-to-date on the latest IDL support.)

As long as the dictionary member is not required in the IDL, I would think we can use optional directly.

Comment on attachment 344127 [details]
Patch v1

Clearing r? flag until I can post a new patch.

Comment on attachment 344127 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=344127&action=review

>> Source/WebCore/Modules/webvr/VRDisplayEvent.h:39
>> +        VRDisplayEventReason reason { VRDisplayEventReason::Mounted };
> 
> Looking at this one, reason is not required, so it should either be std::optional<VRDisplayEventReason> or something like VRDisplayEventReason::None.

Youenn is right the reason is optional. However I'd advice not to change this because I'm going to modify it in a patch that is about to be uploaded for review.

(In reply to Sergio Villar Senin from comment #12)
> Comment on attachment 344127 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344127&action=review
> 
> >> Source/WebCore/Modules/webvr/VRDisplayEvent.h:39
> >> +        VRDisplayEventReason reason { VRDisplayEventReason::Mounted };
> > 
> > Looking at this one, reason is not required, so it should either be std::optional<VRDisplayEventReason> or something like VRDisplayEventReason::None.
> 
> Youenn is right the reason is optional. However I'd advice not to change
> this because I'm going to modify it in a patch that is about to be uploaded
> for review.

Okay.  Assuming this is Bug 187343.