[webkit-reviews] review granted: [Bug 11850] Webarchive fails to save images referenced in CSS : [Attachment 25821] 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 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 Darin Adler <darin at apple.com>
Test case seems a little huge, but I'm glad it has thorough coverage.

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.

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?

> +void
CSSMutableStyleDeclaration::addSubresourceStyleURLs(PassRefPtr<CSSPrimitiveValu
e> primitiveValue, ListHashSet<KURL>& urls, const KURL& baseURL)

This doesn't take ownership of the value, so it should take a raw pointer
rather than a PassRefPtr.

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


> +    if (property.value()->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE) {

> +	  
addSubresourceStyleURLs(static_cast<CSSPrimitiveValue*>(property.value()),
urls, baseURL);
> +	   return;
> +    }
> +
> +    if (property.value()->cssValueType() == CSSValue::CSS_VALUE_LIST) {
> +	   RefPtr<CSSValueList> valueList =
static_cast<CSSValueList*>(property.value());
> +	   unsigned max = valueList->length();
> +	   for (unsigned i = 0; i < max; ++i) {
> +	       if (valueList->itemWithoutBoundsCheck(i)->cssValueType() ==
CSSValue::CSS_PRIMITIVE_VALUE)
> +		  
addSubresourceStyleURLs(static_cast<CSSPrimitiveValue*>(valueList->itemWithoutB
oundsCheck(i)), urls, baseURL);
> +	   }
> +    }

I'd rather see a switch statement here than two if statements.

Also, valueList->itemWithoutBoundsCheck(i) is done twice. I think you should
use a local variable instead.

> +		   RefPtr<CSSValueList> srcList =
static_cast<CSSValueList*>((*it).value());

The syntax it-> should work here -- no need for (*it). It it doesn't work, then
I guess it's OK to leave it this way. We'll change it when we eliminate
DeprecatedValueList. Actually, I think Antti has already done that so you might
have to change if he lands first.

> +		       RefPtr<CSSFontFaceSrcValue> value =
static_cast<CSSFontFaceSrcValue*>(srcList->itemWithoutBoundsCheck(i));

This doesn't need to be a RefPtr -- a raw pointer will do fine.

> +		       RefPtr<CSSValue> value =
static_cast<CSSBorderImageValue*>((*it).value())->imageValue();

Ditto.

> +		   if (value->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE)

Ditto.

> +    RefPtr<CSSMutableStyleDeclaration> style = inlineStyleDecl();

Ditto.

My concerns are all doubts, not definite problems. So review+