[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