Bug 142719

Summary: AX: richer text change notifications
Product: WebKit Reporter: Doug Russell <d_russell>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: aestes, ap, buildbot, commit-queue, darin, jdiggs, mrobinson, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=25898
Bug Depends on: 144164, 144447    
Bug Blocks:    
Attachments:
Description Flags
Edit commands infrastructure
none
value change notifications
none
selection change notifications
none
tests
none
debugging
none
selection change notifications
none
value change notifications
none
selection change notifications
none
value change notifications
none
selection change notifications
none
debugging
none
value change notifications
none
selection change notifications
none
Edit commands infrastructure
none
value change notifications
none
selection change notifications
none
tests
none
debugging
none
Full patch
none
Full Patch
none
value change notifications
none
selection change notifications
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full patch
none
Edit commands infrastructure
none
value Change Notifications
none
selection change notifications
none
Full Patch
none
value change notifications
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
none
Full Patch
buildbot: commit-queue-
Edit commands infrastructure
none
value change notifications
none
selection change notifications
none
tests
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Full Patch
none
tests
none
Full Patch
none
value change notifications
none
selection change notifications
none
tests
none
Full Patch
none
Full Patch
none
Edit commands infrastructure
none
value change notifications
none
tests
none
selection change notifications
none
tests
none
Full Patch
darin: review-
Update based on review
none
Update based on review (fixed iOS/GTK build)
none
Update based on review (fixed iOS/GTK build take 2)
none
Update based on review (fixed iOS/GTK build take 3)
none
Update from review
none
Update from review
none
Update from review
none
Update from review
none
Update from review
none
Update from review
none
Update from review
none
Update from review
none
Update from review
none
Fix windows builds
none
Remove subscripting/object literals
none
Fix changelogs
none
patch
none
patch
none
patch
none
patch none

Description Doug Russell 2015-03-15 21:34:16 PDT
Text selection changed notifications allow assistive technology to respond to changing content and user navigation. The fidelity and reliability of these responses could be improved by including context data in the notifications as to what the user intent behind the change was.

Useful context data:

Edit operations like insert, delete, cut, paste. To help in discerning why text showed up or went away, not just that it did. Also knowing the source of an edit operation can be useful, like in the case of dictation, where speech echo could disrupt ongoing dictation.

