[webkit-reviews] review cancelled: [Bug 11850] Webarchive fails to save images referenced in CSS : [Attachment 25821] Patch v1

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
Clearing Darin Adler's review+ flag to address issues in Comment #8.  I need to
research the encoding issue.  (Does a stand-alone stylesheet have a document()?
 It may have its own character encoding, though.)

If I didn't reply to a comment below, I will address it in the next patch.

(In reply to comment #8)
> (From update of attachment 25821 [review])
> Test case seems a little huge, but I'm glad it has thorough coverage.

Yes, I was aiming for complete coverage.  Note that I reused the same apple.gif
image in nearly all of the CSS styles but simply added a query string to the
end of the URL.

> I'm not sure I like the change to CSSBorderImageValue.h. Normally the concept

> is that if someone just wants to get at some of the CSSBorderImageValue class

> definition without actually creating one, they don't need to include one
> additional header. The file CSSMutableStyleDeclaration.cpp is one example.
But
> I could be wrong -- you might end up having to add tons of includes of
> "Rect.h"; I'll go along with whatever you decide.

I looked at this.  Ironically, CSSMutableStyleDeclaration.cpp is the first time
that Rect.h would NOT be included with CSSBorderImageValue.h anyway.  I didn't
want to #include Rect.h there just because it was needed for
CSSBorderImageValue.h.	Here's the compiler error:

/symroots/Release/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h: In
destructor ‘WTF::PassRefPtr<T>::~PassRefPtr() [with T = WebCore::Rect]’:
WebCore/css/CSSBorderImageValue.h:36:	instantiated from here
/symroots/Release/JavaScriptCore.framework/PrivateHeaders/PassRefPtr.h:43:
error: invalid use of incomplete type ‘struct WebCore::Rect’
WebCore/css/CSSPrimitiveValue.h:33: error: forward declaration of ‘struct
WebCore::Rect’

> Do we have to add this new test to the skip file for most platforms, since
they
> don't support web archives in this format?

No, the SkippedList for all the non-Mac platforms already includes
LayoutTests/webarchive.

> > +	 addSubresourceURL(urls, KURL(baseURL, parseURL(value->cssText())));
> 
> Is this the right way to construct the URL relative to the base? Don't we
have
> to pass the encoding the way Document::completeURL does? Maybe it would be
> better to pass in a Document* and use completeURL instead of passing a
baseURL.

Will research this.  For stylesheets declared in <style></style> tags, the
document's encoding would be appropriate.  For standalone stylesheets, we'd
need to get the encoding of the given stylesheet instead.