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

[bugzilla-daemon at webkit.org]

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

--- Comment #110 from Brent Fulgham <bfulgham at webkit.org> ---
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

> LayoutTests/ChangeLog:8
> +        Richer accessibility value change notifications. Introduce AXTextEditType, postTextStateChangeNotification and postTextReplacementNotification to give assistive tech apps more reliable context for responding to changes in web content. Also implement a mechanism to post value changes in password form fields in coalesced ticks to thwart analyzing the cadence of changes.

Nit: These lines are kind of long. We usually hard-return at around 80 characters or so since many of our tools don't do auto-line-wrap.

> Source/WebCore/accessibility/AXObjectCache.cpp:872
> +void AXObjectCache::showIntent(const AXTextStateChangeIntent &intent)

Your using objc syntax here. Should be "const AXTextStateChangeIntent& intent)

> Source/WebCore/accessibility/AXObjectCache.cpp:985
> +void AXObjectCache::setTextSelectionIntent(AXTextStateChangeIntent intent)

Why is this passed by value here, when you pass by reference in the above method? If AXTextStateChangeIntent is non-trivial in size, we should be reference passing it everywhere.

> Source/WebCore/editing/Editor.cpp:2851
> +void Editor::changeSelectionAfterCommand(const VisibleSelection& newSelection, FrameSelection::SetSelectionOptions options, AXTextStateChangeIntent intent)

Seems like AXTextStateChangeIntent should be passed by reference, since it's an object?

> Source/WebCore/editing/FrameSelection.cpp:331
> +void FrameSelection::setSelection(const VisibleSelection& selection, SetSelectionOptions options, AXTextStateChangeIntent intent, CursorAlignOnScroll align, TextGranularity granularity)

Seems like AXTextStateChangeIntent should be passed as a reference.

> Source/WebCore/editing/FrameSelection.cpp:371
> +void FrameSelection::updateAndRevealSelection(AXTextStateChangeIntent intent)

const AXTextStateChangeIntent& ?

> Source/WebCore/editing/FrameSelection.cpp:1036
> +    AXTextStateChangeIntent intent = AXTextStateChangeIntent();

I think this could just be "AXTextStateChangeIntent intent;"

> Source/WebCore/editing/FrameSelection.h:148
> +    WEBCORE_EXPORT void setSelection(const VisibleSelection&, SetSelectionOptions = defaultSetSelectionOptions(), AXTextStateChangeIntent = AXTextStateChangeIntent(), CursorAlignOnScroll = AlignCursorOnScrollIfNeeded, TextGranularity = CharacterGranularity);

These AXTextStateChangeIntent seem like they should be references.

> Source/WebCore/editing/FrameSelection.h:276
> +    void updateAndRevealSelection(AXTextStateChangeIntent = AXTextStateChangeIntent());

Ditto.

> Source/WebCore/editing/FrameSelection.h:303
> +    void notifyAccessibilityForSelectionChange(AXTextStateChangeIntent = AXTextStateChangeIntent());

Ditto.

> Source/WebCore/editing/FrameSelection.h:305
> +    void notifyAccessibilityForSelectionChange(AXTextSelectionIntent) { }

Ditto.

> Source/WebCore/editing/FrameSelection.h:372
> +inline void FrameSelection::notifyAccessibilityForSelectionChange(AXTextStateChangeIntent)

Ditto.

> Source/WebCore/editing/atk/FrameSelectionAtk.cpp:81
> +void FrameSelection::notifyAccessibilityForSelectionChange(AXTextStateChangeIntent)

const AXTextStateChangeIntent& ?

> Source/WebCore/editing/mac/FrameSelectionMac.mm:50
> +void FrameSelection::notifyAccessibilityForSelectionChange(AXTextStateChangeIntent intent)

const AXTextStateChangeIntent& ?

-- 
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/4195f7e3/attachment-0001.html>