[Webkit-unassigned] [Bug 142719] AX: richer text change notifications

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #251541|review?                     |review+, commit-queue+
              Flags|                            |

--- Comment #109 from Darin Adler <darin at apple.com> ---
Comment on attachment 251541
  --> https://bugs.webkit.org/attachment.cgi?id=251541
Fix windows builds

View in context: https://bugs.webkit.org/attachment.cgi?id=251541&action=review

r=me; seems good as is, comments are about future clean-up and refinement.

> Source/WebCore/accessibility/AXObjectCache.cpp:759
> +    for (const auto& notification : notifications)

Not sure there’s an advantage of const auto& over auto&.

> Source/WebCore/accessibility/AXObjectCache.cpp:1041
> +    nodeTextChangePlatformNotification(object, textChangeForEditType(type), position.deepEquivalent().deprecatedEditingOffset(), text);

Really sad that we are using the deprecatedEditingOffset here; there must be a more modern way to do this. (Doesn’t need to be fixed this time around I guess.)

> Source/WebCore/accessibility/AXObjectCache.cpp:1045
> +void AXObjectCache::postTextReplacementNotification(Node* node, AXTextEditType deletionType, const String& deletedText, AXTextEditType insertionType, const String& insertedText, const VisiblePosition& position)

A shame that there is so much nearly identical boilerplate code between this and postTextStateChangeNotification. Would be nice to reorganize to share a bit more code. (Doesn’t need to be fixed this time around I guess.)

> Source/WebCore/accessibility/AXObjectCache.cpp:1082
> +    if (!m_passwordNotificationsToPost.contains(observableObject)) {
> +        m_passwordNotificationsToPost.append(observableObject);

A vector is the wrong data structure if we are enforcing set semantics here. But I suppose the chance that this vector will get long is infinitesimal, so OK.

> Source/WebCore/accessibility/AXObjectCache.h:284
> +    bool enqueuePasswordValueChangeNotification(AccessibilityObject*);
> +
> +    AXTextStateChangeIntent m_textSelectionIntent;

We normally try to keep all the data members together at the end, after all the function members. It might be nice to reorganize this class in that fashion later. The current mix has some appeal because of the way it groups some data and the functions that work with that data, but it makes it a little hard to get the overview of what’s going on in the class.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:307
> +    AccessibilityObject* webArea = rootWebArea();

No need to put this into a local variable since it’s only used once.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:310
> +    [userInfo release];

Should have a blank line before this so it’s not paragraphed with the code just above and more clearly balances the alloc farther above.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:335
> +    AccessibilityObject* webArea = rootWebArea();

No need to put this into a local variable since it’s only used once.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:338
> +    [userInfo release];

Should have a blank line before this so it’s not paragraphed with the code just above and more clearly balances the alloc farther above.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.cpp:40
> +void ReplaceDeleteFromTextNodeCommand::notifyAccessibilityForTextChange(Node*, AXTextEditType, const String&, const VisiblePosition&)
> +{
> +}

Why is this function empty? Maybe it should be pure virtual instead.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150424/0c914672/attachment-0001.html>