[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





--- Comment #10 from Jay Civelli <jcivelli at chromium.org>  2011-03-15 17:07:33 PST ---
(In reply to comment #8)
> (From update of attachment 85026 [details])
> 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?
Removed.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:34
> > +#include "DocumentLoader.h"
> 
> Where is the document loaded used in here?
At line 134:
KURL frameURL = frame->loader()->documentLoader()->request().url();


> > Source/WebKit/chromium/src/WebPageSerializer.cpp:63
> > +    if (element.hasTagName(HTMLNames::imgTag) || element.hasTagName(HTMLNames::scriptTag)) {
> 
> Single line clause shouldn't have surrounding {}
I thought when following else statement had {} then we should use them on the "then" clause as well?

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:82
> > +            // TODO(jnd): Add support for extracting links of sub-resources which
> 
> s/TODO({^)]*)/FIXME/
Done.

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:84
> > +            // See bug: http://b/issue?id=1111667.
> 
> Internal bug reference doesn't seem appropriate.
Removed.

> > 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.
Here I am using a Vector, not a WebVector, and it does not have a contain.
Actually I now realize I am using the WebVector::find() in the test only, and I would probably only need a contains method.  So I replaced find() with contains().

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

> > Source/WebKit/chromium/src/WebPageSerializer.cpp:163
> > +        if (node->isElementNode()) {
> 
> if (!node->isElementNode())
>     continue;
> 
> (Less indenting and "fail fast" approach.)
OK.

> > Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:55
> > +    WebPageSerializerTest() : m_webView(0), m_supportedSchemes(3U)
> 
> 3U? Do we need the U?
Yes, the fails to compile otherwise. (it's using the wrong constructor in that case).

> > Source/WebKit/chromium/tests/WebPageSerializerTest.cpp:59
> > +        m_supportedSchemes[1] = "file";
> 
> Should this be [2] ?
Arg! Yes.

> 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?
Good idea, added.

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