Bug 56439 – Get rid of nearestMailBlockquote

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

[Ryosuke Niwa]

nearestMailBlockquote doesn't respect editing boundary and takes Node* instead of Position.  Both of these are unconventional and all call sites of nearestMailBlockquote should be calling enclosingNodeOfType or highestEnclosingNodeOfType instead.

[Ryosuke Niwa]

Created attachment 85896 [details]
cleanup + minor crash fix

[Ryosuke Niwa]

(In reply to comment #1)
> Created an attachment (id=85896) [details]
> cleanup + minor crash fix

I included the bug fix because this was a relatively small patch but I'll split it if other people think it's better that way.

[Tony Chang]

Comment on attachment 85896 [details]
cleanup + minor crash fix

View in context: https://bugs.webkit.org/attachment.cgi?id=85896&action=review

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:626
> +    Node* blockquoteNode = (!context || isMailPasteAsQuotationNode(context)) ? context : enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary);

Nit: It took me a while to figure out why you added !context (i.e., it's ok to pass NULL to isMailPasteAsQuotationNode, but it's not OK to pass NULL to firstPositionInNode).  Maybe pull this out into a helper function (it could also be used in line 559).  Alternately, maybe firstPositionInNode should return a null position when passed in a NULL pointer.

[Ryosuke Niwa]

Comment on attachment 85896 [details]
cleanup + minor crash fix

View in context: https://bugs.webkit.org/attachment.cgi?id=85896&action=review

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:626
>> +    Node* blockquoteNode = (!context || isMailPasteAsQuotationNode(context)) ? context : enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary);
> 
> Nit: It took me a while to figure out why you added !context (i.e., it's ok to pass NULL to isMailPasteAsQuotationNode, but it's not OK to pass NULL to firstPositionInNode).  Maybe pull this out into a helper function (it could also be used in line 559).  Alternately, maybe firstPositionInNode should return a null position when passed in a NULL pointer.

Extracting a helper function sounds good.  isMailPasteAsQuotationOrMailBlockquoteDescendent?

[Tony Chang]

Comment on attachment 85896 [details]
cleanup + minor crash fix

View in context: https://bugs.webkit.org/attachment.cgi?id=85896&action=review

>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:626
>>> +    Node* blockquoteNode = (!context || isMailPasteAsQuotationNode(context)) ? context : enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary);
>> 
>> Nit: It took me a while to figure out why you added !context (i.e., it's ok to pass NULL to isMailPasteAsQuotationNode, but it's not OK to pass NULL to firstPositionInNode).  Maybe pull this out into a helper function (it could also be used in line 559).  Alternately, maybe firstPositionInNode should return a null position when passed in a NULL pointer.
> 
> Extracting a helper function sounds good.  isMailPasteAsQuotationOrMailBlockquoteDescendent?

Sure.  I was actually just thinking of pulling out enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary) with a NULL check (nearestParentMailBlockquote?).  But that works too.

[Ryosuke Niwa]

On my second thought, my initial proposal doesn't work and adding a null check to firstPositionInNode/lastPositionInNode makes most sense now.  Will be uploading a patch soon.

[Ryosuke Niwa]

Created attachment 85984 [details]
Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode

[Tony Chang]

Comment on attachment 85984 [details]
Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode

View in context: https://bugs.webkit.org/attachment.cgi?id=85984&action=review

> Source/WebCore/editing/htmlediting.h:118
> +    return node && editingIgnoresContent(node) ? positionBeforeNode(node) : firstPositionInNode(node);

This doesn't look right.  && has precedence over ?, so if |node| is NULL, we return firstPositionInNode(NULL).  That seems safe, but in lastPositionInOrAfterNode, we return lastPositionInNode(NULL), which calls lastOffsetInNode(NULL), which will try to dereference NULL.

[Ryosuke Niwa]

Comment on attachment 85984 [details]
Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode

View in context: https://bugs.webkit.org/attachment.cgi?id=85984&action=review

>> Source/WebCore/editing/htmlediting.h:118
>> +    return node && editingIgnoresContent(node) ? positionBeforeNode(node) : firstPositionInNode(node);
> 
> This doesn't look right.  && has precedence over ?, so if |node| is NULL, we return firstPositionInNode(NULL).  That seems safe, but in lastPositionInOrAfterNode, we return lastPositionInNode(NULL), which calls lastOffsetInNode(NULL), which will try to dereference NULL.

Ah that's a good point.  I should just do "!node ||"

[Ryosuke Niwa]

Created attachment 85992 [details]
Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode

[Ryosuke Niwa]

Thanks for the review!  Landing it now.

[Ryosuke Niwa]

Committed r81295: <http://trac.webkit.org/changeset/81295>