Bug 177575 – Split some logic out of VisitedLinkStore and make it reusable

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Split some logic out of VisitedLinkStore and make it reusable for other purposes than visited links and from other processes than the UIProcess.

[Chris Dumez]

Created attachment 322032 [details]
Patch

[Alex Christensen]

Comment on attachment 322032 [details]
Patch

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

> Source/WebKit/Shared/LinkHashTable.h:35
> +class LinkHashTable {

Since we're going to be using this for things that aren't links, could we call this something like SharedStringHashTable?

> Source/WebKit/Shared/LinkHashTable.h:45
> +    bool contains(WebCore::LinkHash) const;

Could we rename LinkHash to SharedStringHash?

[Alex Christensen]

Specifically, I think we're going to be putting SecurityOrigins into the shared hash table for service workers, so they won't even be URLs.

[Chris Dumez]

(In reply to Alex Christensen from comment #3)
> Specifically, I think we're going to be putting SecurityOrigins into the
> shared hash table for service workers, so they won't even be URLs.

A SecurityOrigin can be stored as an URL and thus as a LinkHash as far as I can tell? Renaming LinkHash would make sense though.

[Chris Dumez]

(In reply to Alex Christensen from comment #3)
> Specifically, I think we're going to be putting SecurityOrigins into the
> shared hash table for service workers, so they won't even be URLs.(In reply to Chris Dumez from comment #4)
> (In reply to Alex Christensen from comment #3)
> > Specifically, I think we're going to be putting SecurityOrigins into the
> > shared hash table for service workers, so they won't even be URLs.
> 
> A SecurityOrigin can be stored as an URL and thus as a LinkHash as far as I
> can tell? Renaming LinkHash would make sense though.

Also, if we eventually store the scope (path), storing as a URL makes even more sense.

[Alex Christensen]

Either way, it will be a String that is not a link.

[Chris Dumez]

(In reply to Alex Christensen from comment #6)
> Either way, it will be a String that is not a link.

How about renaming LinkHash to URLHash since it is constructed from a URL?
Then I'd rename LinkHashTable to URLHashTable.

[Sam Weinig]

Comment on attachment 322032 [details]
Patch

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

> Source/WebKit/Shared/WebSharedLinkHashStore.h:36
> +class WebSharedLinkHashStore {

Why the Web prefix? Seems like SharedLinkHashStore would be fine.

[Alex Christensen]

(In reply to Chris Dumez from comment #7)
> (In reply to Alex Christensen from comment #6)
> > Either way, it will be a String that is not a link.
> 
> How about renaming LinkHash to URLHash since it is constructed from a URL?
> Then I'd rename LinkHashTable to URLHashTable.
It's constructed from a String.

[Chris Dumez]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 322032 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322032&action=review
> 
> > Source/WebKit/Shared/WebSharedLinkHashStore.h:36
> > +class WebSharedLinkHashStore {
> 
> Why the Web prefix? Seems like SharedLinkHashStore would be fine.

Good to know. I have no idea when it is suitable to use the Web prefixed and when it is not :)

[Chris Dumez]

Created attachment 322136 [details]
Patch

[Chris Dumez]

Created attachment 322137 [details]
Patch

[Chris Dumez]

Created attachment 322140 [details]
Patch

[Chris Dumez]

Created attachment 322145 [details]
Patch

[Sam Weinig]

(In reply to Chris Dumez from comment #10)
> (In reply to Sam Weinig from comment #8)
> > Comment on attachment 322032 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=322032&action=review
> > 
> > > Source/WebKit/Shared/WebSharedLinkHashStore.h:36
> > > +class WebSharedLinkHashStore {
> > 
> > Why the Web prefix? Seems like SharedLinkHashStore would be fine.
> 
> Good to know. I have no idea when it is suitable to use the Web prefixed and
> when it is not :)

Never. It was a mistake of my long forgotten youth.

[Alex Christensen]

Comment on attachment 322145 [details]
Patch

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

> Source/WebCore/platform/SharedStringHash.cpp:225
> +static ALWAYS_INLINE void computeSharedStringHashInline(const URL& base, const CharacterType* characters, unsigned length, Vector<CharacterType, 512>& buffer)

It looks like this is specific to the visited links, since it does a bunch of stuff with URLs.  This should probably not be shared.

[Chris Dumez]

Comment on attachment 322145 [details]
Patch

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

>> Source/WebCore/platform/SharedStringHash.cpp:225
>> +static ALWAYS_INLINE void computeSharedStringHashInline(const URL& base, const CharacterType* characters, unsigned length, Vector<CharacterType, 512>& buffer)
> 
> It looks like this is specific to the visited links, since it does a bunch of stuff with URLs.  This should probably not be shared.

It computes a SharedStringHash from a URL. Not sure I see anything wrong with using generic naming here.

[Alex Christensen]

Comment on attachment 322145 [details]
Patch

I guess you're right.  r=me.

[WebKit Commit Bot]

Comment on attachment 322145 [details]
Patch

Clearing flags on attachment: 322145

Committed r222664: <http://trac.webkit.org/changeset/222664>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34747281>

[Darin Adler]

Comment on attachment 322145 [details]
Patch

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

>>> Source/WebCore/platform/SharedStringHash.cpp:225
>>> +static ALWAYS_INLINE void computeSharedStringHashInline(const URL& base, const CharacterType* characters, unsigned length, Vector<CharacterType, 512>& buffer)
>> 
>> It looks like this is specific to the visited links, since it does a bunch of stuff with URLs.  This should probably not be shared.
> 
> It computes a SharedStringHash from a URL. Not sure I see anything wrong with using generic naming here.

I think the generic naming might not be great.

What makes this the hash of a URL a "link hash" is that when computing the hash of a URL, the code ignores things to allow links that are “equivalent” in a high level way have the same hash. It’s not just a hash of the contents of the URL, it’s a hash that goes out of its way to ignore certain classes of differences and guarantees that these equivalent sets of links have the same hashes. I don’t think the name “link hash” is exactly right for this either.

The second thing that makes this hash different from the other hashes we provide in WTF is that this is a hash that is intended to be used without a fall back to string comparison. We use this hash to check if URLs are the same and we don’t compare the URL strings at all. That’s why it has to be larger than 32 bits.

I don’t think that “shared string hash” makes it clear that the hash of a URL has these properties, nor that the hash of the strings does, so I’m not sure it’s a great name. A great name would make it clear that it’s a hash used for direct comparisons rather than a hash used along with actual equality comparisons, and that’s why it’s large, and could drive other tradeoffs. A great name might also make it clear that the URL function is specifically designed for “link coloring” and might not be appropriate for all cases of hashing URLs.

[Chris Dumez]

Comment on attachment 322145 [details]
Patch

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

>>>> Source/WebCore/platform/SharedStringHash.cpp:225
>>>> +static ALWAYS_INLINE void computeSharedStringHashInline(const URL& base, const CharacterType* characters, unsigned length, Vector<CharacterType, 512>& buffer)
>>> 
>>> It looks like this is specific to the visited links, since it does a bunch of stuff with URLs.  This should probably not be shared.
>> 
>> It computes a SharedStringHash from a URL. Not sure I see anything wrong with using generic naming here.
> 
> I think the generic naming might not be great.
> 
> What makes this the hash of a URL a "link hash" is that when computing the hash of a URL, the code ignores things to allow links that are “equivalent” in a high level way have the same hash. It’s not just a hash of the contents of the URL, it’s a hash that goes out of its way to ignore certain classes of differences and guarantees that these equivalent sets of links have the same hashes. I don’t think the name “link hash” is exactly right for this either.
> 
> The second thing that makes this hash different from the other hashes we provide in WTF is that this is a hash that is intended to be used without a fall back to string comparison. We use this hash to check if URLs are the same and we don’t compare the URL strings at all. That’s why it has to be larger than 32 bits.
> 
> I don’t think that “shared string hash” makes it clear that the hash of a URL has these properties, nor that the hash of the strings does, so I’m not sure it’s a great name. A great name would make it clear that it’s a hash used for direct comparisons rather than a hash used along with actual equality comparisons, and that’s why it’s large, and could drive other tradeoffs. A great name might also make it clear that the URL function is specifically designed for “link coloring” and might not be appropriate for all cases of hashing URLs.

I don't have a good name suggestion at the moment. How about I keep the previous naming (computeVisitedLinkHash) for this function?

[Darin Adler]

(In reply to Chris Dumez from comment #23)
> I don't have a good name suggestion at the moment. How about I keep the
> previous naming (computeVisitedLinkHash) for this function?

Sounds OK to me.