[webkit-reviews] review denied: [Bug 25677] [GTK] Implement support for get_character_extents and get_range_extents : [Attachment 62755] Patch proposal + unit tests

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 25677: [GTK] Implement support for get_character_extents and
get_range_extents
https://bugs.webkit.org/show_bug.cgi?id=25677

Attachment 62755: Patch proposal + unit tests
https://bugs.webkit.org/attachment.cgi?id=62755&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
WebCore/accessibility/AccessibilityObject.cpp:380
 +  
you could do here
if (!textLength) {
   Node* node = node();
   if (node) {
      RenderText* renderText = toRenderText(node->renderer());
  }


WebCore/accessibility/AccessibilityObject.cpp:381
 +	    // Plain text a11y object first
don't use made up words like a11y. make sure to write full sentences for your
comments

WebCore/accessibility/AccessibilityObject.cpp:386
 +	    if (isWebArea() || isGroup() || isLink() || isHeading()) {
this should be else if, no?

WebCore/accessibility/AccessibilityObject.cpp:389
 +		textLength = textValue.length();
can't you do textLength = textUnderElement().length();

WebCore/accessibility/AccessibilityObject.cpp:387
 +		// Check composite objects just in case
this comment should go above the if and should be a full sentence. you should
also enumerate what "just in case" means

WebCore/accessibility/AccessibilityRenderObject.cpp:2414
 +  #else
you also check isWebArea in the other locations. do you not want to do it here?


WebCore/accessibility/AccessibilityObject.cpp:387
 +		// Check composite objects just in case
make sure to end comments with a period

WebCore/accessibility/AccessibilityRenderObject.cpp:2676
 +	bool isTextObject =  isTextControl() || m_renderer->isText()
too much white space

WebCore/accessibility/AccessibilityRenderObject.cpp:2680
 +  #endif
instead of doing this PLATFORM stuff and enumerating in each instance, i think
you should have a platform specific method on AccessibilityObject, like

AccessibilityObject::allowsTextRanges() or something appropriate

then in your AccessibilityObjectGTK.cpp you can override. for the other
platforms you can have the default impl live in AccessibilityObject and do what
it does now

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1270
 +	    rangeLength = textLength;
if the rangelength == 0 are you sure you want to reset to the text length?

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1276
 +  
i don't see renderObject being used anywhere