Bug 157722 – More CTTE and other cleanups for HTML editing header

• Component: HTML Editing
• Status: RESOLVED FIXED

[Darin Adler]

More CTTE and other cleanups for HTML editing header

[Darin Adler]

Created attachment 278977 [details]
Patch

[Chris Dumez]

red bubbles

[Darin Adler]

Created attachment 278978 [details]
Patch

[Darin Adler]

Comment on attachment 278978 [details]
Patch

Oops, tests not passing locally yet!

[Darin Adler]

Created attachment 278979 [details]
Patch

[Darin Adler]

Comment on attachment 278979 [details]
Patch

OK, tests passing locally now.

[Darin Adler]

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.

[Chris Dumez]

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).

[Darin Adler]

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.

[Darin Adler]

Committed r200931: <http://trac.webkit.org/changeset/200931>