[webkit-reviews] review denied: [Bug 58947] [Chromium] Add a way to serialize a WebView back to HTML : [Attachment 90288] Fixing 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 58947: [Chromium] Add a way to serialize a WebView back to HTML
https://bugs.webkit.org/show_bug.cgi?id=58947

Attachment 90288: Fixing style
https://bugs.webkit.org/attachment.cgi?id=90288&action=review

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

This looks great.  The comments below are just a ton of nits.  Once we get this
working, we should remove the Save Web Page As code in favor of this code path.


> Source/WebCore/editing/MarkupAccumulator.h:82
> +    virtual void appendCustomAttributes(Vector<UChar>&, Element*,
Namespaces*) { }

Please don't put implementations of virtual methods in headers.  That just
makes the linker work hard.

> Source/WebKit/chromium/public/WebHTMLSerializer.h:46
> +// In the serialized version, any JavaScript tag or link is removed (event

JavaScript tag -> script tags

link -> ???

> Source/WebKit/chromium/public/WebHTMLSerializer.h:57
> +	   SerializedResource() { }
> +	   SerializedResource(const WebURL& url, const WebCString& mimeType,
const WebCString& data)
> +	       : url(url), mimeType(mimeType), data(data) { }

This isn't proper WebKit style.

> Source/WebKit/chromium/public/WebHTMLSerializer.h:62
> +    WEBKIT_API static void serialize(WebView*,
WebVector<SerializedResource>* resources);

This feels like a method of WebView rather than a static, but those are
questions for fishd.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:61
> +static const WebCore::QualifiedName* ignoredTags[] = {
> +    &HTMLNames::scriptTag,
> +    &HTMLNames::noscriptTag,
> +};

These could be references instead of pointers.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:72
> +class WebHTMLSerializerImpl::SerializerMarkupAccumulator : public
MarkupAccumulator {

I'd just use a normal class name and put all this stuff in an anonymous
namespace.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:99
> +    : MarkupAccumulator(nodes, AbsoluteURLs), m_serializer(serializer)

This should be on two lines for proper WebKit style.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:118
> +    // FIXME: for object (plugins) tags and video tag we could replace them
by an image fo their current contents.

fo -> of

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:124
> +    if (!element->hasTagName(HTMLNames::iframeTag) &&
!element->hasTagName(HTMLNames::frameTag))
> +	   return;

What about other FrameOwnerElement subclasses, like <object> ?

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:129
> +    if (url.isValid() && url != blankURL())
> +	   return;

comparing against blankURL isn't right.  You should look to see if it has the
scheme "about"

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:132
> +    url = m_serializer->getURLForBlankFrame(frameElement->contentFrame());

getURLForBlankFrame ->urlForBlankFrame (or something...  we don't start
function names with "get").

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:133
> +    RefPtr<Attribute> attribute = Attribute::create(HTMLNames::srcAttr,
AtomicString(url.string()));

You don't need to call AtomicString explicitly.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:144
> +    : m_resources(resources), m_blankFrameCounter(0)

Same style problem.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:154
> +    if (!url.isValid() || url == blankURL())
> +	   // For blank frames we generate a fake URL so they can be referenced
by their containing frame.
> +	   url = getURLForBlankFrame(frame);

Multi-line ifs should have { } even if they only have one statement.  Also,
same blankURL bug.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:180
> +	       KURL url =
document->completeURL(imageElem->getAttribute(HTMLNames::srcAttr));

This won't be the URL of the image in the case where there's a redirect.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:183
> +	   } else if (element->hasTagName(HTMLNames::linkTag) &&
equalIgnoringCase(element->getAttribute(HTMLNames::typeAttr), "text/css")) {

This is wrong.	You want to know whether the rel attribute contains stylesheet.
 You should be able to ask the HTMLLinkElement if it's a stylesheet.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:186
> +	       if (sheet->isCSSStyleSheet()) {

Yeah, looks like you've already do that.  You should remove the typeAttr check.


> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:189
> +		   // Note that serializeCSSStyleSheet took care of adding the
style sheet to the resources vector.

Can you change this into an ASSERT?

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:200
> +    Frame* childFrame = frame->tree()->firstChild();
> +    while (childFrame) {

This looks like a for loop in disguise.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:247
> +    String mimeType =
MIMETypeRegistry::getMIMETypeForExtension(image->image()->filenameExtension());


This is wrong.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.cpp:297
> +    String url("http://chromium_phony_frame/");

Oh man.  That's not a good idea.  How about "wyciwyg:" as the stem?

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.h:60
> +    class SerializerMarkupAccumulator;
> +    friend class SerializerMarkupAccumulator;

This doen't necessarily need to be a friend.  We already have the API layer as
a shield.

> Source/WebKit/chromium/src/WebHTMLSerializerImpl.h:62
> +    // Serializes the style-sheet back to text and adds it to the resources
if URL is not-empty.

style-sheet => stylesheet

> Source/WebKit/chromium/tests/WebHTMLSerializerTest.cpp:61
> +    WebHTMLSerializerTest() :
m_htmlMimeType(WebString::fromUTF8("text/html")),
m_cssMimeType(WebString::fromUTF8("text/css")),
m_pngMimeType(WebString::fromUTF8("img/png")) { }

Same style problem.