Bug 212424

Created attachment 400358 [details]
REVERT of r260023

Any committer can land this patch automatically by marking it commit-queue+.  The commit-queue will build and test the patch before landing to ensure that the revert will be successful.  This process takes approximately 15 minutes.

If you would like to land the revert faster, you can use the following command:

  webkit-patch land-attachment ATTACHMENT_ID

where ATTACHMENT_ID is the ID of this attachment.

<rdar://problem/63683055>

*** Bug 212422 has been marked as a duplicate of this bug. ***

*** Bug 212423 has been marked as a duplicate of this bug. ***

Looks like the revert patch needs to be fixed up.  Working on that now.

Created attachment 400367 [details]
Changes required  on top of revert

Changes necessary on top of revert when rolling out r260023.

Created attachment 400370 [details]
Patch v2

Comment on attachment 400370 [details]
Patch v2

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

> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)

Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?

Comment on attachment 400370 [details]
Patch v2

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

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
>> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)
> 
> Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?

That would work too. Or it could return an optional boolean where the tristate is true == found and decoded a value, nullopt == didn't find a value, false == found a value but failed to decode it.

Yet another option is to return a bool but create empty instances for false in ResourceLoadStatistics::decode() instead of bailing out.

Comment on attachment 400370 [details]
Patch v2

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

>>> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
>>> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)
>> 
>> Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?
> 
> That would work too. Or it could return an optional boolean where the tristate is true == found and decoded a value, nullopt == didn't find a value, false == found a value but failed to decode it.
> 
> Yet another option is to return a bool but create empty instances for false in ResourceLoadStatistics::decode() instead of bailing out.

Now is not the time to redesign the code.  We're trying to do a rollout.

I went with a third (hybrid) option that changes decodeHashCountedSet() and decodeHashSet() to return the `bool` value from KeyedDecoder::decodeObjects(), which fixes the compiler warning but leaves the return value of these static methods unchecked.  Semantically, this is no worse than what the patch was doing before—we just pass the bool up to a different function to ignore, and assume the caller knows what it's doing.

Created attachment 400389 [details]
Patch v3

Comment on attachment 400370 [details]
Patch v2

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

>>>> Source/WebCore/loader/ResourceLoadStatistics.cpp:126
>>>> +static void decodeHashCountedSet(KeyedDecoder& decoder, const String& label, HashCountedSet<RegistrableDomain>& hashCountedSet)
>>> 
>>> Wouldn't it be simpler to just return true here with a comment explaining why we need to always return true here, for compatibility?
>> 
>> That would work too. Or it could return an optional boolean where the tristate is true == found and decoded a value, nullopt == didn't find a value, false == found a value but failed to decode it.
>> 
>> Yet another option is to return a bool but create empty instances for false in ResourceLoadStatistics::decode() instead of bailing out.
> 
> Now is not the time to redesign the code.  We're trying to do a rollout.
> 
> I went with a third (hybrid) option that changes decodeHashCountedSet() and decodeHashSet() to return the `bool` value from KeyedDecoder::decodeObjects(), which fixes the compiler warning but leaves the return value of these static methods unchecked.  Semantically, this is no worse than what the patch was doing before—we just pass the bool up to a different function to ignore, and assume the caller knows what it's doing.

Could also add a IGNORE_CLANG_WARNINGS_BEGIN("unused-result")/IGNORE_CLANG_WARNINGS_END around the KeyedDecoder::decodeObjects() function calls and change decodeHashCountedSet() and decodeHashSet() back to void functions.  Maybe that's a bit more self-documenting.

Created attachment 400394 [details]
Patch v4

Comment on attachment 400394 [details]
Patch v4

Yes, I like this version better. Self-documenting, as you say.

Comment on attachment 400389 [details]
Patch v3

Marking obsolete in order to take Patch v4.  Thanks for the review, Alex.

Waiting for EWS before marking cq+.

Comment on attachment 400394 [details]
Patch v4

Windows layout test failures appear to be unrelated to this change, so marking cq+.

Committed r262228: <https://trac.webkit.org/changeset/262228>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400394 [details].