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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 30 10:00:43 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 44025: Added WebPageSerializer to WebKit API.
https://bugs.webkit.org/attachment.cgi?id=44025&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/public/WebDocument.h
...
> +class WebDocument : public WebNode {
> +public:
> +    WebDocument() { }
> +    WebDocument(const WebDocument& e) : WebNode(e) { }
> +
> +    WebDocument& operator=(const WebDocument& e) { WebNode::assign(e);
return *this; }
> +    WEBKIT_API void assign(const WebDocument& e) { WebNode::assign(e); }

since this assign method is implemented inline, it should not be
exported from a WebKit.dll, so drop the WEBKIT_API prefix.


> +    WEBKIT_API WebFrame* frame() const;

perhaps we should document the fact that document's frame might be null?


> +++ b/WebKit/chromium/public/WebElement.h
...
>	   WEBKIT_API void assign(const WebElement& e) { WebNode::assign(e); }

ditto.	drop the WEBKIT_API prefix.


> +++ b/WebKit/chromium/public/WebFrame.h
...
> +    // Returns the frame inside a given frame or iframe element. Returns 0
if
> +    // the given element is not a frame, iframe or if the frame is empty.
> +    WEBKIT_API static WebFrame* fromFrameOwnerElement(WebElement&);

the parameter should be a |const WebElement&|


> +++ b/WebKit/chromium/public/WebNodeCollection.h
...
> +// Provides readonly access to some properties of a DOM node.
> +class WebNodeCollection {
> +public:
> +    virtual ~WebNodeCollection() { reset(); }

does this destructor need to be virtual?  this class doesn't have
any other virtual methods.  if we can avoid a vtable, then we should.


> +++ b/WebKit/chromium/public/WebNodeList.h
...
> +    virtual ~WebNodeList() { reset(); }

ditto.	seems that we should drop the virtual keyword here.


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

nit: no need for "= 0" since that is the default.


> +++ b/WebKit/chromium/public/WebString.h

this patch has grown quite large.  i think in the future it would be much
better to break it up into multiple patches.


> @@ -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);

given the implicit WebString(const char*) constructor, I think we can do
without the operator=.	We already have operator=(const WebString&), so
the compiler should invoke the WebString(const char*) constructor when
you write expressions like this:

  WebString s;
  s = "foo";

also, please remember that any public method not implemented inline needs
WEBKIT_API prefix.  however, please avoid adding that to constructors.
i've tried to not need to export constructors.

also, see WebData which has a fancy constructor for initialization from
a string literal that avoids a strlen call.

i think i'd change to this:

  template <int N>
  WebString(const char (&data)[N]) : m_private(0) { assign(fromUTF8(data, N -
1)); }

we could optimize this later to avoid the fromUTF8.


> +++ b/WebKit/chromium/public/WebView.h
> @@ -44,6 +44,7 @@ class WebFrameClient;
>  class WebNode;
>  class WebSettings;
>  class WebString;
> +class WebURL;
>  class WebViewClient;
>  struct WebMediaPlayerAction;
>  struct WebPoint;

^^^ is this change needed?


> +++ b/WebKit/chromium/src/DOMUtilitiesPrivate.cpp
> +++ b/WebKit/chromium/src/WebDocument.cpp
...
> +WebDocument::WebDocument(const WTF::PassRefPtr<WebCore::Document>& elem)
> +    : WebNode(elem.releaseRef())
> +{
> +}
> +
> +WebDocument& WebDocument::operator=(const
WTF::PassRefPtr<WebCore::Document>& elem)

please drop the WTF:: prefix and the WebCore:: prefix throughout this file.


> +++ b/WebKit/chromium/src/WebFrameImpl.cpp
...
> +WebFrameImpl* WebFrameImpl::fromFrameOwnerElement(Element* element)
> +{
> +    if (!element
> +	   || !element->isFrameOwnerElement()
> +	   || (!element->hasTagName(HTMLNames::iframeTag)
> +	       && !element->hasTagName(HTMLNames::frameTag)))
> +	   return 0;
> +
> +    WebCore::HTMLFrameOwnerElement* frameElement =
> +	   static_cast<WebCore::HTMLFrameOwnerElement*>(element);

drop the WebCore:: prefix


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

> +WebNode::NodeType WebNode::nodeType() const
> +{
> +    return static_cast<WebNode::NodeType>(m_private->nodeType());
> +}

nit: no need for the WebNode:: prefix within the implementation of
a WebNode method.


>  WebNode::WebNode(const WTF::PassRefPtr<WebCore::Node>& node)
>      : m_private(static_cast<WebNodePrivate*>(node.releaseRef()))
>  {

nit: please avoid the WTF:: and WebCore:: prefixes in .cpp files.


> +++ b/WebKit/chromium/src/WebNodeCollection.cpp
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (C) 2009 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *	  * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"
> +#include "WebNodeCollection.h"
> +
> +#include "HTMLCollection.h"
> +#include "Node.h"
> +
> +#include "WebNode.h"
> +
> +#include <wtf/PassRefPtr.h>

nit: please place the wtf include in the same group as the WebCore headers.
in this case just below Node.h


> +WebNodeCollection::WebNodeCollection(const WTF::PassRefPtr<HTMLCollection>&
col)
> +    : m_private(static_cast<HTMLCollection*>(col.releaseRef()))

drop WTF:: prefix


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

nit: please place the wtf include in the same group as the WebCore headers.
in this case just below Node.h


> +WebNodeList::WebNodeList(const WTF::PassRefPtr<NodeList>& col)
> +    : m_private(static_cast<NodeList*>(col.releaseRef()))

drop WTF:: prefix


> +++ 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: place the wtf headers up in the same section as the WebCore includes.


> +++ b/WebKit/chromium/src/WebViewImpl.cpp
> @@ -50,6 +50,7 @@
>  #include "FrameView.h"
>  #include "GraphicsContext.h"
>  #include "HitTestResult.h"
> +#include "HTMLAllCollection.h"
>  #include "HTMLInputElement.h"
>  #include "HTMLMediaElement.h"
>  #include "HTMLNames.h"

^^^ is this include needed?


More information about the webkit-reviews mailing list