[webkit-reviews] review denied: [Bug 58947] Add a way to serialize a WebView back to HTML : [Attachment 90610] Addressed Adam's and Alexey's comments

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jay Civelli <jcivelli at chromium.org>'s
request for review:
Bug 58947: Add a way to serialize a WebView back to HTML
https://bugs.webkit.org/show_bug.cgi?id=58947

Attachment 90610: Addressed Adam's and Alexey's comments
https://bugs.webkit.org/attachment.cgi?id=90610&action=review

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

I feel like I'm nit-picking at this point.  The main things to address are
moving the function to HTMLFrameOwnerElement and style attributes.

>> Source/WebCore/editing/MarkupAccumulator.h:96
>>	void appendOpenTag(Vector<UChar>& out, Element* element, Namespaces*);
>>	void appendCloseTag(Vector<UChar>& out, Element* element);
>> -	void appendAttribute(Vector<UChar>& out, Element* element, const
Attribute&, Namespaces*);
>> +	virtual void appendAttribute(Vector<UChar>& out, Element* element,
const Attribute&, Namespaces*);
> 
> The parameter name "element" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I'd fix all the instance of this issue in this block of code.

> Source/WebCore/page/PageSerializer.cpp:65
> +static const WebCore::QualifiedName* frameOwnerSrcAttributes[] = {

This array does not appear to be used.

> Source/WebCore/page/PageSerializer.cpp:81
> +    // FIXME: ideally HTMLFrameOwnerElement would have a method to return
the URL attribute name.

I'd just add this method to HTMLFrameOwnerElement

> Source/WebCore/page/PageSerializer.cpp:133
> +    if (element->hasTagName(HTMLNames::metaTag) && attribute.name() ==
HTMLNames::contentAttr) {

Do we want to make sure the http-equiv attribute of this element is
Content-Type ?	Otherwise, you could be munging other http-equiv meta tags.

> Source/WebCore/page/PageSerializer.cpp:136
> +	   if (charset != "utf-8" && charset != "utf8") {

Doe we need a case-insensitive compare here?  If not, we should ASSERT that
charset is lowercase.

> Source/WebCore/page/PageSerializer.cpp:142
> +    }

This isn't the only case.  There's also:

<meta charset="utf-8">

> Source/WebCore/page/PageSerializer.cpp:166
> +    if (!urlAttribute) {
> +	   LOG_ERROR("No URL attribute found for frame owner.");
> +	   return;
> +    }

Why is this an error?  It can definitely happen.

> Source/WebCore/page/PageSerializer.cpp:219
> +    Vector<Node*> nodes;
> +    SerializerMarkupAccumulator accumulator(this, &nodes);
> +    CString frameHTML =
accumulator.serializeNodes(document->documentElement(), 0, IncludeNode).utf8();
   
> +    m_resources->append(Resource(url, String("text/html"),
SharedBuffer::create(frameHTML.data(), frameHTML.length())));
> +    m_resourceURLs.add(url);

I wonder if you want to add a <meta charset="UTF-8"> to the serialization just
in case there's no meta element.

Also, is document always text/html?  What if it's application/svg+xml ?