[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
bugzilla-daemon at webkit.org
Thu Dec 4 16:59:30 PST 2008
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.
More information about the webkit-reviews
mailing list