CTTE for the HTML editing header
Created attachment 278840 [details] Patch
Created attachment 278932 [details] Patch
Some red bubbles.
Created attachment 278934 [details] Patch
Comment on attachment 278934 [details] Patch Attachment 278934 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1321249 Number of test failures exceeded the failure limit.
Created attachment 278936 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 278934 [details] Patch Attachment 278934 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1321250 Number of test failures exceeded the failure limit.
Created attachment 278937 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 278934 [details] Patch Attachment 278934 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1321247 Number of test failures exceeded the failure limit.
Created attachment 278938 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Oops, broke something. I’ll upload a new patch after fixing it.
Comment on attachment 278934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278934&action=review > Source/WebCore/dom/Position.cpp:217 > + return lastOffsetForEditing(*m_anchorNode); Not sure what guarantee m_anchorNode is non null here. Maybe we could have a null check to be safe? I understand the previous code would have hit an assertion but it wouldn't have crashed in release because lastOffsetForEditing() did have a null check. > Source/WebCore/editing/ApplyStyleCommand.cpp:370 > + if (startNode->isTextNode() && start.deprecatedEditingOffset() >= caretMaxOffset(*startNode)) { is<Text>(*startNode) > Source/WebCore/editing/ApplyStyleCommand.cpp:1372 > + if (is<Element>(nextSibling.get()) && nextSibling->hasEditableStyle() && areIdenticalElements(*element, *nextSibling)) Do we really need the is<Element>(nextSibling.get()) check ? areIdenticalElements() should take care of this already, no? I think this could be a simple null check: if (nextSibling && nextSibling->hasEditableStyle() && areIdenticalElements(*element, *nextSibling) > Source/WebCore/editing/ApplyStyleCommand.cpp:1378 > + if (is<Element>(*mergedElement) && mergedElement->hasEditableStyle() && areIdenticalElements(*previousSibling, *mergedElement)) Do we really need the is<Element>(*mergedElement) check ? areIdenticalElements() should take care of this already, no? > Source/WebCore/editing/htmlediting.cpp:110 > + ContainerNode* highestEditableRoot = editableRootForPosition(position, editableType); auto* ? > Source/WebCore/editing/htmlediting.cpp:414 > +bool isTableStructureNode(const Node* node) Should take a reference. > Source/WebCore/editing/htmlediting.cpp:426 > +static bool isSpecialElement(const Node* node) We could rename this to "isSpecialHTMLElement()" to make it clearer it is safe to downcast the node to HTMLElement afterwards. > Source/WebCore/editing/htmlediting.cpp:567 > +bool isListElement(Node* node) Could be renamed to isListHTMLElement() to make it clearer it is safe to cast to HTMLElement afterwards. > Source/WebCore/editing/htmlediting.cpp:597 > + auto* root = rule == CannotCrossEditingBoundary ? highestEditableRoot(p) : 0; nullptr > Source/WebCore/editing/htmlediting.cpp:614 > + auto* root = rule == CannotCrossEditingBoundary ? highestEditableRoot(p) : 0; nullptr > Source/WebCore/editing/htmlediting.cpp:896 > + return is<HTMLSpanElement>(node) && downcast<HTMLSpanElement>(*node).fastGetAttribute(classAttr) == AppleTabSpanClass; fastGetAttribute() for class is not OK > Source/WebCore/editing/htmlediting.cpp:977 > +bool isMailBlockquote(const Node* node) Should take a ref. > Source/WebCore/editing/htmlediting.cpp:997 > + if (node.isTextNode() && node.renderer()) is<Text>(node) > Source/WebCore/editing/htmlediting.cpp:998 > + return node.renderer()->caretMaxOffset(); downcast<Text>(node).renderer() to get a RenderText*
Comment on attachment 278934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278934&action=review > Source/WebCore/editing/htmlediting.h:227 > +inline bool editingIgnoresContent(const Node* node) Should take a ref. (Let's at least add a FIXME comment if it is too large a refactoring to do in this patch). > Source/WebCore/editing/htmlediting.h:232 > +inline bool canHaveChildrenForEditing(const Node* node) Should take a ref. (Let's at least add a FIXME comment if it is too large a refactoring to do in this patch). > Source/WebCore/editing/htmlediting.h:234 > + return !node->isTextNode() && node->canContainRangeEndPoint(); !is<Text>(*node) > Source/WebKit/ios/WebCoreSupport/WebFrameIOS.mm:851 > Node *currentNode = currentVisiblePosition.deepEquivalent().anchorNode(); star on wrong side, and could use auto* > Source/WebKit/ios/WebCoreSupport/WebFrameIOS.mm:907 > Node *currentNode = currentVisiblePosition.deepEquivalent().anchorNode(); star on wrong side, and could use auto*
Comment on attachment 278934 [details] Patch Attachment 278934 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1321271 New failing tests: editing/inserting/insert-composition-whitespace.html editing/style/remove-underline-after-paragraph.html editing/deleting/delete-across-editable-content-boundaries-3.html editing/deleting/maintain-style-after-delete.html editing/execCommand/overtype.html editing/style/remove-underline-after-paragraph-in-bold.html editing/inserting/5002441.html editing/inserting/insert-text-with-newlines.html editing/deleting/delete-3608430-fix.html editing/inserting/edited-whitespace-1.html editing/deleting/5032066.html editing/deleting/delete-across-editable-content-boundaries-2.html editing/style/create-block-for-style-012.html editing/inserting/insert-paragraph-separator-tab-span.html editing/style/underline.html editing/style/unbold-in-bold.html editing/execCommand/delete-line-and-insert-text-in-font-inside-blockquote.html editing/execCommand/5142012-3.html imported/blink/editing/selection/unrooted-selection-start-crash.html
Created attachment 278939 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 278934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278934&action=review > Source/WebCore/editing/htmlediting.cpp:411 > + return changedSomething ? string : String::adopt(rebalancedString); How about the getCharactersWithUpconvert() at the beginning of this function?
You’re quoting the right line, Chris. That’s the bug. The logic in that expression you quoted is backwards. I had already figured this out and I’m testing that it fixes everything. (The getCharactersWithUpconvert is fine.)
Created attachment 278940 [details] Patch
Comment on attachment 278934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278934&action=review >> Source/WebCore/editing/htmlediting.cpp:896 >> + return is<HTMLSpanElement>(node) && downcast<HTMLSpanElement>(*node).fastGetAttribute(classAttr) == AppleTabSpanClass; > > fastGetAttribute() for class is not OK Never mind, I confused style and class.
Comment on attachment 278940 [details] Patch Clearing flags on attachment: 278940 Committed r200922: <http://trac.webkit.org/changeset/200922>
All reviewed patches have been landed. Closing bug.
Wow, I missed lots of good comments. I should follow up and do the things you suggested!
Comment on attachment 278934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278934&action=review Started a new patch for the comments here. >> Source/WebCore/dom/Position.cpp:217 >> + return lastOffsetForEditing(*m_anchorNode); > > Not sure what guarantee m_anchorNode is non null here. Maybe we could have a null check to be safe? I understand the previous code would have hit an assertion but it wouldn't have crashed in release because lastOffsetForEditing() did have a null check. I agree. I will add a null check here. >> Source/WebCore/editing/ApplyStyleCommand.cpp:370 >> + if (startNode->isTextNode() && start.deprecatedEditingOffset() >= caretMaxOffset(*startNode)) { > > is<Text>(*startNode) Did a lot of these. >> Source/WebCore/editing/ApplyStyleCommand.cpp:1372 >> + if (is<Element>(nextSibling.get()) && nextSibling->hasEditableStyle() && areIdenticalElements(*element, *nextSibling)) > > Do we really need the is<Element>(nextSibling.get()) check ? areIdenticalElements() should take care of this already, no? > I think this could be a simple null check: if (nextSibling && nextSibling->hasEditableStyle() && areIdenticalElements(*element, *nextSibling) We need at least a null check. I will keep the null check. >> Source/WebCore/editing/ApplyStyleCommand.cpp:1378 >> + if (is<Element>(*mergedElement) && mergedElement->hasEditableStyle() && areIdenticalElements(*previousSibling, *mergedElement)) > > Do we really need the is<Element>(*mergedElement) check ? areIdenticalElements() should take care of this already, no? Removed it. >> Source/WebCore/editing/htmlediting.cpp:110 >> + ContainerNode* highestEditableRoot = editableRootForPosition(position, editableType); > > auto* ? No, if I made this auto, then it would be Element* and then the assignment below would fail since it’s not known to be an element. >> Source/WebCore/editing/htmlediting.cpp:411 >> + return changedSomething ? string : String::adopt(rebalancedString); > > How about the getCharactersWithUpconvert() at the beginning of this function? While I am not sure what you meant by this, I decided to further optimize this so it doesn’t even allocate memory for the rebalanced string until we find a character that needs to change. >> Source/WebCore/editing/htmlediting.cpp:414 >> +bool isTableStructureNode(const Node* node) > > Should take a reference. Can’t do it at this time because there are call sites that pass isTableStructureNode to enclosingNodeOfType. When I change enclosingNodeOfType to take a function pointer that takes a Node& instead of a Node*, then I can change isTableStructureNode to take a Node&. >> Source/WebCore/editing/htmlediting.cpp:426 >> +static bool isSpecialElement(const Node* node) > > We could rename this to "isSpecialHTMLElement()" to make it clearer it is safe to downcast the node to HTMLElement afterwards. Done. >> Source/WebCore/editing/htmlediting.cpp:567 >> +bool isListElement(Node* node) > > Could be renamed to isListHTMLElement() to make it clearer it is safe to cast to HTMLElement afterwards. OK, done. >> Source/WebCore/editing/htmlediting.cpp:597 >> + auto* root = rule == CannotCrossEditingBoundary ? highestEditableRoot(p) : 0; > > nullptr Done. >> Source/WebCore/editing/htmlediting.cpp:614 >> + auto* root = rule == CannotCrossEditingBoundary ? highestEditableRoot(p) : 0; > > nullptr Done. >> Source/WebCore/editing/htmlediting.cpp:977 >> +bool isMailBlockquote(const Node* node) > > Should take a ref. Same problem with enclosingNodeOfType as above. >> Source/WebCore/editing/htmlediting.cpp:997 >> + if (node.isTextNode() && node.renderer()) > > is<Text>(node) Done. >> Source/WebCore/editing/htmlediting.cpp:998 >> + return node.renderer()->caretMaxOffset(); > > downcast<Text>(node).renderer() to get a RenderText* Done. >> Source/WebCore/editing/htmlediting.h:227 >> +inline bool editingIgnoresContent(const Node* node) > > Should take a ref. (Let's at least add a FIXME comment if it is too large a refactoring to do in this patch). Done. >> Source/WebCore/editing/htmlediting.h:232 >> +inline bool canHaveChildrenForEditing(const Node* node) > > Should take a ref. (Let's at least add a FIXME comment if it is too large a refactoring to do in this patch). Done. >> Source/WebCore/editing/htmlediting.h:234 >> + return !node->isTextNode() && node->canContainRangeEndPoint(); > > !is<Text>(*node) Done. >> Source/WebKit/ios/WebCoreSupport/WebFrameIOS.mm:851 >> Node *currentNode = currentVisiblePosition.deepEquivalent().anchorNode(); > > star on wrong side, and could use auto* Done. >> Source/WebKit/ios/WebCoreSupport/WebFrameIOS.mm:907 >> Node *currentNode = currentVisiblePosition.deepEquivalent().anchorNode(); > > star on wrong side, and could use auto* Done.