Bug 27038

Summary: htmlediting.cpp needs more utility functions to fix the bug 26816
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Severity: Enhancement CC: eric, jparent, justin.garcia
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 26816    
Description Flags
fixes the bug
removed tab from the patch
fixed the style violations
updated ChangeLog
added more comments to each function
removed the change to CompositeEditCommand.cpp
eric: review-
eric: review+
eric: review-
9th submission eric: review+

Description Ryosuke Niwa 2009-07-07 12:48:37 PDT
We need to add several utility functions in htmlediting.cpp to fix the bug 26816.
Comment 1 Ryosuke Niwa 2009-07-07 13:42:19 PDT
Created attachment 32389 [details]
fixes the bug

This patch supplies utility functions to resolve the bug 26816.
Comment 2 Ryosuke Niwa 2009-07-07 13:46:40 PDT
Created attachment 32391 [details]
removed tab from the patch
Comment 3 Eric Seidel (no email) 2009-07-07 13:47:56 PDT
Lots of style nits. :(
Comment 4 Julie Parent 2009-07-07 13:59:17 PDT
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
Comment 5 Ryosuke Niwa 2009-07-07 15:01:44 PDT
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.
Comment 6 Ryosuke Niwa 2009-07-07 15:09:10 PDT
Created attachment 32395 [details]
updated ChangeLog
Comment 7 Julie Parent 2009-07-07 15:09:39 PDT
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?
Comment 8 Ryosuke Niwa 2009-07-07 16:01:19 PDT
Created attachment 32400 [details]
added more comments to each function
Comment 9 Ryosuke Niwa 2009-07-07 17:01:16 PDT
Created attachment 32402 [details]
removed the change to CompositeEditCommand.cpp
Comment 10 Eric Seidel (no email) 2009-07-08 14:09:27 PDT
Comment on attachment 32402 [details]
removed the change to CompositeEditCommand.cpp

 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)

 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.
Comment 11 Ryosuke Niwa 2009-07-08 16:11:05 PDT
Created attachment 32484 [details]
Comment 12 Eric Seidel (no email) 2009-07-14 15:03:23 PDT
Comment on attachment 32484 [details]

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

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)

Comment 13 Ryosuke Niwa 2009-07-17 14:56:01 PDT
Created attachment 32973 [details]
Comment 14 Eric Seidel (no email) 2009-07-17 14:58:38 PDT
Comment on attachment 32973 [details]

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

Seems wrong.
Comment 15 Ryosuke Niwa 2009-07-17 15:09:52 PDT
Created attachment 32977 [details]
9th submission
Comment 16 Eric Seidel (no email) 2009-07-17 15:16:20 PDT
Comment on attachment 32977 [details]
9th submission

Generally we sort headers in alphabetical order, but this is fine to land as-is.
Comment 17 Julie Parent 2009-07-17 15:59:17 PDT
Landed in http://trac.webkit.org/changeset/46063