Bug 237262 – Adopt the modern Hasher more widely

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Adopt the modern Hasher more widely.

[Chris Dumez]

Created attachment 453364 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 453392 [details]
Patch

[Sam Weinig]

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?

[Chris Dumez]

Created attachment 453397 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/89573649>

[Darin Adler]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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>.