More CTTE and other cleanups for HTML editing header
Created attachment 278977 [details] Patch
red bubbles
Created attachment 278978 [details] Patch
Comment on attachment 278978 [details] Patch Oops, tests not passing locally yet!
Created attachment 278979 [details] Patch
Comment on attachment 278979 [details] Patch OK, tests passing locally now.
Here’s what was failing on the mac-debug EWS: Regressions: Unexpected crashes (2) imported/blink/storage/indexeddb/blob-basics-metadata.html [ Crash ] imported/blink/storage/indexeddb/blob-valid-after-deletion.html [ Crash ] I ran these tests locally, and they did not crash. Also seems highly unlikely the changes to editing code could have had any effect on these tests.
Comment on attachment 278979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278979&action=review r=me > Source/WebCore/dom/Position.cpp:219 > + if (!m_anchorNode) Could do a ternary and return on one line. > Source/WebCore/editing/ApplyStyleCommand.cpp:1179 > +bool ApplyStyleCommand::nodeFullySelected(Node* node, const Position& start, const Position& end) const We should either switch to using a reference for node or add a FIXME comment. > Source/WebCore/editing/ApplyStyleCommand.cpp:1191 > +bool ApplyStyleCommand::nodeFullyUnselected(Node* node, const Position& start, const Position& end) const We should either switch to using a reference for node or add a FIXME comment. > Source/WebCore/editing/htmlediting.cpp:396 > + for (unsigned i = 0; i < length; i++) { ++i > Source/WebCore/editing/htmlediting.cpp:417 > + if (!rebalancedString.length()) I would prefer if (rebalancedString.isEmpty()) > Source/WebCore/editing/markup.cpp:541 > + if (auto* parentListNode = downcast<HTMLElement>(enclosingNodeOfType(firstPositionInOrBeforeNode(range->firstNode()), isListItem))) { Is this cast to HTMLElement here is safe? isListItem() can return true for an element that has a list renderer (e.g. elements with "display: list-item" in CSS, which are not HTML list elements).
Comment on attachment 278979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278979&action=review >> Source/WebCore/dom/Position.cpp:219 >> + if (!m_anchorNode) > > Could do a ternary and return on one line. OK, done. >> Source/WebCore/editing/ApplyStyleCommand.cpp:1179 >> +bool ApplyStyleCommand::nodeFullySelected(Node* node, const Position& start, const Position& end) const > > We should either switch to using a reference for node or add a FIXME comment. I changed the type to Element& and the name to element. These files have so many other style issues nearby, but this was easy enough to do. >> Source/WebCore/editing/ApplyStyleCommand.cpp:1191 >> +bool ApplyStyleCommand::nodeFullyUnselected(Node* node, const Position& start, const Position& end) const > > We should either switch to using a reference for node or add a FIXME comment. Ditto. >> Source/WebCore/editing/htmlediting.cpp:396 >> + for (unsigned i = 0; i < length; i++) { > > ++i Done. >> Source/WebCore/editing/htmlediting.cpp:417 >> + if (!rebalancedString.length()) > > I would prefer if (rebalancedString.isEmpty()) OK, done. >> Source/WebCore/editing/markup.cpp:541 >> + if (auto* parentListNode = downcast<HTMLElement>(enclosingNodeOfType(firstPositionInOrBeforeNode(range->firstNode()), isListItem))) { > > Is this cast to HTMLElement here is safe? isListItem() can return true for an element that has a list renderer (e.g. elements with "display: list-item" in CSS, which are not HTML list elements). It’s not; you are right. I changed the cast to Element instead of HTMLElement.
Committed r200931: <http://trac.webkit.org/changeset/200931>