[webkit-reviews] review denied: [Bug 55859] [Chromium] The method that retrieves all resources from a page should be moved from Chromium to WebKit : [Attachment 85026] Fix build and style issue

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jay Civelli
<jcivelli at chromium.org>'s request for review:
Bug 55859: [Chromium] The method that retrieves all resources from a page
should be moved from Chromium to WebKit
https://bugs.webkit.org/show_bug.cgi?id=55859

Attachment 85026: Fix build and style issue
https://bugs.webkit.org/attachment.cgi?id=85026&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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?