Bug 157676 – CTTE for the HTML editing header

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

CTTE for the HTML editing header

[Darin Adler]

Created attachment 278840 [details]
Patch

[Darin Adler]

Created attachment 278932 [details]
Patch

[Chris Dumez]

Some red bubbles.

[Darin Adler]

Created attachment 278934 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

Oops, broke something. I’ll upload a new patch after fixing it.

[Chris Dumez]

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*

[Chris Dumez]

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*

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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?

[Darin Adler]

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

[Darin Adler]

Created attachment 278940 [details]
Patch

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 278940 [details]
Patch

Clearing flags on attachment: 278940

Committed r200922: <http://trac.webkit.org/changeset/200922>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Wow, I missed lots of good comments. I should follow up and do the things you suggested!

[Darin Adler]

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.