[webkit-reviews] review denied: [Bug 58947] Add a way to serialize a WebView back to HTML : [Attachment 90935] Addressing Alexey's comments

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap 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 90935: Addressing Alexey's comments
https://bugs.webkit.org/attachment.cgi?id=90935&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90935&action=review

> This is not exactly what I had in mind

Please ignore this comment - I've now looked at the code, and understood what
it's doing.

I agree that this should be landed sooner rather than later, and I avoided
nitpicking on actual serialization code. r- for still not having all project
file changes, and I think that my code quality comments below need to be
addressed, too. But we are getting close, hopefully the next iteration will be
landable.

> Source/WebCore/html/HTMLFrameOwnerElement.cpp:116
> +    for (size_t i = 0; i < attributeMap->length(); ++i) {
> +	   Attribute* attribute = attributeMap->attributeItem(i);
> +	   if (isURLAttribute(attribute))
> +	       return &(attribute->name());
> +    }

This still stands out to me as something that should be fixed in initial patch.
Is there some difficulty getting rid of the loop? The only non-null result this
functions can ever return is srcAttr, so why does it even exist?

>> Source/WebCore/html/parser/HTMLMetaCharsetParser.cpp:118
>> +TextEncoding HTMLMetaCharsetParser::getEncodingFromMetaAttributes(const
Vector<pair<AtomicString, String> >& attributes)
> 
> We usually don't start functions with the word "get".

Yes, in WebKit coding style, "get..." is for functions that return a result via
a reference (or pointer) argument.

> Source/WebCore/page/PageSerializer.cpp:74
> +    for (size_t i = 0; i < WTF_ARRAY_LENGTH(ignoredTags); ++i) {
> +	   if (element->hasTagName(*ignoredTags[i]))
> +	       return true;
> +    }
> +    return false;

I suggest changing this to:

    return element->hasTagName(HTMLNames::scriptTag) ||
element->hasTagName(HTMLNames:: noscriptTag);

It's much shorter, and the current code isn't scalable anyway (you'd need to
start using a HashMap with many ignored tags).

> Source/WebCore/page/PageSerializer.cpp:107
> +    PageSerializer* m_serializer;

What guarantees that this doesn't become a dangling pointer?

> Source/WebCore/page/PageSerializer.cpp:108
> +    Document* m_document;

And this?

> Source/WebCore/page/PageSerializer.cpp:124
> +    // HTML might or not have their encoding specified, we'll add it if it's
missing.

This is not sufficient. It is very common to have a <meta> that is wrong - HTTP
headers override <meta>, so people leave broken ones in files all the time.

> Source/WebCore/page/PageSerializer.cpp:183
> +    RefPtr<Attribute> attribute = Attribute::create(HTMLNames::srcAttr,
url.string());
> +    appendAttribute(out, element, *attribute, namespaces);

Do you really need to use the internal Attribute class directly? What about
Element::setAttribute()?

> Source/WebCore/page/PageSerializer.cpp:233
> +    if (!textEncoding.isValid()) {

You can just ASSERT, document->encoding() is never invalid.