Created attachment 322032 [details] Patch

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?

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

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

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

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

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.

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

(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 :)

Created attachment 322136 [details] Patch

Created attachment 322137 [details] Patch

Created attachment 322140 [details] Patch

Created attachment 322145 [details] Patch

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

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.

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.

Comment on attachment 322145 [details] Patch I guess you're right. r=me.

Comment on attachment 322145 [details] Patch Clearing flags on attachment: 322145 Committed r222664: <http://trac.webkit.org/changeset/222664>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34747281>

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.

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?

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