[webkit-reviews] review granted: [Bug 11850] Webarchive fails to save images referenced in CSS : [Attachment 26184] Patch v3

[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 26184: Patch v3
https://bugs.webkit.org/attachment.cgi?id=26184&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
This looks really good!

I have two major suggestions:

    1) Maybe this code could be written to use more virtual functions. In
CSSValue and CSSRule. To avoid the switch statements on types of value and
types of rule.

    2) Perhaps this could be written to ignore the property ID and do
everything based on the type of value rather than the property. I think that
would be more robust against changes to CSS; is there a reason not to do it
that way?

Many of my comments below about switch statement style and type vs. is
functions will be irrelevant if you use virtual functions instead of switch
statements.

> +	   RefPtr<CSSValueList> valueList =
static_cast<CSSValueList*>(property.value());

No need to use a RefPtr here; the local code doesn't have to take ownership of
the list; this should just be a raw pointer. I see that mistake in multiple
places in this new code, and it's unnecessarily inefficient. (More detailed
discussion below.)

> +	   unsigned max = valueList->length();

To my taste, it's a little strange to call this "max". My general rule is to
adopt the same naming as the class in question. So if CSSValueList calls this
length(), then I call the local variable length. I won't repeat this comment
about other variables named "max" in the patch, but it applies to them as well.


> +	       if (value->cssValueType() == CSSValue::CSS_PRIMITIVE_VALUE)
> +		  
addSubresourceStyleURLs(static_cast<CSSPrimitiveValue*>(value), urls,
styleSheet);

I think it might be better to use isPrimitiveValue() instead of cssValueType(),
since there's no switch statement involved and the syntax would be easier to
read. But also I don't understand why primitive values are the only types that
could have URLs in them. What about font values, for example, and image values?
As I mentioned above, I'd prefer an approach that lets the rules and values
handle things with virtual functions and have fewer assumptions about what
types can appear in lists, and what types might have URLs.

> +    default:
> +	   break;

My usual approach in switch statements like this is to list the types that I'm
not handling, to take advantage of gcc's warning about missing cases. That way
if a new enum value is added later, the person adding it will be reminded to
consider whether to include a case at this site, because they'll have to fix a
compiler warning by adding a case to the switch for the new type. Including
"default: break" prevents that warning. Would you consider listing the other
enum types instead? One good thing about having those cases is that you can
then have a comment explaining why those types can be skipped.

> +    RefPtr<CSSStyleSheet> styleSheet =
static_cast<CSSStyleSheet*>(stylesheet());

Should be a raw pointer, not RefPtr (see explanation above). Also, the only
reason there's no name conflict here is case (styleSheet vs. stylesheet). It's
annoying we have a mix of cases for this term!

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

Another case where it should be a raw pointer.

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

And another.

> +	   default:
> +	       break;

In this case, the property ID switch statement, I think we *do* need a default,
because there are simply too many other properties to list. But I'd like a
comment here saying something specific about wby the other properties aren't
listed here and explaining which types of properties would need to be added to
the switch in the future. But this is an argument in favor of a design that
ignores the property IDs and does the whole thing based on the CSS values
instead, suggestion (2) above.

> +    static void addSubresourceStyleURLs(CSSPrimitiveValue*,
ListHashSet<KURL>&, const CSSStyleSheet* styleSheet);
> +    static void addSubresourceStyleURLs(const CSSProperty& property,
ListHashSet<KURL>&, const CSSStyleSheet* styleSheet);

You should omit the argument name for the CSSProperty and CSSStyleSheet
arguments in these declarations. The type speaks for itself.

> +    typedef ListHashSet<RefPtr<CSSStyleSheet> > CSSStyleSheetList;

I think this list can collect raw pointers instead of RefPtr. The only reason
you'd need to use RefPtr would be if the code does something that can mutate
the DOM or CSS during the loop. Accumulating URLs in a collection should not
fall into that category. And raw pointers are more efficient.

> +	   RefPtr<CSSStyleSheet> styleSheet = *it;

Again, raw pointer please.

> +	       RefPtr<CSSRule> rule = ruleList->item(i);

Raw pointer.

> +	       switch (rule->type()) {

Consider having virtual functions in CSSRule instead of using a switch
statement here, suggestion (2) above.

> +	       default:
> +		   break;

Same comment about default as above.

> -StyleSheet* StyleBase::stylesheet()
> +StyleSheet* StyleBase::stylesheet() const

In general, I think it's a bad idea to have const member functions in classes
that are part of a tree such as the DOM and render trees. We have a lot of
them, and I'd like to cut down on that. If you have a const pointer to one
element in the tree, and look at its parent, then you end up with non-const
pointer so const doesn't really work all that well. I'd rather move in the
direction of less const rather than more in the future. So if it was me, I
would not add const here.

> +void StyledElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls)
const
> +{
> +    CSSMutableStyleDeclaration* style = inlineStyleDecl();
> +    if (style)
> +	   style->addSubresourceStyleURLs(urls);
> +}

Putting the definition inside the if statement can be good for a function like
this:

    if (CSSMutableStyleDeclaration* style = inlineStyleDecl())
	style->addSubresourceStyleURLs(urls);

Please consider that style for cases like this when it's practical. One benefit
is that you can't accidentally use the null pointer because when it's null it's
not even in scpe.

>  void HTMLEmbedElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls)
const
>  {
> +    StyledElement::addSubresourceAttributeURLs(urls);
> +
>      addSubresourceURL(urls, document()->completeURL(src()));
>  }

The normal style here is to name the base class when calling the base class's
version of the function, so in this case the call would be
HTMLPlugInImageElement::addSubresourceAttributeURLs. It's true that the base
class doesn't override this function and so the one that will actually be
called is the one in StyledElement, but calling for StyledElement explicitly
has the unusual effect of skipping overrides in intervening classes. Although
there are no such overrides now, they could be added later. Calling directly to
a class farther up the hierarchy should normally only be done when there's a
specific reason skip some clases.

Same comment about the other addSubresourceAttributeURLs functions such as the
one in HTMLBodyElement, etc.

>      StyleSheet* styleSheet = const_cast<HTMLStyleElement*>(this)->sheet();
>      if (styleSheet)
> -	   styleSheet->addSubresourceStyleURLs(urls,
ownerDocument()->baseURL());
> +	   styleSheet->addSubresourceStyleURLs(urls);

Same comment about putting the definition inside the if statement as above.

And here's yet another example where const is not serving us well! All those
const_cast start to get annoying and I don't think the const is providing much
benefit.

This patch is good, and could be landed as-is, r=me.

But ideally you'd resolve some of the things I mention in this comment either
before or after landing the changes. I'll leave it up to you know to handle
that.