[Webkit-unassigned] [Bug 123153] [ATK] Avoid explicit traversal of text controls and the render tree in AtkText implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123153





--- Comment #5 from Mario Sanchez Prada <mario at webkit.org>  2013-10-30 04:03:24 PST ---
(From update of attachment 214850)
View in context: https://bugs.webkit.org/attachment.cgi?id=214850&action=review

>> Source/WebCore/ChangeLog:8
>> +        Remove functions from the AtkText implementation that manually walk the
> 
> can you add why you want to do this. its not clear what the purpose is

Sure. I will expand the description in the ChangeLog with more detail, more in the line of this bug's description.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1976
>> +        | TextIteratorIgnoresChildrenForReplacedObjects);
> 
> is there ever a case where you don't want to do these two things together?

Not for ATK. However, I added this extra option because I assume other port might be using the TextIteratorEmitsObjectReplacementCharacters value, since it was there already before I added this bit some time ago.

In any case, that does not seem to keep being the case, since a quick grep tells me nobody other than this function is actually requiring that behaviour:

  $ git grep TextIteratorEmitsObjectReplacementCharacters
  WebCore/ChangeLog-2011-06-04:        enumeration (TextIteratorEmitsObjectReplacementCharacters) and new
  WebCore/accessibility/AccessibilityObject.cpp:    behavior = static_cast<TextIteratorBehavior>(behavior | TextIteratorEmitsObjectReplacementCharacters);
  WebCore/editing/TextIterator.cpp:    , m_emitsObjectReplacementCharacters(behavior & TextIteratorEmitsObjectReplacementCharacters)
  WebCore/editing/TextIterator.h:    TextIteratorEmitsObjectReplacementCharacters = 1 << 4,


Thus, I think it makes sense to merge them in two, probably by keeping the old name and just expanding its effect so it ignores the children when enabled.

I don't know... maybe this was used in the past by ports such as chromium or Qt?

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:659
>> +        if (!m_renderer->isAnonymous()) {
> 
> can this check be done by just checking if this->node() == 0?

Probably, actually node() includes the isAnonymous() check anyway:

  Node* RenderObject::node() const { return isAnonymous() ? 0 : &m_node; }

I'll change that, if you think it's better.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:671
> +                ASSERT(!firstChildRenderer->isAnonymous());
> +                ASSERT(!lastChildRenderer->isAnonymous());

I'll change this asserts too to check that ASSERT({first|last}ChildRenderer->node())

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.