[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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86250|review?                     |review-, commit-queue-
               Flag|                            |




--- Comment #9 from David Levin <levin at chromium.org>  2011-03-25 10:53:12 PST ---
(From update of attachment 86250)
View in context: https://bugs.webkit.org/attachment.cgi?id=86250&action=review

Does this do an url rewriting?

If a page comes from http://www.example.com/ and refers to "http://www.example.com/other.jpg", how does "other.jpg" get loaded from the serialized version?

Many places where I ask questions are things that are not obvious to me from looking at the code and may be are indicative of comments that should be added to the code.  (Also since I don't know this code well of course some questions have obvious answers to you but not to me :).)

> Source/WebKit/chromium/src/WebPageSerializer.cpp:90
> +        || element->hasTagName(HTMLNames::objectTag) || element->hasTagName(HTMLNames::embedTag))

The indentation on these two continuation lines seems odd. Typically it should align just inside the parenthesis that is continued and that happens on neither line.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:92
> +        Frame* frame = static_cast<const HTMLFrameOwnerElement*>(element)->contentFrame();

Why do we continue through the code if the element has that tag name but !element->isFrameOwnerElement?

Is this code important to that case:
if (CSSStyleDeclaration* styleDeclaration = element->style())
        retrieveResourcesForCSSStyleDeclaration(styleDeclaration, resourceURLs);

and if so, why isn't it important when element->isFrameOwnerElement ?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:101
>      if (element->hasTagName(HTMLNames::imgTag) || element->hasTagName(HTMLNames::scriptTag))

I would like this better as a separate function. 

Then the code would change as well ideally.

It would look something like this (and have a name that tells me what it is doing).

if (element->hasTagName(HTMLNames::imgTag) || element->hasTagName(HTMLNames::scriptTag))
    return &HTMLNames::srcAttr;

if (element->hasTagName(HTMLNames::inputTag)) {
    if (input->isImageButton())
        return &HTMLNames::srcAttr;
    return 0;
}

> Source/WebKit/chromium/src/WebPageSerializer.cpp:140
> +        // Ignore javascript content.

How is this ignoring javascript content?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:144
> +            // does no have a cache mechanism, we skip it as well.

s/no/not/

> Source/WebKit/chromium/src/WebPageSerializer.cpp:219
> +            // Non cached-image are just place-holders and do not contain data.

images (since you used "are", etc.)

> Source/WebKit/chromium/src/WebPageSerializer.cpp:222
> +                resourceURLs->add(static_cast<CSSStyleSheet*>(styleDeclaration->stylesheet())->document()->completeURL(url));

How do you know that it is a CSSStyleSheet?

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

Use importRule and avoid another cast.

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

How do you know the items are CSSFontFaceSrcValue?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:310
> +        iter != styleSheets.end(); ++iter) {

Alignment (as I mentioned below).

> Source/WebKit/chromium/src/WebPageSerializer.cpp:319
> +        iter != resourceKURLs.end(); ++iter, i++) {

Might as well do ++i for consistency with other places.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:330
> +        iter != frameKURLs.end(); ++iter, ++i) {

Align with interior of ( 
or just don't wrap the line. Up to you.

> Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:53
> +    TestWebFrameClient() : m_scriptEnabled(false) {}

space in the {}

> Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:54
> +    virtual bool allowScript(WebFrame*, bool enabledPerSettings) { return m_scriptEnabled; }

I would comment out "enabledPerSettings" like /* enabledPerSettings*/ since it is unused that may cause errors in some builds.

> Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:181
> +                          

Pls don't do unnecessary space changes.

> Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:226
> +        m_webView, m_supportedSchemes, &resources, &frames));    

You don't need to wrap the line but if it makes you happy...

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