[Webkit-unassigned] [Bug 31737] Chromium WebKit API WebPageSerializer
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 30 10:00:45 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=31737
Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #44025|review?, commit-queue? |review-
Flag| |
--- Comment #9 from Darin Fisher (:fishd, Google) <fishd at chromium.org> 2009-11-30 10:00:44 PST ---
(From update of attachment 44025)
> +++ 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?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list