[Webkit-unassigned] [Bug 55859] [Chromium] The method that retrieves all resources from a page should be moved from Chromium to WebKit

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #85026|review?                     |review-
               Flag|                            |




--- Comment #8 from David Levin <levin at chromium.org>  2011-03-15 10:33:31 PST ---
(From update of attachment 85026)
View in context: https://bugs.webkit.org/attachment.cgi?id=85026&action=review

Just a few things to address and then this should be fine.

> Source/WebKit/chromium/src/WebDataSourceImpl.cpp:38
> +#include <googleurl/src/gurl.h>

Why do you need to include this?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:34
> +#include "DocumentLoader.h"

Where is the document loaded used in here?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:63
> +    if (element.hasTagName(HTMLNames::imgTag) || element.hasTagName(HTMLNames::scriptTag)) {

Single line clause shouldn't have surrounding {}

> Source/WebKit/chromium/src/WebPageSerializer.cpp:82
> +            // TODO(jnd): Add support for extracting links of sub-resources which

s/TODO({^)]*)/FIXME/

> Source/WebKit/chromium/src/WebPageSerializer.cpp:84
> +            // See bug: http://b/issue?id=1111667.

Internal bug reference doesn't seem appropriate.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:109
> +        if (visitedFrames->find(frame) == notFound)

While find is more flexible, I wonder if there should be a contains method as well which just does this but would be more readable in these pretty common cases.


Reading more of this code, I like this idea more and more.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:120
> +    if (!url.protocolIs("http") && !url.protocolIs("https") && !url.protocolIs("file"))

url.protocolInHTTPFamily
url.protocolIsLocalFile

> Source/WebKit/chromium/src/WebPageSerializer.cpp:163
> +        if (node->isElementNode()) {

if (!node->isElementNode())
    continue;

(Less indenting and "fail fast" approach.)

> Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:55
> +    WebPageSerializerTest() : m_webView(0), m_supportedSchemes(3U)

3U? Do we need the U?

> Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:59
> +        m_supportedSchemes[1] = "file";

Should this be [2] ?

This seems to indicate a hole in the test coverage.  Should it have https resources in it? What about file resources? What about unsupported schemes?

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