https://trac.webkit.org/changeset/260023 broke the build: Regression in ITP sparse plist decoding. This is an automatic bug report generated by webkitbot. If this bug report was created because of a flaky test, please file a bug for the flaky test (if we don't already have one on file) and dup this bug against that bug so that we can track how often these flaky tests fail.
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].