[webkit-reviews] review granted: [Bug 22666] Clean up data structures used when collecting URLs of subresources for webarchives : [Attachment 25780] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 22666: Clean up data structures used when collecting URLs of subresources
for webarchives
https://bugs.webkit.org/show_bug.cgi?id=22666

Attachment 25780: Patch v1
https://bugs.webkit.org/attachment.cgi?id=25780&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    virtual void addSubresourceURLStrings(HashSet<String>&, const String&
baseURL) const;
> +    virtual void getSubresourceStyleURLs(ListHashSet<KURL>&, const KURL&
baseURL) const;

> -    virtual void addSubresourceURLStrings(HashSet<String>&, const String&
baseURL) const { }
> +    virtual void getSubresourceStyleURLs(ListHashSet<KURL>&, const KURL&
baseURL) const { }

It seems that if a function is named "get" then it can assume it starts with an
empty set and either add to it or replace the set with a whole new one or
whatever. But if its job is only to add URLs to an existing set, then it should
be named "add". Which kind of functions are these?

> +	       ListHashSet<KURL>::iterator kurlIteratorEnd =
subresourceURLs.end();
> +	       for (ListHashSet<KURL>::iterator kurlIterator =
subresourceURLs.begin(); kurlIterator != kurlIteratorEnd; ++kurlIterator) {

I don't think it's good to use "kurl" in this name. Later we'll rename it.
Maybe subresourceIterator, or it, or iter?

> +inline void addSubresourceURLForLegacyWebArchive(ListHashSet<KURL>& urls,
const KURL& url)

It might be cleaner and inspire less need to include LegacyWebArchive.h if you
make this a static member function of the same class that defines the function
that has to accumulate the URLs. But I also think it's OK as-is.

I think it's a little strange that this function mentions LegacyWebArchive in
its name, but functions like getSubresourceStyleURLs don't. They work as a
team.

> +    addSubresourceURLForLegacyWebArchive(urls,
document()->completeURL(href()));

You also might want a helper that does the completeURL work. Are you sure you
got this right, calling completeURL in all the cases where it's needed and none
of the cases where it's already done?

r=me as-is despite the comments above