[Webkit-unassigned] [Bug 31737] Chromium WebKit API WebPageSerializer
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 25 12:12:34 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=31737
Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #43859|review? |review-
Flag| |
--- Comment #7 from Darin Fisher (:fishd, Google) <fishd at chromium.org> 2009-11-25 12:12:32 PST ---
(From update of attachment 43859)
> +++ 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.
--
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