[Webkit-unassigned] [Bug 56650] [Chromium] The WebPageSerializer::retrieveAllResources() should retrieve CSS resources

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56650





--- Comment #4 from Adam Barth <abarth at webkit.org>  2011-03-18 15:59:26 PST ---
(From update of attachment 86223)
View in context: https://bugs.webkit.org/attachment.cgi?id=86223&action=review

I realize you removed the review request, but here are some random nits.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:132
> +    // Process the attribute if one was found found.

"found found" => "found"

> Source/WebKit/chromium/src/WebPageSerializer.cpp:137
> +            KURL url(ParsedURLString, element->document()->completeURL(value));

No need to use ParsedURLString.  You can just write:

KURL url = element->document()->completeURL(value);

> Source/WebKit/chromium/src/WebPageSerializer.cpp:143
> +            if (!url.isEmpty() && !url.isNull() && url.isValid()
> +                && (url.protocolIs("http") || url.protocolIs("https") || url.protocolIs("file"))) {
> +                resourceURLs->add(url);
> +            }

You should do the JavaScript URL check together with these.  Also "!url.isEmpty() && !url.isNull() && url.isValid()" are all redundant with the later checks, right?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:149
> +    CSSStyleDeclaration* styleDeclaration = element->style();
> +    if (styleDeclaration)

We usually combine these lines:

if (CSSStyleDeclaration* styleDeclaration = element->style())
  ...

> Source/WebKit/chromium/src/WebPageSerializer.cpp:161
> +    // If we have already seen that frame, ignore it.

This comment is a "what" not a "why" and should be removed.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:189
> +        HTMLElement* element = toHTMLElement(allNodes->item(i));
> +        if (element) {

These should be combined.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:209
> +    // Retrieving the background property would probably be good enough.
> +    // Iterating for any other image properties to be safe (I am not sure there
> +    // are actually others).

Maybe list bullets can have images?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:213
> +        // prop ID, but CSSStyleDeclaration only gives access to property names,

"prop" <-- please don't use abbreviations.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:216
> +        RefPtr<CSSValue> value =
> +            styleDeclaration->getPropertyCSSValue(styleDeclaration->item(i));

There's not 80 col limit.  Please don't add unneeded line breaks.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:225
> +                    KURL(ParsedURLString,
> +                         static_cast<CSSStyleSheet*>(styleDeclaration->stylesheet())->document()->completeURL(url)));

No need to use ParsedURLString.  Doesn't completeURL return a KURL?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:240
> +    if (!visitedStyleSheets->add(styleSheet).second)
> +        return; // We have already seen that styleSheet.

That's a very strange way to test if we've got the stylesheet.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:253
> +            resourceURLs->add(
> +                KURL(ParsedURLString,
> +                    styleSheet->document()->completeURL(importRule->href())));

No ParsedURLString, all on one line.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:257
> +            retrieveResourcesForCSSStyleSheet(
> +                static_cast<CSSImportRule*>(item)->styleSheet(),
> +                visitedStyleSheets, supportedSchemes, resourceURLs);

All on one line.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:265
> +                    CSSFontFaceSrcValue* fontFaceSrc =
> +                        static_cast<CSSFontFaceSrcValue*>(valueList->item(j));

one line.  :)

> Source/WebKit/chromium/src/WebPageSerializer.cpp:315
> +    // Now retrieve CSS resources from style-sheets.

Again, a what, not a why.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.