[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 #6 from Jay Civelli <jcivelli at chromium.org>  2011-03-18 18:19:13 PST ---
(In reply to comment #4)

Thanks for the comments!

> (From update of attachment 86223 [details])
> 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"
Done.

> > 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);
Done everywhere.

> > 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?
Looking at KURL.cpp, it seems we still need isValid(), but you are right we don't need the rest or even to check for the JS protocol.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:149
> > +    CSSStyleDeclaration* styleDeclaration = element->style();
> > +    if (styleDeclaration)
> 
> We usually combine these lines:
> 
> if (CSSStyleDeclaration* styleDeclaration = element->style())
>   ...
OK

> 
> > 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.
I thought the comment useful as it is not really obvious that ListHashSet::add() returns a std::pair with second being a bool that indicates whether the element was inserted.

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

> > 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?
You are right! Updated the command and the test.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:213
> > +        // prop ID, but CSSStyleDeclaration only gives access to property names,
> 
> "prop" <-- please don't use abbreviations.
OK

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

> > 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?
Done.

> > 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.
Yeah, LinkedHashSet:add() inserts and returns a pair, second is a boolean that indicates if the value has been inserted.

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

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

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

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:315
> > +    // Now retrieve CSS resources from style-sheets.
> 
> Again, a what, not a why.
Changed.

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