[webkit-reviews] review denied: [Bug 31737] Chromium WebKit API WebPageSerializer : [Attachment 43859] Patch 3

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 25 12:12:32 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 43859: Patch 3
https://bugs.webkit.org/attachment.cgi?id=43859&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/ChangeLog

^^^ I think this ChangeLog needs to be updated to include WebDocument.


> +++ b/WebKit/chromium/public/WebDocument.h
...
> +// Provides readonly access to some properties of a DOM element node.

nit: did you mean "some properties of a DOM document"?


> +class WebDocument : public WebNode {
> +
> +public:

^^^ no new line before "public:"


> +    WebDocument() : WebNode() { }

nit: no need to call WebNode() explicitly since that happens implicitly.
it is more common to not mention the base class constructor in such cases.


> +    template<typename T> T toElementConst() const
> +    {
> +	   T res;
> +	   res.m_private = m_private;
> +	   return res;
> +    }

^^^ Shouldn't toElementConst return 'const T' to preserve constness?

Also, a naming nit:  You have constUnwrap, which uses const as a prefix,
but here you have it as a suffix.   maybe constUnwrap should be renamed
to unwrapConst for consistency?


> +++ b/WebKit/chromium/public/WebElement.h
...
>	   WebElement& operator=(const WTF::PassRefPtr<WebCore::Element>&);
>	   operator WTF::PassRefPtr<WebCore::Element>() const;
>  #endif
> +	   WEBKIT_API bool hasTagName(const WebString&) const;
> +	   WEBKIT_API bool hasAttribute(const WebString&) const;
> +	   WEBKIT_API WebString getAttribute(const WebString&) const;

We usually put the WEBKIT_IMPLEMENTATION helpers at the bottom of the
public section.  So, I think you should move these accessors up above
the WEBKIT_IMPLEMENTATION section.


> +++ b/WebKit/chromium/public/WebFrame.h
...
> +    // Return the frame's encoding.
> +    virtual WebString encoding() const = 0;

We already have WebView::pageEncoding().  Should we deprecate that in
favor of this?


> +    // Returns the page's document element.
> +    virtual WebDocument document() const = 0;

WebDocument is not an element.	A "document element" is something else.

I think you can just delete this comment.


> +    // 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.
> +    virtual WebFrame* fromFrameElement(WebElement&) const = 0;

^^^ This should be a static function, and it should probably be named
fromFrameOwnerElement.	I think the parameter should be a "const WebElement&"

I would move this to the top-most section of WebFrame, where have other static
methods that return a WebFrame pointer.


> +++ b/WebKit/chromium/public/WebNode.h
...
>      WEBKIT_API WebFrame* frame() const;

I think we should deprecate the "frame()" accessor and replace it with a
document() accessor.  WebDocument should then get a frame() accessor.  This way

it better matches WebCore.

The container for nodes is a document and the container for a document is a
frame.	It seems cleaner for the API to just expose the next containing layer
for any given layer.  Makes sense?


> +++ b/WebKit/chromium/public/WebNodeCollection.h
...
> +#if WEBKIT_IMPLEMENTATION
> +    WebNodeCollection(const WTF::PassRefPtr<WebCore::HTMLCollection>&);
> +#endif
> +
> +    WEBKIT_API unsigned length() const;
> +    WEBKIT_API WebNode nextItem() const;
> +    WEBKIT_API WebNode firstItem() const;

Same nit as before.  Please push the WEBKIT_IMPLEMENTATION section to
the bottom of the public section.


> +protected:
> +    void assign(WebCore::HTMLCollection*);
> +    WebCore::HTMLCollection* m_private;
> +};

^^^ why protected and not private?  Do you anticipate subclassing?


> +++ b/WebKit/chromium/public/WebPageSerializerClient.h
...
> +#ifndef WebDomSerializerClient_h
> +#define WebDomSerializerClient_h

Please fix this include guard.


> +    WebPageSerializerClient() { }
> +protected:
> +    virtual ~WebPageSerializerClient() { }
> +};

^^^ nit: please add a new line above "protected:"


> +++ b/WebKit/chromium/public/WebView.h
...
> +    // Returns the frame with the given url. Returns 0 if no frame has this
url.
> +    // If more than one frame has this url, returns the first one.
> +    virtual WebFrame* findFrameByURL(const WebURL& url) = 0;
>  
>      // Focus ---------------------------------------------------------------


^^^ nit: please preserve the two blank line separator between sections


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

> +    // Optimization: Create StringBuilder only if value has any entities.
> +/*	 while (len--) {
> +	   if (m_entitiesMap.contains(*curPos)){
> +	       len++;
> +	       break;
> +	   }
> +	   curPos++;
> +    }
> +    if (!len)
> +	   return value; */

^^^ delete this commented-out code?


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

