[webkit-reviews] review canceled: [Bug 212424] REGRESSION(r260023): ITP sparse plist decoding. : [Attachment 400370] Patch v2

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has canceled David Kilzer
(:ddkilzer) <ddkilzer at webkit.org>'s request for review:
Bug 212424: REGRESSION(r260023): ITP sparse plist decoding.
https://bugs.webkit.org/show_bug.cgi?id=212424

Attachment 400370: Patch v2

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




--- Comment #10 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 400370
  --> https://bugs.webkit.org/attachment.cgi?id=400370
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.