Description
Doug Russell
2015-03-15 21:34:16 PDT
(This is an extension of the work done in https://bugs.webkit.org/show_bug.cgi?id=25898) 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
Created attachment 250042 [details]
value change notifications
Created attachment 250043 [details]
selection change notifications
Created attachment 250044 [details]
tests
Created attachment 250045 [details]
debugging
Created attachment 250048 [details]
selection change notifications
Fix method missing from previous patch
Created attachment 250136 [details]
value change notifications
Finish some todos and other cleanup
Created attachment 250137 [details]
selection change notifications
Finish some todos and other cleanup
Created attachment 250233 [details]
value change notifications
Created attachment 250234 [details]
selection change notifications
Created attachment 250235 [details]
debugging
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.
Created attachment 250236 [details]
value change notifications
Created attachment 250237 [details]
selection change notifications
Created attachment 250241 [details]
Edit commands infrastructure
added change logs
Created attachment 250242 [details]
value change notifications
add change logs
Created attachment 250243 [details]
selection change notifications
add change logs
Created attachment 250244 [details]
tests
adding change logs
Created attachment 250245 [details]
debugging
adding change logs
Created attachment 250246 [details]
Full patch
Combined patch for EWS analysis
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.
Created attachment 250289 [details]
Full Patch
Patch for EWS analysis updated to fix failing builds on non mac platforms
Created attachment 250290 [details]
value change notifications
update for builds failing on non mac platforms
Created attachment 250291 [details]
selection change notifications
update for builds failing on non mac platforms
Created attachment 250292 [details]
Full Patch
Created attachment 250294 [details]
Full Patch
Created attachment 250296 [details]
Full Patch
Created attachment 250297 [details]
Full Patch
Created attachment 250298 [details]
Full Patch
Created attachment 250300 [details]
Full Patch
Created attachment 250304 [details]
Full Patch
Created attachment 250307 [details]
Full Patch
Created attachment 250309 [details]
Full Patch
Created attachment 250314 [details]
Full Patch
Created attachment 250320 [details]
Full Patch
Created attachment 250322 [details]
Full Patch
Created attachment 250325 [details]
Full Patch
Created attachment 250329 [details]
Full Patch
Created attachment 250332 [details]
Full Patch
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 (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 Created attachment 250368 [details]
Full patch
Created attachment 250369 [details]
Edit commands infrastructure
Created attachment 250370 [details]
value Change Notifications
Created attachment 250371 [details]
selection change notifications
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.
Created attachment 250372 [details]
Full Patch
Created attachment 250373 [details]
value change notifications
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.
Created attachment 250377 [details]
Full Patch
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.
Created attachment 250378 [details]
Full Patch
Created attachment 250454 [details]
Full Patch
Created attachment 250456 [details]
Full Patch
Created attachment 250548 [details]
Full Patch
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.
Created attachment 250550 [details]
Full Patch
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.
Created attachment 250552 [details]
Full Patch
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.
Created attachment 250554 [details]
Full Patch
Created attachment 250721 [details]
Full Patch
Created attachment 250722 [details]
Edit commands infrastructure
Created attachment 250723 [details]
value change notifications
Created attachment 250724 [details]
selection change notifications
Created attachment 250725 [details]
tests
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 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
Created attachment 250735 [details]
Full Patch
Created attachment 250736 [details]
tests
Created attachment 250825 [details]
Full Patch
Created attachment 250854 [details]
value change notifications
Created attachment 250856 [details]
selection change notifications
Created attachment 250857 [details]
tests
Created attachment 250989 [details]
Full Patch
Fix delete and cut commands being swapped
Created attachment 250991 [details]
Full Patch
Fix delete and cut commands being swapped
Minor test updates
Created attachment 250992 [details]
Edit commands infrastructure
Remove method removed in subsequent patch
Created attachment 250993 [details]
value change notifications
Fix delete and cut commands being swapped
Created attachment 250994 [details]
tests
Created attachment 251051 [details]
selection change notifications
Switch showIntent() from printf() to dataLogF()
Created attachment 251052 [details]
tests
Add tests for value change notifications
Created attachment 251054 [details]
Full Patch
Switch showIntent() from printf() to dataLogF()
Add tests for value change notifications
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. (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. 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.
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.
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.
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.
Created attachment 251203 [details]
Update from review
Remove a redundant length check missed in earlier updates from review
(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 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. 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)
Created attachment 251485 [details]
Update from review
Fix ATK build
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.
Created attachment 251487 [details]
Update from review
Created attachment 251488 [details]
Update from review
Created attachment 251491 [details]
Update from review
Created attachment 251495 [details]
Update from review
Created attachment 251507 [details]
Update from review
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. (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 > > 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?
(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. Created attachment 251536 [details]
Update from review
* General cleanup based on review
* PassRefPtr to RefPtr&&
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. Created attachment 251541 [details]
Fix windows builds
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 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 on attachment 251541 [details] Fix windows builds Clearing flags on attachment: 251541 Committed r183266: <http://trac.webkit.org/changeset/183266> All reviewed patches have been landed. Closing bug. 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? Looking into it 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. 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. Re-opened since this is blocked by bug 144164 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? (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. 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 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. Created attachment 251604 [details]
Remove subscripting/object literals
For 32 bit builds
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.
Created attachment 251605 [details]
Fix changelogs
Created attachment 251657 [details]
patch
General cleanup
Change m_passwordNotificationsToPost to a set
Change from AXStateChangeIntent to const AXStateChangeIntent&
Created attachment 251658 [details]
patch
Fix GTK build
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. Created attachment 251663 [details]
patch
std:set -> ListHashSet
Fix windows build
Created attachment 251666 [details]
patch
After discussing with Alexey, marking select-text.html as flakey.
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 on attachment 251666 [details] patch Clearing flags on attachment: 251666 Committed r183368: <http://trac.webkit.org/changeset/183368> All reviewed patches have been landed. Closing bug. 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? Landed a build fix in r183420. 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. More build fix in r183434. And more in r183439. 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. 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! 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. (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. ;) (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> |