[webkit-reviews] review cancelled: [Bug 11850] Webarchive fails to save images referenced in CSS : [Attachment 25753] Pre-patch #2 v1

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has cancelled David Kilzer
(ddkilzer) <ddkilzer at webkit.org>'s request for review:
Bug 11850: Webarchive fails to save images referenced in CSS
https://bugs.webkit.org/show_bug.cgi?id=11850

Attachment 25753: Pre-patch #2 v1
https://bugs.webkit.org/attachment.cgi?id=25753&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
(In reply to comment #4)
> (In reply to comment #3)
> > - The change to WebCore::Node::getSubresourceURLs() removes the parseURL()
> > method from being called on most (if not all) subresource URL strings. 
> > However, this doesn't seem to be necessary for methods like
> > WebCore::HTMLAnchorElement::href().
> 
> Here's what parseURL does:
> 
>     1) Strip leading and trailing spaces.
>     2) Strip URL() if it surrounds the string, allowing any mix of uppercase
> and lowercase.
>     3) Strip leading and trailing spaces.
>     4) Strip balanced single or double quotes.
>     5) Strip leading and trailing spaces.
>     6) Strip all characters in the range U+0000-U+000C
> 
> It's very strange that we want to do this anywhere except in CSS. In fact, I
> think it's a bug almost every where this function is used. Perhaps some of
this
> is needed but obviously (2) is something specific to CSS property values even

> if some of the other behavior is useful.
> 
> Someone should create some tests cases and investigate the behavior of other
> web browsers for the various places we call this function. And at the same
time
> we should probably rename it.

Filed Bug 22664.

> > - WebCore::KURLHash::hash() in KURLHash.h was modified to handle "null"
KURL
> > objects (which would otherwise cause a crash if you tried to add one to a
> > hash).
> 
> Changing the hash function in this way won't work. The null KURL is the empty

> value, so it can't be used as a value in a set. It's not just the hash
> function. In fact, you should get an assertion in the HashTable
implementation
> if you try to add an empty KURL, and if you don't then there is some sort of
> bug.

I really wanted to avoid adding code to check whether KURL.isNull() every place
that adds KURL objects to the list.  I don't see an easy way to do this right
now.

> Using a set introduces a sort of randomness (not something truly random, but
> close enough) into the process that wasn't there before. Perhaps we should
sort
> the URLs in the set so we process them in a predictable order?

I could use a ListHashSet instead.

Unfortunately I don't have time to fix this clean-up patch, so I filed Bug
22666.