[webkit-reviews] review requested: [Bug 25677] [GTK] Implement support for get_character_extents and get_range_extents : [Attachment 63189] Patch proposal + unit tests
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 2 04:30:06 PDT 2010
Mario Sanchez Prada <msanchez at igalia.com> has asked 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 63189: Patch proposal + unit tests
https://bugs.webkit.org/attachment.cgi?id=63189&action=review
------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(In reply to comment #8)
> (From update of attachment 62755 [details])
> WebCore/accessibility/AccessibilityObject.cpp:380
> +
> you could do here
> if (!textLength) {
> Node* node = node();
> if (node) {
> RenderText* renderText = toRenderText(node->renderer());
> }
True. Done.
> 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
Fixed, sorry.
> WebCore/accessibility/AccessibilityObject.cpp:386
> + if (isWebArea() || isGroup() || isLink() || isHeading()) {
> this should be else if, no?
Not really, because I want to enter even if I entered the previous if branch,
when textLength keeps being zero.
However it's true something was missing there, an extra check for !textLengh:
[...]
if (renderText)
textLength = renderText->textLength();
// Get the text length from the elements under the
// accessibility object if not a RenderText object.
if (!textLength && allowsTextRanges())
textLength = textUnderElement().length();
[...]
> WebCore/accessibility/AccessibilityObject.cpp:389
> + textLength = textValue.length();
> can't you do textLength = textUnderElement().length();
Done, as you can see in the snipped above.
> 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
Done, as you can see in the snipped above.
> WebCore/accessibility/AccessibilityRenderObject.cpp:2414
> + #else
> you also check isWebArea in the other locations. do you not want to do it
here?
You were right. Fixed in the new AccessibilityObject::allowsTextRange()
function
> WebCore/accessibility/AccessibilityObject.cpp:387
> + // Check composite objects just in case
> make sure to end comments with a period
Done
> WebCore/accessibility/AccessibilityRenderObject.cpp:2676
> + bool isTextObject = isTextControl() || m_renderer->isText()
> too much white space
Fixed
> 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
Fixed, by creating such a function as you suggessted (I agree it's better that
way).
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1270
> + rangeLength = textLength;
> if the rangelength == 0 are you sure you want to reset to the text length?
Technically, that never will happen (as code inside ATK library won't allow
that), but I agree with you it's strange to do it so, so I've changed that <=
to just <.
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1276
> +
> i don't see renderObject being used anywhere
True, looks like it belongs to some previous version of the patch and I've just
overlooked it. Sorry for the noise
Now attaching a new version of the patch addressing all these issues. Thanks
for the feedback, Chris
More information about the webkit-reviews
mailing list