Adopt the modern Hasher more widely.
Created attachment 453364 [details] Patch
Comment on attachment 453364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453364&action=review review- because Color’s hashing is wrong. > Source/WebCore/platform/graphics/Color.h:262 > + add(hasher, color.m_colorAndFlags); When the color is out of line, this is wrong. We need to hash the data in the out-of-line color, not the pointer, since two colors with different pointers can have the same value. That’s what the old Color::hash function did, and this needs to do the same. Also, this should take const Color& since it’s expensive to copy a Color when it’s out-of-line.
Comment on attachment 453364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453364&action=review >> Source/WebCore/platform/graphics/Color.h:262 >> + add(hasher, color.m_colorAndFlags); > > When the color is out of line, this is wrong. We need to hash the data in the out-of-line color, not the pointer, since two colors with different pointers can have the same value. That’s what the old Color::hash function did, and this needs to do the same. > > Also, this should take const Color& since it’s expensive to copy a Color when it’s out-of-line. My bad, I did look at the previous hasher but missed the fact that we could end up with a pointer. I'll re-introduce the previous hasher behavior.
Created attachment 453392 [details] Patch
Comment on attachment 453392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453392&action=review Love seeing this. > Source/WebKit/Platform/IPC/StringReference.cpp:62 > + return computeHash(a); This parameter names could be better. string? reference? stringReference?
Created attachment 453397 [details] Patch
Committed r290610 (247883@main): <https://commits.webkit.org/247883@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453397 [details].
<rdar://problem/89573649>
Comment on attachment 453397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453397&action=review > Source/WebCore/platform/graphics/Color.h:265 > + if (color.isOutOfLine()) > + add(hasher, color.asOutOfLine().unresolvedComponents(), color.colorSpace(), color.flags().toRaw()); > + else > + add(hasher, color.asPackedInline().value, color.flags().toRaw()); Surprised calling toRaw was needed. We should fix the Hasher mechanism so we don’t need that.
(In reply to Darin Adler from comment #9) > Comment on attachment 453397 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453397&action=review > > > Source/WebCore/platform/graphics/Color.h:265 > > + if (color.isOutOfLine()) > > + add(hasher, color.asOutOfLine().unresolvedComponents(), color.colorSpace(), color.flags().toRaw()); > > + else > > + add(hasher, color.asPackedInline().value, color.flags().toRaw()); > > Surprised calling toRaw was needed. We should fix the Hasher mechanism so we > don’t need that. Oh, I don’t know for sure that it was needed, I copied from the pre-existing hashing function.
(In reply to Chris Dumez from comment #10) > (In reply to Darin Adler from comment #9) > > Comment on attachment 453397 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=453397&action=review > > > > > Source/WebCore/platform/graphics/Color.h:265 > > > + if (color.isOutOfLine()) > > > + add(hasher, color.asOutOfLine().unresolvedComponents(), color.colorSpace(), color.flags().toRaw()); > > > + else > > > + add(hasher, color.asPackedInline().value, color.flags().toRaw()); > > > > Surprised calling toRaw was needed. We should fix the Hasher mechanism so we > > don’t need that. > > Oh, I don’t know for sure that it was needed, I copied from the pre-existing > hashing function. It wasn't needed. I fixed it in <https://commits.webkit.org/r290616>.