Bug 27038 – htmlediting.cpp needs more utility functions to fix the bug 26816

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

[Ryosuke Niwa]

We need to add several utility functions in htmlediting.cpp to fix the bug 26816.

[Ryosuke Niwa]

Created attachment 32389 [details]
fixes the bug

This patch supplies utility functions to resolve the bug 26816.

[Ryosuke Niwa]

Created attachment 32391 [details]
removed tab from the patch

[Eric Seidel (no email)]

Lots of style nits. :(

[Julie Parent]

Comment on attachment 32391 [details]
removed tab from the patch

I'm not a real reviewer, but I saw a couple style violations.  See below for details.  Also, I know it isn't the style of these files, but could you add function level descriptions for the new functions you are adding?  Particularly for functions that are designed to be re-usable utilities, documentation will really help others use them in the future, rather than rolling their own.
 
> +PassRefPtr<Range> createRange(PassRefPtr<Document> document, const VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec)
> +{
> +    ec=0;
> +    RefPtr<Range> selectedRange = Range::create(document);
> +    selectedRange->setStart( start.deepEquivalent().containerNode(), start.deepEquivalent().computeOffsetInContainerNode(), ec);

Style, here and in a few other places in this file: Do not place spaces between a function and its parentheses, or between a parenthesis and its content.

> +PassRefPtr<Range> exntendRangeToWrappingNodes(PassRefPtr<Range> rangeToExtend, PassRefPtr<Range> maximumRange, Node* rootNode)

Spelling: extend, not exntend

> +{
> +    ASSERT(rangeToExtend);
> +    ASSERT(maximumRange);
> +    
> +    ExceptionCode ec=0;

Style:  Here, and in other places in this file. Do place spaces around binary and ternary operators.

> Index: WebCore/editing/htmlediting.h
> ===================================================================
> --- WebCore/editing/htmlediting.h	(revision 45564)
> +++ WebCore/editing/htmlediting.h	(working copy)
> @@ -28,6 +28,7 @@
>  
>  #include <wtf/Forward.h>
>  #include "HTMLNames.h"
> +#include "CompositeEditCommand.h"
>  
>  namespace WebCore {
>  
> @@ -61,6 +62,7 @@ Position previousVisuallyDistinctCandida
>  bool isEditablePosition(const Position&);
>  bool isRichlyEditablePosition(const Position&);
>  Element* editableRootForPosition(const Position&);
> +    Element* unsplittableElementForPosition(const Position& p);

Style: spacing

[Ryosuke Niwa]

Created attachment 32394 [details]
fixed the style violations

I removed getVisibleRangeOfNode and added visiblePositionBeforeNode and visiblePositionAfterNode.  This would clean up the rest of my patch a little bit.

[Ryosuke Niwa]

Created attachment 32395 [details]
updated ChangeLog

[Julie Parent]

Comment on attachment 32395 [details]
updated ChangeLog

> Index: WebCore/editing/htmlediting.cpp
> ===================================================================
> --- WebCore/editing/htmlediting.cpp	(revision 45564)
> +++ WebCore/editing/htmlediting.cpp	(working copy)
> @@ -207,6 +207,15 @@ Element* editableRootForPosition(const P
>      return node->rootEditableElement();
>  }
>  
> +// Find the unsplittable element enclosing the specified position.
> +// The returned element is either table cell or root editable node.

Sorry I wasn't more clear before.  I asked for function level comments because it wasn't clear to me from the function names what they should/should not be used for.  These comments only repeat the function names, which isn't much more useful.  For example, what does it mean to be an unsplittable element?

> +Element* unsplittableElementForPosition(const Position& p)
> +{
> +    Element* enclosingCell = static_cast<Element*>(enclosingNodeOfType(p, &isTableCell));
> +    
> +    return enclosingCell ? enclosingCell : editableRootForPosition(p);
> +}

This isn't quite right - what if the editableRoot is inside of the td?

[Ryosuke Niwa]

Created attachment 32400 [details]
added more comments to each function

[Ryosuke Niwa]

Created attachment 32402 [details]
removed the change to CompositeEditCommand.cpp

[Eric Seidel (no email)]

Comment on attachment 32402 [details]
removed the change to CompositeEditCommand.cpp

Style:
 63     short comparePoint(Node* refNode, int offset, ExceptionCode& ec) const;

I would have used early return:
 215     Element* enclosingCell = static_cast<Element*>(enclosingNodeOfType(p, &isTableCell, true));
 216     // Since enclosingNodeOfType won't search beyond the highest root editable node,
 217     // this code works even if the closest table cell was outside of the root editable node.
 218     return enclosingCell ? enclosingCell : editableRootForPosition(p);

This could assert ASSERT(node->parentNode()):
 592 VisiblePosition visiblePositionBeforeNode(const Node* node)

Please add a FIXME: to the other createRange:
 612 PassRefPtr<Range> createRange(PassRefPtr<Document> document, const VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec)

PassRefPtr seems the wrong type for these args:
 627 PassRefPtr<Range> extendRangeToWrappingNodes(PassRefPtr<Range> rangeToExtend, PassRefPtr<Range> maximumRange, Node* rootNode)

Explicit visiblePosition construction is not needed here:
 1076     return visiblePositionBeforeNode(node) == VisiblePosition(selectedRange.startPosition())
(but it also doesn't hurt)

Style:
 65 Element* unsplittableElementForPosition(const Position& p);
 78 PassRefPtr<Range> createRange(PassRefPtr<Document> document, const VisiblePosition& start, const VisiblePosition& end, ExceptionCode& ec);
 145 bool isNodeVisiblyContainedWithin(Node* node, const Range& selectedRange);

Looks fine.  Normally we don't add unused code, but I can see how this will help make your future patches smaller.

Please re-submit a fixed version, and then we might wait to land this until the other patch which depends on these changes is approved to.

[Ryosuke Niwa]

Created attachment 32484 [details]
resubmission

[Eric Seidel (no email)]

Comment on attachment 32484 [details]
resubmission

Looks fine.

I think the intent of the old comments (that something needs fixing) is lost:
    // FIXME: This should ASSERT(node->parentNode())
 582     ASSERT(node);
 583     // FIXME: adding ASSERT(node->parentNode()); results in editing/deleting/delete-ligature-001.html crashing
569584

No need to be const:
 595 VisiblePosition visiblePositionBeforeNode(const Node* node)

Generally we don't make RefCounted objects const, ever.  Because well... they're refcounted. :)  You can't ref them w/ them being const.

I probably would have named the argument "range" and the returned value "extendRange" but this is fine too:
 632 PassRefPtr<Range> extendRangeToWrappingNodes(PassRefPtr<Range> rangeToExtend, const Range* maximumRange, const Node* rootNode)

Thanks.!

[Ryosuke Niwa]

Created attachment 32973 [details]
re-resubmission

[Eric Seidel (no email)]

Comment on attachment 32973 [details]
re-resubmission

Why is this needed?
 #include "HTMLNames.h"
+#include "CompositeEditCommand.h"

Seems wrong.

[Ryosuke Niwa]

Created attachment 32977 [details]
9th submission

[Eric Seidel (no email)]

Comment on attachment 32977 [details]
9th submission

Generally we sort headers in alphabetical order, but this is fine to land as-is.

[Julie Parent]

Landed in http://trac.webkit.org/changeset/46063