<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: richer text change notifications"
href="https://bugs.webkit.org/show_bug.cgi?id=142719">bug 142719</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #251541 Flags</td>
<td>review?
</td>
<td>review+, commit-queue+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: richer text change notifications"
href="https://bugs.webkit.org/show_bug.cgi?id=142719#c109">Comment # 109</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: richer text change notifications"
href="https://bugs.webkit.org/show_bug.cgi?id=142719">bug 142719</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=251541&action=diff" name="attach_251541" title="Fix windows builds">attachment 251541</a> <a href="attachment.cgi?id=251541&action=edit" title="Fix windows builds">[details]</a></span>
Fix windows builds
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251541&action=review">https://bugs.webkit.org/attachment.cgi?id=251541&action=review</a>
r=me; seems good as is, comments are about future clean-up and refinement.
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:759
> + for (const auto& notification : notifications)</span >
Not sure there’s an advantage of const auto& over auto&.
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1041
> + nodeTextChangePlatformNotification(object, textChangeForEditType(type), position.deepEquivalent().deprecatedEditingOffset(), text);</span >
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.)
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1045
> +void AXObjectCache::postTextReplacementNotification(Node* node, AXTextEditType deletionType, const String& deletedText, AXTextEditType insertionType, const String& insertedText, const VisiblePosition& position)</span >
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.)
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1082
> + if (!m_passwordNotificationsToPost.contains(observableObject)) {
> + m_passwordNotificationsToPost.append(observableObject);</span >
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.
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:284
> + bool enqueuePasswordValueChangeNotification(AccessibilityObject*);
> +
> + AXTextStateChangeIntent m_textSelectionIntent;</span >
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.
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:307
> + AccessibilityObject* webArea = rootWebArea();</span >
No need to put this into a local variable since it’s only used once.
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:310
> + [userInfo release];</span >
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.
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:335
> + AccessibilityObject* webArea = rootWebArea();</span >
No need to put this into a local variable since it’s only used once.
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:338
> + [userInfo release];</span >
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.
<span class="quote">> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.cpp:40
> +void ReplaceDeleteFromTextNodeCommand::notifyAccessibilityForTextChange(Node*, AXTextEditType, const String&, const VisiblePosition&)
> +{
> +}</span >
Why is this function empty? Maybe it should be pure virtual instead.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>