[webkit-reviews] review denied: [Bug 31737] Chromium WebKit API WebPageSerializer : [Attachment 44703] Added WebPageSerializer to WebKit API.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Yaar Schnitman
<yaar at chromium.org>'s request for review:
Bug 31737: Chromium WebKit API WebPageSerializer
https://bugs.webkit.org/show_bug.cgi?id=31737

Attachment 44703: Added WebPageSerializer to WebKit API.
https://bugs.webkit.org/attachment.cgi?id=44703&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
Please fix these nits and then R=me.


> +++ b/WebKit/chromium/public/WebNodeCollection.h
...
> +    WebNodeCollection() : m_private(0) { };

^^^ nit: delete the trailing semicolon


> +++ b/WebKit/chromium/public/WebNodeList.h
...
> +    WebNodeList() : m_private(0) { };

ditto


> +    WEBKIT_API WebString& operator=(const char* s);

why not implement this using the same template approach?  it would be
nice for the conversion constructor to be similar to the operator=.

> +++ b/WebKit/chromium/src/DOMUtilitiesPrivate.cpp
...
>  }
>  
> +
> +bool elementHasLegalLinkAttribute(const Element* element,

^^^ nit: no extra new line above.


> +++ b/WebKit/chromium/src/WebDocument.cpp

> +#include "Document.h"
> +#include "Element.h"
> +#include "HTMLAllCollection.h"
> +#include "HTMLBodyElement.h"
> +#include "HTMLCollection.h"
> +#include "HTMLElement.h"
> +#include "HTMLHeadElement.h"
> +
> +#include "WebElement.h"
> +#include "WebFrameImpl.h"
> +#include "WebNodeCollection.h"
> +#include "WebURL.h"
> +
> +#include <wtf/PassRefPtr.h>

^^^ nit: please move wtf/ includes up with the WebCore includes.


> +++ b/WebKit/chromium/src/WebEntities.cpp
...
> +#include "config.h"
> +#include "WebEntities.h"
> +
> +#include "PlatformString.h"
> +#include "StringBuilder.h"
> +
> +#include "WebString.h"
> +
> +#include <string.h>
> +#include <wtf/HashMap.h>

the above should be:

#include "config.h"
#include "WebEntities.h"

#include <string.h>

#include "PlatformString.h"
#include "StringBuilder.h"
#include <wtf/HashMap.h>

#include "WebString.h"




> +
> +
> +using namespace WebCore;
> +
> +namespace {
> +// Note that this file is also included by HTMLTokenizer.cpp so we are
getting
> +// two copies of the data in memory.  We can fix this by changing the script

> +// that generated the array to create a static const that is its length, but

> +// this is low priority since the data is less than 4K. We use anonymous
> +// namespace to prevent name collisions.
> +#include "HTMLEntityNames.c" // NOLINT

nit: what is the NOLINT comment doing here?  i don't think we want to use that
in the webkit repository.


> +++ b/WebKit/chromium/src/WebNodeCollection.cpp
...
> +#include "config.h"
> +#include "WebNodeCollection.h"
> +
> +#include "HTMLCollection.h"
> +#include "Node.h"
> +
> +#include "WebNode.h"
> +
> +#include <wtf/PassRefPtr.h>

^^^ same nit about wtf/ header placement.  please group with webcore headers.


> +++ b/WebKit/chromium/src/WebNodeList.cpp
...
> +#include "config.h"
> +#include "WebNodeList.h"
> +
> +#include "Node.h"
> +#include "NodeList.h"
> +
> +#include "WebNode.h"
> +
> +#include <wtf/PassRefPtr.h>

ditto


> +++ b/WebKit/chromium/src/WebPageSerializerImpl.cpp
...
> +#include "config.h"
> +#include "WebPageSerializerImpl.h"
> +
> +#include "DOMUtilitiesPrivate.h"
> +#include "Document.h"
> +#include "DocumentType.h"
> +#include "Element.h"
> +#include "FrameLoader.h"
> +#include "HTMLAllCollection.h"
> +#include "HTMLElement.h"
> +#include "HTMLFormElement.h"
> +#include "HTMLMetaElement.h"
> +#include "HTMLNames.h"
> +#include "KURL.h"
> +#include "PlatformString.h"
> +#include "StringBuilder.h"
> +#include "TextEncoding.h"
> +
> +#include "WebFrameImpl.h"
> +#include "WebURL.h"
> +#include "WebVector.h"
> +
> +#include "markup.h"

^^^ markup.h should be grouped with the other webcore headers.


> +++ b/WebKit/chromium/src/WebPageSerializerImpl.h
...
> +#include "PlatformString.h"
> +#include "StringBuilder.h"
> +#include "StringHash.h"
> +
> +#include "WebEntities.h"
> +#include "WebPageSerializer.h"
> +#include "WebPageSerializerClient.h"
> +#include "WebString.h"
> +#include "WebURL.h"
> +
> +#include <wtf/HashMap.h>
> +#include <wtf/Vector.h>

^^^ nit: please group wtf/ headers with the webcore headers.