> +WebFrame* WebFrameImpl::fromFrameElement(WebElement& element) const
> +{
> +    RefPtr<Element> e = element.operator
WTF::PassRefPtr<WebCore::Element>();
> +    return fromFrameOwnerElement(e.get());

it is a bit ugly to call that operator directly like this.  i think it
is better to write it like so:

  return fromFrameOwnerElement(PassRefPtr<Element>(element).get());

Or, perhaps the unwrap and constUnwrap methods should be made public.


> +++ b/WebKit/chromium/src/WebFrameImpl.h
...
> @@ -157,7 +160,7 @@ public:
>      virtual WebURL completeURL(const WebString& url) const;
>      virtual WebString contentAsText(size_t maxChars) const;
>      virtual WebString contentAsMarkup() const;
> -
> +    
>      static PassRefPtr<WebFrameImpl> create(WebFrameClient* client);

^^^ nit: please avoid adding unnecessary whitespace



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

> +WebVector<WebNode> WebNode::childNodes()
> +{
> +    WTF::Vector<WebNode> res;
> +    WTF::PassRefPtr<NodeList> nodeList = m_private->childNodes();
> +    for (unsigned i = 0; i < nodeList->length(); i++)
> +	   res.append(nodeList->item(i));
> +    return WebVector<WebNode>(res);

^^^ no need to specify WTF:: in .cpp files.

Also, should we have a WebNodeList type?


> +++ b/WebKit/chromium/src/WebPageSerializerImpl.cpp
> +    , isInScriptOrStyleTag(false)
> +    , hasDocDeclaration(false)
> +{
> +    // Cache the value since we check it lots of times.
> +   isHTMLDocument = doc->isHTMLDocument();

nit: ^^^ please indent by 4 spaces


> +	   // See http://bugs.webkit.org/show_bug.cgi?id=16621.
> +	   // First we generate new content for writing correct META element.
> +	   result.append(WebPageSerializer::generateMetaCharsetDeclaration(
> +	       String(param->textEncoding.name())));
> +
> +	   param->hasAddedContentsBeforeEnd = true;
> +	   // Will search each META which has charset declaration, and skip
them all
> +	   // in PreActionBeforeSerializeOpenTag.
> +    } else if (element->hasTagName(HTMLNames::scriptTag) ||
> +		  element->hasTagName(HTMLNames::styleTag)) {

nit: the || should be at the beginning of the next line


> +    switch (node->nodeType()) {
> +    case Node::ELEMENT_NODE: {
> +	   // Process open tag of element.
> +	   openTagToString(static_cast<const Element*>(node), param);
> +	   // Walk through the children nodes and process it.
> +	   for (const Node *child = node->firstChild(); child != NULL;
> +		child = child->nextSibling())
> +	       buildContentForNode(child, param);
> +	   // Process end tag of element.
> +	   endTagToString(static_cast<const Element*>(node), param);
> +	   break;
> +    }
> +    case Node::TEXT_NODE: {
> +	   saveHTMLContentToBuffer(createMarkup(node), param);
> +	   break;
> +    }
> +    case Node::ATTRIBUTE_NODE:
> +    case Node::DOCUMENT_NODE:
> +    case Node::DOCUMENT_FRAGMENT_NODE: {
> +	   // Should not exist.
> +	   ASSERT(false);
> +	   break;
> +    }
> +	   // Document type node can be in DOM?
> +    case Node::DOCUMENT_TYPE_NODE:
> +	   param->hasDoctype = true;
> +    default: {
> +	   // For other type node, call default action.
> +	   saveHTMLContentToBuffer(createMarkup(node), param);
> +	   break;
> +    }
> +    }

^^^ the brackets for each case are not needed.	you only need
those if there are local variables.

nit: ASSERT(false) should be changed to ASSERT_NOT_REACHED.


> +    // Go through all frames for serializing DOM for whole page, include
> +    // sub-frames.
> +    for (int i = 0; i < static_cast<int>(m_frames.size()); ++i) {

nit: use size_t for i instead of int so you can avoid the static_cast?


> +++ b/WebKit/chromium/src/WebPageSerializerImpl.h
> +public:
> +    // Do serialization action. Return false means no available frame has
been
> +    // serialized, otherwise return true.
> +    bool serialize();
> +    // The parameter specifies which frame need to be serialized.

nit: please add a new line after 'serialize()'


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

> +WebFrame* WebViewImpl::findFrameByURL(const WebURL& url)
> +{
> +    if (!mainFrameImpl())
> +	   return 0;
> +    KURL kurl = url;
> +
> +    WTF::Vector<WebFrameImpl*> frames;

nit: please drop the WTF:: prefix in .cpp files


> +    // First, process main frame.
> +    frames.append(mainFrameImpl());
> +    // Collect all frames inside the specified frame.
> +    for (int i = 0; i < static_cast<int>(frames.size()); ++i) {

nit: please use size_t instead of int for i


> +	   WebFrameImpl* currentFrame = frames[i];
> +	   // Check whether current frame has the desired url or not.
> +	   const KURL& currentFrameURL =
> +	       currentFrame->frame()->loader()->url();
> +	   if (kurl == currentFrameURL)
> +	       return currentFrame;
> +	   // Go through nodes and look for sub-frames.
> +	   RefPtr<HTMLCollection> all =
currentFrame->frame()->document()->all();
> +	   for (Node* node = all->firstItem(); node; node = all->nextItem()) {
> +	       if (!node->isHTMLElement())
> +		   continue;
> +	       Element* element = static_cast<WebCore::Element*>(node);
> +	   
> +	       // Check frame tag and iframe tag.
> +	       WebFrameImpl* webFrame =
WebFrameImpl::fromFrameOwnerElement(element);
> +	       if (webFrame)
> +		   frames.append(webFrame);
> +	   }
> +    }
> +
> +    return 0;
> +}

Given all of the new additions to the WebKit API, I don't think you need
to expose findFrameByURL on WebViewImpl.  We can write that function in
the Chromium code base without any troubles.  Let's keep the WebKit API
as minimal as possible.


More information about the webkit-reviews mailing list