[webkit-reviews] review denied: [Bug 41569] [Chromium] Expose basic DOM information in WebAccessibilityObject : [Attachment 60933] Fix style errors in previous patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 8 13:15:26 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 60933: Fix style errors in previous patch
https://bugs.webkit.org/attachment.cgi?id=60933&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
overall, this looks great!

WebKit/chromium/public/WebAttribute.h:69
> +	 WebCore::Attribute* m_private;
please use the WebPrivatePtr class here.  we are not yet using it
consistently throughout the webkit api, but it is designed to make
this kind of code easier to write and helps avoid some errors.

WebKit/chromium/public/WebNamedNodeMap.h:69
> +	 WebCore::NamedNodeMap* m_private;
use WebPrivatePtr here

WebKit/chromium/public/WebRenderStyle.h:68
> +	 WebCore::RenderStyle* m_private;
use WebPrivatePtr here

WebKit/chromium/src/WebRenderStyle.cpp:72
> +	 return
WebString(CSSPrimitiveValue::create(m_private->display())->getStringValue());
one day, we might actually want a WebCSSValue, WebCSSPrimitiveValue, etc.
for now, though... what are doing here is fine.

as mentioned over IM, it looks like WebRenderStyle is unused.  either it
should be used or it should be excluded from this patch.


More information about the webkit-reviews mailing list