Bug 212230 – Use an OptionSet instead of uint8_t for MessageFlags

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use an OptionSet instead of uint8_t for MessageFlags

[Alex Christensen]

Created attachment 399979 [details]
Patch

[Alex Christensen]

<rdar://problem/63496543>

[Darin Adler]

Comment on attachment 399979 [details]
Patch

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

> Source/WebKit/Platform/IPC/ArgumentCoders.h:58
> +        encoder << (static_cast<typename OptionSet<T>::StorageType>(optionSet.toRaw()));

I don’t think the static_cast is needed any more here. OptionSet::toRaw already returns this type. Could just be:

    encoder << optionSet.toRaw();

> Source/WebKit/Platform/IPC/Decoder.cpp:29
> +#include "ArgumentCoders.h"

Don’t understand why this is now needed and wasn't before, but I assume there’s a reason.

> Source/WebKit/Platform/IPC/Encoder.cpp:117
> -        *buffer() |= DispatchMessageWhenWaitingForUnboundedSyncReply;
> -        *buffer() &= ~DispatchMessageWhenWaitingForSyncReply;
> +        messageFlags().remove(MessageFlags::DispatchMessageWhenWaitingForSyncReply);
> +        messageFlags().add(MessageFlags::DispatchMessageWhenWaitingForUnboundedSyncReply);

Above you kept the order the same, but here you changed the order. Any reason?

> Source/WebKit/Platform/IPC/Encoder.cpp:179
> +    static_assert(sizeof(OptionSet<MessageFlags>::StorageType) == sizeof(uint8_t), "Encoder uses the first byte of the buffer for message flags.");

The use of sizeof(uint8_t) here is a bit confusing to me. Maybe just write 1 instead?

> Source/WebKit/Platform/IPC/Encoder.h:40
> +enum class ShouldDispatchWhenWaitingForSyncReply : uint8_t;
> +enum class MessageFlags : uint8_t;
>  enum class MessageName : uint16_t;

Re-sort alphabetically?

> Source/WebKit/Platform/IPC/MessageFlags.h:41
> -enum class ShouldDispatchWhenWaitingForSyncReply { No, Yes, YesDuringUnboundedIPC };
> +enum class ShouldDispatchWhenWaitingForSyncReply : uint8_t {
> +    No,
> +    Yes,
> +    YesDuringUnboundedIPC
> +};

Adding uint8_t, definitely an improvement. Spreading this out over 5 lines, doesn’t seem as obviously an improvement to me.

[Alex Christensen]

Comment on attachment 399979 [details]
Patch

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

>> Source/WebKit/Platform/IPC/Decoder.cpp:29
>> +#include "ArgumentCoders.h"
> 
> Don’t understand why this is now needed and wasn't before, but I assume there’s a reason.

In code I didn't change, we encode m_messageFlags which now needs the OptionSet encoder because the type changed.

>> Source/WebKit/Platform/IPC/Encoder.cpp:117
>> +        messageFlags().add(MessageFlags::DispatchMessageWhenWaitingForUnboundedSyncReply);
> 
> Above you kept the order the same, but here you changed the order. Any reason?

I figured as long as I'm changing both lines, the order being consistent with case ShouldDispatchWhenWaitingForSyncReply::Yes is nicer.

>> Source/WebKit/Platform/IPC/MessageFlags.h:41
>> +};
> 
> Adding uint8_t, definitely an improvement. Spreading this out over 5 lines, doesn’t seem as obviously an improvement to me.

This makes the change history cleaner if we ever add another one.

[Alex Christensen]

http://trac.webkit.org/r262032