[webkit-reviews] review denied: [Bug 55859] [Chromium] The method that retrieves all resources from a page should be moved from Chromium to WebKit : [Attachment 85884] Fixed style.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.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 85884: Fixed style.
https://bugs.webkit.org/attachment.cgi?id=85884&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85884&action=review

WebPageSerializer.cpp is such great sadness.  I re-wrote the whole thing, but
never landed the new version.  :(

Almost every line of code in that file is wrong.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:60
> +KURL getSubResourceURLFromElement(const Element& element)

I usually pass Elements by pointer, not by reference.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:92
> +    if (value.isNull() || value.isEmpty() || value.startsWith("javascript:",
false))

value.startsWith("javascript:", false) is wrong.  It could start with "  
javascript:" also, for example.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:106
> +    if ((element.hasTagName(HTMLNames::iframeTag) ||
element.hasTagName(HTMLNames::frameTag))
> +	       && element.isFrameOwnerElement()) {

What if it's an object or an embed element that has a frame?

> Source/WebKit/chromium/src/WebPageSerializer.cpp:114
> +    if (url.isEmpty() || url.isNull() || !url.isValid())

isEmpty includes isNull, right?