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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 30 12:50:36 PST 2009


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 44029: Added WebPageSerializer to WebKit API.
https://bugs.webkit.org/attachment.cgi?id=44029&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/public/WebNodeCollection.h
...
> +class WebNodeCollection {
> +public:
> +    ~WebNodeCollection() { reset(); }
> +
> +    WebNodeCollection(const WebNodeCollection& n) : m_private(0) {
assign(n); }
> +    WebNodeCollection& operator=(const WebNodeCollection& n)
> +    {
> +	   assign(n);
> +	   return *this;
> +    }
> +
> +    WEBKIT_API void reset();
> +    WEBKIT_API void assign(const WebNodeCollection&);
> +
> +    WEBKIT_API unsigned length() const;
> +    WEBKIT_API WebNode nextItem() const;
> +    WEBKIT_API WebNode firstItem() const;
> +
> +#if WEBKIT_IMPLEMENTATION
> +    WebNodeCollection(const WTF::PassRefPtr<WebCore::HTMLCollection>&);
> +#endif
> +
> +private:
> +    void assign(WebCore::HTMLCollection*);
> +    WebCore::HTMLCollection* m_private;
> +};

It looks like this class is missing a default constructor.  I think that'll
cause some crashes.


> +++ b/WebKit/chromium/public/WebNodeList.h
...
> +class WebNodeList {
> +public:
> +    ~WebNodeList() { reset(); }
> +
> +    WebNodeList(const WebNodeList& n) : m_private(0) { assign(n); }
> +    WebNodeList& operator=(const WebNodeList& n)
> +    {
> +	   assign(n);
> +	   return *this;
> +    }
> +
> +    WEBKIT_API void reset();
> +    WEBKIT_API void assign(const WebNodeList&);
> +
> +    WEBKIT_API unsigned length() const;
> +    WEBKIT_API WebNode item(size_t) const;
> +
> +#if WEBKIT_IMPLEMENTATION
> +    WebNodeList(const WTF::PassRefPtr<WebCore::NodeList>&);
> +#endif
> +
> +private:
> +    void assign(WebCore::NodeList*);
> +    WebCore::NodeList* m_private;
> +};

ditto



> +++ b/WebKit/chromium/public/WebPageSerializerClient.h
...
> +    enum PageSerializationStatus {
> +	   CurrentFrameIsNotFinished = 0,

See previous nit about removing the "= 0"...


> +++ b/WebKit/chromium/public/WebString.h
> @@ -87,6 +87,9 @@ public:
>      WEBKIT_API static WebString fromUTF8(const char* data, size_t length);
>      WEBKIT_API static WebString fromUTF8(const char* data);
>  
> +    WebString(const char* data);
> +    WebString& operator=(const char* s);

looks like this is the same as before.	did you update the wrong patch?


> +++ b/WebKit/chromium/src/WebNode.cpp
...
> +WebNode::NodeType WebNode::nodeType() const
> +{
> +    return static_cast<WebNode::NodeType>(m_private->nodeType());
> +}

see previous nit about dropping the WebNode:: prefix within the body
of a WebNode method.


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

see previous nit about moving the wtf header up to the section
including Node.h

[stopping here since i'm repeating myself... is this the right patch?]


More information about the webkit-reviews mailing list