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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 22 15:39:06 PST 2008


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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (!isLocal())
> +	   addSubresourceURL(urls, styleSheet->completeURL(m_resource));

Is the isLocal() check an optimization or a correctness thing?

> --- a/WebCore/css/CSSPrimitiveValue.cpp
> +++ b/WebCore/css/CSSPrimitiveValue.cpp
> @@ -23,10 +23,12 @@
>  
>  #include "CSSHelper.h"
>  #include "CSSPropertyNames.h"
> +#include "CSSStyleSheet.h"
>  #include "CSSValueKeywords.h"
>  #include "Color.h"
>  #include "Counter.h"
>  #include "ExceptionCode.h"
> +#include "Node.h"
>  #include "Pair.h"
>  #include "Rect.h"
>  #include "RenderStyle.h"

Why the include of Node.h?

> +    while (styleSheetList.size() > 0) {
> +	   CSSStyleSheetList::iterator it = styleSheetList.begin();
> +	   CSSStyleSheet* styleSheet = *it;
> +	   styleSheetList.remove(it);

It seems like a mistake to remove style sheets from the list, because then you
could end up processing the same sheet twice. Or maybe this should just be a
Deque rather than a ListHashSet.

> +	       if (rule->isImportRule())
> +		   if (CSSStyleSheet* ruleStyleSheet =
static_cast<CSSImportRule*>(rule)->styleSheet())
> +		       styleSheetList.add(ruleStyleSheet);

Need braces around this multiple line if statement.

> +    unsigned size = m_values.size();
> +    for (unsigned i = 0; i < size; ++i)
> +	   m_values[i]->addSubresourceStyleURLs(urls, styleSheet);

Should be size_t rather than unsigned.

r=me


More information about the webkit-reviews mailing list