Selection change granularity, direction, type. To help in discerning how to echo the change to the user. For example moving right by one word using option+right arrow should be echoed differently than moving the cursor to the next word by clicking on it.
Comment 1 Radar WebKit Bug Importer 2015-03-15 21:34:52 PDT
<rdar://problem/20169058>
Comment 2 Doug Russell 2015-03-15 22:50:59 PDT
(This is an extension of the work done in https://bugs.webkit.org/show_bug.cgi?id=25898)
Comment 3 Doug Russell 2015-04-02 23:05:08 PDT
Created attachment 250041 [details]
Edit commands infrastructure

Work split into 5 parts
1 Edit command infrastructure - changes that aren't directly AX related but that add the context AX needs
2 Value change notifications
3 Selection change notifications
4 Tests - Right now this just fixes the one failing test
5 Debug Logging - useful logging for testing, this will be omitted in the final patch
Comment 4 Doug Russell 2015-04-02 23:05:46 PDT
Created attachment 250042 [details]
value change notifications
Comment 5 Doug Russell 2015-04-02 23:06:05 PDT
Created attachment 250043 [details]
selection change notifications
Comment 6 Doug Russell 2015-04-02 23:06:42 PDT
Created attachment 250044 [details]
tests
Comment 7 Doug Russell 2015-04-02 23:06:59 PDT
Created attachment 250045 [details]
debugging
Comment 8 Doug Russell 2015-04-03 00:01:12 PDT
Created attachment 250048 [details]
selection change notifications

Fix method missing from previous patch
Comment 9 Doug Russell 2015-04-04 13:24:13 PDT
Created attachment 250136 [details]
value change notifications

Finish some todos and other cleanup
Comment 10 Doug Russell 2015-04-04 13:25:08 PDT
Created attachment 250137 [details]
selection change notifications

Finish some todos and other cleanup
Comment 11 Doug Russell 2015-04-06 15:52:03 PDT
Created attachment 250233 [details]
value change notifications
Comment 12 Doug Russell 2015-04-06 15:52:48 PDT
Created attachment 250234 [details]
selection change notifications
Comment 13 Doug Russell 2015-04-06 15:53:38 PDT
Created attachment 250235 [details]
debugging
Comment 14 WebKit Commit Bot 2015-04-06 15:57:10 PDT
Attachment 250041 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:62:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/editing/DeleteFromTextNodeCommand.cpp:58:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/editing/InsertNodeBeforeCommand.cpp:55:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Doug Russell 2015-04-06 16:06:35 PDT
Created attachment 250236 [details]
value change notifications
Comment 16 Doug Russell 2015-04-06 16:07:11 PDT
Created attachment 250237 [details]
selection change notifications
Comment 17 Doug Russell 2015-04-06 17:03:00 PDT
Created attachment 250241 [details]
Edit commands infrastructure

added change logs
Comment 18 Doug Russell 2015-04-06 17:04:04 PDT
Created attachment 250242 [details]
value change notifications

add change logs
Comment 19 Doug Russell 2015-04-06 17:04:29 PDT
Created attachment 250243 [details]
selection change notifications

add change logs
Comment 20 Doug Russell 2015-04-06 17:05:16 PDT
Created attachment 250244 [details]
tests

adding change logs
Comment 21 Doug Russell 2015-04-06 17:05:48 PDT
Created attachment 250245 [details]
debugging

adding change logs
Comment 22 Doug Russell 2015-04-06 17:21:33 PDT
Created attachment 250246 [details]
Full patch

Combined patch for EWS analysis
Comment 23 WebKit Commit Bot 2015-04-06 17:23:29 PDT
Attachment 250246 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:17:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/ChangeLog:215:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/ChangeLog:231:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/ChangeLog:287:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/ChangeLog:407:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 5 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Doug Russell 2015-04-07 13:24:26 PDT
Created attachment 250289 [details]
Full Patch

Patch for EWS analysis updated to fix failing builds on non mac platforms
Comment 25 Doug Russell 2015-04-07 13:25:26 PDT
Created attachment 250290 [details]
value change notifications

update for builds failing on non mac platforms
Comment 26 Doug Russell 2015-04-07 13:25:59 PDT
Created attachment 250291 [details]
selection change notifications

update for builds failing on non mac platforms
Comment 27 Doug Russell 2015-04-07 13:31:32 PDT
Created attachment 250292 [details]
Full Patch
Comment 28 Doug Russell 2015-04-07 13:46:36 PDT
Created attachment 250294 [details]
Full Patch
Comment 29 Doug Russell 2015-04-07 13:51:54 PDT
Created attachment 250296 [details]
Full Patch
Comment 30 Doug Russell 2015-04-07 13:55:36 PDT
Created attachment 250297 [details]
Full Patch
Comment 31 Doug Russell 2015-04-07 14:04:39 PDT
Created attachment 250298 [details]
Full Patch
Comment 32 Doug Russell 2015-04-07 14:22:31 PDT
Created attachment 250300 [details]
Full Patch
Comment 33 Doug Russell 2015-04-07 14:37:53 PDT
Created attachment 250304 [details]
Full Patch
Comment 34 Doug Russell 2015-04-07 14:59:36 PDT
Created attachment 250307 [details]
Full Patch
Comment 35 Doug Russell 2015-04-07 15:41:15 PDT
Created attachment 250309 [details]
Full Patch
Comment 36 Doug Russell 2015-04-07 16:45:07 PDT
Created attachment 250314 [details]
Full Patch
Comment 37 Doug Russell 2015-04-07 17:32:41 PDT
Created attachment 250320 [details]
Full Patch
Comment 38 Doug Russell 2015-04-07 18:43:14 PDT
Created attachment 250322 [details]
Full Patch
Comment 39 Doug Russell 2015-04-07 19:47:11 PDT
Created attachment 250325 [details]
Full Patch
Comment 40 Doug Russell 2015-04-07 20:53:29 PDT
Created attachment 250329 [details]
Full Patch
Comment 41 Doug Russell 2015-04-07 21:47:37 PDT
Created attachment 250332 [details]
Full Patch
Comment 42 chris fleizach 2015-04-08 10:03:28 PDT
Comment on attachment 250290 [details]
value change notifications

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

> Source/WebCore/ChangeLog:5
> +        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.

description should go after the Reviewed by line

> Source/WebCore/accessibility/AXObjectCache.cpp:757
> +        // Delegate to the right platform

comment is unnecessary. we only need comments generally to explain why something is happening. what is happening should be self evident from the code

> Source/WebCore/accessibility/AXObjectCache.cpp:874
> +        text = obj.passwordFieldOrContainingPasswordField()->passwordFieldValue().substring(position.deepEquivalent().deprecatedEditingOffset(), text.length());

how is this sanitizing the value? is there not a method on HTMLInputType to give you the sanitized version already?

> Source/WebCore/accessibility/AXObjectCache.cpp:893
> +        if (sanitizePasswordText(*obj, sanitizedText, position)) {

why do we have this special code for password fields?

> Source/WebCore/accessibility/AXObjectCache.cpp:906
> +    AccessibilityObject* obj = getOrCreate(node);

this line is common to both platforms and can be moved above

> Source/WebCore/accessibility/AXObjectCache.cpp:924
> +    if (obj) {

if there's no obj can we do an early return?

> Source/WebCore/accessibility/AXObjectCache.cpp:938
>      AccessibilityObject* obj = getOrCreate(node);

ditto about AXObject

> Source/WebCore/accessibility/AXObjectCache.cpp:1168
> +    if (AccessibilityObject *rootObject = this->rootObject()) {

is the root object always a ScrollView? I think it probably is. in that case we can cast to scroll view and call the webArea() method directly instead of relying on that it will be the first object

> Source/WebCore/accessibility/AXObjectCache.h:192
> +    enum AXTextStateChangeType {

what other change types can you envision?

> Source/WebCore/accessibility/AXObjectCache.h:201
> +        AXTextEditTypeTyping, // Insert via typing

how is insert/delete different from typing? why would you use typing instead of insert

> Source/WebCore/accessibility/AXObjectCache.h:210
> +    void postTextReplacementNotification(Node*, AXTextEditType, const String&, AXTextEditType, const String&, const VisiblePosition&);

why are there two AXTextEditTypes?
because there are two we should name them here in the header

> Source/WebCore/accessibility/AXObjectCache.h:240
> +        AXTextDeleted,

can you also have a text moved type? (i.e. you can drag blocks of text on OS X)

> Source/WebCore/accessibility/AXObjectCache.h:290
> +    Timer m_passwordNotificationPostTimer;

still curious why we need special timers for passwords

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:568
> +            if (AXObjectCache *cache = axObjectCache())

* on wrong side

> Source/WebCore/accessibility/AccessibilityNodeObject.h:91
> +    virtual bool isContainedByPasswordField() const override;

i think we can move this implementation in AccessibilityObject and then we can remove the virtual

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:195
> +static NSDictionary* textReplacementChangeDictionary(AccessibilityObject* obj, AXObjectCache::AXTextEditType type, const String& string, const VisiblePosition& position)

ObjcC stars should be on the other side

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:197
> +    NSString* text = (NSString *)string;

ditto about this star.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:246
> +    if (!obj)

is it possible to combine the implementation of this method with the more detailed one (or rather vice versa in this case)

> Source/WebCore/editing/CutDeleteFromTextNodeCommand.h:44
> +    virtual AXObjectCache::AXTextEditType applyEditType() override;

should this be final override

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp:53
> +    case AXObjectCache::AXTextEditTypePaste:

can you just make the default: case be ASSERT_NOT_REACHED
Comment 43 Doug Russell 2015-04-08 10:46:58 PDT
(In reply to comment #42)
> Comment on attachment 250290 [details]
> value change notifications
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250290&action=review
> 
> > Source/WebCore/ChangeLog:5
> > +        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.
> 
> description should go after the Reviewed by line

Will do

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:757
> > +        // Delegate to the right platform
> 
> comment is unnecessary. we only need comments generally to explain why
> something is happening. what is happening should be self evident from the
> code

Will do

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:874
> > +        text = obj.passwordFieldOrContainingPasswordField()->passwordFieldValue().substring(position.deepEquivalent().deprecatedEditingOffset(), text.length());
> 
> how is this sanitizing the value? is there not a method on HTMLInputType to
> give you the sanitized version already?

It's using passwordFieldValue() if the element is a password field or contained by a password field

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:893
> > +        if (sanitizePasswordText(*obj, sanitizedText, position)) {
> 
> why do we have this special code for password fields?

To make sure that we're not posting password characters in notifications and to make sure we're posting password notifications at regular ticks so they can't be analyzed

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:906
> > +    AccessibilityObject* obj = getOrCreate(node);
> 
> this line is common to both platforms and can be moved above

Will do

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:924
> > +    if (obj) {
> 
> if there's no obj can we do an early return?

The Mac logic handles cases where obj == nullptr

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:938
> >      AccessibilityObject* obj = getOrCreate(node);
> 
> ditto about AXObject

Will do

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1168
> > +    if (AccessibilityObject *rootObject = this->rootObject()) {
> 
> is the root object always a ScrollView? I think it probably is. in that case
> we can cast to scroll view and call the webArea() method directly instead of
> relying on that it will be the first object

Sounds reasonable, I'll check.

> 
> > Source/WebCore/accessibility/AXObjectCache.h:192
> > +    enum AXTextStateChangeType {
> 
> what other change types can you envision?

One I've considered is an explicit type for find and replace, especially in the multiple find and replace case where echoing the result might be complex. To answer a later review question, possibly a move type if there's a need to discern that from a delete followed by an insert.

> 
> > Source/WebCore/accessibility/AXObjectCache.h:201
> > +        AXTextEditTypeTyping, // Insert via typing
> 
> how is insert/delete different from typing? why would you use typing instead
> of insert

A good example of insert not being typing is using javascript to set a form value

> 
> > Source/WebCore/accessibility/AXObjectCache.h:210
> > +    void postTextReplacementNotification(Node*, AXTextEditType, const String&, AXTextEditType, const String&, const VisiblePosition&);
> 
> why are there two AXTextEditTypes?
> because there are two we should name them here in the header

For the deletion and insert, I'll add variable names

> 
> > Source/WebCore/accessibility/AXObjectCache.h:240
> > +        AXTextDeleted,
> 
> can you also have a text moved type? (i.e. you can drag blocks of text on OS
> X)

Answered above

> 
> > Source/WebCore/accessibility/AXObjectCache.h:290
> > +    Timer m_passwordNotificationPostTimer;
> 
> still curious why we need special timers for passwords

To thwart analyzing the cadence of the notifications to narrow down possible password

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:568
> > +            if (AXObjectCache *cache = axObjectCache())
> 
> * on wrong side
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.h:91
> > +    virtual bool isContainedByPasswordField() const override;
> 
> i think we can move this implementation in AccessibilityObject and then we
> can remove the virtual

Will do

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:195
> > +static NSDictionary* textReplacementChangeDictionary(AccessibilityObject* obj, AXObjectCache::AXTextEditType type, const String& string, const VisiblePosition& position)
> 
> ObjcC stars should be on the other side

Will do

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:197
> > +    NSString* text = (NSString *)string;
> 
> ditto about this star.

Will do

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:246
> > +    if (!obj)
> 
> is it possible to combine the implementation of this method with the more
> detailed one (or rather vice versa in this case)

Do you mean combine postTextStateChangePlatformNotification and postTextReplacementPlatformNotification?

> 
> > Source/WebCore/editing/CutDeleteFromTextNodeCommand.h:44
> > +    virtual AXObjectCache::AXTextEditType applyEditType() override;
> 
> should this be final override

Will do

> 
> > Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp:53
> > +    case AXObjectCache::AXTextEditTypePaste:
> 
> can you just make the default: case be ASSERT_NOT_REACHED

I could do that for non debug builds, but not having a default means I get warnings if I change the enum which is quite helpful
Comment 44 Doug Russell 2015-04-08 12:19:40 PDT
Created attachment 250368 [details]
Full patch
Comment 45 Doug Russell 2015-04-08 12:20:18 PDT
Created attachment 250369 [details]
Edit commands infrastructure
Comment 46 Doug Russell 2015-04-08 12:21:07 PDT
Created attachment 250370 [details]
value Change Notifications
Comment 47 Doug Russell 2015-04-08 12:21:37 PDT
Created attachment 250371 [details]
selection change notifications
Comment 48 WebKit Commit Bot 2015-04-08 12:22:37 PDT
Attachment 250368 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AXObjectCache.cpp:758:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Doug Russell 2015-04-08 12:29:37 PDT
Created attachment 250372 [details]
Full Patch
Comment 50 Doug Russell 2015-04-08 12:30:45 PDT
Created attachment 250373 [details]
value change notifications
Comment 51 WebKit Commit Bot 2015-04-08 12:32:36 PDT
Attachment 250372 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AXObjectCache.cpp:758:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Doug Russell 2015-04-08 12:51:27 PDT
Created attachment 250377 [details]
Full Patch
Comment 53 WebKit Commit Bot 2015-04-08 12:54:57 PDT
Attachment 250377 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AXObjectCache.cpp:758:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 54 Doug Russell 2015-04-08 12:58:53 PDT
Created attachment 250378 [details]
Full Patch
Comment 55 Doug Russell 2015-04-09 13:10:13 PDT
Created attachment 250454 [details]
Full Patch
Comment 56 Doug Russell 2015-04-09 13:20:02 PDT
Created attachment 250456 [details]
Full Patch
Comment 57 Doug Russell 2015-04-10 18:29:06 PDT
Created attachment 250548 [details]
Full Patch
Comment 58 WebKit Commit Bot 2015-04-10 18:31:07 PDT
Attachment 250548 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AXObjectCache.cpp:1071:  Declaration has space between type name and * in AccessibilityObject *observableObject  [whitespace/declaration] [3]
Total errors found: 1 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 59 Doug Russell 2015-04-10 18:40:29 PDT
Created attachment 250550 [details]
Full Patch
Comment 60 WebKit Commit Bot 2015-04-10 18:43:55 PDT
Attachment 250550 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AXObjectCache.cpp:1071:  Declaration has space between type name and * in AccessibilityObject *observableObject  [whitespace/declaration] [3]
Total errors found: 1 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 61 Doug Russell 2015-04-10 18:44:39 PDT
Created attachment 250552 [details]
Full Patch
Comment 62 WebKit Commit Bot 2015-04-10 18:48:35 PDT
Attachment 250552 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:21:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 63 Doug Russell 2015-04-10 18:51:31 PDT
Created attachment 250554 [details]
Full Patch
Comment 64 Doug Russell 2015-04-14 12:13:18 PDT
Created attachment 250721 [details]
Full Patch
Comment 65 Doug Russell 2015-04-14 12:25:23 PDT
Created attachment 250722 [details]
Edit commands infrastructure
Comment 66 Doug Russell 2015-04-14 12:26:10 PDT
Created attachment 250723 [details]
value change notifications
Comment 67 Doug Russell 2015-04-14 12:26:32 PDT
Created attachment 250724 [details]
selection change notifications
Comment 68 Doug Russell 2015-04-14 12:26:52 PDT
Created attachment 250725 [details]
tests
Comment 69 Build Bot 2015-04-14 13:06:57 PDT
Comment on attachment 250721 [details]
Full Patch

Attachment 250721 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5707236219289600

New failing tests:
platform/mac/accessibility/selection-change-userinfo.html
Comment 70 Build Bot 2015-04-14 13:07:02 PDT
Created attachment 250730 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 71 Doug Russell 2015-04-14 13:47:34 PDT
Created attachment 250735 [details]
Full Patch
Comment 72 Doug Russell 2015-04-14 14:01:35 PDT
Created attachment 250736 [details]
tests
Comment 73 Doug Russell 2015-04-15 11:13:58 PDT
Created attachment 250825 [details]
Full Patch
Comment 74 Doug Russell 2015-04-15 13:55:38 PDT
Created attachment 250854 [details]
value change notifications
Comment 75 Doug Russell 2015-04-15 13:56:51 PDT
Created attachment 250856 [details]
selection change notifications
Comment 76 Doug Russell 2015-04-15 13:57:26 PDT
Created attachment 250857 [details]
tests
Comment 77 Doug Russell 2015-04-16 19:31:34 PDT
Created attachment 250989 [details]
Full Patch

Fix delete and cut commands being swapped
Comment 78 Doug Russell 2015-04-16 19:36:14 PDT
Created attachment 250991 [details]
Full Patch

Fix delete and cut commands being swapped

Minor test updates
Comment 79 Doug Russell 2015-04-16 19:37:35 PDT
Created attachment 250992 [details]
Edit commands infrastructure

Remove method removed in subsequent patch
Comment 80 Doug Russell 2015-04-16 19:38:17 PDT
Created attachment 250993 [details]
value change notifications

Fix delete and cut commands being swapped
Comment 81 Doug Russell 2015-04-16 19:38:42 PDT
Created attachment 250994 [details]
tests
Comment 82 Doug Russell 2015-04-17 14:48:01 PDT
Created attachment 251051 [details]
selection change notifications

Switch showIntent() from printf() to dataLogF()
Comment 83 Doug Russell 2015-04-17 14:48:34 PDT
Created attachment 251052 [details]
tests

Add tests for value change notifications
Comment 84 Doug Russell 2015-04-17 14:51:10 PDT
Created attachment 251054 [details]
Full Patch

Switch showIntent() from printf() to dataLogF()
Add tests for value change notifications
Comment 85 Darin Adler 2015-04-18 12:02:51 PDT
Comment on attachment 251054 [details]
Full Patch

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

Very ambitious! Lots of small mistakes, generally seems like it’s OK, but I am not sure that the basic concept is right. This pattern with lots of different classes for all these variations of commands seems like overkill. Instead we should probably simply track the small bit of data that is different for these in a data member rather than making a different class for each.

One other small nit, classes that have nothing derived from them should be marked final.

One piece of advice: I suggest talking to someone who’s an expert on the code before making such a large patch. Maybe you did, but if you talked to an editing expert you might have saved the tons of time I’m sure you spent making all these derived classes.

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

Should call this notification, not object.

> Source/WebCore/accessibility/AXObjectCache.cpp:873
> +        dataLogF("Unknown");

These should all be dataLog. The only time you use dataLogF is when for some reason you need to use a C-style format string with % formatting.

> Source/WebCore/accessibility/AXObjectCache.cpp:987
> +void AXObjectCache::postTextStateChangeNotification(Node *node, AXTextStateChangeIntent intent, const VisibleSelection& selection)

Formatting here is incorrect. Should be "Node* node".

> Source/WebCore/accessibility/AXObjectCache.cpp:998
>      AccessibilityObject* obj = getOrCreate(node);

Should call this object, not obj.

> Source/WebCore/accessibility/AXObjectCache.cpp:1023
> +    AccessibilityObject* obj = getOrCreate(node);

Should call this object, not obj.

> Source/WebCore/accessibility/AXObjectCache.cpp:1032
> +    if (obj) {
> +        if (obj->isPasswordField() || obj->isContainedByPasswordField()) {
> +            enqueuePasswordValueChangeNotification(obj);
> +            return;
> +        }
> +        obj = obj->observableObject();
> +    }
> +

This code sequence is related over and over again. You should make a helper function for this so we don’T have lots of copies of it.

> Source/WebCore/accessibility/AXObjectCache.cpp:1048
> +    AccessibilityObject* obj = getOrCreate(node);

Should call this object, not obj.

> Source/WebCore/accessibility/AXObjectCache.cpp:1065
> +void AXObjectCache::enqueuePasswordValueChangeNotification(AccessibilityObject* obj)

Should call this object, not obj.

> Source/WebCore/accessibility/AXObjectCache.cpp:1074
> +    if (std::find(m_passwordNotificationsToPost.begin(), m_passwordNotificationsToPost.end(), observableObject) == m_passwordNotificationsToPost.end()) {

Vector has a function named contains that should be used instead of std::find for this. But also, if this operation is needed, then I suggest using a WTF::ListHashSet, which is the right data structure if you want an ordered set of objects but don’t want to add duplicates. Finally, we normally find code like this is more readable if we use early return.

> Source/WebCore/accessibility/AXObjectCache.cpp:1077
> +            m_passwordNotificationPostTimer.startOneShot(0.025);

Where does this 0.025 second number come from? Why is that an appropriate delay? Normally such constants are named at the top of the file along with a comment explaining the rationale.

> Source/WebCore/accessibility/AXObjectCache.cpp:1305
> +    if (AccessibilityObject *rootObject = this->rootObject()) {

Formatting here is wrong; should be AccessibilityObject*. Also, I suggest writing this with early return instead of nesting.

> Source/WebCore/accessibility/AXObjectCache.h:192
> +    enum AXTextStateChangeType {

Since this type is a member of AXObjectCache, it’s a little strange to also give it an AX prefix. Namespacing means this is all inside AX, and so it’s kind of like “I heard you like AX, so I put an AX inside your AX”.

> Source/WebCore/accessibility/AXObjectCache.h:246
> +    static AXTextSelection TextSelection(AXTextSelectionDirection direction = AXTextSelectionDirectionUnknown, AXTextSelectionGranularity granularity = AXTextSelectionGranularityUnknown)
> +    {
> +        auto selection = AXTextSelection();
> +        selection.direction = direction;
> +        selection.granularity = granularity;
> +        return selection;
> +    }

Functions should be named with a lowercase letter.

The implementation of this function should just be one line:

    return { direction, granularity };

Or it may need to be:

    return AXTextSelection { direction, granularity };

But it doesn’t need to be four lines.

I’m not sure this function is needed at all. Unless the default values are really valuable, I think we could just use the AXTextSelection { xxx } syntax at the call sites.

> Source/WebCore/accessibility/AXObjectCache.h:248
> +    struct AXTextStateChangeIntent {

This struct definition does not need to be inside the class definition. We can just forward declare it in the class and then put the actual definition later in the header file. I think this might make the class definition much easier to read. It’s getting kind of colossal and hard to understand.

> Source/WebCore/accessibility/AXObjectCache.h:254
> +        bool sync;

The word “sync” is not a great name for a boolean, because the boolean is not a “sync”. A boolean should be named with an adjective phrase of the predicate it represents, such as "needsSynchronization" or whatever it is that "sync" means here.

> Source/WebCore/accessibility/AXObjectCache.h:260
> +        { }

WebKit coding style is to put these on successive lines once the thing is “vertical” like this and only do it on one line if the whole function is on one line.

> Source/WebCore/accessibility/AXObjectCache.h:265
> +            , sync(false)

This default should be dealt with when the member is declared:

    bool sync { false };

Then you don’t need to repeat it in each constructor.

> Source/WebCore/accessibility/AXObjectCache.h:270
> +            , selection(AXTextSelection())

I think you can write this as:

    , selection()

And get the same result. The AXTextSelection() part isn’t needed.

> Source/WebCore/accessibility/AXObjectCache.h:317
> +    inline AXTextChange textChangeForEditType(AXTextEditType type)

The “inline” here is not needed and has no effect.

> Source/WebCore/accessibility/AXObjectCache.h:322
> +            return AXObjectCache::AXTextDeleted;

The AXObjectCache:: prefix here is not needed since this is inside an AXObjectCache member function.

> Source/WebCore/accessibility/AXObjectCache.h:327
> +            return AXObjectCache::AXTextInserted;

Ditto.

> Source/WebCore/accessibility/AXObjectCache.h:329
> +        default:

The best practice is to leave the default case out of this switch statement so that the compiler will warn if we forget to include any of the enum cases. Since all the cases are returning, we can still ASSERT_NOT_REACHED for a bad value that happens to occur runtime; the code for that goes after the switch statement outside of it.

> Source/WebCore/accessibility/AXObjectCache.h:331
> +            return AXObjectCache::AXTextInserted;

Ditto.

> Source/WebCore/accessibility/AXObjectCache.h:446
> +#if PLATFORM(COCOA)
> +inline void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*, AXTextStateChangeIntent, const VisibleSelection&) { }
> +inline void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*, AXTextEditType, const String&, const VisiblePosition&) { }
> +inline void AXObjectCache::postTextReplacementPlatformNotification(AccessibilityObject*, AXTextEditType, const String&, AXTextEditType, const String&, const VisiblePosition&) { }
> +#else
>  inline void AXObjectCache::nodeTextChangePlatformNotification(AccessibilityObject*, AXTextChange, unsigned, const String&) { }
> +#endif

It’s better to not mix the conditional code up with the unconditional. Please put this in a separate paragraph after the rest.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:555
> +AccessibilityObject* AccessibilityNodeObject::passwordFieldOrContainingPasswordField() const

Why does this function need to be const?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:567
> +    if (Element* element = node->shadowHost()) {
> +        if (is<HTMLInputElement>(*element)) {

Another way to write this without the nesting is:

    Element* element = node->shadowHost();
    if (is<HTMLInputElement>(element)) {

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:568
> +            if (AXObjectCache *cache = axObjectCache())

Incorrect formatting of the AXObjectCache* here.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2666
> +    HTMLInputElement* inputElement = nullptr;
> +    if (Element* element = node->shadowHost()) {
> +        if (is<HTMLInputElement>(*element))
> +            inputElement = downcast<HTMLInputElement>(element);
> +    }
> +    if (!inputElement)
> +        return false;
> +    
> +    return inputElement->isPasswordField();

Here’s how I would write this:

    Element* element = node->shadowHost();
    return is<HTMLInputElement>(element) && downcast<HTMLInputElement>(*element).isPasswordField();

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1500
> +    Position pos1 = Position(node, range.start, Position::PositionIsOffsetInAnchor);
> +    Position pos2 = Position(node, range.start + range.length, Position::PositionIsOffsetInAnchor);

We try to avoid abbreviations and names such as "pos1" and "pos2". How about "start" and "end" or "startPosition" and "endPosition" or if you much prefer numbers, "position1" and "position2"?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1501
> +    VisibleSelection visibleSelection = VisibleSelection(pos1, pos2, DOWNSTREAM);

I would write:

    VisibleSelection newSelection(start, end, DOWNSTREAM);

I don’t think he word “visible” in the variable name is helpful, and I think it’s unnecessarily wordy to write:

    VisibleSelection x = VisibleSelection(...);

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:203
> +void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject* obj, AXTextStateChangeIntent intent, const VisibleSelection& selection)

Please don’t name a local variable “obj”.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:236
> +        case AXTextStateChangeTypeEdit:
> +            break;
> +        default:
> +            break;

I don’t understand why you have a case here for both AXTextStateChangeTypeEdit and default. I suggest omitting the default if we are covering all the cases, or omitting AXTextStateChangeTypeEdit if we are not covering all the cases.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:250
> +    // Used by DRT to know when notifications are posted.

These are not good comments. First of all, it’s not appropriate for us to put code into WebKit just for DumpRenderTree. Second of all, the tool won’t necessarily always be named DumpRenderTree, which is kind of a crazy name, and the abbreviation DRT will be really mysterious in the future. Finally, the comment doesn’t really explain this sufficiently. I’m assuming the issue is that it’s too hard for us to hook our testing tool into the real accessibility notification machinery so we made a second one just for testing. I’m not sure that strategy is a good one.

I suggest we make a helper function that does both NSAccessibilityPostNotificationWithUserInfo and calls the wrapper. That helper function could have a longer comment that explained more clearly what this is, without the DRT abbreviation. Generally speaking the comment should explain in a slightly more abstract way what this is for, rather than specifically saying there is exactly one client, DRT, using it. But also it’s critical not to repeat the code and the comment over and over again.

I’d go further and say that it would generally be good to factor this to share more of the code for clarity. There’s a lot of copied and pasted code here.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:254
> +    // Used by DRT to know when notifications are posted.

Please use shared function as mentioned above.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:260
> +static NSDictionary *textReplacementChangeDictionary(AccessibilityObject* obj, AXObjectCache::AXTextEditType type, const String& string, const VisiblePosition& position)

Please don’t use the abbreviation "obj". I believe the WebKit coding style guidelines documents explains the rationale for this.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:268
> +    static const NSUInteger truncationLength = 1000;

Part of the value of a constant in a case like this is to explain where the number came from. Please put in a why comment explaining the choice of 1000 UTF-16 code units as a reasonable limit.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:273
> +    if (length > 0)

WebKit coding style says to use "if (length)" here. But also, this function returned nil earlier if length was zero, so this if should be omitted.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:282
> +void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject* obj, AXTextEditType type, const String& text, const VisiblePosition& position)

Please don’t use the abbreviation "obj". I believe the WebKit coding style guidelines documents explains the rationale for this.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:305
> +    // Used by DRT to know when notifications are posted.

Please use shared function as mentioned above.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:309
> +    // Used by DRT to know when notifications are posted.

Please use shared function as mentioned above.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:340
> +    // Used by DRT to know when notifications are posted.

Please use shared function as mentioned above.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:344
> +    // Used by DRT to know when notifications are posted.

Please use shared function as mentioned above.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:398
> +    for (NSUInteger idx = 0; idx < [mutableArray count]; idx++) {

Please don’t use the abbreviation "idx". If you don’t want to use the customary “i”, which we use almost everywhere in WebKit code, then please use a word “index”. The in-between “idx” isn’t right for our coding style.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:405
> +            [mutableArray removeObjectAtIndex:idx];

This won’t work. We will end up skipping one of the element in the array, because we’ll remove the object at this index, but we will still move on to the next index. So if index 0 is neither a string nor a number, we’ll next move on to the object that was originally at index 2 and never process the object that was originally at index 1.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:417
> +            mutableDictionary[key] = dictionaryRemovingNonJSONTypes(value);

I don’t think NSDictionary allows modification of a dictionary while it is being enumerated, even if it’s just the value being modified.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:419
> +            mutableDictionary[key] = arrayRemovingNonJSONTypes(value);

I don’t think NSDictionary allows modification of a dictionary while it is being enumerated, even if it’s just the value being modified.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:421
> +            [mutableDictionary removeObjectForKey:key];

NSDictionary definitely doesn’t support "for in" iteration while the dictionary is being enumerated, so this code won’t work. How did you test it?

> Source/WebCore/editing/AppendNodeCommand.cpp:59
> +    const String text = node->nodeValue();

The const here doesn’t add much. Please don’t declare specific local variables const unless there is a good reason to single them out. Almost all local variables could be marked "const".

> Source/WebCore/editing/CompositeEditCommand.cpp:171
> +    if (m_commands.size())
> +        return m_commands[m_commands.size()-1]->unapplyIntent();

WebKit coding style requires spaces around the "-" operator. But also, here’s how you’d write this:

    if (!m_commands.isEmpty())
        return m_commands.last()->unapplyIntent();

> Source/WebCore/editing/CompositeEditCommand.h:110
> +    virtual Ref<EditCommand> deleteFromTextNodeCommand(PassRefPtr<Text>, unsigned, unsigned);

Why is this function virtual? The two arguments need names. The type “unsigned” alone is not good.

Argument types for new functions should not be PassRefPtr even though legacy ones use that type. Instead if we are handing off ownership they should be RefPtr&& or maybe even Ref&& if they must be non-null. If we are not handing off ownership they should be Node* or, even better, Node&.

insertNodeBeforeCommand is not a good name for a function that creates a command. It should have the word “create” in its name. When we name a function with just a noun, then it implies something different -- returning something owned by this object -- not creating something.

> Source/WebCore/editing/CompositeEditCommand.h:117
> +    virtual Ref<EditCommand> insertNodeBeforeCommand(PassRefPtr<Node>, PassRefPtr<Node>, ShouldAssumeContentIsAlwaysEditable);

Same comments.

> Source/WebCore/editing/CompositeEditCommand.h:121
> +    virtual Ref<EditCommand> insertTextIntoNodeCommand(PassRefPtr<Text>, unsigned, const String&);

Same comments.

> Source/WebCore/editing/CompositeEditCommand.h:140
> +    virtual Ref<DeleteFromTextNodeCommand> replaceDeleteFromTextNodeCommand(PassRefPtr<Text>, unsigned, unsigned);

Same comments.

> Source/WebCore/editing/CompositeEditCommand.h:141
> +    virtual Ref<EditCommand> replaceInsertIntoTextNodeCommand(PassRefPtr<Text>, unsigned, const String&, const String&);

Same comments.
Comment 86 Doug Russell 2015-04-18 17:01:55 PDT
(In reply to comment #85)
> Comment on attachment 251054 [details]
> Full Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251054&action=review
> 
> Very ambitious! Lots of small mistakes, generally seems like it’s OK, but I
> am not sure that the basic concept is right. This pattern with lots of
> different classes for all these variations of commands seems like overkill.
> Instead we should probably simply track the small bit of data that is
> different for these in a data member rather than making a different class
> for each.

So that would be 4 new members:

AXObjectCache::AXTextEditType m_applyEditType;
AXObjectCache::AXTextEditType m_unapplyEditType;
AXObjectCache::AXTextStateChangeIntent m_applyIntent;
AXObjectCache::AXTextStateChangeIntent m_unapplyIntent;

I'll add those and update the appropriate constructors.

This is definitely better if we're ok with adding those data members.

> 
> One other small nit, classes that have nothing derived from them should be
> marked final.
> 
> One piece of advice: I suggest talking to someone who’s an expert on the
> code before making such a large patch. Maybe you did, but if you talked to
> an editing expert you might have saved the tons of time I’m sure you spent
> making all these derived classes.

I sent it out to webkit-dev, but haven't heard any responses: https://lists.webkit.org/pipermail/webkit-dev/2015-April/027377.html.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:756
> > +    for (const auto& object : notifications)
> 
> Should call this notification, not object.

Will do. (And will do on everything without a comment after this.)

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:873
> > +        dataLogF("Unknown");
> 
> These should all be dataLog. The only time you use dataLogF is when for some
> reason you need to use a C-style format string with % formatting.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:987
> > +void AXObjectCache::postTextStateChangeNotification(Node *node, AXTextStateChangeIntent intent, const VisibleSelection& selection)
> 
> Formatting here is incorrect. Should be "Node* node".
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:998
> >      AccessibilityObject* obj = getOrCreate(node);
> 
> Should call this object, not obj.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1023
> > +    AccessibilityObject* obj = getOrCreate(node);
> 
> Should call this object, not obj.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1032
> > +    if (obj) {
> > +        if (obj->isPasswordField() || obj->isContainedByPasswordField()) {
> > +            enqueuePasswordValueChangeNotification(obj);
> > +            return;
> > +        }
> > +        obj = obj->observableObject();
> > +    }
> > +
> 
> This code sequence is related over and over again. You should make a helper
> function for this so we don’T have lots of copies of it.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1048
> > +    AccessibilityObject* obj = getOrCreate(node);
> 
> Should call this object, not obj.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1065
> > +void AXObjectCache::enqueuePasswordValueChangeNotification(AccessibilityObject* obj)
> 
> Should call this object, not obj.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1074
> > +    if (std::find(m_passwordNotificationsToPost.begin(), m_passwordNotificationsToPost.end(), observableObject) == m_passwordNotificationsToPost.end()) {
> 
> Vector has a function named contains that should be used instead of
> std::find for this. But also, if this operation is needed, then I suggest
> using a WTF::ListHashSet, which is the right data structure if you want an
> ordered set of objects but don’t want to add duplicates. Finally, we
> normally find code like this is more readable if we use early return.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1077
> > +            m_passwordNotificationPostTimer.startOneShot(0.025);
> 
> Where does this 0.025 second number come from? Why is that an appropriate
> delay? Normally such constants are named at the top of the file along with a
> comment explaining the rationale.

It's a 40hz cadence to match existing timing in AppKit that was provided by Product Security.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1305
> > +    if (AccessibilityObject *rootObject = this->rootObject()) {
> 
> Formatting here is wrong; should be AccessibilityObject*. Also, I suggest
> writing this with early return instead of nesting.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:192
> > +    enum AXTextStateChangeType {
> 
> Since this type is a member of AXObjectCache, it’s a little strange to also
> give it an AX prefix. Namespacing means this is all inside AX, and so it’s
> kind of like “I heard you like AX, so I put an AX inside your AX”.

It is possible we'll be duplicating these enum layouts in other implementations where enums are not namespaced and ideally we can give them the same names to ease comparing logic. If it's alright I'd like to leave them as is.

> 
> > Source/WebCore/accessibility/AXObjectCache.h:246
> > +    static AXTextSelection TextSelection(AXTextSelectionDirection direction = AXTextSelectionDirectionUnknown, AXTextSelectionGranularity granularity = AXTextSelectionGranularityUnknown)
> > +    {
> > +        auto selection = AXTextSelection();
> > +        selection.direction = direction;
> > +        selection.granularity = granularity;
> > +        return selection;
> > +    }
> 
> Functions should be named with a lowercase letter.
> 
> The implementation of this function should just be one line:
> 
>     return { direction, granularity };
> 
> Or it may need to be:
> 
>     return AXTextSelection { direction, granularity };
> 
> But it doesn’t need to be four lines.
> 
> I’m not sure this function is needed at all. Unless the default values are
> really valuable, I think we could just use the AXTextSelection { xxx }
> syntax at the call sites.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:248
> > +    struct AXTextStateChangeIntent {
> 
> This struct definition does not need to be inside the class definition. We
> can just forward declare it in the class and then put the actual definition
> later in the header file. I think this might make the class definition much
> easier to read. It’s getting kind of colossal and hard to understand.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:254
> > +        bool sync;
> 
> The word “sync” is not a great name for a boolean, because the boolean is
> not a “sync”. A boolean should be named with an adjective phrase of the
> predicate it represents, such as "needsSynchronization" or whatever it is
> that "sync" means here.

It's meant to convey that the selection is the result of syncing up with an assistive app. I'll change it to isSynchronizing.

> 
> > Source/WebCore/accessibility/AXObjectCache.h:260
> > +        { }
> 
> WebKit coding style is to put these on successive lines once the thing is
> “vertical” like this and only do it on one line if the whole function is on
> one line.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:265
> > +            , sync(false)
> 
> This default should be dealt with when the member is declared:
> 
>     bool sync { false };
> 
> Then you don’t need to repeat it in each constructor.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:270
> > +            , selection(AXTextSelection())
> 
> I think you can write this as:
> 
>     , selection()
> 
> And get the same result. The AXTextSelection() part isn’t needed.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:317
> > +    inline AXTextChange textChangeForEditType(AXTextEditType type)
> 
> The “inline” here is not needed and has no effect.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:322
> > +            return AXObjectCache::AXTextDeleted;
> 
> The AXObjectCache:: prefix here is not needed since this is inside an
> AXObjectCache member function.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:327
> > +            return AXObjectCache::AXTextInserted;
> 
> Ditto.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:329
> > +        default:
> 
> The best practice is to leave the default case out of this switch statement
> so that the compiler will warn if we forget to include any of the enum
> cases. Since all the cases are returning, we can still ASSERT_NOT_REACHED
> for a bad value that happens to occur runtime; the code for that goes after
> the switch statement outside of it.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:331
> > +            return AXObjectCache::AXTextInserted;
> 
> Ditto.
> 
> > Source/WebCore/accessibility/AXObjectCache.h:446
> > +#if PLATFORM(COCOA)
> > +inline void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*, AXTextStateChangeIntent, const VisibleSelection&) { }
> > +inline void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*, AXTextEditType, const String&, const VisiblePosition&) { }
> > +inline void AXObjectCache::postTextReplacementPlatformNotification(AccessibilityObject*, AXTextEditType, const String&, AXTextEditType, const String&, const VisiblePosition&) { }
> > +#else
> >  inline void AXObjectCache::nodeTextChangePlatformNotification(AccessibilityObject*, AXTextChange, unsigned, const String&) { }
> > +#endif
> 
> It’s better to not mix the conditional code up with the unconditional.
> Please put this in a separate paragraph after the rest.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:555
> > +AccessibilityObject* AccessibilityNodeObject::passwordFieldOrContainingPasswordField() const
> 
> Why does this function need to be const?
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:567
> > +    if (Element* element = node->shadowHost()) {
> > +        if (is<HTMLInputElement>(*element)) {
> 
> Another way to write this without the nesting is:
> 
>     Element* element = node->shadowHost();
>     if (is<HTMLInputElement>(element)) {
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:568
> > +            if (AXObjectCache *cache = axObjectCache())
> 
> Incorrect formatting of the AXObjectCache* here.
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:2666
> > +    HTMLInputElement* inputElement = nullptr;
> > +    if (Element* element = node->shadowHost()) {
> > +        if (is<HTMLInputElement>(*element))
> > +            inputElement = downcast<HTMLInputElement>(element);
> > +    }
> > +    if (!inputElement)
> > +        return false;
> > +    
> > +    return inputElement->isPasswordField();
> 
> Here’s how I would write this:
> 
>     Element* element = node->shadowHost();
>     return is<HTMLInputElement>(element) &&
> downcast<HTMLInputElement>(*element).isPasswordField();
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1500
> > +    Position pos1 = Position(node, range.start, Position::PositionIsOffsetInAnchor);
> > +    Position pos2 = Position(node, range.start + range.length, Position::PositionIsOffsetInAnchor);
> 
> We try to avoid abbreviations and names such as "pos1" and "pos2". How about
> "start" and "end" or "startPosition" and "endPosition" or if you much prefer
> numbers, "position1" and "position2"?
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1501
> > +    VisibleSelection visibleSelection = VisibleSelection(pos1, pos2, DOWNSTREAM);
> 
> I would write:
> 
>     VisibleSelection newSelection(start, end, DOWNSTREAM);
> 
> I don’t think he word “visible” in the variable name is helpful, and I think
> it’s unnecessarily wordy to write:
> 
>     VisibleSelection x = VisibleSelection(...);
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:203
> > +void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject* obj, AXTextStateChangeIntent intent, const VisibleSelection& selection)
> 
> Please don’t name a local variable “obj”.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:236
> > +        case AXTextStateChangeTypeEdit:
> > +            break;
> > +        default:
> > +            break;
> 
> I don’t understand why you have a case here for both
> AXTextStateChangeTypeEdit and default. I suggest omitting the default if we
> are covering all the cases, or omitting AXTextStateChangeTypeEdit if we are
> not covering all the cases.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:250
> > +    // Used by DRT to know when notifications are posted.
> 
> These are not good comments. First of all, it’s not appropriate for us to
> put code into WebKit just for DumpRenderTree. Second of all, the tool won’t
> necessarily always be named DumpRenderTree, which is kind of a crazy name,
> and the abbreviation DRT will be really mysterious in the future. Finally,
> the comment doesn’t really explain this sufficiently. I’m assuming the issue
> is that it’s too hard for us to hook our testing tool into the real
> accessibility notification machinery so we made a second one just for
> testing. I’m not sure that strategy is a good one.
> 
> I suggest we make a helper function that does both
> NSAccessibilityPostNotificationWithUserInfo and calls the wrapper. That
> helper function could have a longer comment that explained more clearly what
> this is, without the DRT abbreviation. Generally speaking the comment should
> explain in a slightly more abstract way what this is for, rather than
> specifically saying there is exactly one client, DRT, using it. But also
> it’s critical not to repeat the code and the comment over and over again.
> 
> I’d go further and say that it would generally be good to factor this to
> share more of the code for clarity. There’s a lot of copied and pasted code
> here.

You're right about it not being ideal. We use WebKitTestRunner by default now, so the reference to DRT is out of date. I'll fix it for the new logic and file a bug for to make other instances consistent.

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:254
> > +    // Used by DRT to know when notifications are posted.
> 
> Please use shared function as mentioned above.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:260
> > +static NSDictionary *textReplacementChangeDictionary(AccessibilityObject* obj, AXObjectCache::AXTextEditType type, const String& string, const VisiblePosition& position)
> 
> Please don’t use the abbreviation "obj". I believe the WebKit coding style
> guidelines documents explains the rationale for this.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:268
> > +    static const NSUInteger truncationLength = 1000;
> 
> Part of the value of a constant in a case like this is to explain where the
> number came from. Please put in a why comment explaining the choice of 1000
> UTF-16 code units as a reasonable limit.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:273
> > +    if (length > 0)
> 
> WebKit coding style says to use "if (length)" here. But also, this function
> returned nil earlier if length was zero, so this if should be omitted.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:282
> > +void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject* obj, AXTextEditType type, const String& text, const VisiblePosition& position)
> 
> Please don’t use the abbreviation "obj". I believe the WebKit coding style
> guidelines documents explains the rationale for this.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:305
> > +    // Used by DRT to know when notifications are posted.
> 
> Please use shared function as mentioned above.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:309
> > +    // Used by DRT to know when notifications are posted.
> 
> Please use shared function as mentioned above.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:340
> > +    // Used by DRT to know when notifications are posted.
> 
> Please use shared function as mentioned above.
> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:344
> > +    // Used by DRT to know when notifications are posted.
> 
> Please use shared function as mentioned above.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:398
> > +    for (NSUInteger idx = 0; idx < [mutableArray count]; idx++) {
> 
> Please don’t use the abbreviation "idx". If you don’t want to use the
> customary “i”, which we use almost everywhere in WebKit code, then please
> use a word “index”. The in-between “idx” isn’t right for our coding style.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:405
> > +            [mutableArray removeObjectAtIndex:idx];
> 
> This won’t work. We will end up skipping one of the element in the array,
> because we’ll remove the object at this index, but we will still move on to
> the next index. So if index 0 is neither a string nor a number, we’ll next
> move on to the object that was originally at index 2 and never process the
> object that was originally at index 1.

Derp. Will make the increment conditional.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:417
> > +            mutableDictionary[key] = dictionaryRemovingNonJSONTypes(value);
> 
> I don’t think NSDictionary allows modification of a dictionary while it is
> being enumerated, even if it’s just the value being modified.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:419
> > +            mutableDictionary[key] = arrayRemovingNonJSONTypes(value);
> 
> I don’t think NSDictionary allows modification of a dictionary while it is
> being enumerated, even if it’s just the value being modified.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:421
> > +            [mutableDictionary removeObjectForKey:key];
> 
> NSDictionary definitely doesn’t support "for in" iteration while the
> dictionary is being enumerated, so this code won’t work. How did you test it?

I'm enumerating the immutable dictionary and mutating the mutable dictionary. I tested it with the attached layout tests and manual testing with no issues.

> 
> > Source/WebCore/editing/AppendNodeCommand.cpp:59
> > +    const String text = node->nodeValue();
> 
> The const here doesn’t add much. Please don’t declare specific local
> variables const unless there is a good reason to single them out. Almost all
> local variables could be marked "const".
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:171
> > +    if (m_commands.size())
> > +        return m_commands[m_commands.size()-1]->unapplyIntent();
> 
> WebKit coding style requires spaces around the "-" operator. But also,
> here’s how you’d write this:
> 
>     if (!m_commands.isEmpty())
>         return m_commands.last()->unapplyIntent();
> 
> > Source/WebCore/editing/CompositeEditCommand.h:110
> > +    virtual Ref<EditCommand> deleteFromTextNodeCommand(PassRefPtr<Text>, unsigned, unsigned);
> 
> Why is this function virtual? The two arguments need names. The type
> “unsigned” alone is not good.
> 
> Argument types for new functions should not be PassRefPtr even though legacy
> ones use that type. Instead if we are handing off ownership they should be
> RefPtr&& or maybe even Ref&& if they must be non-null. If we are not handing
> off ownership they should be Node* or, even better, Node&.
> 
> insertNodeBeforeCommand is not a good name for a function that creates a
> command. It should have the word “create” in its name. When we name a
> function with just a noun, then it implies something different -- returning
> something owned by this object -- not creating something.
> 
> > Source/WebCore/editing/CompositeEditCommand.h:117
> > +    virtual Ref<EditCommand> insertNodeBeforeCommand(PassRefPtr<Node>, PassRefPtr<Node>, ShouldAssumeContentIsAlwaysEditable);
> 
> Same comments.
> 
> > Source/WebCore/editing/CompositeEditCommand.h:121
> > +    virtual Ref<EditCommand> insertTextIntoNodeCommand(PassRefPtr<Text>, unsigned, const String&);
> 
> Same comments.
> 
> > Source/WebCore/editing/CompositeEditCommand.h:140
> > +    virtual Ref<DeleteFromTextNodeCommand> replaceDeleteFromTextNodeCommand(PassRefPtr<Text>, unsigned, unsigned);
> 
> Same comments.
> 
> > Source/WebCore/editing/CompositeEditCommand.h:141
> > +    virtual Ref<EditCommand> replaceInsertIntoTextNodeCommand(PassRefPtr<Text>, unsigned, const String&, const String&);
> 
> Same comments.
Comment 87 Doug Russell 2015-04-20 13:59:26 PDT
Created attachment 251187 [details]
Update based on review

Fixed errors based on review.
Removed several EditCommand subclasses.
Added storage for m_applyEditType to EditCommand, made unapplyEditType a calculated property, got rid of applyIntent() and unapplyIntent() in favor of building the intent where it's handed off to selection.
Comment 88 Doug Russell 2015-04-20 14:28:46 PDT
Created attachment 251190 [details]
Update based on review (fixed iOS/GTK build)

Fixed errors based on review.
Removed several EditCommand subclasses.
Added storage for m_applyEditType to EditCommand, made unapplyEditType a calculated property, got rid of applyIntent() and unapplyIntent() in favor of building the intent where it's handed off to selection.
Comment 89 Doug Russell 2015-04-20 14:39:05 PDT
Created attachment 251191 [details]
Update based on review (fixed iOS/GTK build take 2)

Fixed errors based on review.
Removed several EditCommand subclasses.
Added storage for m_applyEditType to EditCommand, made unapplyEditType a calculated property, got rid of applyIntent() and unapplyIntent() in favor of building the intent where it's handed off to selection.
Comment 90 Doug Russell 2015-04-20 15:18:31 PDT
Created attachment 251192 [details]
Update based on review (fixed iOS/GTK build take 3)

Fixed errors based on review.
Removed several EditCommand subclasses.
Added storage for m_applyEditType to EditCommand, made unapplyEditType a calculated property, got rid of applyIntent() and unapplyIntent() in favor of building the intent where it's handed off to selection.
Comment 91 Doug Russell 2015-04-20 16:26:32 PDT
Created attachment 251203 [details]
Update from review

Remove a redundant length check missed in earlier updates from review
Comment 92 Darin Adler 2015-04-21 09:16:56 PDT
(In reply to comment #86)
> > NSDictionary definitely doesn’t support "for in" iteration while the
> > dictionary is being enumerated, so this code won’t work. How did you test it?
> 
> I'm enumerating the immutable dictionary and mutating the mutable
> dictionary. I tested it with the attached layout tests and manual testing
> with no issues.

This points out a major problem. We have code to remove objects of the wrong types from these arrays and dictionaries, but we didn’t create any test cases that exercise that code. That must be fixed. We need to figure out how to test that code. We must not just check it in without ever trying it.
Comment 93 Darin Adler 2015-04-21 09:39:27 PDT
Comment on attachment 251203 [details]
Update from review

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

I started reviewing but I didn’t get to the end yet.

Generally this patch is messing up the editing machinery with way too much explicit accessibility-specific code and should be done more elegantly if possible.

> Source/WebCore/ChangeLog:210
> +2015-04-17  Doug Russell  <d_russell@apple.com>
> +
> +        AX: richer text change notifications
> +        https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> +        Richer accessibility selection change notifications. Introduce AXTextStateChangeIntent, and an overload of postTextReplacementNotification to give assistive tech apps more reliable context for responding to changes in web content selection. Also block posting selection changes on password fields.
> +
> +2015-04-14  Doug Russell  <d_russell@apple.com>
> +
> +        AX: richer text change notifications
> +        https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> +        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.
> +
> +2015-04-14  Doug Russell  <d_russell@apple.com>
> +
> +        AX: richer text change notifications
> +        https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> +        New EditCommand subclasses to give the context that accessibility needs to post more detailed value and selection change notifications. This commit is all infrastructure and doesn't contain any accessibility specific changes.
> +

Need to remove these extra change log entries.

> Source/WebCore/accessibility/AXObjectCache.cpp:1319
> +    if (!rootObject)
> +        return nullptr;
> +    if (rootObject->isAccessibilityScrollView())
> +        return downcast<AccessibilityScrollView>(*rootObject).webAreaObject();
> +    return nullptr;

Slightly nicer to stick 100% with the early return style so the real code is on the left, not nested. I’d write it like this:

    if (!rootObject || !rootObject->isAccessibiltyScrollView())
        return nullptr;
    return downcast<AccessibilityScrollView>(*rootObject).webAreaObject();

> Source/WebCore/accessibility/AXObjectCache.h:199
> +    // Simple text value change

I don’t think this comment adds much.

> Source/WebCore/accessibility/AXObjectCache.h:201
> +    // Compound text value change

I don’t think this comment adds much.

> Source/WebCore/accessibility/AXObjectCache.h:203
> +    // Selection change

I don’t think this comment adds much.

> Source/WebCore/accessibility/AXObjectCache.h:224
> +#if PLATFORM(COCOA) && !PLATFORM(IOS)

Should instead be:

    #if PLATFORM(MAC)

> Source/WebCore/accessibility/AXObjectCache.h:257
> +    AXTextChange textChangeForEditType(AXTextEditType type)
> +    {
> +        switch (type) {
> +        case AXTextEditTypeCut:
> +        case AXTextEditTypeDelete:
> +            return AXTextDeleted;
> +        case AXTextEditTypeInsert:
> +        case AXTextEditTypeDictation:
> +        case AXTextEditTypeTyping:
> +        case AXTextEditTypePaste:
> +            return AXTextInserted;
> +        default:
> +            break;
> +        }
> +        ASSERT_NOT_REACHED();
> +        return AXTextInserted;
> +    }

Why does this function have to be here in the header? I suggest declaring the function here, but defining it in the cpp file. Or we could define it later in this file. Putting the entire definition in the class makes the class unnecessarily hard to read. Also, this should be a static member function, not a member function. The "default: break;” code should be omitted. Including a default case causes the compiler to turn off its checking that the switch statement covers all enum values.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:563
> +            return const_cast<AccessibilityNodeObject*>(this);

No need for this const_cast.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:567
> +    if (!is<HTMLInputElement>(*element))

Need to remove the * here, in case node->shadowHost() is null. The is function will handle null, but only if passed a pointer rather than a reference.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:571
> +    if (AXObjectCache* cache = axObjectCache())
> +        return cache->getOrCreate(element);

Missing a null check for element here, I think.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:92
> +static NSUInteger const NSAccessibilityValueChangeTruncationLength = 1000;

We normally put the "const" before the type in declarations like this one.

It’s not appropriate to define something here with an NS prefix; that’s reserved for AppKit, and WebKit shouldn’t use that prefix for its own internal constants.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:111
> +void AXObjectCache::setShouldRepostNotificationsForTests(bool value)

Missing blank lines between functions here.

> Source/WebCore/editing/CompositeEditCommand.cpp:332
> +static void setCommandApplyEditTypeForEditingAction(SimpleEditCommand *command, EditAction editingAction)

Incorrect formatting of SimpleEditCommand* here.

> Source/WebCore/editing/CutDeleteSelectionCommand.h:43
> +    virtual EditAction editingAction() const override;

Do we really need an entire new DeleteSelectionCommand subclass just to customize the editing action? Instead we should just have a new create function in DeleteSelectionCommand and have it store a flag to record the fact that it’s for a cut.

> Source/WebCore/editing/DeleteFromTextNodeCommand.h:29
> +#include "AXObjectCache.h"

Doesn’t make sense to add this include. Why is it needed?

> Source/WebCore/editing/DeleteFromTextNodeCommand.h:43
> +    String& deletedText();

This seems wrong. We don’t want to expose the deleted text as a non-const reference. We don’t want to allow someone to change this underneath us.

> Source/WebCore/editing/DeleteFromTextNodeCommand.h:49
> +

Please don’t add this blank line here.

> Source/WebCore/editing/DictationInsertTextCommand.h:43
> +    virtual EditAction editingAction() const override;

Same comment. Don’t create an entire new derived class just to customize the editing action. Just add a new create function and a data member instead.

> Source/WebCore/editing/EditCommand.h:29
> +#include "AXObjectCache.h"

Can we instead include only the header that defines the types, not the entire AXObjectCache.h header?

> Source/WebCore/editing/Editor.cpp:386
> +    if (editingAction == EditActionCut)
> +        applyCommand(CutDeleteSelectionCommand::create(document(), smartDelete));
> +    else
> +        applyCommand(DeleteSelectionCommand::create(document(), smartDelete));

Clearly we should just pass in the editingAction here instead of having two completely separate classes!

> Source/WebCore/editing/Editor.cpp:1008
> +    changeSelectionAfterCommand(newSelection, options, AXTextStateChangeIntent(cmd->applyEditType()));

Should just be able to pass cmd->applyEditType() here. Should not need an explicit conversion to AXTextStateChangeIntent, because the constructor is not marked explicit.

> Source/WebCore/editing/Editor.cpp:1039
> +    changeSelectionAfterCommand(newSelection, FrameSelection::defaultSetSelectionOptions(), AXTextStateChangeIntent(cmd->unapplyEditType()));

Should just be able to pass cmd->applyEditType() here. Should not need an explicit conversion to AXTextStateChangeIntent, because the constructor is not marked explicit.

> Source/WebCore/editing/Editor.h:177
> +    WEBCORE_EXPORT void deleteSelectionWithSmartDelete(bool smartDelete, EditAction editingAction = EditActionDelete);

Should omit the argument name editingAction here.

> Source/WebCore/editing/EditorCommand.cpp:196
> +    command->setApplyEditType(AXTextEditTypeInsert);

Should add this as a constructor argument, not require a separate set function after the fact for this.

> Source/WebCore/editing/FrameSelection.cpp:261
> +    if (AXObjectCache::accessibilityEnabled()) {
> +        if (newSelection.isCaret())

Should just use && here.

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

This code isn’t needed. The variable intent has already been initialized and there’s no need to set it again.

> Source/WebCore/editing/FrameSelection.cpp:1046
> +        if (isRange()) {
> +            if (directionOfSelection() == RTL)
> +                flip = true;
> +        }

Reads better as:

    flip = isRange() && directionOfSelection() == RTL;

> Source/WebCore/editing/FrameSelection.h:136
> +    static inline AXTextStateChangeIntent defaultSetSelectionIntent()

The inline keyword here is not needed and not helpful. Also seems overkill to write a function for this since the default is the default value for the class.

> Source/WebCore/editing/InsertIntoTextNodeCommand.h:29
> +#include "AXObjectCache.h"

Should not add this include.

> Source/WebCore/editing/InsertIntoTextNodeCommand.h:43
> +    String& insertedText();

Same problem as above. No reason to expose a non-const reference to internal state.
Comment 94 Doug Russell 2015-04-23 13:17:11 PDT
Created attachment 251475 [details]
Update from review

* General cleanup based on review
* Removed unneeded EditCommand subclasses
* Made editingAction() a constructor argument on EditCommand
* Switched applyEditType and unapplyEditType to calculated properties based on editingAction()
* Updated DumpRenderTree and WebKitTestRunner to explicitly remove observers instead of relying on dealloc to do it (fixes assertions that fail in debug builds)
Comment 95 Doug Russell 2015-04-23 13:56:21 PDT
Created attachment 251485 [details]
Update from review

Fix ATK build
Comment 96 WebKit Commit Bot 2015-04-23 13:59:30 PDT
Attachment 251485 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AXObjectCache.h:360:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 97 Doug Russell 2015-04-23 14:05:38 PDT
Created attachment 251487 [details]
Update from review
Comment 98 Doug Russell 2015-04-23 14:09:06 PDT
Created attachment 251488 [details]
Update from review
Comment 99 Doug Russell 2015-04-23 14:30:57 PDT
Created attachment 251491 [details]
Update from review
Comment 100 Doug Russell 2015-04-23 14:49:44 PDT
Created attachment 251495 [details]
Update from review
Comment 101 Doug Russell 2015-04-23 15:59:45 PDT
Created attachment 251507 [details]
Update from review
Comment 102 Darin Adler 2015-04-23 18:08:41 PDT
Comment on attachment 251507 [details]
Update from review

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

> Source/WebCore/ChangeLog:2238
> +2015-04-14  Doug Russell  <d_russell@apple.com>
> +
> +        AX: richer text change notifications
> +        https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> +        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.
> +
> +2015-04-14  Doug Russell  <d_russell@apple.com>
> +
> +        AX: richer text change notifications
> +        https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> +        New EditCommand subclasses to give the context that accessibility needs to post more detailed value and selection change notifications. This commit is all infrastructure and doesn't contain any accessibility specific changes.
> +>>>>>>> <rdar://problem/20169058> AX: richer text change notifications (142719)
> +

Extra change log entry here due to merging problems I assume.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:339
> +    NSAccessibilityPostNotificationWithUserInfo(webArea->wrapper(), NSAccessibilityValueChangedNotification, userInfo);
> +    // Used by DRT to know when notifications are posted.
> +    [webArea->wrapper() accessibilityPostedNotification:NSAccessibilityValueChangedNotification userInfo:userInfo];

Use AXPostNotificationWithUserInfo.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:343
> +    NSAccessibilityPostNotificationWithUserInfo(object->wrapper(), NSAccessibilityValueChangedNotification, userInfo);
> +    // Used by DRT to know when notifications are posted.
> +    [object->wrapper() accessibilityPostedNotification:NSAccessibilityValueChangedNotification userInfo:userInfo];

Use AXPostNotificationWithUserInfo.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:386
> +#if PLATFORM(COCOA) && !PLATFORM(IOS)

#if PLATFORM(MAC)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:400
> +    NSMutableArray *mutableArray = [array mutableCopy];

A shame that this function makes a mutable copy of the array in the almost 100% common case where it does contain only JSON types.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:419
> +    NSMutableDictionary *mutableDictionary = [dictionary mutableCopy];

A shame that this function makes a mutable copy of the dictionary in the almost 100% common case where it does contain only JSON types.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.h:36
> +- (id)textMarkerRangeFromVisiblePositions:(WebCore::VisiblePosition)startPosition endPosition:(WebCore::VisiblePosition)endPosition;

Shouldn’t the arguments be const VisiblePosition& like in the method just below?

> Source/WebCore/editing/CompositeEditCommand.cpp:179
> +    default:
> +        break;

Normally we omit the default so we get a warning when we add a new enum value and don’t yet handle it in this switch statement.

> Source/WebCore/editing/CompositeEditCommand.h:36
> +class DeleteFromTextNodeCommand;

Why is this needed? I don’t see it used below.

> Source/WebCore/editing/CompositeEditCommand.h:63
> +    AXTextEditType unapplyEditType();

Should be a const member function.

> Source/WebCore/editing/DeleteFromTextNodeCommand.cpp:53
> +const String& DeleteFromTextNodeCommand::deletedText()
> +{
> +    return m_text;
> +}

Should probably be inlined in the header. Just add "inline" and put it at the bottom of the header.

> Source/WebCore/editing/EditCommand.cpp:133
> +    default:
> +        break;

Same comment about default as above.

> Source/WebCore/editing/FrameSelection.cpp:1035
> +AXTextStateChangeIntent inline FrameSelection::TextSelectionIntent(EAlteration alter, SelectionDirection direction, TextGranularity granularity)

Why inline? That doesn’t seem like a good idea. Also, we normally put inline to the left of the type.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56
> +const String& InsertIntoTextNodeCommand::insertedText()
> +{
> +    return m_text;
> +}

Inline in header?

> Source/WebCore/editing/InsertIntoTextNodeCommand.h:44
> +    const String& insertedText();
> +protected:
> +    InsertIntoTextNodeCommand(PassRefPtr<Text> node, unsigned offset, const String& text, EditAction editingAction);

Missing blank line before protected. Also, should just make this private again. I believe the reason to make it protected is obsolete.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:33
> +class Text;

Not needed.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:35
> +class ReplaceDeleteFromTextNodeCommand : public DeleteFromTextNodeCommand {

Should mark this class final.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:37
> +    static Ref<ReplaceDeleteFromTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, unsigned count)

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:43
> +    ReplaceDeleteFromTextNodeCommand(PassRefPtr<Text>, unsigned, unsigned);

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

This should be private, not protected.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:33
> +class Text;

Not needed.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:35
> +class ReplaceInsertIntoTextNodeCommand : public InsertIntoTextNodeCommand {

Should mark this class final.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:37
> +    static Ref<ReplaceInsertIntoTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, const String& text, const String& deletedText, EditAction editingAction)

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:43
> +    ReplaceInsertIntoTextNodeCommand(PassRefPtr<Text>, unsigned, const String&, const String&, EditAction);

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

This should be private, not protected.

> Source/WebCore/editing/ReplaceSelectionCommand.h:124
> +    AXTextEditType m_applyEditType;

This looks uninitialized and unused. Please omit it.
Comment 103 Doug Russell 2015-04-23 19:16:26 PDT
(In reply to comment #102)
> Comment on attachment 251507 [details]
> Update from review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251507&action=review
> 
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:400
> > +    NSMutableArray *mutableArray = [array mutableCopy];
> 
> A shame that this function makes a mutable copy of the array in the almost
> 100% common case where it does contain only JSON types.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:419
> > +    NSMutableDictionary *mutableDictionary = [dictionary mutableCopy];
> 
> A shame that this function makes a mutable copy of the dictionary in the
> almost 100% common case where it does contain only JSON types.
> 

Currrently the only methods that pass a non nil userInfo always include non JSON types. We could definitely make the copy conditional if that changes.

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:179
> > +    default:
> > +        break;
> 
> Normally we omit the default so we get a warning when we add a new enum
> value and don’t yet handle it in this switch statement.
> 
> > Source/WebCore/editing/EditCommand.cpp:133
> > +    default:
> > +        break;
> 
> Same comment about default as above.

There are a lot of EditActions that aren't accessibility relevant that the default covers.

> > Source/WebCore/editing/InsertIntoTextNodeCommand.h:44
> > +    const String& insertedText();
> > +protected:
> > +    InsertIntoTextNodeCommand(PassRefPtr<Text> node, unsigned offset, const String& text, EditAction editingAction);
> 
> Missing blank line before protected. Also, should just make this private
> again. I believe the reason to make it protected is obsolete.

This needs to be protected for ReplaceInsertIntoTextNodeCommand
Comment 104 Doug Russell 2015-04-23 19:26:30 PDT
> > Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:37
> > +    static Ref<ReplaceInsertIntoTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, const String& text, const String& deletedText, EditAction editingAction)
> 
> New code should not use PassRefPtr. See
> <https://www.webkit.org/coding/RefPtr.html>.
> 
> > Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:43
> > +    ReplaceInsertIntoTextNodeCommand(PassRefPtr<Text>, unsigned, const String&, const String&, EditAction);
> 
> New code should not use PassRefPtr. See
> <https://www.webkit.org/coding/RefPtr.html>.

Switching these methods to RefPtr results in style errors:
ERROR: Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.cpp:33:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]

What's the appropriate type here?
Comment 105 Doug Russell 2015-04-23 19:31:29 PDT
(In reply to comment #104)
> > > Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:37
> > > +    static Ref<ReplaceInsertIntoTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, const String& text, const String& deletedText, EditAction editingAction)
> > 
> > New code should not use PassRefPtr. See
> > <https://www.webkit.org/coding/RefPtr.html>.
> > 
> > > Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:43
> > > +    ReplaceInsertIntoTextNodeCommand(PassRefPtr<Text>, unsigned, const String&, const String&, EditAction);
> > 
> > New code should not use PassRefPtr. See
> > <https://www.webkit.org/coding/RefPtr.html>.
> 
> Switching these methods to RefPtr results in style errors:
> ERROR: Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.cpp:33:  The
> parameter type should use PassRefPtr instead of RefPtr. 
> [readability/pass_ptr] [5]
> 
> What's the appropriate type here?

Nevermind, didn't have a &&

> Function arguments
>  If a function does not take ownership of an object, the argument should be a raw reference or raw pointer.
>  If a function does take ownership of an object, the argument should be a Ref&& or a RefPtr&&. This includes many setter functions.
Comment 106 Doug Russell 2015-04-23 21:56:16 PDT
Created attachment 251536 [details]
Update from review

* General cleanup based on review
* PassRefPtr to RefPtr&&
Comment 107 Darin Adler 2015-04-23 22:51:15 PDT
Windows build is failing with this error:

unresolved external symbol "protected: static enum WebCore::AXTextChange __cdecl WebCore::AXObjectCache::textChangeForEditType(enum WebCore::AXTextEditType)"

That’s because we didn’t add the function textChangeForEditType to AXObjectCacheWin.cpp.
Comment 108 Doug Russell 2015-04-23 22:58:24 PDT
Created attachment 251541 [details]
Fix windows builds
Comment 109 Darin Adler 2015-04-24 09:03:50 PDT
Comment on attachment 251541 [details]
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.
Comment 110 Brent Fulgham 2015-04-24 09:23:45 PDT
Comment on attachment 251541 [details]
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& ?
Comment 111 WebKit Commit Bot 2015-04-24 09:50:59 PDT
Comment on attachment 251541 [details]
Fix windows builds

Clearing flags on attachment: 251541

Committed r183266: <http://trac.webkit.org/changeset/183266>
Comment 112 WebKit Commit Bot 2015-04-24 09:51:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 113 Alexey Proskuryakov 2015-04-24 12:29:17 PDT
This patch broke platform/mac/accessibility/select-text.html, which is now flaky on Mac Debug WebKit1:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=platform%2Fmac%2Faccessibility%2Fselect-text.html

Doug, do you know how to fix this right now, or should I roll out?
Comment 114 Doug Russell 2015-04-24 12:36:12 PDT
Looking into it
Comment 115 mitz 2015-04-24 14:52:24 PDT
Comment on attachment 251541 [details]
Fix windows builds

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:228
> +        userInfo[NSAccessibilityTextStateSyncKey] = @YES;

Dictionary subscripting requires the modern Objective-C runtime. This broke the 32-bit build.
Comment 116 Alexey Proskuryakov 2015-04-24 14:54:08 PDT
Was just about to post the same comment. Since the test is also still broken, I think that we need to roll out for now, sorry.
Comment 117 WebKit Commit Bot 2015-04-24 14:56:47 PDT
Re-opened since this is blocked by bug 144164
Comment 118 Doug Russell 2015-04-24 15:11:25 PDT
I believe I've used subscripting in previous bugs without issue. What are the constraints for when I can and can't use subscripts/object literals?
Comment 119 Andy Estes 2015-04-24 15:52:26 PDT
(In reply to comment #118)
> I believe I've used subscripting in previous bugs without issue. What are
> the constraints for when I can and can't use subscripts/object literals?

http://clang.llvm.org/docs/ObjectiveCLiterals.html#id4 tells you:

"An Objective-C subscript expression occurs when the base operand of the C subscript operator has an Objective-C object pointer type. Since this potentially collides with pointer arithmetic on the value, these expressions are only supported under the modern Objective-C runtime, which categorically forbids such arithmetic."

As Alexey pointed out, WebKit still supports the legacy runtime for 32-bit Mac applications. Any code that compiles in that configuration cannot use modern runtime features.
Comment 120 Doug Russell 2015-04-24 16:10:09 PDT
I have a patch to remove subscripting/object literals, but I haven't been able to repro the test timing out.

The build log from the dashboard lists the machine failing as a Yosemite x64 WK1 debug configuration.

Building on a Retina iMac running 10.10.3 14D136 with

./Tools/Scripts/build-webkit --debug

and running the test with

./Tools/Scripts/run-webkit-tests --dump-render-tree --debug

I don't get this timeout
Comment 121 Alexey Proskuryakov 2015-04-24 16:45:42 PDT
Sadly, it may be difficult to investigate if the failure does not reproduce locally.

FWIW, it happened on both Yosemite and Mavericks bots, according to the dashboard.
Comment 122 Doug Russell 2015-04-24 19:32:38 PDT
Created attachment 251604 [details]
Remove subscripting/object literals

For 32 bit builds
Comment 123 WebKit Commit Bot 2015-04-24 19:36:01 PDT
Attachment 251604 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:15:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/ChangeLog:142:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:428:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 4 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 124 Doug Russell 2015-04-24 19:55:44 PDT
Created attachment 251605 [details]
Fix changelogs
Comment 125 Doug Russell 2015-04-25 17:56:36 PDT
Created attachment 251657 [details]
patch

General cleanup
Change m_passwordNotificationsToPost to a set
Change from AXStateChangeIntent to const AXStateChangeIntent&
Comment 126 Doug Russell 2015-04-25 18:06:22 PDT
Created attachment 251658 [details]
patch

Fix GTK build
Comment 127 Darin Adler 2015-04-25 18:31:36 PDT
Comment on attachment 251658 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1079
> +    if (m_passwordNotificationsToPost.find(observableObject) == m_passwordNotificationsToPost.end()) {
> +        m_passwordNotificationsToPost.insert(observableObject);

This is not good, it’s a double lookup, once for find and then again for insert.

> Source/WebCore/accessibility/AXObjectCache.h:270
> +    template <class T = void> struct RefPtrCompare : std::binary_function<RefPtr<T>, RefPtr<T>, bool> {
> +        bool operator()(const RefPtr<T>& x, const RefPtr<T>& y) const { return x.get() < y.get(); }
> +    };

No, please don’t add adapters to use std::set. Use the set data structure from WTF: HashSet or perhaps for your use ListHashSet.

> Source/WebCore/accessibility/AXObjectCache.h:288
> +    std::set<RefPtr<AccessibilityObject>, RefPtrCompare<AccessibilityObject>> m_passwordNotificationsToPost;

Please don’t use std::set in WebKit. You need to use one of the WebKit collections. Given that we probably want these notifications to have a stable order, you should be using ListHashSet.
Comment 128 Doug Russell 2015-04-25 18:58:04 PDT
Created attachment 251663 [details]
patch

std:set -> ListHashSet
Fix windows build
Comment 129 Doug Russell 2015-04-25 20:01:05 PDT
Created attachment 251666 [details]
patch

After discussing with Alexey, marking select-text.html as flakey.
Comment 130 Darin Adler 2015-04-26 09:10:07 PDT
Comment on attachment 251666 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:567
> +    if (!element || !is<HTMLInputElement>(element))

Null check here is redundant; is<HTMLInputElement>(element) includes a null check.
Comment 131 WebKit Commit Bot 2015-04-26 15:18:26 PDT
Comment on attachment 251666 [details]
patch

Clearing flags on attachment: 251666

Committed r183368: <http://trac.webkit.org/changeset/183368>
Comment 132 WebKit Commit Bot 2015-04-26 15:18:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 133 Alexey Proskuryakov 2015-04-27 14:00:37 PDT
There are still bad array subscripts (and I don't know when it's allowed and when not; this only happens on some bots). Maybe Xcode version dependent.

/Volumes/Data/slave/faraday-mavericks-production-archive/build/build-Production/WebCore.roots/Sources/WebCore/accessibility/mac/AXObjectCacheMac.mm:304:13: error: array subscript is not an integer
/Volumes/Data/slave/faraday-mavericks-production-archive/build/build-Production/WebCore.roots/Sources/WebCore/accessibility/mac/AXObjectCacheMac.mm:316:17: error: array subscript is not an integer

platform/mac/accessibility/select-text.html is failing nearly every time on bots, what were you planning to do to debug that?
Comment 134 Alexey Proskuryakov 2015-04-27 14:04:03 PDT
Landed a build fix in r183420.
Comment 135 Alexey Proskuryakov 2015-04-27 16:16:55 PDT
There is some strange output in system log implying that DumpRenderTree exits normally during this test. Even though PIDs don't match, this makes me curious about what's going on, so I'm turning an unexpected exit() into a crash in bug 144288.

There isn't much explanation for the regression, or even for pre-existing flaky slowness of the test.
Comment 136 Alexey Proskuryakov 2015-04-27 16:19:54 PDT
More build fix in r183434.
Comment 137 Alexey Proskuryakov 2015-04-27 17:24:29 PDT
And more in r183439.
Comment 138 Alexey Proskuryakov 2015-04-28 11:51:49 PDT
Splitting the test into smaller parts in bug 144301 appears to have addressed this. I'd love to know the root cause, but we can't get down to it this time I guess.
Comment 139 Martin Robinson 2015-04-30 10:45:59 PDT
I think this change also caused every test on the GTK+ debug bot to fail assertions. Here's a stack trace: 

STDERR: ASSERTION FAILED: !needsStyleRecalc() || !document().childNeedsStyleRecalc()
STDERR: ../../Source/WebCore/dom/Element.cpp(483) : virtual bool WebCore::Element::isFocusable() const
STDERR: 1   0x7f7200253b8e /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7f7200253b8e]
STDERR: 2   0x7f7205e48abd /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Element::isFocusable() const+0xbf) [0x7f7205e48abd]
STDERR: 3   0x7f7205a97754 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::AccessibilityNodeObject::determineAccessibilityRole()+0x456) [0x7f7205a97754]
STDERR: 4   0x7f7205a96abf /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::AccessibilityNodeObject::init()+0x5d) [0x7f7205a96abf]
STDERR: 5   0x7f7205a73b50 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::AXObjectCache::getOrCreate(WebCore::Node*)+0x2b4) [0x7f7205a73b50]
STDERR: 6   0x7f7206c26681 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::AXObjectCache::platformHandleFocusedUIElementChanged(WebCore::Node*, WebCore::Node*)+0xb7) [0x7f7206c26681]
STDERR: 7   0x7f7205a756c6 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::AXObjectCache::handleFocusedUIElementChanged(WebCore::Node*, WebCore::Node*)+0x3e) [0x7f7205a756c6]
STDERR: 8   0x7f7205deb6cc /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Document::setFocusedElement(WTF::PassRefPtr<WebCore::Element>, WebCore::FocusDirection)+0x80e) [0x7f7205deb6cc]
STDERR: 9   0x7f7206431844 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::FocusController::setFocusedElement(WebCore::Element*, WTF::PassRefPtr<WebCore::Frame>, WebCore::FocusDirection)+0x43e) [0x7f7206431844]
STDERR: 10  0x7f7205e4fa9d /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::Element::focus(bool, WebCore::FocusDirection)+0x14f) [0x7f7205e4fa9d]
STDERR: 11  0x7f7206e7c3d7 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebCore::jsElementPrototypeFunctionFocus(JSC::ExecState*)+0x158) [0x7f7206e7c3d7]
STDERR: 12  0x7f71a9fff0a8 [0x7f71a9fff0a8]

Any hints on how to fix this would be much appreciated!
Comment 140 Darin Adler 2015-04-30 14:14:47 PDT
Doug, are you going to take care of that?

Martin, I am not sure whether it’s OK that AXObjectCache::getOrCreate requires that style is already up to date. We might add code inside that function to do that work.
Comment 141 Joanmarie Diggs 2015-04-30 14:24:53 PDT
(In reply to comment #140)
> Doug, are you going to take care of that?

I'm taking a stab at it in bug 144447. Though I'm happy to leave what I don't get do to Doug. ;)
Comment 142 Joanmarie Diggs 2015-04-30 18:03:28 PDT
(In reply to comment #141)
> (In reply to comment #140)
> > Doug, are you going to take care of that?
> 
> I'm taking a stab at it in bug 144447.

Editing tests should be crash-free pending review and commit.

As for the crash Martin reported, I opened bug 144481 and will dig into that next.

@Doug: I don't yet know if my platform has an drag-related crashes. But it's within the realm of reasonable possibility. Since you said you "just haven't implemented it yet," I'll happily leave its implementation to you. If you open a bug for it, please CC me. Ditto on CreateLink and Unlink. (Happy to leave it to you, please CC me.)

With any luck, that's the last of the regressions. <fingers crossed>