[webkit-reviews] review denied: [Bug 41569] [Chromium] Expose basic DOM information in WebAccessibilityObject : [Attachment 60454] Fix style errors in previous patch.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 7 08:57:02 PDT 2010
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Dominic Mazzoni
<dmazzoni at google.com>'s request for review:
Bug 41569: [Chromium] Expose basic DOM information in WebAccessibilityObject
https://bugs.webkit.org/show_bug.cgi?id=41569
Attachment 60454: Fix style errors in previous patch.
https://bugs.webkit.org/attachment.cgi?id=60454&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebAccessibilityObject.h:108
+ WEBKIT_API void attributeAt(unsigned index, WebString* name, WebString*
value) const;
nit: webkit style is to use reference types for out params: WebString& name,
WebString& value
WebKit/chromium/public/WebAccessibilityObject.h:111
+ WEBKIT_API WebString documentUrl() const;
nit: documentUrl -> documentURL
but, why do you need to expose these document properties? Why not just
expose WebDocument (and add properties to it as needed)?
WebKit/chromium/src/WebAccessibilityObject.cpp:420
+ Node* node = renderObject->node();
instead of exposing a tagName property, how about just exposing
a WebNode getter? then, the consumer can call WebNode::nodeName.
WebKit/chromium/src/WebAccessibilityObject.cpp:440
+ if (!node || !node->hasAttributes())
this also feels like something that should be provided by exposing
an attributes getter on WebElement. Use the WebNodeList type.
WebKit/chromium/src/WebAccessibilityObject.cpp:507
+ WebString WebAccessibilityObject::documentUrl() const
this method should definitely be eliminated in favor of accessing
the related WebNode, and then query its document. from the
WebDocument you can get the WebFrame, and that has a "url" property.
WebKit/chromium/src/WebAccessibilityObject.cpp:532
+ return document->title();
eliminate this method in favor of querying WebDocument::title.
WebKit/chromium/src/WebAccessibilityObject.cpp:543
+ if (document && document->isXHTMLDocument())
WebDocument already has an isXHTMLDocument accessor
WebKit/chromium/src/WebAccessibilityObject.cpp:560
+ DocumentType* doctype = document->doctype();
please expose a "WebDocumentType WebDocument::docType()" accessor.
that means creating the WebDocumentType class and adding the methods
to it that you need (just name for now). it is fairly easy to
create a new subclass of WebNode. just look at WebDocument and
WebElement to see good examples of it.
More information about the webkit-reviews
mailing list