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

Tue Mar 29 00:35:26 PDT 2011

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


--- Comment #10 from Jay Civelli <jcivelli at chromium.org>  2011-03-29 00:35:26 PST ---
(In reply to comment #9)
> (From update of attachment 86250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86250&action=review
> 
> Does this do an url rewriting?
No it does not. It simply returns the URLs for the resources found in the page. 

> 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?
This CL does not deal with that. The "save-as" page feature stores the resources from the page into a directory and rewrite the URLs in an extra step.

> 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.
OK, I tried to do that right.


> > 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?
object and embed tags can be used as iframe if they source is HTML, in which case isFrameOwnerElement returns true. Otherwise they should simply be considered as a resource.
Note that an iframe can also be used to display an image I believe, and that case does not seem to be supported.
I filed a bug https://bugs.webkit.org/show_bug.cgi?id=57300 and will address that separately.

> 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 ?
I don't think style applies to the iframe tag itself, so we would not get anything interesting from that.

> > 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;
> }
Good idea, done.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:140
> > +        // Ignore javascript content.
> 
> How is this ignoring javascript content?
Out-of-date comment, removed.

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

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

> > 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?
Intuition... What? Not good enough?
OK, now explicitly testing for CSS type before casting.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:250
> > +            retrieveResourcesForCSSStyleSheet(static_cast<CSSImportRule*>(item)->styleSheet(), visitedStyleSheets, supportedSchemes, resourceURLs);
> 
> Use importRule and avoid another cast.
Done.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:257
> > +                    CSSFontFaceSrcValue* fontFaceSrc = static_cast<CSSFontFaceSrcValue*>(valueList->item(j));
> 
> How do you know the items are CSSFontFaceSrcValue?
Sadly there does not seem to be a reliable way to know.
I am doing like WebCore/css/CSSFontSelector.cpp does, which is assume the CSSPropertySrc value only contains CSSFontFaceSrcValue.
Added a comment to reflect this. 

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:310
> > +        iter != styleSheets.end(); ++iter) {
> 
> Alignment (as I mentioned below).
Done.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:319
> > +        iter != resourceKURLs.end(); ++iter, i++) {
> 
> Might as well do ++i for consistency with other places.
Done.

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

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

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

> > Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:181
> > +                          
> 
> Pls don't do unnecessary space changes.
Removed.

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

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